Received-SPF: pass (google.com: best guess record for domain of linux-ext4-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Date:   Mon, 23 Dec 2019 15:53:41 +0800
From:   Murphy Zhou <jencce.kernel@gmail.com>
To:     "Theodore Y. Ts'o" <tytso@mit.edu>
Cc:     Murphy Zhou <jencce.kernel@gmail.com>, linux-ext4@vger.kernel.org,
        Jan Kara <jack@suse.cz>
Subject: Re: [PATCH] ext4: ensure revoke credits when set xattr
Message-ID: <20191223075341.wrg4xzn3vp3kugol@xzhoux.usersys.redhat.com>
References: <20191221105508.nrvonawwtz5a6bfz@xzhoux.usersys.redhat.com>
 <20191222020627.GA108990@mit.edu>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20191222020627.GA108990@mit.edu>
Sender: linux-ext4-owner@vger.kernel.org
Precedence: bulk

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