Re: [PATCH] Fix queueing work if !bdi_cap_writeback_dirty()

[Fengguang Wu <fengguang.wu@intel.com>]

On Wed, Sep 12, 2012 at 03:28:42AM +0900, OGAWA Hirofumi wrote:
> 
> If bdi has BDI_CAP_NO_WRITEBACK, bdi_forker_thread() doesn't start
> writeback thread. This means there is no consumer of work item made
> by bdi_queue_work().
> 
> This adds to checking of !bdi_cap_writeback_dirty(sb->s_bdi) before
> calling bdi_queue_work(), otherwise queued work never be consumed.

Thanks for catching this! Does this bug have any side effects other
than memory leaking? It may be possible for some caller that actually
expect it to do some work to make progress, otherwise will eventually
block.  If so, we'll need to fix the caller.

Thanks,
Fengguang

> Signed-off-by: OGAWA Hirofumi <hirofumi@mail.parknet.co.jp>
> ---
> 
>  fs/fs-writeback.c |    7 +++++--
>  1 file changed, 5 insertions(+), 2 deletions(-)
> 
> diff -puN fs/fs-writeback.c~noop_backing_dev_info-check-fix fs/fs-writeback.c
> --- linux/fs/fs-writeback.c~noop_backing_dev_info-check-fix	2012-09-11 06:12:30.000000000 +0900
> +++ linux-hirofumi/fs/fs-writeback.c	2012-09-11 06:12:30.000000000 +0900
> @@ -120,6 +120,9 @@ __bdi_start_writeback(struct backing_dev
>  {
>  	struct wb_writeback_work *work;
>  
> +	if (!bdi_cap_writeback_dirty(bdi))
> +		return;
> +
>  	/*
>  	 * This is WB_SYNC_NONE writeback, so if allocation fails just
>  	 * wakeup the thread for old dirty data writeback
> @@ -1310,7 +1313,7 @@ void writeback_inodes_sb_nr(struct super
>  		.reason			= reason,
>  	};
>  
> -	if (sb->s_bdi == &noop_backing_dev_info)
> +	if (!bdi_cap_writeback_dirty(sb->s_bdi))
>  		return;
>  	WARN_ON(!rwsem_is_locked(&sb->s_umount));
>  	bdi_queue_work(sb->s_bdi, &work);
> @@ -1396,7 +1399,7 @@ void sync_inodes_sb(struct super_block *
>  	};
>  
>  	/* Nothing to do? */
> -	if (sb->s_bdi == &noop_backing_dev_info)
> +	if (!bdi_cap_writeback_dirty(sb->s_bdi))
>  		return;
>  	WARN_ON(!rwsem_is_locked(&sb->s_umount));
>  
> _
> 
> -- 
> OGAWA Hirofumi <hirofumi@mail.parknet.co.jp>