From mboxrd@z Thu Jan 1 00:00:00 1970 From: Jan Kara Subject: Re: [PATCH v3] writeback: Do not sync data dirtied after sync start Date: Thu, 10 Oct 2013 00:14:19 +0200 Message-ID: <20131009221419.GC25608@quack.suse.cz> References: <1380275080-31736-1-git-send-email-jack@suse.cz> <20131008151409.6b7415fc9ad7108f5de04873@linux-foundation.org> <20131009140211.GA13650@quack.suse.cz> <20131009150325.GA25870@quack.suse.cz> <20131009142125.9c7a2b3f5d68ddc6d11c7d3a@linux-foundation.org> Mime-Version: 1.0 Content-Type: multipart/mixed; boundary="fUYQa+Pmc3FrFX/N" Cc: Jan Kara , Al Viro , Wu Fengguang , linux-fsdevel@vger.kernel.org, dchinner@redhat.com To: Andrew Morton Return-path: Received: from cantor2.suse.de ([195.135.220.15]:40691 "EHLO mx2.suse.de" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752834Ab3JIWOW (ORCPT ); Wed, 9 Oct 2013 18:14:22 -0400 Content-Disposition: inline In-Reply-To: <20131009142125.9c7a2b3f5d68ddc6d11c7d3a@linux-foundation.org> Sender: linux-fsdevel-owner@vger.kernel.org List-ID: --fUYQa+Pmc3FrFX/N Content-Type: text/plain; charset=us-ascii Content-Disposition: inline On Wed 09-10-13 14:21:25, Andrew Morton wrote: > On Wed, 9 Oct 2013 17:03:25 +0200 Jan Kara wrote: > > > From: Jan Kara > > Date: Wed, 9 Oct 2013 15:41:50 +0200 > > Subject: [PATCH] writeback: Use older_than_this_is_set instead of magic > > older_than_this == 0 > > > > Currently we use 0 as a special value of work->older_than_this to > > indicate that wb_writeback() should set work->older_that_this to current > > time. This works but it is a bit magic. So use a special flag in > > work_struct for that. > > OK. > > > - if (!work->older_than_this) > > + if (!work->older_than_this_is_set) > > work->older_than_this = jiffies; > > It would be logical although presumably unneeded to set > older_than_this_is_set here? Yes. Updated. > > Also fixup writeback from workqueue rescuer to include all inodes. > > There's nothing in the patch which matches this sentence? The sentence is about the hunk below. writeback_inodes_wb() is special in that it directly calls queue_io() (everything else goes through wb_writeback()) and my previous patch thus resulted in using 0 as an older_than_this value => likely we wouldn't queue any inodes for writeback. I've added WARN_ON_ONCE into move_expired_inodes() to increase a chance of catching such mistakes in future (although in this particular case it wouldn't really help because writeback_inodes_wb() gets hardly ever called). > @@ -732,6 +737,8 @@ static long writeback_inodes_wb(struct b > .sync_mode = WB_SYNC_NONE, > .range_cyclic = 1, > .reason = reason, > + .older_than_this = jiffies, > + .older_than_this_is_set = 1, > }; > > spin_lock(&wb->list_lock); Updated version of the patch attached. Honza -- Jan Kara SUSE Labs, CR --fUYQa+Pmc3FrFX/N Content-Type: text/x-patch; charset=us-ascii Content-Disposition: attachment; filename="0001-writeback-Use-older_than_this_is_set-instead-of-magi.patch" >>From a872d2374364ac1d8f0a9a6fa529afdc06f9d1a1 Mon Sep 17 00:00:00 2001 From: Jan Kara Date: Wed, 9 Oct 2013 15:41:50 +0200 Subject: [PATCH] writeback: Use older_than_this_is_set instead of magic older_than_this == 0 Currently we use 0 as a special value of work->older_than_this to indicate that wb_writeback() should set work->older_that_this to current time. This works but it is a bit magic. So use a special flag in work_struct for that. Also fixup writeback from workqueue rescuer (writeback_inodes_wb()) to include all inodes. Currently it would use 0 as an older_than_this value thus queue_io() would likely not queue any inodes for writeback. Signed-off-by: Jan Kara --- fs/fs-writeback.c | 13 ++++++++++++- 1 file changed, 12 insertions(+), 1 deletion(-) diff --git a/fs/fs-writeback.c b/fs/fs-writeback.c index 70837dadad72..ba332d2c596a 100644 --- a/fs/fs-writeback.c +++ b/fs/fs-writeback.c @@ -39,6 +39,10 @@ struct wb_writeback_work { long nr_pages; struct super_block *sb; + /* + * Write only inodes dirtied before this time. Don't forget to set + * older_than_this_is_set when you set this. + */ unsigned long older_than_this; enum writeback_sync_modes sync_mode; unsigned int tagged_writepages:1; @@ -46,6 +50,7 @@ struct wb_writeback_work { unsigned int range_cyclic:1; unsigned int for_background:1; unsigned int for_sync:1; /* sync(2) WB_SYNC_ALL writeback */ + unsigned int older_than_this_is_set:1; enum wb_reason reason; /* why was writeback initiated? */ struct list_head list; /* pending work list */ @@ -246,6 +251,7 @@ static int move_expired_inodes(struct list_head *delaying_queue, int do_sb_sort = 0; int moved = 0; + WARN_ON_ONCE(!work->older_than_this_is_set); while (!list_empty(delaying_queue)) { inode = wb_inode(delaying_queue->prev); if (inode_dirtied_after(inode, work->older_than_this)) @@ -732,6 +738,8 @@ static long writeback_inodes_wb(struct bdi_writeback *wb, long nr_pages, .sync_mode = WB_SYNC_NONE, .range_cyclic = 1, .reason = reason, + .older_than_this = jiffies, + .older_than_this_is_set = 1, }; spin_lock(&wb->list_lock); @@ -793,8 +801,10 @@ static long wb_writeback(struct bdi_writeback *wb, struct inode *inode; long progress; - if (!work->older_than_this) + if (!work->older_than_this_is_set) { work->older_than_this = jiffies; + work->older_than_this_is_set = 1; + } spin_lock(&wb->list_lock); for (;;) { @@ -1356,6 +1366,7 @@ void sync_inodes_sb(struct super_block *sb, unsigned long older_than_this) .sync_mode = WB_SYNC_ALL, .nr_pages = LONG_MAX, .older_than_this = older_than_this, + .older_than_this_is_set = 1, .range_cyclic = 0, .done = &done, .reason = WB_REASON_SYNC, -- 1.8.1.4 --fUYQa+Pmc3FrFX/N--