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

[Anthony PERARD]

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 <address@hidden>; address@hidden; address@hidden;
> > address@hidden
> > Cc: Kevin Wolf <address@hidden>; Stefano Stabellini <address@hidden>; Max 
> > Reitz
> > <address@hidden>; Stefan Hajnoczi <address@hidden>; Anthony Perard 
> > <address@hidden>
> > 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.

-- 
Anthony PERARD