* [PATCH v2 0/3] loop: LO_FLAGS_BLOCKSIZE fixes
@ 2017-08-18 19:27 Omar Sandoval
2017-08-18 19:27 ` [PATCH v2 1/3] loop: fix hang if LOOP_SET_STATUS gets invalid blocksize or encrypt type Omar Sandoval
` (3 more replies)
0 siblings, 4 replies; 8+ messages in thread
From: Omar Sandoval @ 2017-08-18 19:27 UTC (permalink / raw)
To: linux-block; +Cc: kernel-team, Hannes Reinecke, Ming Lei, Karel Zak
From: Omar Sandoval <osandov@fb.com>
Patches 1 and 3 are from the original series.
Patch 2 gets rid of the redundant struct loop_device.lo_logical_blocksize
in favor of using the queue's own logical_block_size. Karel, I decided
against adding another sysfs entry since it will always be the same as
queue/logical_block_size, is that alright with you?
Omar Sandoval (3):
loop: fix hang if LOOP_SET_STATUS gets invalid blocksize or encrypt
type
loop: use queue limit instead of private lo_logical_blocksize
loop: always return block size in LOOP_GET_STATUS
drivers/block/loop.c | 71 ++++++++++++++++++++++++++--------------------------
drivers/block/loop.h | 1 -
2 files changed, 36 insertions(+), 36 deletions(-)
--
2.14.1
^ permalink raw reply [flat|nested] 8+ messages in thread* [PATCH v2 1/3] loop: fix hang if LOOP_SET_STATUS gets invalid blocksize or encrypt type 2017-08-18 19:27 [PATCH v2 0/3] loop: LO_FLAGS_BLOCKSIZE fixes Omar Sandoval @ 2017-08-18 19:27 ` Omar Sandoval 2017-08-18 19:27 ` [PATCH v2 2/3] loop: use queue limit instead of private lo_logical_blocksize Omar Sandoval ` (2 subsequent siblings) 3 siblings, 0 replies; 8+ messages in thread From: Omar Sandoval @ 2017-08-18 19:27 UTC (permalink / raw) To: linux-block; +Cc: kernel-team, Hannes Reinecke, Ming Lei, Karel Zak From: Omar Sandoval <osandov@fb.com> In both of these error cases, we need to make sure to unfreeze the queue before we return. Fixes: ecdd09597a57 ("block/loop: fix race between I/O and set_status") Fixes: f2c6df7dbf9a ("loop: support 4k physical blocksize") Reviewed-by: Hannes Reinecke <hare@suse.com> Signed-off-by: Omar Sandoval <osandov@fb.com> --- drivers/block/loop.c | 24 ++++++++++++++++-------- 1 file changed, 16 insertions(+), 8 deletions(-) diff --git a/drivers/block/loop.c b/drivers/block/loop.c index ef8334949b42..26548e07bc31 100644 --- a/drivers/block/loop.c +++ b/drivers/block/loop.c @@ -1125,11 +1125,15 @@ loop_set_status(struct loop_device *lo, const struct loop_info64 *info) if (info->lo_encrypt_type) { unsigned int type = info->lo_encrypt_type; - if (type >= MAX_LO_CRYPT) - return -EINVAL; + if (type >= MAX_LO_CRYPT) { + err = -EINVAL; + goto exit; + } xfer = xfer_funcs[type]; - if (xfer == NULL) - return -EINVAL; + if (xfer == NULL) { + err = -EINVAL; + goto exit; + } } else xfer = NULL; @@ -1144,10 +1148,14 @@ loop_set_status(struct loop_device *lo, const struct loop_info64 *info) if (LO_INFO_BLOCKSIZE(info) != 512 && LO_INFO_BLOCKSIZE(info) != 1024 && LO_INFO_BLOCKSIZE(info) != 2048 && - LO_INFO_BLOCKSIZE(info) != 4096) - return -EINVAL; - if (LO_INFO_BLOCKSIZE(info) > lo->lo_blocksize) - return -EINVAL; + LO_INFO_BLOCKSIZE(info) != 4096) { + err = -EINVAL; + goto exit; + } + if (LO_INFO_BLOCKSIZE(info) > lo->lo_blocksize) { + err = -EINVAL; + goto exit; + } } if (lo->lo_offset != info->lo_offset || -- 2.14.1 ^ permalink raw reply related [flat|nested] 8+ messages in thread
* [PATCH v2 2/3] loop: use queue limit instead of private lo_logical_blocksize 2017-08-18 19:27 [PATCH v2 0/3] loop: LO_FLAGS_BLOCKSIZE fixes Omar Sandoval 2017-08-18 19:27 ` [PATCH v2 1/3] loop: fix hang if LOOP_SET_STATUS gets invalid blocksize or encrypt type Omar Sandoval @ 2017-08-18 19:27 ` Omar Sandoval 2017-08-21 5:58 ` Hannes Reinecke 2017-08-18 19:27 ` [PATCH v2 3/3] loop: always return block size in LOOP_GET_STATUS Omar Sandoval 2017-08-22 10:53 ` [PATCH v2 0/3] loop: LO_FLAGS_BLOCKSIZE fixes Milan Broz 3 siblings, 1 reply; 8+ messages in thread From: Omar Sandoval @ 2017-08-18 19:27 UTC (permalink / raw) To: linux-block; +Cc: kernel-team, Hannes Reinecke, Ming Lei, Karel Zak From: Omar Sandoval <osandov@fb.com> There's no reason to track this separately; just use the logical_block_size queue limit. Signed-off-by: Omar Sandoval <osandov@fb.com> --- drivers/block/loop.c | 44 ++++++++++++++++++-------------------------- drivers/block/loop.h | 1 - 2 files changed, 18 insertions(+), 27 deletions(-) diff --git a/drivers/block/loop.c b/drivers/block/loop.c index 26548e07bc31..94227a327ce2 100644 --- a/drivers/block/loop.c +++ b/drivers/block/loop.c @@ -221,8 +221,7 @@ static void __loop_update_dio(struct loop_device *lo, bool dio) } static int -figure_loop_size(struct loop_device *lo, loff_t offset, loff_t sizelimit, - loff_t logical_blocksize) +figure_loop_size(struct loop_device *lo, loff_t offset, loff_t sizelimit) { loff_t size = get_size(offset, sizelimit, lo->lo_backing_file); sector_t x = (sector_t)size; @@ -234,12 +233,6 @@ figure_loop_size(struct loop_device *lo, loff_t offset, loff_t sizelimit, lo->lo_offset = offset; if (lo->lo_sizelimit != sizelimit) lo->lo_sizelimit = sizelimit; - if (lo->lo_flags & LO_FLAGS_BLOCKSIZE) { - lo->lo_logical_blocksize = logical_blocksize; - blk_queue_physical_block_size(lo->lo_queue, lo->lo_blocksize); - blk_queue_logical_block_size(lo->lo_queue, - lo->lo_logical_blocksize); - } set_capacity(lo->lo_disk, x); bd_set_size(bdev, (loff_t)get_capacity(bdev->bd_disk) << 9); /* let user-space know about the new size */ @@ -820,7 +813,7 @@ static void loop_config_discard(struct loop_device *lo) struct file *file = lo->lo_backing_file; struct inode *inode = file->f_mapping->host; struct request_queue *q = lo->lo_queue; - int lo_bits = 9; + int lo_bits = blksize_bits(queue_logical_block_size(q)); /* * We use punch hole to reclaim the free space used by the @@ -840,8 +833,6 @@ static void loop_config_discard(struct loop_device *lo) q->limits.discard_granularity = inode->i_sb->s_blocksize; q->limits.discard_alignment = 0; - if (lo->lo_flags & LO_FLAGS_BLOCKSIZE) - lo_bits = blksize_bits(lo->lo_logical_blocksize); blk_queue_max_discard_sectors(q, UINT_MAX >> lo_bits); blk_queue_max_write_zeroes_sectors(q, UINT_MAX >> lo_bits); @@ -938,7 +929,6 @@ static int loop_set_fd(struct loop_device *lo, fmode_t mode, lo->use_dio = false; lo->lo_blocksize = lo_blocksize; - lo->lo_logical_blocksize = 512; lo->lo_device = bdev; lo->lo_flags = lo_flags; lo->lo_backing_file = file; @@ -950,6 +940,9 @@ static int loop_set_fd(struct loop_device *lo, fmode_t mode, if (!(lo_flags & LO_FLAGS_READ_ONLY) && file->f_op->fsync) blk_queue_write_cache(lo->lo_queue, true, false); + blk_queue_logical_block_size(lo->lo_queue, 512); + blk_queue_physical_block_size(lo->lo_queue, 512); + blk_queue_io_min(lo->lo_queue, 512); loop_update_dio(lo); set_capacity(lo->lo_disk, size); @@ -1137,14 +1130,7 @@ loop_set_status(struct loop_device *lo, const struct loop_info64 *info) } else xfer = NULL; - err = loop_init_xfer(lo, xfer, info); - if (err) - goto exit; - if (info->lo_flags & LO_FLAGS_BLOCKSIZE) { - if (!(lo->lo_flags & LO_FLAGS_BLOCKSIZE)) - lo->lo_logical_blocksize = 512; - lo->lo_flags |= LO_FLAGS_BLOCKSIZE; if (LO_INFO_BLOCKSIZE(info) != 512 && LO_INFO_BLOCKSIZE(info) != 1024 && LO_INFO_BLOCKSIZE(info) != 2048 && @@ -1158,18 +1144,25 @@ loop_set_status(struct loop_device *lo, const struct loop_info64 *info) } } + err = loop_init_xfer(lo, xfer, info); + if (err) + goto exit; + if (lo->lo_offset != info->lo_offset || lo->lo_sizelimit != info->lo_sizelimit || - lo->lo_flags != lo_flags || - ((lo->lo_flags & LO_FLAGS_BLOCKSIZE) && - lo->lo_logical_blocksize != LO_INFO_BLOCKSIZE(info))) { - if (figure_loop_size(lo, info->lo_offset, info->lo_sizelimit, - LO_INFO_BLOCKSIZE(info))) { + lo->lo_flags != lo_flags) { + if (figure_loop_size(lo, info->lo_offset, info->lo_sizelimit)) { err = -EFBIG; goto exit; } } + if (info->lo_flags & LO_FLAGS_BLOCKSIZE) { + blk_queue_logical_block_size(lo->lo_queue, LO_INFO_BLOCKSIZE(info)); + blk_queue_physical_block_size(lo->lo_queue, LO_INFO_BLOCKSIZE(info)); + blk_queue_io_min(lo->lo_queue, LO_INFO_BLOCKSIZE(info)); + } + loop_config_discard(lo); memcpy(lo->lo_file_name, info->lo_file_name, LO_NAME_SIZE); @@ -1356,8 +1349,7 @@ static int loop_set_capacity(struct loop_device *lo) if (unlikely(lo->lo_state != Lo_bound)) return -ENXIO; - return figure_loop_size(lo, lo->lo_offset, lo->lo_sizelimit, - lo->lo_logical_blocksize); + return figure_loop_size(lo, lo->lo_offset, lo->lo_sizelimit); } static int loop_set_dio(struct loop_device *lo, unsigned long arg) diff --git a/drivers/block/loop.h b/drivers/block/loop.h index 2c096b9a17b8..fecd3f97ef8c 100644 --- a/drivers/block/loop.h +++ b/drivers/block/loop.h @@ -49,7 +49,6 @@ struct loop_device { struct file * lo_backing_file; struct block_device *lo_device; unsigned lo_blocksize; - unsigned lo_logical_blocksize; void *key_data; gfp_t old_gfp_mask; -- 2.14.1 ^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH v2 2/3] loop: use queue limit instead of private lo_logical_blocksize 2017-08-18 19:27 ` [PATCH v2 2/3] loop: use queue limit instead of private lo_logical_blocksize Omar Sandoval @ 2017-08-21 5:58 ` Hannes Reinecke 2017-08-21 16:45 ` Omar Sandoval 0 siblings, 1 reply; 8+ messages in thread From: Hannes Reinecke @ 2017-08-21 5:58 UTC (permalink / raw) To: Omar Sandoval, linux-block; +Cc: kernel-team, Ming Lei, Karel Zak On 08/18/2017 09:27 PM, Omar Sandoval wrote: > From: Omar Sandoval <osandov@fb.com> > > There's no reason to track this separately; just use the > logical_block_size queue limit. > > Signed-off-by: Omar Sandoval <osandov@fb.com> > --- > drivers/block/loop.c | 44 ++++++++++++++++++-------------------------- > drivers/block/loop.h | 1 - > 2 files changed, 18 insertions(+), 27 deletions(-) > Curiously enough, this is what I attempted initially. But that got shut down due to incompability. In the hope that you'll succeed: Reviewed-by: Hannes Reinecke <hare@suse.com> 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) ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v2 2/3] loop: use queue limit instead of private lo_logical_blocksize 2017-08-21 5:58 ` Hannes Reinecke @ 2017-08-21 16:45 ` Omar Sandoval 0 siblings, 0 replies; 8+ messages in thread From: Omar Sandoval @ 2017-08-21 16:45 UTC (permalink / raw) To: Hannes Reinecke; +Cc: linux-block, kernel-team, Ming Lei, Karel Zak On Mon, Aug 21, 2017 at 07:58:52AM +0200, Hannes Reinecke wrote: > On 08/18/2017 09:27 PM, Omar Sandoval wrote: > > From: Omar Sandoval <osandov@fb.com> > > > > There's no reason to track this separately; just use the > > logical_block_size queue limit. > > > > Signed-off-by: Omar Sandoval <osandov@fb.com> > > --- > > drivers/block/loop.c | 44 ++++++++++++++++++-------------------------- > > drivers/block/loop.h | 1 - > > 2 files changed, 18 insertions(+), 27 deletions(-) > > > > Curiously enough, this is what I attempted initially. > But that got shut down due to incompability. > In the hope that you'll succeed: Is that thread archived anywhere? I don't see how this would be a compatability problem, it doesn't change the behavior if you're not using the new option, and if you are using the new option then there's no compatability to speak of. > Reviewed-by: Hannes Reinecke <hare@suse.com> Thanks! ^ permalink raw reply [flat|nested] 8+ messages in thread
* [PATCH v2 3/3] loop: always return block size in LOOP_GET_STATUS 2017-08-18 19:27 [PATCH v2 0/3] loop: LO_FLAGS_BLOCKSIZE fixes Omar Sandoval 2017-08-18 19:27 ` [PATCH v2 1/3] loop: fix hang if LOOP_SET_STATUS gets invalid blocksize or encrypt type Omar Sandoval 2017-08-18 19:27 ` [PATCH v2 2/3] loop: use queue limit instead of private lo_logical_blocksize Omar Sandoval @ 2017-08-18 19:27 ` Omar Sandoval 2017-08-21 5:59 ` Hannes Reinecke 2017-08-22 10:53 ` [PATCH v2 0/3] loop: LO_FLAGS_BLOCKSIZE fixes Milan Broz 3 siblings, 1 reply; 8+ messages in thread From: Omar Sandoval @ 2017-08-18 19:27 UTC (permalink / raw) To: linux-block; +Cc: kernel-team, Hannes Reinecke, Ming Lei, Karel Zak From: Omar Sandoval <osandov@fb.com> When I was writing a test for the new loop device block size functionality, I noticed a couple of issues with how LOOP_GET_STATUS handles the block size: - lo_init[0] is never filled in with the logical block size we previously set - lo_flags returned from LOOP_GET_STATUS will have LO_FLAGS_BLOCKSIZE set if we previously called LOOP_SET_STATUS with LO_FLAGS_BLOCKSIZE set, which doesn't really mean anything Instead, for LOOP_GET_STATUS, let's always fill in lo_init[0] and set the LO_FLAGS_BLOCKSIZE flag to indicate we support it. Signed-off-by: Omar Sandoval <osandov@fb.com> --- drivers/block/loop.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/drivers/block/loop.c b/drivers/block/loop.c index 94227a327ce2..a1ac50bc4812 100644 --- a/drivers/block/loop.c +++ b/drivers/block/loop.c @@ -1224,7 +1224,7 @@ loop_get_status(struct loop_device *lo, struct loop_info64 *info) info->lo_rdevice = huge_encode_dev(lo->lo_device ? stat.rdev : stat.dev); info->lo_offset = lo->lo_offset; info->lo_sizelimit = lo->lo_sizelimit; - info->lo_flags = lo->lo_flags; + info->lo_flags = lo->lo_flags | LO_FLAGS_BLOCKSIZE; memcpy(info->lo_file_name, lo->lo_file_name, LO_NAME_SIZE); memcpy(info->lo_crypt_name, lo->lo_crypt_name, LO_NAME_SIZE); info->lo_encrypt_type = @@ -1234,6 +1234,7 @@ loop_get_status(struct loop_device *lo, struct loop_info64 *info) memcpy(info->lo_encrypt_key, lo->lo_encrypt_key, lo->lo_encrypt_key_size); } + LO_INFO_BLOCKSIZE(info) = queue_logical_block_size(lo->lo_queue); return 0; } -- 2.14.1 ^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH v2 3/3] loop: always return block size in LOOP_GET_STATUS 2017-08-18 19:27 ` [PATCH v2 3/3] loop: always return block size in LOOP_GET_STATUS Omar Sandoval @ 2017-08-21 5:59 ` Hannes Reinecke 0 siblings, 0 replies; 8+ messages in thread From: Hannes Reinecke @ 2017-08-21 5:59 UTC (permalink / raw) To: Omar Sandoval, linux-block; +Cc: kernel-team, Ming Lei, Karel Zak On 08/18/2017 09:27 PM, Omar Sandoval wrote: > From: Omar Sandoval <osandov@fb.com> > > When I was writing a test for the new loop device block size > functionality, I noticed a couple of issues with how LOOP_GET_STATUS > handles the block size: > > - lo_init[0] is never filled in with the logical block size we > previously set > - lo_flags returned from LOOP_GET_STATUS will have LO_FLAGS_BLOCKSIZE > set if we previously called LOOP_SET_STATUS with LO_FLAGS_BLOCKSIZE > set, which doesn't really mean anything > > Instead, for LOOP_GET_STATUS, let's always fill in lo_init[0] and set > the LO_FLAGS_BLOCKSIZE flag to indicate we support it. > > Signed-off-by: Omar Sandoval <osandov@fb.com> > --- > drivers/block/loop.c | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > > diff --git a/drivers/block/loop.c b/drivers/block/loop.c > index 94227a327ce2..a1ac50bc4812 100644 > --- a/drivers/block/loop.c > +++ b/drivers/block/loop.c > @@ -1224,7 +1224,7 @@ loop_get_status(struct loop_device *lo, struct loop_info64 *info) > info->lo_rdevice = huge_encode_dev(lo->lo_device ? stat.rdev : stat.dev); > info->lo_offset = lo->lo_offset; > info->lo_sizelimit = lo->lo_sizelimit; > - info->lo_flags = lo->lo_flags; > + info->lo_flags = lo->lo_flags | LO_FLAGS_BLOCKSIZE; > memcpy(info->lo_file_name, lo->lo_file_name, LO_NAME_SIZE); > memcpy(info->lo_crypt_name, lo->lo_crypt_name, LO_NAME_SIZE); > info->lo_encrypt_type = > @@ -1234,6 +1234,7 @@ loop_get_status(struct loop_device *lo, struct loop_info64 *info) > memcpy(info->lo_encrypt_key, lo->lo_encrypt_key, > lo->lo_encrypt_key_size); > } > + LO_INFO_BLOCKSIZE(info) = queue_logical_block_size(lo->lo_queue); > return 0; > } > > Reviewed-by: Hannes Reinecke <hare@suse.com> 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) ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v2 0/3] loop: LO_FLAGS_BLOCKSIZE fixes 2017-08-18 19:27 [PATCH v2 0/3] loop: LO_FLAGS_BLOCKSIZE fixes Omar Sandoval ` (2 preceding siblings ...) 2017-08-18 19:27 ` [PATCH v2 3/3] loop: always return block size in LOOP_GET_STATUS Omar Sandoval @ 2017-08-22 10:53 ` Milan Broz 3 siblings, 0 replies; 8+ messages in thread From: Milan Broz @ 2017-08-22 10:53 UTC (permalink / raw) To: Omar Sandoval, linux-block Cc: kernel-team, Hannes Reinecke, Ming Lei, Karel Zak On 08/18/2017 09:27 PM, Omar Sandoval wrote: > From: Omar Sandoval <osandov@fb.com> > > Patches 1 and 3 are from the original series. > > Patch 2 gets rid of the redundant struct loop_device.lo_logical_blocksize > in favor of using the queue's own logical_block_size. Karel, I decided > against adding another sysfs entry since it will always be the same as > queue/logical_block_size, is that alright with you? > > Omar Sandoval (3): > loop: fix hang if LOOP_SET_STATUS gets invalid blocksize or encrypt > type > loop: use queue limit instead of private lo_logical_blocksize > loop: always return block size in LOOP_GET_STATUS I tested these with cryptsetup & loop mapping with added block size setting and it fixes loop problems I see in 4.13. I think all these patches should go to 4.13. Thanks! Milan ^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2017-08-22 10:53 UTC | newest] Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2017-08-18 19:27 [PATCH v2 0/3] loop: LO_FLAGS_BLOCKSIZE fixes Omar Sandoval 2017-08-18 19:27 ` [PATCH v2 1/3] loop: fix hang if LOOP_SET_STATUS gets invalid blocksize or encrypt type Omar Sandoval 2017-08-18 19:27 ` [PATCH v2 2/3] loop: use queue limit instead of private lo_logical_blocksize Omar Sandoval 2017-08-21 5:58 ` Hannes Reinecke 2017-08-21 16:45 ` Omar Sandoval 2017-08-18 19:27 ` [PATCH v2 3/3] loop: always return block size in LOOP_GET_STATUS Omar Sandoval 2017-08-21 5:59 ` Hannes Reinecke 2017-08-22 10:53 ` [PATCH v2 0/3] loop: LO_FLAGS_BLOCKSIZE fixes Milan Broz
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.