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:   Sat, 21 Dec 2019 21:06:27 -0500
From:   "Theodore Y. Ts'o" <tytso@mit.edu>
To:     Murphy Zhou <jencce.kernel@gmail.com>
Cc:     linux-ext4@vger.kernel.org, Jan Kara <jack@suse.cz>
Subject: Re: [PATCH] ext4: ensure revoke credits when set xattr
Message-ID: <20191222020627.GA108990@mit.edu>
References: <20191221105508.nrvonawwtz5a6bfz@xzhoux.usersys.redhat.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20191221105508.nrvonawwtz5a6bfz@xzhoux.usersys.redhat.com>
User-Agent: Mutt/1.12.2 (2019-09-21)
Sender: linux-ext4-owner@vger.kernel.org
Precedence: bulk

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