[RFC PATCH] loop: set queue logical block size

[RFC PATCH] loop: set queue logical block size

[Omar Sandoval]

From: Omar Sandoval <osandov@fb.com>

The request queue created when we create a loop device has the default
logical block size of 512. When we associate the device with an fd, we
set the block size on the block_device but don't update the logical
block size of the request_queue. This makes it impossibe to use direct
I/O with a backing file on a device with a block size >512, as the
following check in __loop_update_dio() fails:

	sb_bsize = bdev_logical_block_size(inode->i_sb->s_bdev);
	if (queue_logical_block_size(lo->lo_queue) >= sb_bsize &&
	    ...

Fix it by updating the logical block size when we set the fd.

Signed-off-by: Omar Sandoval <osandov@fb.com>
---
 drivers/block/loop.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/block/loop.c b/drivers/block/loop.c
index cc981f34e017..1bb22903ad1a 100644
--- a/drivers/block/loop.c
+++ b/drivers/block/loop.c
@@ -941,6 +941,7 @@ static int loop_set_fd(struct loop_device *lo, fmode_t mode,
 	/* let user-space know about the new size */
 	kobject_uevent(&disk_to_dev(bdev->bd_disk)->kobj, KOBJ_CHANGE);
 
+	blk_queue_logical_block_size(lo->lo_queue, lo_blocksize);
 	set_blocksize(bdev, lo_blocksize);
 
 	lo->lo_state = Lo_bound;
-- 
2.12.2

Re: [RFC PATCH] loop: set queue logical block size

[Ming Lei]

On Thu, Apr 06, 2017 at 01:19:45AM -0700, Omar Sandoval wrote:
> From: Omar Sandoval <osandov@fb.com>
> 
> The request queue created when we create a loop device has the default
> logical block size of 512. When we associate the device with an fd, we
> set the block size on the block_device but don't update the logical
> block size of the request_queue. This makes it impossibe to use direct
> I/O with a backing file on a device with a block size >512, as the
> following check in __loop_update_dio() fails:
> 
> 	sb_bsize = bdev_logical_block_size(inode->i_sb->s_bdev);
> 	if (queue_logical_block_size(lo->lo_queue) >= sb_bsize &&
> 	    ...
> 
> Fix it by updating the logical block size when we set the fd.
> 
> Signed-off-by: Omar Sandoval <osandov@fb.com>
> ---
>  drivers/block/loop.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/drivers/block/loop.c b/drivers/block/loop.c
> index cc981f34e017..1bb22903ad1a 100644
> --- a/drivers/block/loop.c
> +++ b/drivers/block/loop.c
> @@ -941,6 +941,7 @@ static int loop_set_fd(struct loop_device *lo, fmode_t mode,
>  	/* let user-space know about the new size */
>  	kobject_uevent(&disk_to_dev(bdev->bd_disk)->kobj, KOBJ_CHANGE);
>  
> +	blk_queue_logical_block_size(lo->lo_queue, lo_blocksize);
>  	set_blocksize(bdev, lo_blocksize);
>  
>  	lo->lo_state = Lo_bound;

Looks fine!

Reviewed-by: Ming Lei <minlei@redhat.com>

Thanks,
Ming

Re: [RFC PATCH] loop: set queue logical block size

[Hannes Reinecke]

On 04/06/2017 10:19 AM, Omar Sandoval wrote:
> From: Omar Sandoval <osandov@fb.com>
> 
> The request queue created when we create a loop device has the default
> logical block size of 512. When we associate the device with an fd, we
> set the block size on the block_device but don't update the logical
> block size of the request_queue. This makes it impossibe to use direct
> I/O with a backing file on a device with a block size >512, as the
> following check in __loop_update_dio() fails:
> 
> 	sb_bsize = bdev_logical_block_size(inode->i_sb->s_bdev);
> 	if (queue_logical_block_size(lo->lo_queue) >= sb_bsize &&
> 	    ...
> 
> Fix it by updating the logical block size when we set the fd.
> 
> Signed-off-by: Omar Sandoval <osandov@fb.com>
> ---
>  drivers/block/loop.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/drivers/block/loop.c b/drivers/block/loop.c
> index cc981f34e017..1bb22903ad1a 100644
> --- a/drivers/block/loop.c
> +++ b/drivers/block/loop.c
> @@ -941,6 +941,7 @@ static int loop_set_fd(struct loop_device *lo, fmode_t mode,
>  	/* let user-space know about the new size */
>  	kobject_uevent(&disk_to_dev(bdev->bd_disk)->kobj, KOBJ_CHANGE);
>  
> +	blk_queue_logical_block_size(lo->lo_queue, lo_blocksize);
>  	set_blocksize(bdev, lo_blocksize);
>  
>  	lo->lo_state = Lo_bound;
> 
Gnaa.

I've tried so change exactly this by my patchset titled 'loop: enable
different logical blocksizes'.
And I even got agreement from Jens to merge it once it got another
review. Which, of course, no-one did.

Care to review that instead so that it finally can go in?

Cheers,

Hannes
-- 
Dr. Hannes Reinecke		   Teamlead Storage & Networking
hare@suse.de			               +49 911 74053 688
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 N�rnberg
GF: F. Imend�rffer, J. Smithard, J. Guild, D. Upmanyu, G. Norton
HRB 21284 (AG N�rnberg)

Re: [RFC PATCH] loop: set queue logical block size

[Ming Lei]

On Thu, Apr 06, 2017 at 04:43:18PM +0800, Ming Lei wrote:
> On Thu, Apr 06, 2017 at 01:19:45AM -0700, Omar Sandoval wrote:
> > From: Omar Sandoval <osandov@fb.com>
> > 
> > The request queue created when we create a loop device has the default
> > logical block size of 512. When we associate the device with an fd, we
> > set the block size on the block_device but don't update the logical
> > block size of the request_queue. This makes it impossibe to use direct
> > I/O with a backing file on a device with a block size >512, as the
> > following check in __loop_update_dio() fails:
> > 
> > 	sb_bsize = bdev_logical_block_size(inode->i_sb->s_bdev);
> > 	if (queue_logical_block_size(lo->lo_queue) >= sb_bsize &&
> > 	    ...
> > 
> > Fix it by updating the logical block size when we set the fd.
> > 
> > Signed-off-by: Omar Sandoval <osandov@fb.com>
> > ---
> >  drivers/block/loop.c | 1 +
> >  1 file changed, 1 insertion(+)
> > 
> > diff --git a/drivers/block/loop.c b/drivers/block/loop.c
> > index cc981f34e017..1bb22903ad1a 100644
> > --- a/drivers/block/loop.c
> > +++ b/drivers/block/loop.c
> > @@ -941,6 +941,7 @@ static int loop_set_fd(struct loop_device *lo, fmode_t mode,
> >  	/* let user-space know about the new size */
> >  	kobject_uevent(&disk_to_dev(bdev->bd_disk)->kobj, KOBJ_CHANGE);
> >  
> > +	blk_queue_logical_block_size(lo->lo_queue, lo_blocksize);
> >  	set_blocksize(bdev, lo_blocksize);
> >  
> >  	lo->lo_state = Lo_bound;
> 
> Looks fine!
> 
> Reviewed-by: Ming Lei <minlei@redhat.com>

Turns out I am wrong, and we can't change logical block size in this way
because we need to support 512 byte sector filesystem image on backing
device with bigger sector size.

And we don't support dio in this situation yet, but Hannes Reinecke's
patch might help this case. So I have to recall the reviewed-by.

Sorry for the noise, :-(

Thanks,
Ming

Re: [RFC PATCH] loop: set queue logical block size

[Hannes Reinecke]

On 04/06/2017 02:03 PM, Ming Lei wrote:
> On Thu, Apr 06, 2017 at 04:43:18PM +0800, Ming Lei wrote:
>> On Thu, Apr 06, 2017 at 01:19:45AM -0700, Omar Sandoval wrote:
>>> From: Omar Sandoval <osandov@fb.com>
>>>
>>> The request queue created when we create a loop device has the default
>>> logical block size of 512. When we associate the device with an fd, we
>>> set the block size on the block_device but don't update the logical
>>> block size of the request_queue. This makes it impossibe to use direct
>>> I/O with a backing file on a device with a block size >512, as the
>>> following check in __loop_update_dio() fails:
>>>
>>> 	sb_bsize = bdev_logical_block_size(inode->i_sb->s_bdev);
>>> 	if (queue_logical_block_size(lo->lo_queue) >= sb_bsize &&
>>> 	    ...
>>>
>>> Fix it by updating the logical block size when we set the fd.
>>>
>>> Signed-off-by: Omar Sandoval <osandov@fb.com>
>>> ---
>>>  drivers/block/loop.c | 1 +
>>>  1 file changed, 1 insertion(+)
>>>
>>> diff --git a/drivers/block/loop.c b/drivers/block/loop.c
>>> index cc981f34e017..1bb22903ad1a 100644
>>> --- a/drivers/block/loop.c
>>> +++ b/drivers/block/loop.c
>>> @@ -941,6 +941,7 @@ static int loop_set_fd(struct loop_device *lo, fmode_t mode,
>>>  	/* let user-space know about the new size */
>>>  	kobject_uevent(&disk_to_dev(bdev->bd_disk)->kobj, KOBJ_CHANGE);
>>>  
>>> +	blk_queue_logical_block_size(lo->lo_queue, lo_blocksize);
>>>  	set_blocksize(bdev, lo_blocksize);
>>>  
>>>  	lo->lo_state = Lo_bound;
>>
>> Looks fine!
>>
>> Reviewed-by: Ming Lei <minlei@redhat.com>
> 
> Turns out I am wrong, and we can't change logical block size in this way
> because we need to support 512 byte sector filesystem image on backing
> device with bigger sector size.
> 
> And we don't support dio in this situation yet, but Hannes Reinecke's
> patch might help this case. So I have to recall the reviewed-by.
> 
Yes, please!

I've been waiting for it far too long.
Should I repost them?

Cheers,

Hannes
-- 
Dr. Hannes Reinecke		   Teamlead Storage & Networking
hare@suse.de			               +49 911 74053 688
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 N�rnberg
GF: F. Imend�rffer, J. Smithard, J. Guild, D. Upmanyu, G. Norton
HRB 21284 (AG N�rnberg)

Re: [RFC PATCH] loop: set queue logical block size

[Ming Lei]

On Thu, Apr 06, 2017 at 02:21:56PM +0200, Hannes Reinecke wrote:
> On 04/06/2017 02:03 PM, Ming Lei wrote:
> > On Thu, Apr 06, 2017 at 04:43:18PM +0800, Ming Lei wrote:
> >> On Thu, Apr 06, 2017 at 01:19:45AM -0700, Omar Sandoval wrote:
> >>> From: Omar Sandoval <osandov@fb.com>
> >>>
> >>> The request queue created when we create a loop device has the default
> >>> logical block size of 512. When we associate the device with an fd, we
> >>> set the block size on the block_device but don't update the logical
> >>> block size of the request_queue. This makes it impossibe to use direct
> >>> I/O with a backing file on a device with a block size >512, as the
> >>> following check in __loop_update_dio() fails:
> >>>
> >>> 	sb_bsize = bdev_logical_block_size(inode->i_sb->s_bdev);
> >>> 	if (queue_logical_block_size(lo->lo_queue) >= sb_bsize &&
> >>> 	    ...
> >>>
> >>> Fix it by updating the logical block size when we set the fd.
> >>>
> >>> Signed-off-by: Omar Sandoval <osandov@fb.com>
> >>> ---
> >>>  drivers/block/loop.c | 1 +
> >>>  1 file changed, 1 insertion(+)
> >>>
> >>> diff --git a/drivers/block/loop.c b/drivers/block/loop.c
> >>> index cc981f34e017..1bb22903ad1a 100644
> >>> --- a/drivers/block/loop.c
> >>> +++ b/drivers/block/loop.c
> >>> @@ -941,6 +941,7 @@ static int loop_set_fd(struct loop_device *lo, fmode_t mode,
> >>>  	/* let user-space know about the new size */
> >>>  	kobject_uevent(&disk_to_dev(bdev->bd_disk)->kobj, KOBJ_CHANGE);
> >>>  
> >>> +	blk_queue_logical_block_size(lo->lo_queue, lo_blocksize);
> >>>  	set_blocksize(bdev, lo_blocksize);
> >>>  
> >>>  	lo->lo_state = Lo_bound;
> >>
> >> Looks fine!
> >>
> >> Reviewed-by: Ming Lei <minlei@redhat.com>
> > 
> > Turns out I am wrong, and we can't change logical block size in this way
> > because we need to support 512 byte sector filesystem image on backing
> > device with bigger sector size.
> > 
> > And we don't support dio in this situation yet, but Hannes Reinecke's
> > patch might help this case. So I have to recall the reviewed-by.
> > 
> Yes, please!
> 
> I've been waiting for it far too long.
> Should I repost them?

Looks your last post can't be applied any more, so please post a new
version.

Thanks,
Ming

Re: [RFC PATCH] loop: set queue logical block size

[Omar Sandoval]

On Thu, Apr 06, 2017 at 10:52:01AM +0200, Hannes Reinecke wrote:
> On 04/06/2017 10:19 AM, Omar Sandoval wrote:
> > From: Omar Sandoval <osandov@fb.com>
> > 
> > The request queue created when we create a loop device has the default
> > logical block size of 512. When we associate the device with an fd, we
> > set the block size on the block_device but don't update the logical
> > block size of the request_queue. This makes it impossibe to use direct
> > I/O with a backing file on a device with a block size >512, as the
> > following check in __loop_update_dio() fails:
> > 
> > 	sb_bsize = bdev_logical_block_size(inode->i_sb->s_bdev);
> > 	if (queue_logical_block_size(lo->lo_queue) >= sb_bsize &&
> > 	    ...
> > 
> > Fix it by updating the logical block size when we set the fd.
> > 
> > Signed-off-by: Omar Sandoval <osandov@fb.com>
> > ---
> >  drivers/block/loop.c | 1 +
> >  1 file changed, 1 insertion(+)
> > 
> > diff --git a/drivers/block/loop.c b/drivers/block/loop.c
> > index cc981f34e017..1bb22903ad1a 100644
> > --- a/drivers/block/loop.c
> > +++ b/drivers/block/loop.c
> > @@ -941,6 +941,7 @@ static int loop_set_fd(struct loop_device *lo, fmode_t mode,
> >  	/* let user-space know about the new size */
> >  	kobject_uevent(&disk_to_dev(bdev->bd_disk)->kobj, KOBJ_CHANGE);
> >  
> > +	blk_queue_logical_block_size(lo->lo_queue, lo_blocksize);
> >  	set_blocksize(bdev, lo_blocksize);
> >  
> >  	lo->lo_state = Lo_bound;
> > 
> Gnaa.
> 
> I've tried so change exactly this by my patchset titled 'loop: enable
> different logical blocksizes'.
> And I even got agreement from Jens to merge it once it got another
> review. Which, of course, no-one did.

Yours looks like a more complete solution, sorry that I missed it. I'll
take a look when you repost the patches.