From: Jan Kara Subject: Re: [PATCH 3/6] writeback: enforce s_umount locking in writeback_inodes_sb Date: Tue, 15 Jun 2010 19:54:13 +0200 Message-ID: <20100615175413.GG3347@quack.suse.cz> References: <20100608161424.GA11735@lst.de> <20100608161451.GD11735@lst.de> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Cc: axboe@kernel.dk, linux-fsdevel@vger.kernel.org To: Christoph Hellwig Return-path: Received: from cantor.suse.de ([195.135.220.2]:55866 "EHLO mx1.suse.de" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752409Ab0FORye (ORCPT ); Tue, 15 Jun 2010 13:54:34 -0400 Content-Disposition: inline In-Reply-To: <20100608161451.GD11735@lst.de> Sender: linux-fsdevel-owner@vger.kernel.org List-ID: On Tue 08-06-10 18:14:51, Christoph Hellwig wrote: > Make sure that not only sync_filesystem but all callers of writeback_inodes_sb > have the superblock protected against remount. As-is this disables all > functionality for these callers, but the next patch relies on this locking to > fix writeback_inodes_sb for sync_filesystem. > > Signed-off-by: Christoph Hellwig > > Index: linux-2.6/fs/fs-writeback.c > =================================================================== > --- linux-2.6.orig/fs/fs-writeback.c 2010-06-08 16:25:35.000000000 +0200 > +++ linux-2.6/fs/fs-writeback.c 2010-06-08 16:27:47.846255651 +0200 > @@ -1180,6 +1180,8 @@ void writeback_inodes_sb(struct super_bl > .sync_mode = WB_SYNC_NONE, > }; > > + WARN_ON(!rwsem_is_locked(&sb->s_umount)); > + > args.nr_pages = nr_dirty + nr_unstable + > (inodes_stat.nr_inodes - inodes_stat.nr_unused); > > @@ -1197,7 +1199,9 @@ EXPORT_SYMBOL(writeback_inodes_sb); > int writeback_inodes_sb_if_idle(struct super_block *sb) > { > if (!writeback_in_progress(sb->s_bdi)) { > + down_read(&sb->s_umount); > writeback_inodes_sb(sb); > + up_read(&sb->s_umount); But this is buggy isn't it? writeback_inodes_sb() returns as soon as flusher threads start submitting the IO. So we release s_umount too early - flusher threads can be still writing out inodes for our work. It's exactly cases like this why I don't like the work to be called "done" when flusher threads actually still do it... Honza > return 1; > } else > return 0; > @@ -1220,6 +1224,8 @@ void sync_inodes_sb(struct super_block * > .range_cyclic = 0, > }; > > + WARN_ON(!rwsem_is_locked(&sb->s_umount)); > + > bdi_queue_work_onstack(&args); > wait_sb_inodes(sb); > } > Index: linux-2.6/fs/ubifs/budget.c > =================================================================== > --- linux-2.6.orig/fs/ubifs/budget.c 2010-06-08 16:22:16.000000000 +0200 > +++ linux-2.6/fs/ubifs/budget.c 2010-06-08 16:26:50.464004778 +0200 > @@ -62,7 +62,9 @@ > */ > static void shrink_liability(struct ubifs_info *c, int nr_to_write) > { > + down_read(&c->vfs_sb->s_umount); > writeback_inodes_sb(c->vfs_sb); > + up_read(&c->vfs_sb->s_umount); > } > > /** > -- > 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