[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-block] [Xen-devel] [PATCH v2 0/2] xen-block: fix sector size c
From: |
Anthony PERARD |
Subject: |
Re: [Qemu-block] [Xen-devel] [PATCH v2 0/2] xen-block: fix sector size confusion |
Date: |
Thu, 28 Mar 2019 11:40:12 +0000 |
User-agent: |
Mutt/1.11.4 (2019-03-13) |
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