Add a refcount check in dput()
PATCH 1/1 Add a refcount check in dput()
Add a BUG_ON() to check for passing an unreferenced dentry to dput().
This is analogous to the similar check in dget(), and will make reference-counting bugs in filesystems more immediately obvious. (I just spent a while debugging an oops that turned out to be due to broken fs reference counting.)
--- fs/dcache.c | 1 + 1 file changed, 1 insertion(+)
fs/dcache.c +1 -0
@@ -176,6 +176,7 @@ void dput(struct dentry *dentry)if (!dentry)return ; +BUG_ON (!atomic_read (&dentry->d_count)); repeat:if (atomic_read (&dentry->d_count) ==1 )might_sleep ();
Quoted 18 lines · On Mon, 10 Sep 2007 15:13:19 -0700 (PDT) [email protected] (Paul Menage) wrote:
Add a
BUG_ON()to check for passing an unreferenced dentry todput().This is analogous to the similar check in
dget(), and will make reference-counting bugs in filesystems more immediately obvious. (I just spent a while debugging an oops that turned out to be due to broken fs reference counting.)Signed-off-by Paul Menage <[email protected]>--- fs/dcache.c | 1 + 1 file changed, 1 insertion(+)Index:
container-2.6.23-rc3-mm1/fs/dcache.c
===================================================================
fs/dcache.c void dput(struct dentry *dentry) · line 182
Quoted 7 lines
fs/dcache.c +1
-0@@ -176,6 +176,7 @@ void dput(struct dentry *dentry)if (!dentry)return ; +BUG_ON (!atomic_read (&dentry->d_count)); repeat:if (atomic_read (&dentry->d_count) ==1 )might_sleep ();
eek, much too aggressive.
I added this:
fs/dcache.c renamed from fs/dcache.c~add-a-refcount-check-in-dput +1 -0
@@ -176,6 +176,7 @@ void dput(struct dentry *dentry)if (!dentry)return ; +WARN_ON_ONCE (!atomic_read (&dentry->d_count)); repeat:if (atomic_read (&dentry->d_count) ==1 )might_sleep ();
_
to the -mm-only-debugging-patches section of the -mm tree.
Quoted 7 lines · Andrew Morton
Quoted 5 lines
+ BUG_ON(!atomic_read(&dentry->d_count));
repeat:if (atomic_read(&dentry->d_count) == 1) might_sleep();eek, much too aggressive.
How about the equivalent BUG_ON() in dget()? I figure that they ought to both be of the same strictness.
Paul
Quoted 11 lines · On Mon, 17 Sep 2007 11:21:35 -0700 "Paul Menage" <[email protected]> wrote:
Quoted 7 lines · Andrew Morton
Quoted 5 lines
+ BUG_ON(!atomic_read(&dentry->d_count));
repeat:if (atomic_read(&dentry->d_count) == 1) might_sleep();eek, much too aggressive.
How about the equivalent
BUG_ON()indget()? I figure that they ought to both be of the same strictness.
The one in dget() is known not to trigger - it's been there for a long time.
My problem with new BUG_ON's is that they get added thinking "no way will this trigger" and lo, they do trigger and lots of people get their testing disrupted for a whole release.
Long-standing checks like the one in dget() should perhaps be removed now that we know they don't trigger - move them under some CONFIG_DEBUG_foo option.