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

[Bart Van Assche <bvanassche@acm.org>]

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. */
+	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,