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

[Paul Durrant <Paul.Durrant@xxxxxxxxxx>]

> -----Original Message-----
> From: Xen-devel [mailto:xen-devel-bounces@xxxxxxxxxxxxxxxxxxxx] On Behalf Of 
> Konrad Rzeszutek Wilk
> Sent: 20 March 2019 14:28
> To: Paul Durrant <Paul.Durrant@xxxxxxxxxx>
> Cc: 'xen-devel@xxxxxxxxxxxxxxxxxxxx' <xen-devel@xxxxxxxxxxxxxxxxxxxx>
> Subject: Re: [Xen-devel] [PATCH] public/io/blkif.h: make the comments on 
> "sectors" self-consistent
> 
> On Wed, Mar 20, 2019 at 02:05:15PM +0000, Paul Durrant wrote:
> > > -----Original Message-----
> > > From: Paul Durrant
> > > Sent: 20 March 2019 13:59
> > > To: 'Konrad Rzeszutek Wilk' <konrad.wilk@xxxxxxxxxx>
> > > Cc: xen-devel@xxxxxxxxxxxxxxxxxxxx
> > > Subject: RE: [PATCH] public/io/blkif.h: make the comments on "sectors" 
> > > self-consistent
> > >
> > > > -----Original Message-----
> > > > From: Konrad Rzeszutek Wilk [mailto:konrad.wilk@xxxxxxxxxx]
> > > > Sent: 20 March 2019 13:53
> > > > To: Paul Durrant <Paul.Durrant@xxxxxxxxxx>
> > > > Cc: xen-devel@xxxxxxxxxxxxxxxxxxxx
> > > > Subject: Re: [PATCH] public/io/blkif.h: make the comments on "sectors" 
> > > > self-consistent
> > > >
> > > > 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.
> > > >
> > > > I thought it actually did work with a CD-ROM backed ISO using blkfront?
> > > >
> > >
> > > I've not tried that. What worries me is the call to xlvbd_alloc_gendisk() 
> > > which takes sectors as
> an
> > > argument and passes it through to set_capacity() without scaling with 
> > > sector-size in any way,
> which is
> > > would presumably need to do is sector-size != 512 (if we believe that 
> > > sectors should be in terms
> of
> > > 512 byte units).
> >
> > Looking at xen-blkback, this actually just echoes the result of 
> > get_capacity() into 'sectors', which
> would suggest the comment at line #613 is the one that is bogus... but how 
> many other frontends have
> been coded against that? So, it would seem to me that the only way to get out 
> of this compatibility
> mess is to make sector-size strictly 512.
> 
> Bye bye 4096 sectore-size :-)
> 
> Ugh, and we do <<9 all over the code so it is fairly baked.

It's going to be a struggle to get out of this but maybe I can at least make it 
work for QEMU as a backend and Windows as a frontend.

> 
> Reviewed-by: Konrad Rzeszutek Wilk <konrad.wilk@xxxxxxxxxx>

Thanks. At least this nails down how the values *should* be interpreted.

  Paul

> 
> 
> >
> >   Paul
> >
> > >
> > >   Paul
> > >
> > > > > ---
> > > > >  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
> > > > > --
> > > > > 2.20.1.2.gb21ebb671
> > > > >
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@xxxxxxxxxxxxxxxxxxxx
> https://lists.xenproject.org/mailman/listinfo/xen-devel
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel