Received-SPF: pass (google.com: domain of linux-ext4+bounces-2536-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45e3:2400::1 as permitted sender) client-ip=2604:1380:45e3:2400::1;
Subject: Re: [PATCH] ext4/jbd2: drop jbd2_transaction_committed()
To: Jan Kara <jack@suse.cz>
Cc: linux-ext4@vger.kernel.org, linux-fsdevel@vger.kernel.org,
 linux-kernel@vger.kernel.org, tytso@mit.edu, adilger.kernel@dilger.ca,
 ritesh.list@gmail.com, yi.zhang@huawei.com, chengzhihao1@huawei.com,
 yukuai3@huawei.com
References: <20240513072119.2335346-1-yi.zhang@huaweicloud.com>
 <20240515002513.yaglghza4i4ldmr5@quack3>
From: Zhang Yi <yi.zhang@huaweicloud.com>
Message-ID: <f0eb115d-dd10-e156-9aed-65b7f479f008@huaweicloud.com>
Date: Thu, 16 May 2024 16:27:25 +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: <20240515002513.yaglghza4i4ldmr5@quack3>
Content-Type: text/plain; charset=utf-8
Content-Language: en-US
Content-Transfer-Encoding: 7bit

On 2024/5/15 8:25, Jan Kara wrote:
> On Mon 13-05-24 15:21:19, Zhang Yi wrote:
>> From: Zhang Yi <yi.zhang@huawei.com>
>>
>> jbd2_transaction_committed() is used to check whether a transaction with
>> the given tid has already committed, it hold j_state_lock in read mode
>> and check the tid of current running transaction and committing
>> transaction, but holding the j_state_lock is expensive.
>>
>> We have already stored the sequence number of the most recently
>> committed transaction in journal t->j_commit_sequence, we could do this
>> check by comparing it with the given tid instead. If the given tid isn't
>> smaller than j_commit_sequence, we can ensure that the given transaction
>> has been committed. That way we could drop the expensive lock and
>> achieve about 10% ~ 20% performance gains in concurrent DIOs on may
>> virtual machine with 100G ramdisk.
>>
>> fio -filename=/mnt/foo -direct=1 -iodepth=10 -rw=$rw -ioengine=libaio \
>>     -bs=4k -size=10G -numjobs=10 -runtime=60 -overwrite=1 -name=test \
>>     -group_reporting
>>
>> Before:
>>   overwrite       IOPS=88.2k, BW=344MiB/s
>>   read            IOPS=95.7k, BW=374MiB/s
>>   rand overwrite  IOPS=98.7k, BW=386MiB/s
>>   randread        IOPS=102k, BW=397MiB/s
>>
>> After:
>>   verwrite:       IOPS=105k, BW=410MiB/s
>>   read:           IOPS=112k, BW=436MiB/s
>>   rand overwrite: IOPS=104k, BW=404MiB/s
>>   randread:       IOPS=111k, BW=432MiB/s
>>
>> CC: Dave Chinner <david@fromorbit.com>
>> Suggested-by: Dave Chinner <david@fromorbit.com>
>> Link: https://lore.kernel.org/linux-ext4/493ab4c5-505c-a351-eefa-7d2677cdf800@huaweicloud.com/T/#m6a14df5d085527a188c5a151191e87a3252dc4e2
>> Signed-off-by: Zhang Yi <yi.zhang@huawei.com>
> 
> I agree this is workable solution and the performance benefits are nice. But
> I have some comments regarding the implementation:
> 
>> @@ -3199,8 +3199,8 @@ static bool ext4_inode_datasync_dirty(struct inode *inode)
>>  	journal_t *journal = EXT4_SB(inode->i_sb)->s_journal;
>>  
>>  	if (journal) {
>> -		if (jbd2_transaction_committed(journal,
>> -			EXT4_I(inode)->i_datasync_tid))
>> +		if (tid_geq(journal->j_commit_sequence,
>> +			    EXT4_I(inode)->i_datasync_tid))
> 
> Please leave the helper jbd2_transaction_committed(), just make the
> implementation more efficient. 

Sure.

> Also accessing j_commit_sequence without any
> lock is theoretically problematic wrt compiler optimization. You should have
> READ_ONCE() there and the places modifying j_commit_sequence need to use
> WRITE_ONCE().
> 

Thanks for pointing this out, but I'm not sure if we have to need READ_ONCE()
here. IIUC, if we add READ_ONCE(), we could make sure to get the latest
j_commit_sequence, if not, there is a window (it might becomes larger) that
we could get the old value and jbd2_transaction_committed() could return false
even if the given transaction was just committed, but I think the window is
always there, so it looks like it is not a big problem, is that right?

Thanks,
Yi.