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

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

[Paul Durrant]

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

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

[Konrad Rzeszutek Wilk]

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?

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

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

[Paul Durrant]

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

  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

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

[Paul Durrant]

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

  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

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

[Konrad Rzeszutek Wilk]

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

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

[Paul Durrant]

> -----Original Message-----
> From: Xen-devel [mailto:xen-devel-bounces@lists.xenproject.org] On Behalf Of Konrad Rzeszutek Wilk
> Sent: 20 March 2019 14:28
> To: Paul Durrant <Paul.Durrant@citrix.com>
> Cc: 'xen-devel@lists.xenproject.org' <xen-devel@lists.xenproject.org>
> 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@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.

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@oracle.com>

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

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

[Anthony PERARD]

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

-- 
Anthony PERARD

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

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

[Paul Durrant]

> -----Original Message-----
> From: Anthony PERARD [mailto:anthony.perard@citrix.com]
> Sent: 21 March 2019 12:22
> To: Paul Durrant <Paul.Durrant@citrix.com>
> Cc: xen-devel@lists.xenproject.org; Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
> Subject: Re: [Xen-devel] [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.
> > ---
> >  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.

  Paul

> 
> --
> Anthony PERARD

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

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

[Anthony PERARD]

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

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

[Paul Durrant]

> -----Original Message-----
> From: Anthony PERARD [mailto:anthony.perard@citrix.com]
> Sent: 21 March 2019 15:24
> To: Paul Durrant <Paul.Durrant@citrix.com>
> Cc: xen-devel@lists.xenproject.org; Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
> Subject: Re: [Xen-devel] [PATCH] public/io/blkif.h: make the comments on "sectors" self-consistent
> 
> 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@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.
> > > > ---
> > > >  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.

Not true, unfortunately. At least the Windows frontends are (mis)coded to use sector numbers directly in blkif_request_t and blkif_request_segment rather than re-scaling to 512 bytes, so setting sector-size != 512 will certainly make them misbehave according to the protocol. This can, of course, be fixed but I think we're at point where the only safe way to set a larger sector-size would be to have the frontend write an 'I'm not broken' flag into xenstore that the backend reads before setting sector-size (and if that means that the backend has to do read-modify-write cycles for an underlying storage device with a larger logical block size then so be it).

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

Again, this is an issue for Windows where I've experimented setting a physical-sector-size == 4096 and found that the storage stack apparently completely ignores it, and just aligns to logical block size. The only way out of that one will be to have the frontend do read-modify-write cycles and that's not really desirable. I don't know whether other guest storage stacks are any better behaved.

  Paul

> 
> Cheers,
> 
> --
> Anthony PERARD

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

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

[Anthony PERARD]

> > 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.
> 
> Not true, unfortunately. At least the Windows frontends are (mis)coded to use sector numbers directly in blkif_request_t and blkif_request_segment rather than re-scaling to 512 bytes, so setting sector-size != 512 will certainly make them misbehave according to the protocol. This can, of course, be fixed but I think we're at point where the only safe way to set a larger sector-size would be to have the frontend write an 'I'm not broken' flag into xenstore that the backend reads before setting sector-size (and if that means that the backend has to do read-modify-write cycles for an underlying storage device with a larger logical block size then so be it).

So, hard-coding "sector-size" to 512 in blkif.h sound good to me.

-- 
Anthony PERARD

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

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

[Paul Durrant]

> -----Original Message-----
> From: Anthony PERARD [mailto:anthony.perard@citrix.com]
> Sent: 21 March 2019 17:23
> To: Paul Durrant <Paul.Durrant@citrix.com>
> Cc: xen-devel@lists.xenproject.org; Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
> Subject: Re: [Xen-devel] [PATCH] public/io/blkif.h: make the comments on "sectors" self-consistent
> 
> > > 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.
> >
> > Not true, unfortunately. At least the Windows frontends are (mis)coded to use sector numbers
> directly in blkif_request_t and blkif_request_segment rather than re-scaling to 512 bytes, so setting
> sector-size != 512 will certainly make them misbehave according to the protocol. This can, of course,
> be fixed but I think we're at point where the only safe way to set a larger sector-size would be to
> have the frontend write an 'I'm not broken' flag into xenstore that the backend reads before setting
> sector-size (and if that means that the backend has to do read-modify-write cycles for an underlying
> storage device with a larger logical block size then so be it).
> 
> So, hard-coding "sector-size" to 512 in blkif.h sound good to me.

Ok, I'll send a v2 that does that. I think it best if I also include a specification my proposed 'I'm not broken' flag so a frontend can enable a backend to set a non-512 sector size.

  Paul

> 
> --
> Anthony PERARD

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