| Message ID | 07d6b9266284b98aa3a4706062bd135275220d38.1491466688.git.osandov@fb.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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
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
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
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
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
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.
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;