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

[Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>]

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@oracle.com>
> > Cc: xen-devel@lists.xenproject.org
> > Subject: RE: [PATCH] public/io/blkif.h: make the comments on "sectors" self-consistent
> > 
> > > -----Original Message-----
> > > From: Konrad Rzeszutek Wilk [mailto:konrad.wilk@oracle.com]
> > > Sent: 20 March 2019 13:53
> > > To: Paul Durrant <Paul.Durrant@citrix.com>
> > > Cc: xen-devel@lists.xenproject.org
> > > 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@citrix.com>
> > > > ---
> > > > Cc: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
> > > >
> > > > 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.

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


> 
>   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@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel