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

[Konrad Rzeszutek Wilk <konrad.wilk@xxxxxxxxxx>]

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.

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


> 
>   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