Re: [PATCH v2] loop: avoid EAGAIN, if offset or block_size are changed

[Jaegeuk Kim <jaegeuk@kernel.org>]

On 11/25, Bart Van Assche wrote:
> On 11/25/19 11:41 AM, Bart Van Assche wrote:
> > On 11/25/19 11:22 AM, Jaegeuk Kim wrote:
> > > On 11/25, Bart Van Assche wrote:
> > > > Thank you for the additional and very helpful clarification. Can
> > > > you have a look at the (totally untested) patch below? I prefer
> > > > that version because it prevents concurrent processing of
> > > > requests and syncing/killing the bdev.
> > > 
> > > Yeah, I thought this was much cleaner way, but wasn't sure it could
> > > be doable
> > > to sync|kill block device after freezing the queue. Is it okay?
> > 
> > Hi Jaegeuk,
> > 
> > That patch was based on an incorrect interpretation of the meaning of
> > lo_device. After having taken another loop at the block driver, I don't
> > think that calling sync after freezing the queue is OK. How about using
> > the following call sequence:
> > * sync_blockdev()
> > * blk_mq_freeze_queue()
> > * kill_bdev()
> 
> This is what I had in mind (still untested):
> 
> 
> Subject: [PATCH] loop: Avoid EAGAIN if the offset or the block_size are changed
> 
> After sync_blockdev() has been called, more requests can be submitted
> to the loop device. These requests dirty additional pages, causing
> loop_set_status() to return -EAGAIN. Not all user space code that
> changes the offset and/or the block size handles -EAGAIN correctly.
> Hence make sure that loop_set_status() does not return -EAGAIN.
> 
> Cc: Jaegeuk Kim <jaegeuk@kernel.org>
> Cc: Gwendal Grignou <gwendal@chromium.org>
> Cc: grygorii tertychnyi <gtertych@cisco.com>
> Cc: Andrew Norrie <andrew.norrie@cgg.com>
> Cc: <stable@vger.kernel.org>
> Fixes: 5db470e229e2 ("loop: drop caches if offset or block_size are changed")
> Reported-by: Gwendal Grignou <gwendal@chromium.org>
> Reported-by: grygorii tertychnyi <gtertych@cisco.com>
> Reported-by: Jaegeuk Kim <jaegeuk@kernel.org>
> Signed-off-by: Bart Van Assche <bvanassche@acm.org>
> ---
>  drivers/block/loop.c | 56 ++++++++++++++++++--------------------------
>  1 file changed, 23 insertions(+), 33 deletions(-)
> 
> diff --git a/drivers/block/loop.c b/drivers/block/loop.c
> index 739b372a5112..84bdb3a6f6d0 100644
> --- a/drivers/block/loop.c
> +++ b/drivers/block/loop.c
> @@ -1264,14 +1264,17 @@ loop_set_status(struct loop_device *lo, const struct loop_info64 *info)
>  		goto out_unlock;
>  	}
> 
> -	if (lo->lo_offset != info->lo_offset ||
> -	    lo->lo_sizelimit != info->lo_sizelimit) {
> -		sync_blockdev(lo->lo_device);
> -		kill_bdev(lo->lo_device);
> -	}
> +	/*
> +	 * Drain the page cache and the request queue. Set the "dying" flag to
> +	 * prevent that kill_bdev() locks up.
> +	 */
> +	sync_blockdev(lo->lo_device);
> 
> -	/* I/O need to be drained during transfer transition */
> -	blk_mq_freeze_queue(lo->lo_queue);
> +	blk_set_queue_dying(lo->lo_queue);
> +	blk_mq_freeze_queue_wait(lo->lo_queue);
> +
> +	/* Kill buffers that got dirtied after the sync_blockdev() call. */

Any race condition where we can truncate any dirty pages written between
sync_blockdev() and kill_bdev()?

Thanks,

> +	kill_bdev(lo->lo_device);
> 
>  	err = loop_release_xfer(lo);
>  	if (err)
> @@ -1298,14 +1301,6 @@ loop_set_status(struct loop_device *lo, const struct loop_info64 *info)
> 
>  	if (lo->lo_offset != info->lo_offset ||
>  	    lo->lo_sizelimit != info->lo_sizelimit) {
> -		/* kill_bdev should have truncated all the pages */
> -		if (lo->lo_device->bd_inode->i_mapping->nrpages) {
> -			err = -EAGAIN;
> -			pr_warn("%s: loop%d (%s) has still dirty pages (nrpages=%lu)\n",
> -				__func__, lo->lo_number, lo->lo_file_name,
> -				lo->lo_device->bd_inode->i_mapping->nrpages);
> -			goto out_unfreeze;
> -		}
>  		if (figure_loop_size(lo, info->lo_offset, info->lo_sizelimit)) {
>  			err = -EFBIG;
>  			goto out_unfreeze;
> @@ -1341,6 +1336,7 @@ loop_set_status(struct loop_device *lo, const struct loop_info64 *info)
>  	__loop_update_dio(lo, lo->use_dio);
> 
>  out_unfreeze:
> +	blk_queue_flag_clear(QUEUE_FLAG_DYING, lo->lo_queue);
>  	blk_mq_unfreeze_queue(lo->lo_queue);
> 
>  	if (!err && (info->lo_flags & LO_FLAGS_PARTSCAN) &&
> @@ -1531,39 +1527,33 @@ static int loop_set_dio(struct loop_device *lo, unsigned long arg)
> 
>  static int loop_set_block_size(struct loop_device *lo, unsigned long arg)
>  {
> -	int err = 0;
> -
>  	if (lo->lo_state != Lo_bound)
>  		return -ENXIO;
> 
>  	if (arg < 512 || arg > PAGE_SIZE || !is_power_of_2(arg))
>  		return -EINVAL;
> 
> -	if (lo->lo_queue->limits.logical_block_size != arg) {
> -		sync_blockdev(lo->lo_device);
> -		kill_bdev(lo->lo_device);
> -	}
> +	/*
> +	 * Drain the page cache and the request queue. Set the "dying" flag to
> +	 * prevent that kill_bdev() locks up.
> +	 */
> +	sync_blockdev(lo->lo_device);
> 
> -	blk_mq_freeze_queue(lo->lo_queue);
> +	blk_set_queue_dying(lo->lo_queue);
> +	blk_mq_freeze_queue_wait(lo->lo_queue);
> 
> -	/* kill_bdev should have truncated all the pages */
> -	if (lo->lo_queue->limits.logical_block_size != arg &&
> -			lo->lo_device->bd_inode->i_mapping->nrpages) {
> -		err = -EAGAIN;
> -		pr_warn("%s: loop%d (%s) has still dirty pages (nrpages=%lu)\n",
> -			__func__, lo->lo_number, lo->lo_file_name,
> -			lo->lo_device->bd_inode->i_mapping->nrpages);
> -		goto out_unfreeze;
> -	}
> +	/* Kill buffers that got dirtied after the sync_blockdev() call. */
> +	kill_bdev(lo->lo_device);
> 
>  	blk_queue_logical_block_size(lo->lo_queue, arg);
>  	blk_queue_physical_block_size(lo->lo_queue, arg);
>  	blk_queue_io_min(lo->lo_queue, arg);
>  	loop_update_dio(lo);
> -out_unfreeze:
> +
> +	blk_queue_flag_clear(QUEUE_FLAG_DYING, lo->lo_queue);
>  	blk_mq_unfreeze_queue(lo->lo_queue);
> 
> -	return err;
> +	return 0;
>  }
> 
>  static int lo_simple_ioctl(struct loop_device *lo, unsigned int cmd,