[PATCH 0/2] loop: LOOP_FLAGS_BLOCKSIZE fixes

[PATCH 0/2] loop: LOOP_FLAGS_BLOCKSIZE fixes

[Omar Sandoval]

From: Omar Sandoval <osandov@fb.com>

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.

Thanks!

Omar Sandoval (2):
  loop: always return block size in LOOP_GET_STATUS
  loop: fix hang if LOOP_SET_STATUS gets invalid blocksize or encrypt
    type

 drivers/block/loop.c | 53 ++++++++++++++++++++++++++++++----------------------
 1 file changed, 31 insertions(+), 22 deletions(-)

-- 
2.14.1

[PATCH 1/2] loop: always return block size in LOOP_GET_STATUS

[Omar Sandoval]

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

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

[PATCH 2/2] loop: fix hang if LOOP_SET_STATUS gets invalid blocksize or encrypt type

[Omar Sandoval]

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")
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 39fa7f48e0c7..6c7609b3305d 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;
 
@@ -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;
 	}
-- 
2.14.1

Re: [PATCH 0/2] loop: LOOP_FLAGS_BLOCKSIZE fixes

[Omar Sandoval]

On Thu, Aug 17, 2017 at 11:15:24PM -0700, Omar Sandoval wrote:
> From: Omar Sandoval <osandov@fb.com>
> 
> 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.

Just pushed the test: https://github.com/osandov/blktests/commit/7320725134cc0b65588bf03259201d74a0ee8226

Re: [PATCH 1/2] loop: always return block size in LOOP_GET_STATUS

[Hannes Reinecke]

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
-- 
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: [PATCH 2/2] loop: fix hang if LOOP_SET_STATUS gets invalid blocksize or encrypt type

[Hannes Reinecke]

On 08/18/2017 08:15 AM, Omar Sandoval wrote:
> 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")
> 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 39fa7f48e0c7..6c7609b3305d 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;
>  
> @@ -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 <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)

Re: [PATCH 1/2] loop: always return block size in LOOP_GET_STATUS

[Omar Sandoval]

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

Re: [PATCH 1/2] loop: always return block size in LOOP_GET_STATUS

[Hannes Reinecke]

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
-- 
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: [PATCH 1/2] loop: always return block size in LOOP_GET_STATUS

[Omar Sandoval]

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.

Re: [PATCH 1/2] loop: always return block size in LOOP_GET_STATUS

[Hannes Reinecke]

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
-- 
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: [PATCH 1/2] loop: always return block size in LOOP_GET_STATUS

[Omar Sandoval]

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?

Re: [PATCH 1/2] loop: always return block size in LOOP_GET_STATUS

[Hannes Reinecke]

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
-- 
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: [PATCH 1/2] loop: always return block size in LOOP_GET_STATUS

[Omar Sandoval]

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.

Re: [PATCH 1/2] loop: always return block size in LOOP_GET_STATUS

[Karel Zak]

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

-- 
 Karel Zak  <kzak@redhat.com>
 http://karelzak.blogspot.com

Re: [PATCH 1/2] loop: always return block size in LOOP_GET_STATUS

[Karel Zak]

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

-- 
 Karel Zak  <kzak@redhat.com>
 http://karelzak.blogspot.com

Re: [PATCH 1/2] loop: always return block size in LOOP_GET_STATUS

[Omar Sandoval]

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!

Re: [PATCH 1/2] loop: always return block size in LOOP_GET_STATUS

[Karel Zak]

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

-- 
 Karel Zak  <kzak@redhat.com>
 http://karelzak.blogspot.com