Re: [Xen-devel] [PATCH v2 0/2] xen-block: fix sector size confusion

[Paul Durrant <Paul.Durrant@xxxxxxxxxx>]

> -----Original Message-----
> From: Andrew Cooper
> Sent: 28 March 2019 11:46
> To: Anthony Perard <anthony.perard@xxxxxxxxxx>; Paul Durrant 
> <Paul.Durrant@xxxxxxxxxx>
> Cc: xen-devel@xxxxxxxxxxxxxxxxxxxx; qemu-block@xxxxxxxxxx; 
> qemu-devel@xxxxxxxxxx; Kevin Wolf
> <kwolf@xxxxxxxxxx>; Stefano Stabellini <sstabellini@xxxxxxxxxx>; Max Reitz 
> <mreitz@xxxxxxxxxx>; Stefan
> Hajnoczi <stefanha@xxxxxxxxxx>
> Subject: Re: [Xen-devel] [PATCH v2 0/2] xen-block: fix sector size confusion
> 
> On 28/03/2019 11:40, Anthony PERARD wrote:
> > On Wed, Mar 27, 2019 at 08:32:28PM +0000, Paul Durrant wrote:
> >>> -----Original Message-----
> >>> From: Andrew Cooper
> >>> Sent: 27 March 2019 18:20
> >>> To: Paul Durrant <Paul.Durrant@xxxxxxxxxx>; 
> >>> xen-devel@xxxxxxxxxxxxxxxxxxxx; qemu-block@xxxxxxxxxx;
> >>> qemu-devel@xxxxxxxxxx
> >>> Cc: Kevin Wolf <kwolf@xxxxxxxxxx>; Stefano Stabellini 
> >>> <sstabellini@xxxxxxxxxx>; Max Reitz
> >>> <mreitz@xxxxxxxxxx>; Stefan Hajnoczi <stefanha@xxxxxxxxxx>; Anthony Perard
> <anthony.perard@xxxxxxxxxx>
> >>> Subject: Re: [Xen-devel] [PATCH v2 0/2] xen-block: fix sector size 
> >>> confusion
> >>>
> >>> On 27/03/2019 17:32, Paul Durrant wrote:
> >>>> The Xen blkif protocol is confusing but discussion with the maintainer
> >>>> has clarified that sector based quantities in requests and the 'sectors'
> >>>> value advertized in xenstore should always be in terms of 512-byte
> >>>> units and not the advertised logical 'sector-size' value.
> >>>>
> >>>> This series fixes xen-block to adhere to the spec.
> >>> I thought we agreed that hardcoding things to 512 bytes was the wrong
> >>> thing to do.
> >> To some extent we decided it was the *only* thing to do.
> >>
> >>> I was expecting something like:
> >>>
> >>> 1) Clarify the spec with the intended meaning, (which is what some
> >>> implementations actually use already) and wont cripple 4k datapaths.
> >>> 2) Introduce a compatibility key for "I don't rely on sector-size being
> >>> 512", which fixed implementations should advertise.
> >>> 3) Specify that because of bugs in the spec which got out into the wild,
> >>> drivers which don't find the key being advertised by the other end
> >>> should emulate sector-size=512 for compatibility with broken
> >>> implementations.
> >> Yes, that's how we are going to fix things.
> >>
> >>> Whatever the eventual way out, the first thing which needs to happen is
> >>> an update to the spec, before actions are taken to alter existing
> >>> implementations.
> >> Well the implementation is currently wrong w.r.t. the spec and these 
> >> patches fix that. As long as
> sector-size remains at 512 then no existing frontend should break, so I guess 
> you could argue that
> patch #2 should also make sure that sector-size is also 512... but that is 
> not yet in the spec.
> >> I guess I'm ok to defer patch #2 until a revised spec. is agreed, but the 
> >> ship has already sailed
> as far as patch #1 goes.
> >>
> >> Anthony, thoughts?
> > So QEMU used to always set "sector-size" to 512, and used that for
> > request. The new implementation (not released yet) doesn't do that
> > anymore, and may set "sector-size" to a different value and used that
> > for requests.
> >
> > patch #1 is one way to fix the requests (and avoid regression) and
> > more clearly spell out the weird thing about the spec.
> >
> > I also think patch #2 is too soon and should point to a commit in
> > xen.git instead of a thread on xen-devel.
> >
> > In the meantime, we should probably set "sector-size" to 512, like QEMU
> > used to do anyway, with a comment about the fact that different
> > implementations uses sector-size differently and a value of 512 would
> > work fine.
> 
> Hmm - I hadn't realised this is an unreleased issue in qemu.
> 
> So, Qemu used to unconditionally set sector-size=512, and your work to
> qdev-ify everything introduced a change which has identified a
> spec/protocol issue?

Basically, yes. I had not realized at the time how bad the spec. is. I was 
referring to prevailing implementations, which seemed to use sector-size as the 
multiple and, in the Windows case, appear to cope with a logical sector size 
other than 512.
Given what I know now, fixing sector-size to be 512 seems like the only way to 
avoid regressions.

> 
> If so, then I think it is fine for this series to state (much more
> clearly than it does) that it is returning qemu's behaviour to match the
> currently released version, because we've discovered an issue in the
> spec/protocol, and that we will subsequently work address the issue in
> the spec and provide a forwards path which doesn't involve nailing our
> feet to the floor.

Ok, I'll expand the commit comment and state that the spec. will be revised to 
allow greater logical block sizes.

  Paul

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