Re: [PATCH][RESEND] fs: always set I_DIRTY_TIME to fsync correctly on lazytime

[Naohiro Aota]

Hello, all

What is the status of this patch? Can this be picked up for some tree?

Regards,
Naohiro

2016-11-01 7:46 GMT+09:00 Jan Kara <jack@xxxxxxx>:
> On Tue 01-11-16 04:02:45, Naohiro Aota wrote:
>> While lazytime states that "The on-disk timestamps are updated only
>> when: ... - the application employs fsync(2), syncfs(2), or sync(2)"
>> [1], it does not write a timestamp update on fsync().
>>
>> [1] http://manpages.ubuntu.com/manpages/xenial/man8/mount.8.html
>>
>> The following commands will reproduce the problem:
>>
>> $ mount -o noatime,lazytime ext4.img /mnt/tmp
>> $ cd /mnt/tmp
>> (create an 128M file to fio, not to observe size update)
>> $ dd if=/dev/zero of=wxyz.0.0 bs=1M count=128
>> (do write/fsync)
>> $ fio --name wxyz --direct=1 --buffered=0 --size=128m --bs=64k --rw=write \
>>   --ioengine=sync --numjobs=1 --fsync=5
>>
>> Since fio invokes 1 fsync per 5 writes, we should see rapid journal
>> commits for timestamp update by tracing jbd2:jbd2_end_commit trace
>> point. Only we can see are, however, some periodic (~5 sec) commits from
>> bdi flush like below.
>>
>> $ trace jbd2:jbd2_end_commit
>>     jbd2/loop0-8-1617  [002] ....    96.637351: jbd2_end_commit: dev 7,0 transaction 5393 sync 0 head 5393
>>     jbd2/loop0-8-1617  [000] ....   101.679411: jbd2_end_commit: dev 7,0 transaction 5394 sync 0 head 5393
>>     jbd2/loop0-8-1617  [003] ....   106.743628: jbd2_end_commit: dev 7,0 transaction 5395 sync 0 head 5393
>>     jbd2/loop0-8-1617  [001] ....   111.801964: jbd2_end_commit: dev 7,0 transaction 5396 sync 0 head 5393
>> ...
>>
>> The problem is __mark_inode_dirty() does not always flag I_DIRTY_TIME.
>> It seems that it is no use to mark an inode I_DIRTY_TIME when the inode
>> is already I_DIRTY_INODE. However, by that decision, we're skipping
>> journal write if we invoke two fsync()s between two bdi flushes. As the
>> following table shows, any fsync after the first fsync do nothing (if
>> there's no update other than timestamp).
>>
>> Event                | i_state      | journal
>> ---------------------+--------------+------------------------
>> <timestamp update>   | I_DIRTY_TIME | no write (lazytime)
>> <fsync>              | I_DIRTY_SYNC | write timestamp update
>> <timestamp update>   | I_DIRTY_SYNC | no write (lazytime)
>> <fsync>              | I_DIRTY_SYNC | no write *BUG*
>> ...
>> <bdi flush>          | 0            |
>> <timestamp update>   | I_DIRTY_TIME | no write (lazytime)
>> <fsync>              | I_DIRTY_SYNC | write timestamp update
>>
>> We should set I_DIRTY_TIME on the second timestamp update to let fsync()
>> notice there's a timestamp update after the last inode writeout.
>>
>> After this patch, we can see rapid trace of journal commit:
>> $ trace jbd2:jbd2_end_commit
>>     jbd2/loop0-8-1879  [002] ....   208.275057: jbd2_end_commit: dev 7,0 transaction 5364 sync 0 head 3343
>>     jbd2/loop0-8-1879  [000] ....   208.302539: jbd2_end_commit: dev 7,0 transaction 5365 sync 0 head 3343
>>     jbd2/loop0-8-1879  [000] ....   208.327238: jbd2_end_commit: dev 7,0 transaction 5366 sync 0 head 3343
>>     jbd2/loop0-8-1879  [003] ....   208.347618: jbd2_end_commit: dev 7,0 transaction 5367 sync 0 head 3343
>> ...
>>
>> Reported-by: Asraa Ali Mardan <asraaiteng@xxxxxxxxx>
>> Signed-off-by: Naohiro Aota <naota@xxxxxxxxx>
>
> Thanks for the patch. It makes sense. You can add:
>
> Reviewed-by: Jan Kara <jack@xxxxxxx>
>
> Jens, can you please merge the patch? Thanks!
>
>                                                                 Honza
>> ---
>>
>>  fs/fs-writeback.c | 7 ++++---
>>  1 file changed, 4 insertions(+), 3 deletions(-)
>>
>> diff --git a/fs/fs-writeback.c b/fs/fs-writeback.c
>> index 05713a5..ace628c 100644
>> --- a/fs/fs-writeback.c
>> +++ b/fs/fs-writeback.c
>> @@ -2100,16 +2100,17 @@ void __mark_inode_dirty(struct inode *inode, int flags)
>>        */
>>       smp_mb();
>>
>> -     if (((inode->i_state & flags) == flags) ||
>> -         (dirtytime && (inode->i_state & I_DIRTY_INODE)))
>> +     if ((inode->i_state & flags) == flags)
>>               return;
>>
>>       if (unlikely(block_dump))
>>               block_dump___mark_inode_dirty(inode);
>>
>>       spin_lock(&inode->i_lock);
>> -     if (dirtytime && (inode->i_state & I_DIRTY_INODE))
>> +     if (dirtytime && (inode->i_state & I_DIRTY_INODE)) {
>> +             inode->i_state |= I_DIRTY_TIME;
>>               goto out_unlock_inode;
>> +     }
>>       if ((inode->i_state & flags) != flags) {
>>               const int was_dirty = inode->i_state & I_DIRTY;
>>
>> --
>> 2.8.2
>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
>> the body of a message to majordomo@xxxxxxxxxxxxxxx
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> --
> Jan Kara <jack@xxxxxxxx>
> SUSE Labs, CR