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

[Omar Sandoval <osandov@osandov.com>]

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.