[PATCH] ext4: More buffer head reference leaks

[PATCH] ext4: More buffer head reference leaks

[Curt Wohlgemuth]

After the patch I posted last week regarding buffer head ref leaks in
no-journal mode, I looked at all the code that uses buffer heads and
searched for more potential leaks.

The patch below fixes the issues I found; these can occur even when a
journal is present.

The change to inode.c fixes a double release if
ext4_journal_get_create_access() fails.

The changes to namei.c are more complicated.  add_dirent_to_buf() will
release the input buffer head EXCEPT when it returns -ENOSPC.  There are
some callers of this routine that don't always do the brelse() in the event
that -ENOSPC is returned.  Unfortunately, to put this fix into ext4_add_entry()
required capturing the return value of make_indexed_dir() and
add_dirent_to_buf().

I'd appreciate comments on these changes, in particular if I'm just missing
something obvious here.

       Signed-off-by: Curt Wohlgemuth <curtw@google.com>

---
diff -Naur orig/fs/ext4/inode.c new/fs/ext4/inode.c
--- orig/fs/ext4/inode.c	2009-07-14 11:19:01.000000000 -0700
+++ new/fs/ext4/inode.c	2009-07-14 11:51:42.000000000 -0700
@@ -758,8 +758,9 @@
 		BUFFER_TRACE(bh, "call get_create_access");
 		err = ext4_journal_get_create_access(handle, bh);
 		if (err) {
+			/* Don't brelse(bh) here; it's done in journal_forget()
+			 * below */
 			unlock_buffer(bh);
-			brelse(bh);
 			goto failed;
 		}

diff -Naur orig/fs/ext4/namei.c new/fs/ext4/namei.c
--- orig/fs/ext4/namei.c	2009-07-14 11:19:46.000000000 -0700
+++ new/fs/ext4/namei.c	2009-07-14 11:19:28.000000000 -0700
@@ -1498,12 +1498,14 @@

 	sb = dir->i_sb;
 	blocksize = sb->s_blocksize;
-	if (!dentry->d_name.len)
-		return -EINVAL;
+	if (!dentry->d_name.len) {
+		retval = -EINVAL;
+		goto out;
+	}
 	if (is_dx(dir)) {
 		retval = ext4_dx_add_entry(handle, dentry, inode);
 		if (!retval || (retval != ERR_BAD_DX_DIR))
-			return retval;
+			goto out;
 		EXT4_I(dir)->i_flags &= ~EXT4_INDEX_FL;
 		dx_fallback++;
 		ext4_mark_inode_dirty(handle, dir);
@@ -1512,23 +1514,31 @@
 	for (block = 0; block < blocks; block++) {
 		bh = ext4_bread(handle, dir, block, 0, &retval);
 		if(!bh)
-			return retval;
+			goto out;
 		retval = add_dirent_to_buf(handle, dentry, inode, NULL, bh);
 		if (retval != -ENOSPC)
-			return retval;
+			goto out;

 		if (blocks == 1 && !dx_fallback &&
-		    EXT4_HAS_COMPAT_FEATURE(sb, EXT4_FEATURE_COMPAT_DIR_INDEX))
-			return make_indexed_dir(handle, dentry, inode, bh);
+		    EXT4_HAS_COMPAT_FEATURE(sb, EXT4_FEATURE_COMPAT_DIR_INDEX)) {
+			retval = make_indexed_dir(handle, dentry, inode, bh);
+			if (retval == -ENOSPC)
+				brelse(bh);
+			goto out;
+		}
 		brelse(bh);
 	}
 	bh = ext4_append(handle, dir, &block, &retval);
 	if (!bh)
-		return retval;
+		goto out;
 	de = (struct ext4_dir_entry_2 *) bh->b_data;
 	de->inode = 0;
 	de->rec_len = ext4_rec_len_to_disk(blocksize, blocksize);
