[rfc][patch] fs: block_dump missing dentry locking

[rfc][patch] fs: block_dump missing dentry locking

[Nick Piggin]

I think the block_dump output in __mark_inode_dirty is missing dentry locking.
Surely the i_dentry list can change any time, so we may not even *get* a
dentry there. If we do get one by chance, then it would appear to be able to
go away or get renamed at any time...

I've just gone the very-safe route and used the dcache helper to find this,
and done the full refcounting/locking thing. Also just moved it to its
own function to reduce clutter a bit.

Is my thinking correct?

Thanks,
Nick
---
 fs/fs-writeback.c |   41 ++++++++++++++++++++++++-----------------
 1 file changed, 24 insertions(+), 17 deletions(-)

Index: linux-2.6/fs/fs-writeback.c
===================================================================
--- linux-2.6.orig/fs/fs-writeback.c
+++ linux-2.6/fs/fs-writeback.c
@@ -64,6 +64,28 @@ static void writeback_release(struct bac
 	clear_bit(BDI_pdflush, &bdi->state);
 }
 
+static noinline void block_dump___mark_inode_dirty(struct inode *inode)
+{
+	if (inode->i_ino || strcmp(inode->i_sb->s_id, "bdev")) {
+		struct dentry *dentry;
+		const char *name = "?";
+
+		dentry = d_find_alias(inode);
+		if (dentry) {
+			spin_lock(&dentry->d_lock);
+			name = (const char *) dentry->d_name.name;
+		}
+		printk(KERN_DEBUG
+		       "%s(%d): dirtied inode %lu (%s) on %s\n",
+		       current->comm, task_pid_nr(current), inode->i_ino,
+		       name, inode->i_sb->s_id);
+		if (dentry) {
+			spin_unlock(&dentry->d_lock);
+			dput(dentry);
+		}
+	}
+}
+
 /**
  *	__mark_inode_dirty -	internal function
  *	@inode: inode to mark
@@ -114,23 +136,8 @@ void __mark_inode_dirty(struct inode *in
 	if ((inode->i_state & flags) == flags)
 		return;
 
-	if (unlikely(block_dump)) {
-		struct dentry *dentry = NULL;
-		const char *name = "?";
-
-		if (!list_empty(&inode->i_dentry)) {
-			dentry = list_entry(inode->i_dentry.next,
-					    struct dentry, d_alias);
-			if (dentry && dentry->d_name.name)
-				name = (const char *) dentry->d_name.name;
-		}
-
-		if (inode->i_ino || strcmp(inode->i_sb->s_id, "bdev"))
-			printk(KERN_DEBUG
-			       "%s(%d): dirtied inode %lu (%s) on %s\n",
-			       current->comm, task_pid_nr(current), inode->i_ino,
-			       name, inode->i_sb->s_id);
-	}
+	if (unlikely(block_dump))
+		block_dump___mark_inode_dirty(inode);
 
 	spin_lock(&inode_lock);
 	if ((inode->i_state & flags) != flags) {

Re: [rfc][patch] fs: block_dump missing dentry locking

[Josef Bacik]

On Thu, May 28, 2009 at 09:01:15AM +0200, Nick Piggin wrote:
> I think the block_dump output in __mark_inode_dirty is missing dentry locking.
> Surely the i_dentry list can change any time, so we may not even *get* a
> dentry there. If we do get one by chance, then it would appear to be able to
> go away or get renamed at any time...
> 
> I've just gone the very-safe route and used the dcache helper to find this,
> and done the full refcounting/locking thing. Also just moved it to its
> own function to reduce clutter a bit.
> 
> Is my thinking correct?
> 
> Thanks,
> Nick
> ---
>  fs/fs-writeback.c |   41 ++++++++++++++++++++++++-----------------
>  1 file changed, 24 insertions(+), 17 deletions(-)
> 
> Index: linux-2.6/fs/fs-writeback.c
> ===================================================================
> --- linux-2.6.orig/fs/fs-writeback.c
> +++ linux-2.6/fs/fs-writeback.c
> @@ -64,6 +64,28 @@ static void writeback_release(struct bac
>  	clear_bit(BDI_pdflush, &bdi->state);
>  }
>  
> +static noinline void block_dump___mark_inode_dirty(struct inode *inode)
> +{
> +	if (inode->i_ino || strcmp(inode->i_sb->s_id, "bdev")) {
> +		struct dentry *dentry;
> +		const char *name = "?";
> +
> +		dentry = d_find_alias(inode);
> +		if (dentry) {
> +			spin_lock(&dentry->d_lock);
> +			name = (const char *) dentry->d_name.name;
> +		}
> +		printk(KERN_DEBUG
> +		       "%s(%d): dirtied inode %lu (%s) on %s\n",
> +		       current->comm, task_pid_nr(current), inode->i_ino,
> +		       name, inode->i_sb->s_id);
> +		if (dentry) {
> +			spin_unlock(&dentry->d_lock);
> +			dput(dentry);
> +		}
> +	}
> +}
> +
>  /**
>   *	__mark_inode_dirty -	internal function
>   *	@inode: inode to mark
> @@ -114,23 +136,8 @@ void __mark_inode_dirty(struct inode *in
>  	if ((inode->i_state & flags) == flags)
>  		return;
>  
> -	if (unlikely(block_dump)) {
> -		struct dentry *dentry = NULL;
> -		const char *name = "?";
> -
> -		if (!list_empty(&inode->i_dentry)) {
> -			dentry = list_entry(inode->i_dentry.next,
> -					    struct dentry, d_alias);
> -			if (dentry && dentry->d_name.name)
> -				name = (const char *) dentry->d_name.name;
> -		}
> -
> -		if (inode->i_ino || strcmp(inode->i_sb->s_id, "bdev"))
> -			printk(KERN_DEBUG
> -			       "%s(%d): dirtied inode %lu (%s) on %s\n",
> -			       current->comm, task_pid_nr(current), inode->i_ino,
> -			       name, inode->i_sb->s_id);
> -	}
> +	if (unlikely(block_dump))
> +		block_dump___mark_inode_dirty(inode);
>  
>  	spin_lock(&inode_lock);
>  	if ((inode->i_state & flags) != flags) {

It looks correct to me, you need to hold the dcache_lock to access the i_dentry
list safely, which you accomplish with d_find_alias, and you need to hold the
d_lock in order to safely access d_name, so it all looks right.  Thanks,

Josef