Unionfs: prevent false lockdep warnings in stacking

A stackable file system like unionfs often performs an operation on a lower
file system, by calling a vfs_* method, having been called possibly by the
very same method from the VFS.  Both calls to the vfs_* method grab a lock
in the same lock class, and hence lockdep complains.  This warning is a
false positive in instances where unionfs only calls the vfs_* method on
lower objects; there's a strict lock ordering here: upper objects first,
then lower objects.

We want to prevent these false positives so that lockdep will not shutdown
so it'd still be able to warn us about potentially true locking problems.
So, we temporarily turn off lockdep ONLY AROUND the calls to vfs methods to
which we pass lower objects, and only for those instances where lockdep
complained.  While this solution may seem unclean, it is not without
precedent: other places in the kernel also do similar temporary disabling,
of course after carefully having checked that it is the right thing to do.

In the long run, lockdep needs to be taught how to handle about stacking.
Then this patch can be removed.  It is likely that such lockdep-stacking
support will do essentially the same as this patch: consider the same
ordering (upper then lower) and consider upper vs. lower locks to be in
different classes.

Signed-off-by: Erez Zadok <ezk@cs.sunysb.edu>

diff --git a/Documentation/filesystems/unionfs/issues.txt b/Documentation/filesystems/unionfs/issues.txt
index bb6ab052c17c7694cdeec96b42d5018369d268ad..f4b7e7e26954a22bb27a74f4941d071facadf20e 100644 (file)
--- a/Documentation/filesystems/unionfs/issues.txt
+++ b/Documentation/filesystems/unionfs/issues.txt
@@ -17,8 +17,12 @@ KNOWN Unionfs 2.x ISSUES:
    an upper object, and then a lower object, in a strict order to avoid
    locking problems; in addition, Unionfs, as a fan-out file system, may
    have to lock several lower inodes.  We are currently looking into Lockdep
-   to see how to make it aware of stackable file systems.  In the meantime,
-   if you get any warnings from Lockdep, you can safely ignore them (or feel
-   free to report them to the Unionfs maintainers, just to be sure).
+   to see how to make it aware of stackable file systems.  For now, we
+   temporarily disable lockdep when calling vfs methods on lower objects,
+   but only for those places where lockdep complained.  While this solution
+   may seem unclean, it is not without precedent: other places in the kernel
+   also do similar temporary disabling, of course after carefully having
+   checked that it is the right thing to do.  Anyway, you get any warnings
+   from Lockdep, please report them to the Unionfs maintainers.
 
 For more information, see <http://unionfs.filesystems.org/>.

diff --git a/fs/unionfs/copyup.c b/fs/unionfs/copyup.c
index 96b3b950ddadf21e5464e66fc5b88aa3180e42ad..33c2d33c2d12b764a8c1bb1546ae74691334b1d6 100644 (file)
--- a/fs/unionfs/copyup.c
+++ b/fs/unionfs/copyup.c
@@ -297,11 +297,14 @@ static int __copyup_reg_data(struct dentry *dentry,
                        break;
                }
 
+               /* see Documentation/filesystems/unionfs/issues.txt */
+               lockdep_off();
                write_bytes =
                        output_file->f_op->write(output_file,
                                                 (char __user *)buf,
                                                 read_bytes,
                                                 &output_file->f_pos);