-	return add_dirent_to_buf(handle, dentry, inode, de, bh);
+	retval = add_dirent_to_buf(handle, dentry, inode, de, bh);
+	if (retval == -ENOSPC)
+		brelse(bh);
+out:
+	return retval;
 }

 /*
@@ -1657,7 +1667,8 @@
 	if (!de)
 		goto cleanup;
 	err = add_dirent_to_buf(handle, dentry, inode, de, bh);
-	bh = NULL;
+	if (err != -ENOSPC)
+		bh = NULL;
 	goto cleanup;

 journal_error:

Re: [PATCH] ext4: More buffer head reference leaks

[Aneesh Kumar K.V]

On Tue, Jul 14, 2009 at 01:58:29PM -0700, Curt Wohlgemuth wrote:
> After the patch I posted last week regarding buffer head ref leaks in
> no-journal mode, I looked at all the code that uses buffer heads and
> searched for more potential leaks.
> 
> The patch below fixes the issues I found; these can occur even when a
> journal is present.
> 
> The change to inode.c fixes a double release if
> ext4_journal_get_create_access() fails.
> 
> The changes to namei.c are more complicated.  add_dirent_to_buf() will
> release the input buffer head EXCEPT when it returns -ENOSPC.  There are
> some callers of this routine that don't always do the brelse() in the event
> that -ENOSPC is returned.  Unfortunately, to put this fix into ext4_add_entry()
> required capturing the return value of make_indexed_dir() and
> add_dirent_to_buf().
> 
> I'd appreciate comments on these changes, in particular if I'm just missing
> something obvious here.
> 
>        Signed-off-by: Curt Wohlgemuth <curtw@google.com>
> 
> ---
> diff -Naur orig/fs/ext4/inode.c new/fs/ext4/inode.c
> --- orig/fs/ext4/inode.c	2009-07-14 11:19:01.000000000 -0700
> +++ new/fs/ext4/inode.c	2009-07-14 11:51:42.000000000 -0700
> @@ -758,8 +758,9 @@
>  		BUFFER_TRACE(bh, "call get_create_access");
>  		err = ext4_journal_get_create_access(handle, bh);
>  		if (err) {
> +			/* Don't brelse(bh) here; it's done in journal_forget()
> +			 * below */
>  			unlock_buffer(bh);
> -			brelse(bh);
>  			goto failed;
>  		}
> 

I am not able to find the journal_foget call in the path. brelse is
dropping the buffer_head reference got from sb_getblk right ? Can you
tell me what is that i am missing ?


> diff -Naur orig/fs/ext4/namei.c new/fs/ext4/namei.c
> --- orig/fs/ext4/namei.c	2009-07-14 11:19:46.000000000 -0700
> +++ new/fs/ext4/namei.c	2009-07-14 11:19:28.000000000 -0700
> @@ -1498,12 +1498,14 @@

.. snip..

-aneesh

Re: [PATCH] ext4: More buffer head reference leaks

[Aneesh Kumar K.V]

> 
> diff -Naur orig/fs/ext4/namei.c new/fs/ext4/namei.c
> --- orig/fs/ext4/namei.c	2009-07-14 11:19:46.000000000 -0700
> +++ new/fs/ext4/namei.c	2009-07-14 11:19:28.000000000 -0700
> @@ -1498,12 +1498,14 @@
> 
>  	sb = dir->i_sb;
>  	blocksize = sb->s_blocksize;
> -	if (!dentry->d_name.len)
> -		return -EINVAL;
> +	if (!dentry->d_name.len) {
> +		retval = -EINVAL;
> +		goto out;
> +	}
>

Do we really need those goto changes. We just return even at the label "out".
Seperating that out of the patch makes review easier

-aneesh

Re: [PATCH] ext4: More buffer head reference leaks

[Curt Wohlgemuth]

