Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1750885AbdCQGNs (ORCPT ); Fri, 17 Mar 2017 02:13:48 -0400 Received: from mail-vk0-f66.google.com ([209.85.213.66]:35460 "EHLO mail-vk0-f66.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750886AbdCQGNq (ORCPT ); Fri, 17 Mar 2017 02:13:46 -0400 MIME-Version: 1.0 In-Reply-To: <20161031224606.GA23793@quack2.suse.cz> References: <20161031190245.13404-1-naota@elisp.net> <20161031224606.GA23793@quack2.suse.cz> From: Naohiro Aota Date: Fri, 17 Mar 2017 15:06:47 +0900 X-Google-Sender-Auth: a3THahDmCw2W3hehuVbuy5me2XA Message-ID: Subject: Re: [PATCH][RESEND] fs: always set I_DIRTY_TIME to fsync correctly on lazytime To: Jan Kara Cc: linux-fsdevel@vger.kernel.org, viro@zeniv.linux.org.uk, "open list:PERFORMANCE EVENT..." , "Theodore Ts'o" , ITeng A , Jens Axboe Content-Type: text/plain; charset=UTF-8 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 4835 Lines: 116 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 : > 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 >> ---------------------+--------------+------------------------ >> | I_DIRTY_TIME | no write (lazytime) >> | I_DIRTY_SYNC | write timestamp update >> | I_DIRTY_SYNC | no write (lazytime) >> | I_DIRTY_SYNC | no write *BUG* >> ... >> | 0 | >> | I_DIRTY_TIME | no write (lazytime) >> | 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 >> Signed-off-by: Naohiro Aota > > Thanks for the patch. It makes sense. You can add: > > Reviewed-by: Jan Kara > > 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@vger.kernel.org >> More majordomo info at http://vger.kernel.org/majordomo-info.html > -- > Jan Kara > SUSE Labs, CR