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