On Tue, Jul 14, 2009 at 10:52 PM, Aneesh Kumar
K.V<aneesh.kumar@linux.vnet.ibm.com> wrote:
> On Tue, Jul 14, 2009 at 01:58:29PM -0700, Curt Wohlgemuth wrote:
>> After the patch I posted last week regarding buffer head ref leaks in
>> no-journal mode, I looked at all the code that uses buffer heads and
>> searched for more potential leaks.
>>
>> The patch below fixes the issues I found; these can occur even when a
>> journal is present.
>>
>> The change to inode.c fixes a double release if
>> ext4_journal_get_create_access() fails.
>>
>> The changes to namei.c are more complicated.  add_dirent_to_buf() will
>> release the input buffer head EXCEPT when it returns -ENOSPC.  There are
>> some callers of this routine that don't always do the brelse() in the event
>> that -ENOSPC is returned.  Unfortunately, to put this fix into ext4_add_entry()
>> required capturing the return value of make_indexed_dir() and
>> add_dirent_to_buf().
>>
>> I'd appreciate comments on these changes, in particular if I'm just missing
>> something obvious here.
>>
>>        Signed-off-by: Curt Wohlgemuth <curtw@google.com>
>>
>> ---
>> diff -Naur orig/fs/ext4/inode.c new/fs/ext4/inode.c
>> --- orig/fs/ext4/inode.c      2009-07-14 11:19:01.000000000 -0700
>> +++ new/fs/ext4/inode.c       2009-07-14 11:51:42.000000000 -0700
>> @@ -758,8 +758,9 @@
>>               BUFFER_TRACE(bh, "call get_create_access");
>>               err = ext4_journal_get_create_access(handle, bh);
>>               if (err) {
>> +                     /* Don't brelse(bh) here; it's done in journal_forget()
>> +                      * below */
>>                       unlock_buffer(bh);
>> -                     brelse(bh);
>>                       goto failed;
>>               }
>>
>
> I am not able to find the journal_foget call in the path. brelse is
> dropping the buffer_head reference got from sb_getblk right ? Can you
> tell me what is that i am missing ?

Look at the code at the "failed" label.  For each of the allocated BHs
thus far, there's a call to ext4_journal_forget().

Oops, I should have put "ext4_journal_forget()" in the comment; my apologies.

I'll resend this out, along with a change to ext4_add_entry() to
remove the gotos.

Thanks,
Curt

>
>
>> diff -Naur orig/fs/ext4/namei.c new/fs/ext4/namei.c
>> --- orig/fs/ext4/namei.c      2009-07-14 11:19:46.000000000 -0700
>> +++ new/fs/ext4/namei.c       2009-07-14 11:19:28.000000000 -0700
>> @@ -1498,12 +1498,14 @@
>
> .. snip..
>
> -aneesh
>
--
To unsubscribe from this list: send the line "unsubscribe linux-ext4" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] ext4: More buffer head reference leaks

[Curt Wohlgemuth]

I've cleaned up the patch to
   - fix the comment in ext4_alloc_branch()
   - change ext4_add_entry() to remove the gotos and label


       Signed-off-by: Curt Wohlgemuth <curtw@google.com>

---
diff -Naur orig/fs/ext4/inode.c new/fs/ext4/inode.c
--- orig/fs/ext4/inode.c	2009-07-14 11:19:01.000000000 -0700
+++ new/fs/ext4/inode.c	2009-07-15 08:12:15.000000000 -0700
@@ -758,8 +758,9 @@
 		BUFFER_TRACE(bh, "call get_create_access");
 		err = ext4_journal_get_create_access(handle, bh);
 		if (err) {
+			/* Don't brelse(bh) here; it's done in
+			 * ext4_journal_forget() below */
 			unlock_buffer(bh);
-			brelse(bh);
 			goto failed;
 		}

diff -Naur orig/fs/ext4/namei.c new/fs/ext4/namei.c
--- orig/fs/ext4/namei.c	2009-07-14 11:19:46.000000000 -0700
+++ new/fs/ext4/namei.c	2009-07-15 08:12:09.000000000 -0700
@@ -1518,8 +1518,12 @@
 			return retval;

 		if (blocks == 1 && !dx_fallback &&
-		    EXT4_HAS_COMPAT_FEATURE(sb, EXT4_FEATURE_COMPAT_DIR_INDEX))
-			return make_indexed_dir(handle, dentry, inode, bh);
+		    EXT4_HAS_COMPAT_FEATURE(sb, EXT4_FEATURE_COMPAT_DIR_INDEX)) {
+			retval = make_indexed_dir(handle, dentry, inode, bh);
+			if (retval == -ENOSPC)
+				brelse(bh);
+			return retval;
+		}
 		brelse(bh);
 	}
 	bh = ext4_append(handle, dir, &block, &retval);
