[PATCH] ext4: ensure revoke credits when set xattr

[PATCH] ext4: ensure revoke credits when set xattr

[Murphy Zhou]

It is possible that we need to release and forget blocks
during set xattr block, especially with 128 inode size,
so we need enough revoke credits to do that. Or we'll
hit WARNING since commit:
	[83448bd] ext4: Reserve revoke credits for freed blocks

This can be triggered easily in a kinda corner case:
--------------

namegen()
{
	echo "fstest_`dd if=/dev/urandom bs=1k count=1 2>/dev/null | md5sum  | cut -c -10`"
}

md0="/`namegen`"
d0=`namegen`
d1=`namegen`
d2=`namegen`

fallocate -l 200m test.img
mkfs.ext4 -F -b 4096 -I 128 test.img
mkdir -p $md0
mount -o loop test.img $md0 || exit
pushd $md0

mkdir ${d0}
setfacl -d -m 'u::rwx' ${d0}
mkdir ${d0}/${d1} # hit warning
echo $?
mkdir ${d0}/${d2}
rm -rf ${d0}

popd
umount -d $md0
rm -rf $md0 test.img
--------------

Which is derived from the pjd test suite[1].

Patch tested by xfstests auto group.

[1] https://sourceforge.net/p/ntfs-3g/pjd-fstest/ci/master/tree/

Signed-off-by: Murphy Zhou <jencce.kernel@gmail.com>
---
 fs/ext4/xattr.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/fs/ext4/xattr.c b/fs/ext4/xattr.c
index 8966a54..5c32c54 100644
--- a/fs/ext4/xattr.c
+++ b/fs/ext4/xattr.c
@@ -2319,6 +2319,12 @@ static struct buffer_head *ext4_xattr_get_block(struct inode *inode)
 			error = -ENOSPC;
 			goto cleanup;
 		}
+		error = ext4_journal_ensure_credits(handle, credits,
+				ext4_trans_default_revoke_credits(inode->i_sb));
+		if (error < 0) {
+			EXT4_ERROR_INODE(inode, "ensure credits (error %d)", error);
+			goto cleanup;
+		}
 	}
 
 	error = ext4_reserve_inode_write(handle, inode, &is.iloc);
-- 
1.8.3.1

Re: [PATCH] ext4: ensure revoke credits when set xattr

