From: Erez Zadok <ezk@cs.sunysb.edu>
To: akpm@linux-foundation.org
Cc: linux-kernel@vger.kernel.org, linux-fsdevel@vger.kernel.org,
viro@ftp.linux.org.uk, hch@infradead.org,
Erez Zadok <ezk@cs.sunysb.edu>
Subject: [PATCH 19/30] Unionfs: prevent false lockdep warnings in stacking
Date: Fri, 28 Dec 2007 15:42:53 -0500 [thread overview]
Message-ID: <1198874594640-git-send-email-ezk@cs.sunysb.edu> (raw)
In-Reply-To: <11988745841003-git-send-email-ezk@cs.sunysb.edu>
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>
---
Documentation/filesystems/unionfs/issues.txt | 10 +++++++---
fs/unionfs/copyup.c | 3 +++
fs/unionfs/inode.c | 21 +++++++++++++++++++--
fs/unionfs/rename.c | 21 +++++++++++----------
fs/unionfs/super.c | 4 ++++
fs/unionfs/unlink.c | 12 ++++++++++--
6 files changed, 54 insertions(+), 17 deletions(-)
diff --git a/Documentation/filesystems/unionfs/issues.txt b/Documentation/filesystems/unionfs/issues.txt
index bb6ab05..f4b7e7e 100644
--- 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 f48209f..0012caf 100644
--- 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 3df9b19..4890f42 100644
--- a/fs/unionfs/inode.c
+++ b/fs/unionfs/inode.c
@@ -80,7 +80,10 @@ static int unionfs_create(struct inode *parent, struct dentry *dentry,
struct dentry *lower_dir_dentry;
lower_dir_dentry = lock_parent(wh_dentry);
+ /* see Documentation/filesystems/unionfs/issues.txt */
+ lockdep_off();
err = vfs_unlink(lower_dir_dentry->d_inode, wh_dentry);
+ lockdep_on();
unlock_dir(lower_dir_dentry);
/*
@@ -262,9 +265,13 @@ static int unionfs_link(struct dentry *old_dentry, struct inode *dir,
/* found a .wh.foo entry, unlink it and then call vfs_link() */
lower_dir_dentry = lock_parent(whiteout_dentry);
err = is_robranch_super(new_dentry->d_sb, dbstart(new_dentry));
- if (!err)
+ if (!err) {
+ /* see Documentation/filesystems/unionfs/issues.txt */
+ lockdep_off();
err = vfs_unlink(lower_dir_dentry->d_inode,
whiteout_dentry);
+ lockdep_on();
+ }
fsstack_copy_attr_times(dir, lower_dir_dentry->d_inode);
dir->i_nlink = unionfs_get_nlinks(dir);
@@ -291,9 +298,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:
@@ -316,10 +327,16 @@ docopyup:
unionfs_lower_dentry(old_dentry);
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 452d1e7..8b04acf 100644
--- a/fs/unionfs/rename.c
+++ b/fs/unionfs/rename.c
@@ -90,16 +90,14 @@ static int __unionfs_rename(struct inode *old_dir, struct dentry *old_dentry,
dput(lower_wh_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);
- lock_rename(lower_old_dir_dentry, lower_new_dir_dentry);
-
- err = is_robranch_super(old_dentry->d_sb, bindex);
- if (err)
- goto out_unlock;
-
/*
* ready to whiteout for old_dentry. caller will create the actual
* whiteout, and must dput(*wh_old)
@@ -110,7 +108,7 @@ static int __unionfs_rename(struct inode *old_dir, struct dentry *old_dentry,
old_dentry->d_name.len);
err = PTR_ERR(whname);
if (unlikely(IS_ERR(whname)))
- goto out_unlock;
+ goto out_dput;
*wh_old = lookup_one_len(whname, lower_old_dir_dentry,
old_dentry->d_name.len +
UNIONFS_WHLEN);
@@ -118,16 +116,19 @@ static int __unionfs_rename(struct inode *old_dir, struct dentry *old_dentry,
err = PTR_ERR(*wh_old);
if (IS_ERR(*wh_old)) {
*wh_old = NULL;
- goto out_unlock;
+ goto out_dput;
}
}
+ /* see Documentation/filesystems/unionfs/issues.txt */
+ lockdep_off();
+ lock_rename(lower_old_dir_dentry, lower_new_dir_dentry);
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();
+out_dput:
dput(lower_old_dir_dentry);
dput(lower_new_dir_dentry);
dput(lower_old_dentry);
diff --git a/fs/unionfs/super.c b/fs/unionfs/super.c
index 8b70aca..45bcf89 100644
--- a/fs/unionfs/super.c
+++ b/fs/unionfs/super.c
@@ -839,7 +839,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 423ff36..677a5ae 100644
--- a/fs/unionfs/unlink.c
+++ b/fs/unionfs/unlink.c
@@ -41,8 +41,12 @@ static int unionfs_unlink_whiteout(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);
+ lockdep_on();
+ }
/* if vfs_unlink succeeded, update our inode's times */
if (!err)
unionfs_copy_attr_times(dentry->d_inode);
@@ -139,8 +143,12 @@ static int unionfs_rmdir_first(struct inode *dir, struct dentry *dentry,
/* avoid destroying the lower inode if the file is in use */
dget(lower_dentry);
err = is_robranch(dentry);
- if (!err)
+ if (!err) {
+ /* see Documentation/filesystems/unionfs/issues.txt */
+ lockdep_off();
err = vfs_rmdir(lower_dir_dentry->d_inode, lower_dentry);
+ lockdep_on();
+ }
dput(lower_dentry);
fsstack_copy_attr_times(dir, lower_dir_dentry->d_inode);
--
1.5.2.2
next prev parent reply other threads:[~2007-12-28 20:46 UTC|newest]
Thread overview: 31+ messages / expand[flat|nested] mbox.gz Atom feed top
2007-12-28 20:42 [GIT PULL -mm] 00/30 Unionfs+fsstack updates/fixes/cleanups Erez Zadok
2007-12-28 20:42 ` [PATCH 01/30] VFS/fs_stack: drop cast on inode passed to i_size_read Erez Zadok
2007-12-28 20:42 ` [PATCH 02/30] VFS/fs_stack: use locking around i_size_write in 32-bit systems Erez Zadok
2007-12-28 20:42 ` [PATCH 03/30] Unionfs: " Erez Zadok
2007-12-28 20:42 ` [PATCH 04/30] Unionfs: clarify usage.txt read/write behavior Erez Zadok
2007-12-28 20:42 ` [PATCH 05/30] Unionfs: interpose cleanup and fix for spliced dentries Erez Zadok
2007-12-28 20:42 ` [PATCH 06/30] Unionfs: initialize inode times for reused inodes Erez Zadok
2007-12-28 20:42 ` [PATCH 07/30] Unionfs: create new special files only in first branch Erez Zadok
2007-12-28 20:42 ` [PATCH 08/30] Unionfs: create new symlinks " Erez Zadok
2007-12-28 20:42 ` [PATCH 09/30] Unionfs: release special files on copyup Erez Zadok
2007-12-28 20:42 ` [PATCH 10/30] Unionfs: mmap fixes Erez Zadok
2007-12-28 20:42 ` [PATCH 11/30] Unionfs: restructure unionfs_setattr and fix truncation order Erez Zadok
2007-12-28 20:42 ` [PATCH 12/30] Unionfs: remove custom read/write methods Erez Zadok
2007-12-28 20:42 ` [PATCH 13/30] Unionfs: prevent deadlock in cache coherency Erez Zadok
2007-12-28 20:42 ` [PATCH 14/30] Unionfs: remove unnecessary conditional inode lock Erez Zadok
2007-12-28 20:42 ` [PATCH 15/30] Unionfs: remove unnecessary lock when deleting whiteouts Erez Zadok
2007-12-28 20:42 ` [PATCH 16/30] Unionfs: remove unnecessary lock in read_inode Erez Zadok
2007-12-28 20:42 ` [PATCH 17/30] Unionfs: remove unnecessary locking in follow-link Erez Zadok
2007-12-28 20:42 ` [PATCH 18/30] Unionfs: remove unnecessary parent lock in create Erez Zadok
2007-12-28 20:42 ` Erez Zadok [this message]
2007-12-28 20:42 ` [PATCH 20/30] Unionfs: implement lockdep classes Erez Zadok
2007-12-28 20:42 ` [PATCH 21/30] Unionfs: minor code rearrangement in rename Erez Zadok
2007-12-28 20:42 ` [PATCH 22/30] Unionfs: handle on lower inodes in lookup Erez Zadok
2007-12-28 20:42 ` [PATCH 23/30] Unionfs: set our superblock a/m/ctime granularity Erez Zadok
2007-12-28 20:42 ` [PATCH 24/30] Unionfs: update inode times after a successful open Erez Zadok
2007-12-28 20:42 ` [PATCH 25/30] Unionfs: minor cleanup in check_empty Erez Zadok
2007-12-28 20:43 ` [PATCH 26/30] Unionfs: initialize namelist variable in rename Erez Zadok
2007-12-28 20:43 ` [PATCH 27/30] Unionfs: cleanup lower inodes after successful unlink Erez Zadok
2007-12-28 20:43 ` [PATCH 28/30] Unionfs: don't check dentry on error Erez Zadok
2007-12-28 20:43 ` [PATCH 29/30] Unionfs: implement d_iput method Erez Zadok
2007-12-28 20:43 ` [PATCH 30/30] Unionfs: don't check parent dentries Erez Zadok
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=1198874594640-git-send-email-ezk@cs.sunysb.edu \
--to=ezk@cs.sunysb.edu \
--cc=akpm@linux-foundation.org \
--cc=hch@infradead.org \
--cc=linux-fsdevel@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=viro@ftp.linux.org.uk \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox
all inboxes | Powered by JetHome®