+               lockdep_on();
                if ((write_bytes < 0) || (write_bytes < read_bytes)) {
                        err = write_bytes;
                        break;

diff --git a/fs/unionfs/inode.c b/fs/unionfs/inode.c
index b378f79095cc67f5317a9fb2b7f7a55f88c09878..ac907565949107dc823b8a7064972b1d768a0776 100644 (file)
--- a/fs/unionfs/inode.c
+++ b/fs/unionfs/inode.c
@@ -205,13 +205,11 @@ static int unionfs_link(struct dentry *old_dentry, struct inode *dir,
        lower_new_dentry = unionfs_lower_dentry(new_dentry);
 
        if (dbstart(old_dentry) > dbstart(new_dentry)) {
-
                /* don't copyup if new_dentry doesn't exist in union */
                if (dbstart(new_dentry) == 0 &&
                    dbstart(new_dentry) == dbend(new_dentry) &&
                    (!unionfs_lower_dentry_idx(new_dentry, 0) ||
                     !unionfs_lower_dentry_idx(new_dentry, 0)->d_inode)) {
-
                        set_dbstart(new_dentry, dbstart(old_dentry));
                        set_dbend(new_dentry, dbstart(old_dentry));
                        create_p = 1;
@@ -221,7 +219,6 @@ static int unionfs_link(struct dentry *old_dentry, struct inode *dir,
                                unionfs_mntput(new_dentry, 0);
                                unionfs_set_lower_mnt_idx(new_dentry, 0, NULL);
                        }
-
                } else {
                        err = -EROFS;
                        goto docopyup;
@@ -251,9 +248,13 @@ static int unionfs_link(struct dentry *old_dentry, struct inode *dir,
        BUG_ON(dbstart(old_dentry) != dbstart(new_dentry));
        lower_dir_dentry = lock_parent(lower_new_dentry);
        err = is_robranch(old_dentry);
-       if (!err)
+       if (!err) {
+               /* see Documentation/filesystems/unionfs/issues.txt */
+               lockdep_off();
                err = vfs_link(lower_old_dentry, lower_dir_dentry->d_inode,
                               lower_new_dentry);
+               lockdep_on();
+       }
        unlock_dir(lower_dir_dentry);
 
 docopyup:
@@ -283,10 +284,13 @@ docopyup:
 
                        lower_dir_dentry =
                                lock_parent(lower_new_dentry);
+                        /* see Documentation/filesystems/unionfs/issues.txt */
+                       lockdep_off();
                        /* do vfs_link */
                        err = vfs_link(lower_old_dentry,
                                       lower_dir_dentry->d_inode,
                                       lower_new_dentry);
+                       lockdep_on();
                        unlock_dir(lower_dir_dentry);
                        goto check_link;
                }

diff --git a/fs/unionfs/rename.c b/fs/unionfs/rename.c
index 8a0a269fd0f920bbb43e80fd6825b769e6411629..e0eacf13005c2814d37e4dd50878f02b5fa46a3d 100644 (file)
--- a/fs/unionfs/rename.c
+++ b/fs/unionfs/rename.c
@@ -99,21 +99,21 @@ static int __unionfs_rename(struct inode *old_dir, struct dentry *old_dentry,
                }
        }
 
+       err = is_robranch_super(old_dentry->d_sb, bindex);
+       if (err)
+               goto out;
+
        dget(lower_old_dentry);
        lower_old_dir_dentry = dget_parent(lower_old_dentry);
        lower_new_dir_dentry = dget_parent(lower_new_dentry);
 
+       /* see Documentation/filesystems/unionfs/issues.txt */
+       lockdep_off();
        lock_rename(lower_old_dir_dentry, lower_new_dir_dentry);
-
-       err = is_robranch_super(old_dentry->d_sb, bindex);
-       if (err)
-               goto out_unlock;
-
        err = vfs_rename(lower_old_dir_dentry->d_inode, lower_old_dentry,
                         lower_new_dir_dentry->d_inode, lower_new_dentry);
-
-out_unlock:
        unlock_rename(lower_old_dir_dentry, lower_new_dir_dentry);
+       lockdep_on();
 
        dput(lower_old_dir_dentry);
        dput(lower_new_dir_dentry);

diff --git a/fs/unionfs/super.c b/fs/unionfs/super.c
index 5df95d5166826b1a856f1aae4b03c6215899c83c..baa2e81b05d27d26eb573f885a894a319aa01f67 100644 (file)
--- a/fs/unionfs/super.c
+++ b/fs/unionfs/super.c
@@ -863,7 +863,11 @@ static void unionfs_clear_inode(struct inode *inode)
                        lower_inode = unionfs_lower_inode_idx(inode, bindex);
                        if (!lower_inode)
                                continue;
+                       unionfs_set_lower_inode_idx(inode, bindex, NULL);
+                       /* see Documentation/filesystems/unionfs/issues.txt */
+                       lockdep_off();
                        iput(lower_inode);
+                       lockdep_on();
                }
        }
 

diff --git a/fs/unionfs/unlink.c b/fs/unionfs/unlink.c
index 461993096bf35f5137a721f38d43d8bc448bed83..b00971109df0564e6b8f720d1d852f7f4421570a 100644 (file)
--- a/fs/unionfs/unlink.c
+++ b/fs/unionfs/unlink.c
@@ -45,10 +45,13 @@ static int unionfs_do_unlink(struct inode *dir, struct dentry *dentry)
                /* avoid destroying the lower inode if the file is in use */
                dget(lower_dentry);
                err = is_robranch_super(dentry->d_sb, bindex);
-               if (!err)
+               if (!err) {
+                       /* see Documentation/filesystems/unionfs/issues.txt */
+                       lockdep_off();
                        err = vfs_unlink(lower_dir_dentry->d_inode,
                                         lower_dentry);
-               else
+                       lockdep_on();
+               } else
                        err = -EROFS;
                /* if vfs_unlink succeeded, update our inode's times */
                if (!err)
@@ -163,10 +166,13 @@ static int unionfs_do_rmdir(struct inode *dir, struct dentry *dentry)
                /* avoid destroying the lower inode if the file is in use */
                dget(lower_dentry);
                err = is_robranch_super(dentry->d_sb, bindex);
-               if (!err)
+               if (!err) {
+                       /* see Documentation/filesystems/unionfs/issues.txt */
+                       lockdep_off();
                        err = vfs_rmdir(lower_dir_dentry->d_inode,
                                        lower_dentry);
-               else
+                       lockdep_on();
+               } else
                        err = -EROFS;
                dput(lower_dentry);