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

[Kevin Wolf <kwolf@xxxxxxxxxx>]

Am 28.03.2019 um 12:46 hat Andrew Cooper geschrieben:
> 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?

The old implementation has the sector size hardcoded:

    #define BLOCK_SIZE  512

Whereas the qdevified version uses DEFINE_BLOCK_PROPERTIES(), which
includes user-visible options for logical/physical_block_size.

So before, you couldn't even define a different sector size and the
question whether 512 or the sector size should be used didn't make a
difference anyway.

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

The closest thing to returning to the old behaviour would be erroring
out during device initialisation if logical_block_size != 512.

Kevin

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