Re: [PATCH 3/6] writeback: enforce s_umount locking in writeback_inodes_sb

[Jan Kara <jack@suse.cz>]

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 <hch@lst.de>
> 
> 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 <jack@suse.cz>
SUSE Labs, CR