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

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

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?

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.

~Andrew

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