| Message ID | 9ff9b291e3e872698a040a281549c39c0637565f.1503036471.git.osandov@fb.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 08/18/2017 08:15 AM, Omar Sandoval wrote: > From: Omar Sandoval <osandov@fb.com> > > When I was writing a test for the new loop device block size > functionality, I realized that the interface is kind of dumb: > > - 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 | 33 +++++++++++++++++---------------- > 1 file changed, 17 insertions(+), 16 deletions(-) > Phew. 'Dumb interface'. 'tis wasn't me who designed the interface; Backwards compability are the watchwords here. Personally, I would have loved to design a new interface. I've got quite some flak for daring to break existing interfaces, most notably setting logical and physical blocksize per default (which I would _love_ to have done, seeing that it really makes sense here). But as this would change the behaviour I've gone through pains (and several _years_ of iterations) to get this sorted. So if you design a blocktest for that ensure that a) the sysfs attributes before and after the patch are _identical_ b) the sysfs attributes will only change if the 'LO_FLAGS_BLOCKSIZE' flag has been set and c) validate the written blocksizes; this is required to be able to install bootloaders there This whole interface was designed such that you can prepare bootable diskimages for S/390 DASDs, which use a native 4k blocksize. Cheers, Hannes
On Fri, Aug 18, 2017 at 09:18:52AM +0200, Hannes Reinecke wrote: > On 08/18/2017 08:15 AM, Omar Sandoval wrote: > > From: Omar Sandoval <osandov@fb.com> > > > > When I was writing a test for the new loop device block size > > functionality, I realized that the interface is kind of dumb: > > > > - 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 | 33 +++++++++++++++++---------------- > > 1 file changed, 17 insertions(+), 16 deletions(-) > > > Phew. 'Dumb interface'. > 'tis wasn't me who designed the interface; > Backwards compability are the watchwords here. > Personally, I would have loved to design a new interface. > > I've got quite some flak for daring to break existing interfaces, most > notably setting logical and physical blocksize per default (which I > would _love_ to have done, seeing that it really makes sense here). > But as this would change the behaviour I've gone through pains (and > several _years_ of iterations) to get this sorted. > > So if you design a blocktest for that ensure that > a) the sysfs attributes before and after the patch are _identical_ > b) the sysfs attributes will only change if the 'LO_FLAGS_BLOCKSIZE' > flag has been set > and > c) validate the written blocksizes; this is required to be able to > install bootloaders there > > This whole interface was designed such that you can prepare bootable > diskimages for S/390 DASDs, which use a native 4k blocksize. Hi, Hannes, Wasn't insulting you at all, the only part of the interface I'm complaining about is LOOP_GET_STATUS missing information :)
On 08/18/2017 09:38 AM, Omar Sandoval wrote: > On Fri, Aug 18, 2017 at 09:18:52AM +0200, Hannes Reinecke wrote: >> On 08/18/2017 08:15 AM, Omar Sandoval wrote: >>> From: Omar Sandoval <osandov@fb.com> >>> >>> When I was writing a test for the new loop device block size >>> functionality, I realized that the interface is kind of dumb: >>> >>> - 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 | 33 +++++++++++++++++---------------- >>> 1 file changed, 17 insertions(+), 16 deletions(-) >>> >> Phew. 'Dumb interface'. >> 'tis wasn't me who designed the interface; >> Backwards compability are the watchwords here. >> Personally, I would have loved to design a new interface. >> >> I've got quite some flak for daring to break existing interfaces, most >> notably setting logical and physical blocksize per default (which I >> would _love_ to have done, seeing that it really makes sense here). >> But as this would change the behaviour I've gone through pains (and >> several _years_ of iterations) to get this sorted. >> >> So if you design a blocktest for that ensure that >> a) the sysfs attributes before and after the patch are _identical_ >> b) the sysfs attributes will only change if the 'LO_FLAGS_BLOCKSIZE' >> flag has been set >> and >> c) validate the written blocksizes; this is required to be able to >> install bootloaders there >> >> This whole interface was designed such that you can prepare bootable >> diskimages for S/390 DASDs, which use a native 4k blocksize. > > Hi, Hannes, > > Wasn't insulting you at all, the only part of the interface I'm > complaining about is LOOP_GET_STATUS missing information :) > Sure. Just saying. But please make sure only to return that information if the LO_FLAGS_BLOCKSIZE flag is set; otherwise there's a rick of confusing losetup. Cheers, Hannes
On Fri, Aug 18, 2017 at 09:43:19AM +0200, Hannes Reinecke wrote: > On 08/18/2017 09:38 AM, Omar Sandoval wrote: > > On Fri, Aug 18, 2017 at 09:18:52AM +0200, Hannes Reinecke wrote: > >> On 08/18/2017 08:15 AM, Omar Sandoval wrote: > >>> From: Omar Sandoval <osandov@fb.com> > >>> > >>> When I was writing a test for the new loop device block size > >>> functionality, I realized that the interface is kind of dumb: > >>> > >>> - 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 | 33 +++++++++++++++++---------------- > >>> 1 file changed, 17 insertions(+), 16 deletions(-) > >>> > >> Phew. 'Dumb interface'. > >> 'tis wasn't me who designed the interface; > >> Backwards compability are the watchwords here. > >> Personally, I would have loved to design a new interface. > >> > >> I've got quite some flak for daring to break existing interfaces, most > >> notably setting logical and physical blocksize per default (which I > >> would _love_ to have done, seeing that it really makes sense here). > >> But as this would change the behaviour I've gone through pains (and > >> several _years_ of iterations) to get this sorted. > >> > >> So if you design a blocktest for that ensure that > >> a) the sysfs attributes before and after the patch are _identical_ > >> b) the sysfs attributes will only change if the 'LO_FLAGS_BLOCKSIZE' > >> flag has been set > >> and > >> c) validate the written blocksizes; this is required to be able to > >> install bootloaders there > >> > >> This whole interface was designed such that you can prepare bootable > >> diskimages for S/390 DASDs, which use a native 4k blocksize. > > > > Hi, Hannes, > > > > Wasn't insulting you at all, the only part of the interface I'm > > complaining about is LOOP_GET_STATUS missing information :) > > > Sure. Just saying. > But please make sure only to return that information if the > LO_FLAGS_BLOCKSIZE flag is set; otherwise there's a rick of confusing > losetup. I actually checked losetup, it works just fine with LO_FLAGS_BLOCKSIZE always set and lo_init[0] always filled in.
On 08/18/2017 09:47 AM, Omar Sandoval wrote: > On Fri, Aug 18, 2017 at 09:43:19AM +0200, Hannes Reinecke wrote: >> On 08/18/2017 09:38 AM, Omar Sandoval wrote: >>> On Fri, Aug 18, 2017 at 09:18:52AM +0200, Hannes Reinecke wrote: [ .. ] >>>> I've got quite some flak for daring to break existing interfaces, most >>>> notably setting logical and physical blocksize per default (which I >>>> would _love_ to have done, seeing that it really makes sense here). >>>> But as this would change the behaviour I've gone through pains (and >>>> several _years_ of iterations) to get this sorted. >>>> >>>> So if you design a blocktest for that ensure that >>>> a) the sysfs attributes before and after the patch are _identical_ >>>> b) the sysfs attributes will only change if the 'LO_FLAGS_BLOCKSIZE' >>>> flag has been set >>>> and >>>> c) validate the written blocksizes; this is required to be able to >>>> install bootloaders there >>>> >>>> This whole interface was designed such that you can prepare bootable >>>> diskimages for S/390 DASDs, which use a native 4k blocksize. >>> >>> Hi, Hannes, >>> >>> Wasn't insulting you at all, the only part of the interface I'm >>> complaining about is LOOP_GET_STATUS missing information :) >>> >> Sure. Just saying. >> But please make sure only to return that information if the >> LO_FLAGS_BLOCKSIZE flag is set; otherwise there's a rick of confusing >> losetup. > > I actually checked losetup, it works just fine with LO_FLAGS_BLOCKSIZE > always set and lo_init[0] always filled in. > The original argument I had with the util-linux maintainer did not revolve so much around technical details :-) Cheers, Hannes
On Fri, Aug 18, 2017 at 09:56:26AM +0200, Hannes Reinecke wrote: > On 08/18/2017 09:47 AM, Omar Sandoval wrote: > > On Fri, Aug 18, 2017 at 09:43:19AM +0200, Hannes Reinecke wrote: > >> On 08/18/2017 09:38 AM, Omar Sandoval wrote: > >>> On Fri, Aug 18, 2017 at 09:18:52AM +0200, Hannes Reinecke wrote: > [ .. ] > >>>> I've got quite some flak for daring to break existing interfaces, most > >>>> notably setting logical and physical blocksize per default (which I > >>>> would _love_ to have done, seeing that it really makes sense here). > >>>> But as this would change the behaviour I've gone through pains (and > >>>> several _years_ of iterations) to get this sorted. > >>>> > >>>> So if you design a blocktest for that ensure that > >>>> a) the sysfs attributes before and after the patch are _identical_ > >>>> b) the sysfs attributes will only change if the 'LO_FLAGS_BLOCKSIZE' > >>>> flag has been set > >>>> and > >>>> c) validate the written blocksizes; this is required to be able to > >>>> install bootloaders there > >>>> > >>>> This whole interface was designed such that you can prepare bootable > >>>> diskimages for S/390 DASDs, which use a native 4k blocksize. > >>> > >>> Hi, Hannes, > >>> > >>> Wasn't insulting you at all, the only part of the interface I'm > >>> complaining about is LOOP_GET_STATUS missing information :) > >>> > >> Sure. Just saying. > >> But please make sure only to return that information if the > >> LO_FLAGS_BLOCKSIZE flag is set; otherwise there's a rick of confusing > >> losetup. > > > > I actually checked losetup, it works just fine with LO_FLAGS_BLOCKSIZE > > always set and lo_init[0] always filled in. > > > The original argument I had with the util-linux maintainer did not > revolve so much around technical details :-) Karel, what were your concerns here?
On 08/18/2017 10:05 AM, Omar Sandoval wrote: > On Fri, Aug 18, 2017 at 09:56:26AM +0200, Hannes Reinecke wrote: >> On 08/18/2017 09:47 AM, Omar Sandoval wrote: [ .. ] >>> >>> I actually checked losetup, it works just fine with LO_FLAGS_BLOCKSIZE >>> always set and lo_init[0] always filled in. >>> >> The original argument I had with the util-linux maintainer did not >> revolve so much around technical details :-) > > Karel, what were your concerns here? > It wasn't Karel, it was our guy. Doesn't make it any better, though... Cheers, Hannes
On Fri, Aug 18, 2017 at 10:12:51AM +0200, Hannes Reinecke wrote: > On 08/18/2017 10:05 AM, Omar Sandoval wrote: > > On Fri, Aug 18, 2017 at 09:56:26AM +0200, Hannes Reinecke wrote: > >> On 08/18/2017 09:47 AM, Omar Sandoval wrote: > [ .. ] > >>> > >>> I actually checked losetup, it works just fine with LO_FLAGS_BLOCKSIZE > >>> always set and lo_init[0] always filled in. > >>> > >> The original argument I had with the util-linux maintainer did not > >> revolve so much around technical details :-) > > > > Karel, what were your concerns here? > > > It wasn't Karel, it was our guy. > Doesn't make it any better, though... I just went through the code and util-linux doesn't mention lo_init at all except for the definition, and everywhere it's using lo_flags would work fine with the behavior I implemented here. Unless there's an actual issue someone can point out, I see no reason to not do it this way.
On Fri, Aug 18, 2017 at 01:22:26AM -0700, Omar Sandoval wrote: > On Fri, Aug 18, 2017 at 10:12:51AM +0200, Hannes Reinecke wrote: > > On 08/18/2017 10:05 AM, Omar Sandoval wrote: > > > On Fri, Aug 18, 2017 at 09:56:26AM +0200, Hannes Reinecke wrote: > > >> On 08/18/2017 09:47 AM, Omar Sandoval wrote: > > [ .. ] > > >>> > > >>> I actually checked losetup, it works just fine with LO_FLAGS_BLOCKSIZE > > >>> always set and lo_init[0] always filled in. > > >>> > > >> The original argument I had with the util-linux maintainer did not > > >> revolve so much around technical details :-) > > > > > > Karel, what were your concerns here? > > > > > It wasn't Karel, it was our guy. > > Doesn't make it any better, though... > > I just went through the code and util-linux doesn't mention lo_init at > all except for the definition, and everywhere it's using lo_flags would > work fine with the behavior I implemented here. Unless there's an actual > issue someone can point out, I see no reason to not do it this way. Hmm.. we have loopdev API enhancement in Linus' tree and losetup maintainer have no clue about it ;-) Anyway, I like the idea with LO_FLAGS_BLOCKSIZE and lo_init[]. It seems like a backwardly compatible way how to make loopdevs usable for dd(1) images from non-512 devices, etc. For now nothing uses lo_init[], so it seems no problem to use it for LOOP_GET_STATUS64. Note that we use LOOP_GET_STATUS64 and struct loop_info64 only, old API around "struct loop_info" is no more used by util-linux. The important detail is that for losetup (to show mapped devices) is more important /sys than LOOP_GET_STATUS64. The /sys is better because it does not require root permissions and it makes losetup (and lsblk) usable for regular users. I guess /sys/block/loopN/queue/minimum_io_size is affected by Hannes' LO_FLAGS_BLOCKSIZE patches, so non-512 I/O sizes for loopdevs are visible in "lsblk -t /dev/loopN" output, right? I'll add --blocksize <size> to losetup, and BLKSZ column to output to make the new feature usable also for end-users. Thanks for CC: Karel
On Fri, Aug 18, 2017 at 01:22:26AM -0700, Omar Sandoval wrote: > On Fri, Aug 18, 2017 at 10:12:51AM +0200, Hannes Reinecke wrote: > > On 08/18/2017 10:05 AM, Omar Sandoval wrote: > > > On Fri, Aug 18, 2017 at 09:56:26AM +0200, Hannes Reinecke wrote: > > >> On 08/18/2017 09:47 AM, Omar Sandoval wrote: > > [ .. ] > > >>> > > >>> I actually checked losetup, it works just fine with LO_FLAGS_BLOCKSIZE > > >>> always set and lo_init[0] always filled in. > > >>> > > >> The original argument I had with the util-linux maintainer did not > > >> revolve so much around technical details :-) > > > > > > Karel, what were your concerns here? > > > > > It wasn't Karel, it was our guy. > > Doesn't make it any better, though... > > I just went through the code and util-linux doesn't mention lo_init at > all except for the definition, and everywhere it's using lo_flags would > work fine with the behavior I implemented here. Unless there's an actual > issue someone can point out, I see no reason to not do it this way. BTW guys, it seems the current LO_FLAGS_BLOCKSIZE implementation does not care about images from disks where is non-zero alignment offset. It mean disks where is extra offset between logical and physical sectors mapping. Something like: modprobe scsi_debug dev_size_mb=100 sector_size=512 physblk_exp=3 lowest_aligned=7 ...just pedantic note; IMHO it's fine to ignore this use-case ;-) Karel
On Fri, Aug 18, 2017 at 11:22:39AM +0200, Karel Zak wrote: > On Fri, Aug 18, 2017 at 01:22:26AM -0700, Omar Sandoval wrote: > > On Fri, Aug 18, 2017 at 10:12:51AM +0200, Hannes Reinecke wrote: > > > On 08/18/2017 10:05 AM, Omar Sandoval wrote: > > > > On Fri, Aug 18, 2017 at 09:56:26AM +0200, Hannes Reinecke wrote: > > > >> On 08/18/2017 09:47 AM, Omar Sandoval wrote: > > > [ .. ] > > > >>> > > > >>> I actually checked losetup, it works just fine with LO_FLAGS_BLOCKSIZE > > > >>> always set and lo_init[0] always filled in. > > > >>> > > > >> The original argument I had with the util-linux maintainer did not > > > >> revolve so much around technical details :-) > > > > > > > > Karel, what were your concerns here? > > > > > > > It wasn't Karel, it was our guy. > > > Doesn't make it any better, though... > > > > I just went through the code and util-linux doesn't mention lo_init at > > all except for the definition, and everywhere it's using lo_flags would > > work fine with the behavior I implemented here. Unless there's an actual > > issue someone can point out, I see no reason to not do it this way. > > Hmm.. we have loopdev API enhancement in Linus' tree and losetup > maintainer have no clue about it ;-) Ha, glad you're hearing about it now before it's too late :) > Anyway, I like the idea with LO_FLAGS_BLOCKSIZE and lo_init[]. It > seems like a backwardly compatible way how to make loopdevs usable > for dd(1) images from non-512 devices, etc. > > For now nothing uses lo_init[], so it seems no problem to use it for > LOOP_GET_STATUS64. Do you see any issues with the behavior I added in this patch? Namely, LOOP_GET_STATUS{,64} will always return lo_flags with LO_FLAGS_BLOCKSIZE set and lo_init[0] will always contain the blocksize. Older userspace which doesn't know about LO_FLAGS_BLOCKSIZE will just ignore it and the LOOP_GET_STATUS64; modify stuff; LOOP_SET_STATUS64 pattern will still work. > Note that we use LOOP_GET_STATUS64 and struct loop_info64 only, old API > around "struct loop_info" is no more used by util-linux. > > The important detail is that for losetup (to show mapped devices) is > more important /sys than LOOP_GET_STATUS64. The /sys is better > because it does not require root permissions and it makes losetup > (and lsblk) usable for regular users. Ah, I'll spin a followup patch to add it to sysfs. > I guess /sys/block/loopN/queue/minimum_io_size is affected by Hannes' > LO_FLAGS_BLOCKSIZE patches, so non-512 I/O sizes for loopdevs are > visible in "lsblk -t /dev/loopN" output, right? Yup: # lsblk -t /dev/loop0 NAME ALIGNMENT MIN-IO OPT-IO PHY-SEC LOG-SEC ROTA SCHED RQ-SIZE RA WSAME loop0 0 4096 0 4096 4096 1 mq-deadline 256 128 0B > I'll add --blocksize <size> to losetup, and BLKSZ column to output to > make the new feature usable also for end-users. Thank you!
On Fri, Aug 18, 2017 at 09:44:25AM -0700, Omar Sandoval wrote: > On Fri, Aug 18, 2017 at 11:22:39AM +0200, Karel Zak wrote: > > On Fri, Aug 18, 2017 at 01:22:26AM -0700, Omar Sandoval wrote: > > > On Fri, Aug 18, 2017 at 10:12:51AM +0200, Hannes Reinecke wrote: > > > > On 08/18/2017 10:05 AM, Omar Sandoval wrote: > > > > > On Fri, Aug 18, 2017 at 09:56:26AM +0200, Hannes Reinecke wrote: > > > > >> On 08/18/2017 09:47 AM, Omar Sandoval wrote: > > > > [ .. ] > > > > >>> > > > > >>> I actually checked losetup, it works just fine with LO_FLAGS_BLOCKSIZE > > > > >>> always set and lo_init[0] always filled in. > > > > >>> > > > > >> The original argument I had with the util-linux maintainer did not > > > > >> revolve so much around technical details :-) > > > > > > > > > > Karel, what were your concerns here? > > > > > > > > > It wasn't Karel, it was our guy. > > > > Doesn't make it any better, though... > > > > > > I just went through the code and util-linux doesn't mention lo_init at > > > all except for the definition, and everywhere it's using lo_flags would > > > work fine with the behavior I implemented here. Unless there's an actual > > > issue someone can point out, I see no reason to not do it this way. > > > > Hmm.. we have loopdev API enhancement in Linus' tree and losetup > > maintainer have no clue about it ;-) > > Ha, glad you're hearing about it now before it's too late :) > > > Anyway, I like the idea with LO_FLAGS_BLOCKSIZE and lo_init[]. It > > seems like a backwardly compatible way how to make loopdevs usable > > for dd(1) images from non-512 devices, etc. > > > > For now nothing uses lo_init[], so it seems no problem to use it for > > LOOP_GET_STATUS64. > > Do you see any issues with the behavior I added in this patch? Namely, > LOOP_GET_STATUS{,64} will always return lo_flags with LO_FLAGS_BLOCKSIZE > set and lo_init[0] will always contain the blocksize. Older userspace > which doesn't know about LO_FLAGS_BLOCKSIZE will just ignore it and the > LOOP_GET_STATUS64; modify stuff; LOOP_SET_STATUS64 pattern will still > work. Yes, I think your idea is correct. All unknown flags should be ignored by userspace. This is not the first time the API (lo_flags) is extended by a new flag. Karel
diff --git a/drivers/block/loop.c b/drivers/block/loop.c index ef8334949b42..39fa7f48e0c7 100644 --- a/drivers/block/loop.c +++ b/drivers/block/loop.c @@ -234,7 +234,7 @@ 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) { + if (lo->lo_logical_blocksize != logical_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, @@ -820,7 +820,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(lo->lo_logical_blocksize); /* * We use punch hole to reclaim the free space used by the @@ -840,8 +840,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); @@ -1061,6 +1059,7 @@ static int loop_clr_fd(struct loop_device *lo) lo->lo_offset = 0; lo->lo_sizelimit = 0; lo->lo_encrypt_key_size = 0; + lo->lo_logical_blocksize = 512; memset(lo->lo_encrypt_key, 0, LO_KEY_SIZE); memset(lo->lo_crypt_name, 0, LO_NAME_SIZE); memset(lo->lo_file_name, 0, LO_NAME_SIZE); @@ -1105,6 +1104,7 @@ loop_set_status(struct loop_device *lo, const struct loop_info64 *info) struct loop_func_table *xfer; kuid_t uid = current_uid(); int lo_flags = lo->lo_flags; + unsigned lo_logical_blocksize; if (lo->lo_encrypt_key_size && !uid_eq(lo->lo_key_owner, uid) && @@ -1138,25 +1138,24 @@ loop_set_status(struct loop_device *lo, const struct loop_info64 *info) 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 && - LO_INFO_BLOCKSIZE(info) != 4096) + lo_logical_blocksize = LO_INFO_BLOCKSIZE(info); + if (lo_logical_blocksize != 512 && + lo_logical_blocksize != 1024 && + lo_logical_blocksize != 2048 && + lo_logical_blocksize != 4096) return -EINVAL; - if (LO_INFO_BLOCKSIZE(info) > lo->lo_blocksize) + if (lo_logical_blocksize > lo->lo_blocksize) return -EINVAL; + } else { + lo_logical_blocksize = lo->lo_logical_blocksize; } 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))) { + lo->lo_logical_blocksize != lo_logical_blocksize) { if (figure_loop_size(lo, info->lo_offset, info->lo_sizelimit, - LO_INFO_BLOCKSIZE(info))) { + lo_logical_blocksize)) { err = -EFBIG; goto exit; } @@ -1223,7 +1222,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 = @@ -1233,6 +1232,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) = lo->lo_logical_blocksize; return 0; } @@ -1835,6 +1835,7 @@ static int loop_add(struct loop_device **l, int i) mutex_init(&lo->lo_ctl_mutex); atomic_set(&lo->lo_refcnt, 0); lo->lo_number = i; + lo->lo_logical_blocksize = 512; spin_lock_init(&lo->lo_lock); disk->major = LOOP_MAJOR; disk->first_minor = i << part_shift;