@@ -1528,7 +1532,10 @@
 	de = (struct ext4_dir_entry_2 *) bh->b_data;
 	de->inode = 0;
 	de->rec_len = ext4_rec_len_to_disk(blocksize, blocksize);
-	return add_dirent_to_buf(handle, dentry, inode, de, bh);
+	retval = add_dirent_to_buf(handle, dentry, inode, de, bh);
+	if (retval == -ENOSPC)
+		brelse(bh);
+	return retval;
 }

 /*
@@ -1657,7 +1664,8 @@
 	if (!de)
 		goto cleanup;
 	err = add_dirent_to_buf(handle, dentry, inode, de, bh);
-	bh = NULL;
+	if (err != -ENOSPC)
+		bh = NULL;
 	goto cleanup;

 journal_error:

Re: [PATCH] ext4: More buffer head reference leaks

[Curt Wohlgemuth]

Any comment on the patch below?  Ted?

Thanks,
Curt


On Wed, Jul 15, 2009 at 8:53 AM, Curt Wohlgemuth<curtw@google.com> wrote:
> I've cleaned up the patch to
>   - fix the comment in ext4_alloc_branch()
>   - change ext4_add_entry() to remove the gotos and label
>
>
>       Signed-off-by: Curt Wohlgemuth <curtw@google.com>
>
> ---
> diff -Naur orig/fs/ext4/inode.c new/fs/ext4/inode.c
> --- orig/fs/ext4/inode.c        2009-07-14 11:19:01.000000000 -0700
> +++ new/fs/ext4/inode.c 2009-07-15 08:12:15.000000000 -0700
> @@ -758,8 +758,9 @@
>                BUFFER_TRACE(bh, "call get_create_access");
>                err = ext4_journal_get_create_access(handle, bh);
>                if (err) {
> +                       /* Don't brelse(bh) here; it's done in
> +                        * ext4_journal_forget() below */
>                        unlock_buffer(bh);
> -                       brelse(bh);
>                        goto failed;
>                }
>
> diff -Naur orig/fs/ext4/namei.c new/fs/ext4/namei.c
> --- orig/fs/ext4/namei.c        2009-07-14 11:19:46.000000000 -0700
> +++ new/fs/ext4/namei.c 2009-07-15 08:12:09.000000000 -0700
> @@ -1518,8 +1518,12 @@
>                        return retval;
>
>                if (blocks == 1 && !dx_fallback &&
> -                   EXT4_HAS_COMPAT_FEATURE(sb, EXT4_FEATURE_COMPAT_DIR_INDEX))
> -                       return make_indexed_dir(handle, dentry, inode, bh);
> +                   EXT4_HAS_COMPAT_FEATURE(sb, EXT4_FEATURE_COMPAT_DIR_INDEX)) {
> +                       retval = make_indexed_dir(handle, dentry, inode, bh);
> +                       if (retval == -ENOSPC)
> +                               brelse(bh);
> +                       return retval;
> +               }
>                brelse(bh);
>        }
>        bh = ext4_append(handle, dir, &block, &retval);
> @@ -1528,7 +1532,10 @@
>        de = (struct ext4_dir_entry_2 *) bh->b_data;
>        de->inode = 0;
>        de->rec_len = ext4_rec_len_to_disk(blocksize, blocksize);
> -       return add_dirent_to_buf(handle, dentry, inode, de, bh);
> +       retval = add_dirent_to_buf(handle, dentry, inode, de, bh);
> +       if (retval == -ENOSPC)
> +               brelse(bh);
> +       return retval;
>  }
>
>  /*
> @@ -1657,7 +1664,8 @@
>        if (!de)
>                goto cleanup;
>        err = add_dirent_to_buf(handle, dentry, inode, de, bh);
> -       bh = NULL;
> +       if (err != -ENOSPC)
> +               bh = NULL;
>        goto cleanup;
>
>  journal_error:
>
--
To unsubscribe from this list: send the line "unsubscribe linux-ext4" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] ext4: More buffer head reference leaks

[Theodore Tso]

On Fri, Jul 17, 2009 at 12:55:49PM -0700, Curt Wohlgemuth wrote:
> Any comment on the patch below?  Ted?

oops, sorry for send a reply; I added it to the ext4 patch queue this morning.

      	    	       	      	      - Ted