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

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

On 11/26/19 10:29 AM, Jaegeuk Kim wrote:
> On 11/25, Bart Van Assche wrote:
>> 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);

Hi Jaegeuk,

As you know sync_blockdev() triggers a call to filemap_write_and_wait(). 
That function in turn calls mapping->a_ops->writepages() with sync_mode 
== WB_SYNC_ALL. I think that causes sync_blockdev() to wait until all 
dirty pages have been written so we don't have to worry about pages 
dirtied before the sync_blockdev() call started.

Should we try to handle read and write requests that are submitted to a 
loop device while the loop device block size, offset and/or size are 
being modified or is it OK to fail such requests? The 
blk_set_queue_dying() and blk_mq_freeze_queue_wait() calls set the DYING 
queue flag and also wait for all ongoing block layer requests submitted 
to the loop device to finish. All later submit_bio(), 
generic_make_request(), direct_make_request() and blk_get_request() 
calls will fail with BLK_STS_IOERR or -ENODEV, including those triggered 
by kill_bdev(). In other words, the above patch causes I/O to fail that 
is submitted concurrently with kill_bdev(). Do you agree with failing 
I/O requests submitted to a loop device while the loop device block 
size, offset and/or size are being modified?

Thanks,

Bart.