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@xxxxxxxxxx>
> Cc: Gwendal Grignou <gwendal@xxxxxxxxxxxx>
> Cc: grygorii tertychnyi <gtertych@xxxxxxxxx>
> Cc: Andrew Norrie <andrew.norrie@xxxxxxx>
> Cc: <stable@xxxxxxxxxxxxxxx>
> Fixes: 5db470e229e2 ("loop: drop caches if offset or block_size are changed")
> Reported-by: Gwendal Grignou <gwendal@xxxxxxxxxxxx>
> Reported-by: grygorii tertychnyi <gtertych@xxxxxxxxx>
> Reported-by: Jaegeuk Kim <jaegeuk@xxxxxxxxxx>
> Signed-off-by: Bart Van Assche <bvanassche@xxxxxxx>
> ---
> 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,
