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@xxxxxxxxxx> >> >> 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@xxxxxxxxxxxxx> >> Suggested-by: Dave Chinner <david@xxxxxxxxxxxxx> >> Link: https://lore.kernel.org/linux-ext4/493ab4c5-505c-a351-eefa-7d2677cdf800@xxxxxxxxxxxxxxx/T/#m6a14df5d085527a188c5a151191e87a3252dc4e2 >> Signed-off-by: Zhang Yi <yi.zhang@xxxxxxxxxx> > > 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.
