Re: [PATCH] blkif: reconcile protocol specification with in-use implementations

[Anthony PERARD <anthony.perard@vates.tech>]

On Wed, Sep 04, 2024 at 11:11:51AM +0200, Roger Pau Monné wrote:
> On Wed, Sep 04, 2024 at 09:39:17AM +0100, Paul Durrant wrote:
> > On 04/09/2024 09:21, Roger Pau Monné wrote:
> > > > In the absence of that I'm afraid it is a little harder to
> > > > judge whether the proposal here is the best we can do at this point.
> > >
> > > While I don't mind looking at what we can do to better handle 4K
> > > sector disks, we need IMO to revert to the specification before
> > > 67e1c050e36b, as that change switched the hardcoded sector based units
> > > from 512 to 'sector-size', thus breaking the existing ABI.
> > >
> >
> > But that's the crux of the problem. What *is* is the ABI? We apparently
> > don't have one that all OS subscribe to.
>
> At least prior to 67e1c050e36b the specification in blkif.h and (what
> I consider) the reference implementation in Linux blk{front,back}
> matched.  Previous to 67e1c050e36b blkif.h stated:
>
> /*
>  * NB. first_sect and last_sect in blkif_request_segment, as well as
>  * sector_number in blkif_request, are always expressed in 512-byte units.
>  * However they must be properly aligned to the real sector size of the
>  * physical disk, which is reported in the "physical-sector-size" node in
>  * the backend xenbus info. Also the xenbus "sectors" node is expressed in
>  * 512-byte units.
>  */
>
> I think it was quite clear, and does in fact match the implementation
> in Linux.

That's wrong, Linux doesn't match the specification before 67e1c050e36b,
in particular for "sectors":

    sectors
         Values:         <uint64_t>

         The size of the backend device, expressed in units of its logical
         sector size ("sector-size").

The only implementation that matches this specification is MiniOS (and
OMVF).

Oh, I didn't notice that that random comment you quoted that comes from
the middle of the header have a different definition for "sectors" ...

Well, the specification doesn't match with the specification ... and the
only possible way to implement the specification is to only ever set
"sector-size" to 512...

No wonder that they are so many different interpretation of the
protocol.

Cheers,

--

Anthony Perard | Vates XCP-ng Developer

XCP-ng & Xen Orchestra - Vates solutions

web: https://vates.tech