[Theodore Y. Ts'o]

On Sat, Dec 21, 2019 at 07:34:28PM +0800, Murphy Zhou wrote:
> It is possible that we need to release and forget blocks
> during set xattr block, especially with 128 inode size,
> so we need enough revoke credits to do that. Or we'll
> hit WARNING since commit:
> 	[83448bd] ext4: Reserve revoke credits for freed blocks
> 
> This can be triggered easily in a kinda corner case...

Thanks for reporting the problem.  However, your fix isn't quite
correct.  The problem is that ext4_journal_ensure_credits() ultimately
calls jbd2_journal_extend(), which has the following documentation.

/**
 * int jbd2_journal_extend() - extend buffer credits.
 * @handle:  handle to 'extend'
 * @nblocks: nr blocks to try to extend by.
 * @revoke_records: number of revoke records to try to extend by.
 *
 * Some transactions, such as large extends and truncates, can be done
 * atomically all at once or in several stages.  The operation requests
 * a credit for a number of buffer modifications in advance, but can
 * extend its credit if it needs more.
 *
 * jbd2_journal_extend tries to give the running handle more buffer credits.
 * It does not guarantee that allocation - this is a best-effort only.
 * The calling process MUST be able to deal cleanly with a failure to
 * extend here.

> +		error = ext4_journal_ensure_credits(handle, credits,
> +				ext4_trans_default_revoke_credits(inode->i_sb));
> +		if (error < 0) {
> +			EXT4_ERROR_INODE(inode, "ensure credits (error %d)", error);
> +			goto cleanup;
> +		}

Calling ext4_error() is not dealing cleanly with failure; doing this
is tricky (see how we do it for truncate) since some change may have
already been made to the file system via the current handle, and
keeping the file system consistent requires a lot of careful design
work.

Fortunately, there's a simpler way to do this.  All we need to do is
to do is to start the handle with the necessary revoke credits:

diff --git a/fs/ext4/xattr.c b/fs/ext4/xattr.c
index 8966a5439a22..c4ae268d5dcb 100644
--- a/fs/ext4/xattr.c
+++ b/fs/ext4/xattr.c
@@ -2475,7 +2475,8 @@ ext4_xattr_set(struct inode *inode, int name_index, const char *name,
 	if (error)
 		return error;
 
-	handle = ext4_journal_start(inode, EXT4_HT_XATTR, credits);
+	handle = ext4_journal_start_with_revoke(inode, EXT4_HT_XATTR, credits,
+			ext4_trans_default_revoke_credits(inode->i_sb));
 	if (IS_ERR(handle)) {
 		error = PTR_ERR(handle);
 	} else {

The other problem is that I'm not able to reproduce the failure using
your shell script.  What version of the kernel were you using, and was
there any thing special needed to trigger the complaint?

      	  		       	  	  - Ted

Re: [PATCH] ext4: ensure revoke credits when set xattr

[Murphy Zhou]

Hi Ted,

On Sat, Dec 21, 2019 at 09:06:27PM -0500, Theodore Y. Ts'o wrote:
> On Sat, Dec 21, 2019 at 07:34:28PM +0800, Murphy Zhou wrote:
> > It is possible that we need to release and forget blocks
> > during set xattr block, especially with 128 inode size,
> > so we need enough revoke credits to do that. Or we'll
> > hit WARNING since commit:
> > 	[83448bd] ext4: Reserve revoke credits for freed blocks
> > 
> > This can be triggered easily in a kinda corner case...
> 
> Thanks for reporting the problem.  However, your fix isn't quite
> correct.  The problem is that ext4_journal_ensure_credits() ultimately
> calls jbd2_journal_extend(), which has the following documentation.
> 
> /**
>  * int jbd2_journal_extend() - extend buffer credits.
>  * @handle:  handle to 'extend'
>  * @nblocks: nr blocks to try to extend by.
>  * @revoke_records: number of revoke records to try to extend by.
>  *
>  * Some transactions, such as large extends and truncates, can be done
>  * atomically all at once or in several stages.  The operation requests
>  * a credit for a number of buffer modifications in advance, but can
>  * extend its credit if it needs more.
>  *
>  * jbd2_journal_extend tries to give the running handle more buffer credits.
>  * It does not guarantee that allocation - this is a best-effort only.
>  * The calling process MUST be able to deal cleanly with a failure to
>  * extend here.
> 
> > +		error = ext4_journal_ensure_credits(handle, credits,
> > +				ext4_trans_default_revoke_credits(inode->i_sb));
> > +		if (error < 0) {
> > +			EXT4_ERROR_INODE(inode, "ensure credits (error %d)", error);
> > +			goto cleanup;
> > +		}
> 
> Calling ext4_error() is not dealing cleanly with failure; doing this
> is tricky (see how we do it for truncate) since some change may have
> already been made to the file system via the current handle, and
> keeping the file system consistent requires a lot of careful design
> work.

Thanks very much for the reviewing and explaination. Much appreciate!
I did not notice this and consider about this.

> 
> Fortunately, there's a simpler way to do this.  All we need to do is
> to do is to start the handle with the necessary revoke credits:
> 
> diff --git a/fs/ext4/xattr.c b/fs/ext4/xattr.c
> index 8966a5439a22..c4ae268d5dcb 100644
> --- a/fs/ext4/xattr.c
> +++ b/fs/ext4/xattr.c
> @@ -2475,7 +2475,8 @@ ext4_xattr_set(struct inode *inode, int name_index, const char *name,
>  	if (error)
>  		return error;
>  
> -	handle = ext4_journal_start(inode, EXT4_HT_XATTR, credits);
> +	handle = ext4_journal_start_with_revoke(inode, EXT4_HT_XATTR, credits,
> +			ext4_trans_default_revoke_credits(inode->i_sb));
>  	if (IS_ERR(handle)) {
>  		error = PTR_ERR(handle);
>  	} else {
> 
> The other problem is that I'm not able to reproduce the failure using
> your shell script.  What version of the kernel were you using, and was
> there any thing special needed to trigger the complaint?

I was using latest Linus tree, and nothing special is needed except
the 128 bit inode size, which requires to find new block.

Aha, after your tag "ext4_for_linus_stable" has been merged into Linus
tree, I can't reproduce it either.

I guess it's fixed by:
	a70fd5ac2ea7 yangerkun ext4: reserve revoke credits in __ext4_new_inode
Becuase the warning i hitting is also in __ext4_new_inode code path.

Thanks!
Xiong

> 
>       	  		       	  	  - Ted