Received-SPF: pass (google.com: domain of linux-ext4+bounces-200-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45d1:ec00::1 as permitted sender) client-ip=2604:1380:45d1:ec00::1;
Date: Mon, 27 Nov 2023 17:11:49 +0100
From: Jan Kara <jack@suse.cz>
To: Zhang Yi <yi.zhang@huaweicloud.com>
Cc: linux-ext4@vger.kernel.org, tytso@mit.edu, adilger.kernel@dilger.ca,
	jack@suse.cz, yi.zhang@huawei.com, chengzhihao1@huawei.com,
	yukuai3@huawei.com
Subject: Re: [PATCH 2/2] jbd2: increase the journal IO's priority
Message-ID: <20231127161149.2s7yizu72gw332s2@quack3>
References: <20231125121740.1035816-1-yi.zhang@huaweicloud.com>
 <20231125121740.1035816-2-yi.zhang@huaweicloud.com>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20231125121740.1035816-2-yi.zhang@huaweicloud.com>

Hello!

On Sat 25-11-23 20:17:39, Zhang Yi wrote:
> From: Zhang Yi <yi.zhang@huawei.com>
> 
> Current jbd2 only add REQ_SYNC for descriptor block, metadata log
> buffer, commit buffer and superblock buffer, the submitted IO could be
> throttled by writeback throttle in block layer, that could lead to
> priority inversion in some cases. The log IO looks like a kind of high
> priority metadata IO, so it should not be throttled by WBT like QOS
> policies in block layer, let's add REQ_SYNC | REQ_IDLE to exempt from
> writeback throttle, and also add REQ_META together indicates it's a
> metadata IO.

So I agree about REQ_META but with REQ_IDLE I'm not so sure. __REQ_IDLE is
documented as "anticipate more IO after this one" so that is a good fit for
normal transaction writes but not so much for journal superblock writes.
OTOH as far as I'm checking XFS uses REQ_IDLE as well for its log IO and
the only places where REQ_IDLE is really checked is in blk-wbt... So I
guess this makes sense.

> diff --git a/include/linux/jbd2.h b/include/linux/jbd2.h
> index 52772c826c86..f7e8274b46ae 100644
> --- a/include/linux/jbd2.h
> +++ b/include/linux/jbd2.h
> @@ -1374,6 +1374,9 @@ JBD2_FEATURE_INCOMPAT_FUNCS(csum2,		CSUM_V2)
>  JBD2_FEATURE_INCOMPAT_FUNCS(csum3,		CSUM_V3)
>  JBD2_FEATURE_INCOMPAT_FUNCS(fast_commit,	FAST_COMMIT)
>  
> +/* Journal high priority write IO operation flags */
> +#define JBD2_REQ_HIPRIO		(REQ_META | REQ_SYNC | REQ_IDLE)

Since all journal IO is submitted with these flags I'd maybe name this
JBD2_JOURNAL_REQ_FLAGS? Otherwise the patch looks good to me so feel free
to add:

Reviewed-by: Jan Kara <jack@suse.cz>

								Honza
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR