loop: LOOP_FLAGS_BLOCKSIZE fixes

From: Omar Sandoval <[email protected]>

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 <[email protected]>

---
 drivers/block/loop.c | 33 +++++++++++++++++----------------
 1 file changed, 17 insertions(+), 16 deletions(-)

Quoted 11 lines · Omar Sandoval

From: Omar Sandoval <[email protected]>

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

Quoted 8 lines

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 <[email protected]>

---
 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.

Quoted 39 lines · Hannes Reinecke

Quoted 11 lines · Omar Sandoval

From: Omar Sandoval <[email protected]>

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

Quoted 8 lines

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 <[email protected]>

---
 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

Quoted 2 lines

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 :)

Quoted 45 lines · Omar Sandoval

Quoted 39 lines · Hannes Reinecke

Quoted 11 lines · Omar Sandoval

From: Omar Sandoval <[email protected]>

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

Quoted 8 lines

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 <[email protected]>

---
 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

Quoted 2 lines

This whole interface was designed such that you can prepare bootable diskimages for S/390 DASDs, which use a native 4k blocksize.

Hi, Hannes,

Quoted 3 lines

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.

Quoted 2 lines · Hannes Reinecke

Quoted 1 line · Omar Sandoval

On Fri, Aug 18, 2017 at 09:18:52AM +0200, Hannes Reinecke wrote:

Quoted 22 lines

Quoted 22 lines

Quoted 14 lines

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

Quoted 2 lines

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 :)

Quoted 4 lines

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.

Quoted 3 lines · Omar Sandoval

Quoted 2 lines · Hannes Reinecke

Quoted 1 line · Omar Sandoval

On Fri, Aug 18, 2017 at 09:18:52AM +0200, Hannes Reinecke wrote:

[ .. ]

Quoted 27 lines

Quoted 26 lines

Quoted 14 lines

Quoted 14 lines

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

Quoted 8 lines

Quoted 2 lines

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.

Quoted 3 lines

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 :-)

Quoted 1 line · Hannes Reinecke

On 08/18/2017 09:47 AM, Omar Sandoval wrote:

Quoted 6 lines

Quoted 4 lines

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?

Quoted 2 lines · Omar Sandoval

Quoted 1 line · Hannes Reinecke

On 08/18/2017 09:47 AM, Omar Sandoval wrote:

[ .. ]

Quoted 4 lines

Quoted 4 lines

Quoted 4 lines

I actually checked losetup, it works just fine with LO_FLAGS_BLOCKSIZE always set and lo_init[0] always filled in.

Quoted 5 lines

Quoted 2 lines

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...

Quoted 13 lines · Hannes Reinecke

Quoted 2 lines · Omar Sandoval

Quoted 1 line · Hannes Reinecke

On 08/18/2017 09:47 AM, Omar Sandoval wrote:

[ .. ]

Quoted 9 lines

Quoted 6 lines

Quoted 4 lines

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?

Quoted 2 lines

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.

Quoted 17 lines · Omar Sandoval

Quoted 15 lines · Hannes Reinecke

Quoted 2 lines · Omar Sandoval

Quoted 1 line · Hannes Reinecke

On 08/18/2017 09:47 AM, Omar Sandoval wrote:

[ .. ]

Quoted 9 lines

Quoted 6 lines

Quoted 4 lines

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...

Quoted 4 lines

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.

Quoted 18 lines · Karel Zak

Quoted 17 lines · Omar Sandoval

Quoted 15 lines · Hannes Reinecke

Quoted 2 lines · Omar Sandoval

Quoted 1 line · Hannes Reinecke

On 08/18/2017 09:47 AM, Omar Sandoval wrote:

[ .. ]

Quoted 9 lines

Quoted 6 lines

Quoted 4 lines

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...

Quoted 7 lines

Quoted 4 lines

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 :)

Quoted 6 lines

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.

Quoted 13 lines · Hannes Reinecke

Quoted 2 lines · Omar Sandoval

Quoted 1 line · Hannes Reinecke

On 08/18/2017 09:47 AM, Omar Sandoval wrote:

[ .. ]

Quoted 9 lines

Quoted 6 lines

Quoted 4 lines

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?

Quoted 2 lines

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.

From: Omar Sandoval <[email protected]>

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")

Signed-off-by Omar Sandoval <[email protected]>

---
 drivers/block/loop.c | 24 ++++++++++++++++--------
 1 file changed, 16 insertions(+), 8 deletions(-)

drivers/block/loop.c +16 -8

index 39fa7f48e0c7..6c7609b3305d 100644
@@ -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;
 

@@ -1142,10 +1146,14 @@ loop_set_status(struct loop_device *lo, const struct loop_info64 *info)
 		if (lo_logical_blocksize != 512 &&
 		    lo_logical_blocksize != 1024 &&
 		    lo_logical_blocksize != 2048 &&
-		    lo_logical_blocksize != 4096)
-			return -EINVAL;
-		if (lo_logical_blocksize > lo->lo_blocksize)
-			return -EINVAL;
+		    lo_logical_blocksize != 4096) {
+			err = -EINVAL;
+			goto exit;
+		}
+		if (lo_logical_blocksize > lo->lo_blocksize) {
+			err = -EINVAL;
+			goto exit;
+		}
 	} else {
 		lo_logical_blocksize = lo->lo_logical_blocksize;
 	}

Reviewed-by Hannes Reinecke <[email protected]>

From: Omar Sandoval <[email protected]>

A couple of bugs I found while adding blktests for the new loop block size feature. Based on v4.13-rc5, we should get these in for 4.13.