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

[Anthony PERARD <anthony.perard@xxxxxxxxxx>]

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.


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"


Cheers,

-- 
Anthony PERARD

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