Re: [Xen-devel] [PATCH] public/io/blkif.h: make the comments on "sectors" self-consistent

[Paul Durrant <Paul.Durrant@xxxxxxxxxx>]

> -----Original Message-----
> From: Anthony PERARD [mailto:anthony.perard@xxxxxxxxxx]
> Sent: 21 March 2019 15:24
> To: Paul Durrant <Paul.Durrant@xxxxxxxxxx>
> Cc: xen-devel@xxxxxxxxxxxxxxxxxxxx; Konrad Rzeszutek Wilk 
> <konrad.wilk@xxxxxxxxxx>
> Subject: Re: [Xen-devel] [PATCH] public/io/blkif.h: make the comments on 
> "sectors" self-consistent
> 
> On Thu, Mar 21, 2019 at 12:30:44PM +0000, Paul Durrant wrote:
> > > On Wed, Mar 20, 2019 at 12:52:28PM +0000, Paul Durrant wrote:
> > > > Currently the comment at line #267 claims that the value should be
> > > > expressed in number logical sectors, whereas the comment at line #613
> > > > states that the value should be expressed strictly in units of 512 
> > > > bytes.
> > > >
> > > > Signed-off-by: Paul Durrant <paul.durrant@xxxxxxxxxx>
> > > > ---
> > > > Cc: Konrad Rzeszutek Wilk <konrad.wilk@xxxxxxxxxx>
> > > >
> > > > Looking at xen-blkfront in Linux, I'm also not convinced that it would
> > > > function correctly is sector-size != 512 anyway so I wonder whether this
> > > > patch should go further and define that sector-size is strictly 512.
> > > > ---
> > > >  xen/include/public/io/blkif.h | 3 +--
> > > >  1 file changed, 1 insertion(+), 2 deletions(-)
> > > >
> > > > diff --git a/xen/include/public/io/blkif.h 
> > > > b/xen/include/public/io/blkif.h
> > > > index 15a71e3fea..d7c904d9dc 100644
> > > > --- a/xen/include/public/io/blkif.h
> > > > +++ b/xen/include/public/io/blkif.h
> > > > @@ -264,8 +264,7 @@
> > > >   * sectors
> > > >   *      Values:         <uint64_t>
> > > >   *
> > > > - *      The size of the backend device, expressed in units of its 
> > > > logical
> > > > - *      sector size ("sector-size").
> > > > + *      The size of the backend device, expressed in units of 512 
> > > > bytes.
> > > >   *
> > > >   
> > > > *****************************************************************************
> > > >   *                            Frontend XenBus Nodes
> > >
> > > But OVMF's frontend, minios' frontend, FreeBSD's frontend all do
> > > sector-size * sectors to figure out the size of the media.
> > > But looks like for at least OVMF, IO requests are handled with a sectors
> > > size of 512.
> > >
> > > I think FreeBSD's backend also set "sectors" based on "sector-size", but
> > > on the other hand, "sector-size" is always set to 512.
> > > I think it the same for the old qemu (before Paul's refactoring).
> > >
> > > I think I would be fine with the patch going further and have
> > > "sector-size" always 512, as some implementation are backed with
> > > this assumption (Linux, which I haven't checked).
> > >
> > > (I don't want to have to patch OVMF because the protocol changed.)
> >
> > The problem we face is backends pointing at disks that don't do 512 byte 
> > logical block emulation.
> There has to be some hope for dealing with them. If we say that the current 
> state of affairs is that
> everything is broken (which I think it is as far as Linux blkfront/blkback 
> are concerned) so we
> enforce sector-size == 512 in the protocol then the only hope I can see is 
> for frontends to introduce
> a flag to say 'no emulation' to prompt the frontend to use 
> physical-sector-size as logical sector-
> size... or something like that.
> 
> I couldn't figure out how Linux interpret the "sectors" node.
> 
> The only problem I see is the contradiction between line #267 and #613
> on the unit of "sectors", "sector-size"-bytes vs 512-bytes.
> 
> Otherwise, `sector_number`, `first_sect` and `last_sect` in
> blkif_request_t and blkif_request_segment are defined as 512-bytes.
> 
> I think this is how the currents implementations are working:
>     media/disk size = "sectors" * "sector-size"
> then, "sectors" and "sector-size" are never used again.

Not true, unfortunately. At least the Windows frontends are (mis)coded to use 
sector numbers directly in blkif_request_t and blkif_request_segment rather 
than re-scaling to 512 bytes, so setting sector-size != 512 will certainly make 
them misbehave according to the protocol. This can, of course, be fixed but I 
think we're at point where the only safe way to set a larger sector-size would 
be to have the frontend write an 'I'm not broken' flag into xenstore that the 
backend reads before setting sector-size (and if that means that the backend 
has to do read-modify-write cycles for an underlying storage device with a 
larger logical block size then so be it).

> 
> 
> I don't know is there is actally a problem with disks don't understand
> 512 bytes logical block, but there is a comment attach to
> blkif_request_t:
>     However they must be properly aligned to the real sector size of the
>     physical disk, "physical-sector-size"
> 

Again, this is an issue for Windows where I've experimented setting a 
physical-sector-size == 4096 and found that the storage stack apparently 
completely ignores it, and just aligns to logical block size. The only way out 
of that one will be to have the frontend do read-modify-write cycles and that's 
not really desirable. I don't know whether other guest storage stacks are any 
better behaved.

  Paul

> 
> Cheers,
> 
> --
> Anthony PERARD

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel