Received-SPF: pass (google.com: domain of linux-ext4+bounces-211-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45d1:ec00::1 as permitted sender) client-ip=2604:1380:45d1:ec00::1;
Subject: Re: [PATCH 2/2] jbd2: increase the journal IO's priority
To: Jan Kara <jack@suse.cz>
Cc: linux-ext4@vger.kernel.org, tytso@mit.edu, adilger.kernel@dilger.ca,
 yi.zhang@huawei.com, chengzhihao1@huawei.com, yukuai3@huawei.com
References: <20231125121740.1035816-1-yi.zhang@huaweicloud.com>
 <20231125121740.1035816-2-yi.zhang@huaweicloud.com>
 <20231127161149.2s7yizu72gw332s2@quack3>
From: Zhang Yi <yi.zhang@huaweicloud.com>
Message-ID: <44d18772-192c-46ad-2622-646b609752d6@huaweicloud.com>
Date: Tue, 28 Nov 2023 11:32:21 +0800
User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:78.0) Gecko/20100101
 Thunderbird/78.12.0
Precedence: bulk
MIME-Version: 1.0
In-Reply-To: <20231127161149.2s7yizu72gw332s2@quack3>
Content-Type: text/plain; charset=utf-8
Content-Language: en-US
Content-Transfer-Encoding: 7bit

Hello!

On 2023/11/28 0:11, Jan Kara wrote:
> 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.
> 

Thanks a lot for your review. Yeah, We've checked the block cgroup related
qos policies and blk-wbt, block cgroup based policies does not throttle IO
with REQ_META, and blk-wbt exempt IO with both REQ_IDLE and REQ_SYNC. But
submit_bh() can make sure the journal IO is always bind to the root cgroup,
so only blk-wbt is left for us to deal with, so I add REQ_IDLE like xfs does.

>> 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:
> 

Sure, JBD2_JOURNAL_REQ_FLAGS looks fine, I will use it in v2.

Thanks,
Yi.