Mailing List Archive: Re: [Qemu-devel] [PATCH] xen_disk: implement BLKIF_OP_FLUSH_DISKCACHE, remove BLKIF_OP_WRITE

Re: [Qemu-devel] [PATCH] xen_disk: implement BLKIF_OP_FLUSH_DISKCACHE, remove BLKIF_OP_WRITE_BARRIER

Apr 25, 2012, 1:45 AM

Post #1 of 10 (461 views)

> - case BLKIF_OP_WRITE_BARRIER:
> + case BLKIF_OP_FLUSH_DISKCACHE:
> if (!ioreq->req.nr_segments) {
> ioreq->presync = 1;
> return 0;
> }
> - ioreq->presync = ioreq->postsync = 1;
> + ioreq->postsync = 1;
> /* fall through */

It might be worth documenting the semantics of BLKIF_OP_FLUSH_DISKCACHE
in a comment here. I haven't found any spec for the xen_disk protocol,
but from looking at the Linux frontend it seems like the semantics
of REQ_FLUSH and REQ_FUA in the Linux block driver are overloaded into
BLKIF_OP_FLUSH_DISKCACHE, which is fairly confusing given that REQ_FLUSH
already overload functionality.

Even worse REQ_FLUSH with a payload implies a preflush, while REQ_FUA
implies a post flush, and it seems like Xen has no way to distinguish
the two, making thing like log writes very inefficient.

Independent of that the implementation should really use a state machine
around bdrv_aio_flush instead of doing guest-sychronous bdrv_flush calls.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [Qemu-devel] [PATCH] xen_disk: implement BLKIF_OP_FLUSH_DISKCACHE, remove BLKIF_OP_WRITE_BARRIER [ In reply to ]

Ian.Campbell at citrix

Apr 25, 2012, 2:02 AM

Post #2 of 10 (472 views)

Permalink

On Wed, 2012-04-25 at 09:45 +0100, Christoph Hellwig wrote:
> > - case BLKIF_OP_WRITE_BARRIER:
> > + case BLKIF_OP_FLUSH_DISKCACHE:
> > if (!ioreq->req.nr_segments) {
> > ioreq->presync = 1;
> > return 0;
> > }
> > - ioreq->presync = ioreq->postsync = 1;
> > + ioreq->postsync = 1;
> > /* fall through */
>
> It might be worth documenting the semantics of BLKIF_OP_FLUSH_DISKCACHE
> in a comment here. I haven't found any spec for the xen_disk protocol,

The blkif spec was recently much improved, you can find it at
http://xenbits.xen.org/docs/unstable/hypercall/include,public,io,blkif.h.html

TBH I'm not sure it actually answers your questions wrt
BLKIF_OP_FLUSH_DISKCACHE, if not please let us know and we can see about
improving it.

Ian

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [Qemu-devel] [PATCH] xen_disk: implement BLKIF_OP_FLUSH_DISKCACHE, remove BLKIF_OP_WRITE_BARRIER [ In reply to ]

hch at lst

Apr 25, 2012, 3:20 AM

Post #3 of 10 (465 views)

Permalink

On Wed, Apr 25, 2012 at 10:02:45AM +0100, Ian Campbell wrote:
> The blkif spec was recently much improved, you can find it at
> http://xenbits.xen.org/docs/unstable/hypercall/include,public,io,blkif.h.html
>
> TBH I'm not sure it actually answers your questions wrt
> BLKIF_OP_FLUSH_DISKCACHE, if not please let us know and we can see about
> improving it.

That description in there is overly simple, and does not match any of the
implementations known to me on either end.

Talking about those: the mainline Linux blkback backend also implements
different semantics from what mainline Linux blkfront seems to expect,
as well as different from qemu. Looking at these three alone I can't see
how Xen ever managed to get data to disk reliably if using the paravirt
interface.
with the implementations in qemu and the Linux kernel frontend and backends,
which

>
> Ian
>
---end quoted text---

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [Qemu-devel] [PATCH] xen_disk: implement BLKIF_OP_FLUSH_DISKCACHE, remove BLKIF_OP_WRITE_BARRIER [ In reply to ]

Ian.Campbell at citrix

Apr 25, 2012, 4:17 AM

Post #4 of 10 (466 views)

Permalink

On Wed, 2012-04-25 at 12:21 +0100, Stefano Stabellini wrote:
> On Wed, 25 Apr 2012, Christoph Hellwig wrote:
> > On Wed, Apr 25, 2012 at 10:02:45AM +0100, Ian Campbell wrote:
> > > The blkif spec was recently much improved, you can find it at
> > > http://xenbits.xen.org/docs/unstable/hypercall/include,public,io,blkif.h.html
> > >
> > > TBH I'm not sure it actually answers your questions wrt
> > > BLKIF_OP_FLUSH_DISKCACHE, if not please let us know and we can see about
> > > improving it.
> >
> > That description in there is overly simple, and does not match any of the
> > implementations known to me on either end.
>
> That is true, in fact I couldn't figure out what I had to implement just
> reading the comment. So I went through the blkback code and tried to
> understand what I had to do, but I got it wrong.
>
> Reading the code again it seems to me that BLKIF_OP_FLUSH_DISKCACHE
> is supposed to have the same semantics as REQ_FLUSH, that implies a
> preflush if nr_segments > 0, not a postflush like I did.
>
> Konrad, can you please confirm this?

... and then provide a patch to blkif.h.

Thanks,

Ian.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [Qemu-devel] [PATCH] xen_disk: implement BLKIF_OP_FLUSH_DISKCACHE, remove BLKIF_OP_WRITE_BARRIER [ In reply to ]

stefano.stabellini at eu

Apr 25, 2012, 4:21 AM

Post #5 of 10 (470 views)

Permalink

On Wed, 25 Apr 2012, Christoph Hellwig wrote:
> On Wed, Apr 25, 2012 at 10:02:45AM +0100, Ian Campbell wrote:
> > The blkif spec was recently much improved, you can find it at
> > http://xenbits.xen.org/docs/unstable/hypercall/include,public,io,blkif.h.html
> >
> > TBH I'm not sure it actually answers your questions wrt
> > BLKIF_OP_FLUSH_DISKCACHE, if not please let us know and we can see about
> > improving it.
>
> That description in there is overly simple, and does not match any of the
> implementations known to me on either end.

That is true, in fact I couldn't figure out what I had to implement just
reading the comment. So I went through the blkback code and tried to
understand what I had to do, but I got it wrong.

Reading the code again it seems to me that BLKIF_OP_FLUSH_DISKCACHE
is supposed to have the same semantics as REQ_FLUSH, that implies a
preflush if nr_segments > 0, not a postflush like I did.

Konrad, can you please confirm this?

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [Qemu-devel] [PATCH] xen_disk: implement BLKIF_OP_FLUSH_DISKCACHE, remove BLKIF_OP_WRITE_BARRIER [ In reply to ]

hch at lst

Apr 25, 2012, 4:23 AM

Post #6 of 10 (460 views)

Permalink

On Wed, Apr 25, 2012 at 12:21:53PM +0100, Stefano Stabellini wrote:
> That is true, in fact I couldn't figure out what I had to implement just
> reading the comment. So I went through the blkback code and tried to
> understand what I had to do, but I got it wrong.
>
> Reading the code again it seems to me that BLKIF_OP_FLUSH_DISKCACHE
> is supposed to have the same semantics as REQ_FLUSH, that implies a
> preflush if nr_segments > 0, not a postflush like I did.

It's worse - blkfront translates both a REQ_FLUSH or a REQ_FUA
into BLKIF_OP_FLUSH_DISKCACHE.

REQ_FLUSH either is a pre flush or a pure flush without a data transfer,
and REQ_FUA is a post flush. So to get the proper semantics you'll have
to do both, _and_ sequence it so that no operation starts before the
previous one finished.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [Qemu-devel] [PATCH] xen_disk: implement BLKIF_OP_FLUSH_DISKCACHE, remove BLKIF_OP_WRITE_BARRIER [ In reply to ]

konrad.wilk at oracle

Apr 26, 2012, 8:41 AM

Post #7 of 10 (458 views)

Permalink

On Wed, Apr 25, 2012 at 01:23:35PM +0200, Christoph Hellwig wrote:
> On Wed, Apr 25, 2012 at 12:21:53PM +0100, Stefano Stabellini wrote:
> > That is true, in fact I couldn't figure out what I had to implement just
> > reading the comment. So I went through the blkback code and tried to
> > understand what I had to do, but I got it wrong.
> >
> > Reading the code again it seems to me that BLKIF_OP_FLUSH_DISKCACHE
> > is supposed to have the same semantics as REQ_FLUSH, that implies a
> > preflush if nr_segments > 0, not a postflush like I did.
>
> It's worse - blkfront translates both a REQ_FLUSH or a REQ_FUA
> into BLKIF_OP_FLUSH_DISKCACHE.

I think that is what remained of the BARRIER request.
>
> REQ_FLUSH either is a pre flush or a pure flush without a data transfer,
> and REQ_FUA is a post flush. So to get the proper semantics you'll have
> to do both, _and_ sequence it so that no operation starts before the
> previous one finished.

If I were to emulate the SCSI SYNC command which one would it be?

I think REQ_FLUSH? In which I would think that the blkfront needs to
get rid of the REQ_FUA part?

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [Qemu-devel] [PATCH] xen_disk: implement BLKIF_OP_FLUSH_DISKCACHE, remove BLKIF_OP_WRITE_BARRIER [ In reply to ]

stefano.stabellini at eu

May 9, 2012, 5:42 AM

Post #8 of 10 (447 views)

Permalink

On Thu, 26 Apr 2012, Konrad Rzeszutek Wilk wrote:
> On Wed, Apr 25, 2012 at 01:23:35PM +0200, Christoph Hellwig wrote:
> > On Wed, Apr 25, 2012 at 12:21:53PM +0100, Stefano Stabellini wrote:
> > > That is true, in fact I couldn't figure out what I had to implement just
> > > reading the comment. So I went through the blkback code and tried to
> > > understand what I had to do, but I got it wrong.
> > >
> > > Reading the code again it seems to me that BLKIF_OP_FLUSH_DISKCACHE
> > > is supposed to have the same semantics as REQ_FLUSH, that implies a
> > > preflush if nr_segments > 0, not a postflush like I did.
> >
> > It's worse - blkfront translates both a REQ_FLUSH or a REQ_FUA
> > into BLKIF_OP_FLUSH_DISKCACHE.
>
> I think that is what remained of the BARRIER request.
> >
> > REQ_FLUSH either is a pre flush or a pure flush without a data transfer,
> > and REQ_FUA is a post flush. So to get the proper semantics you'll have
> > to do both, _and_ sequence it so that no operation starts before the
> > previous one finished.
>
> If I were to emulate the SCSI SYNC command which one would it be?
>
> I think REQ_FLUSH? In which I would think that the blkfront needs to
> get rid of the REQ_FUA part?
>

ping?

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [Qemu-devel] [PATCH] xen_disk: implement BLKIF_OP_FLUSH_DISKCACHE, remove BLKIF_OP_WRITE_BARRIER [ In reply to ]

konrad.wilk at oracle

Dec 19, 2012, 10:46 AM

Post #9 of 10 (420 views)

Permalink

On Wed, May 09, 2012 at 01:42:41PM +0100, Stefano Stabellini wrote:
> On Thu, 26 Apr 2012, Konrad Rzeszutek Wilk wrote:
> > On Wed, Apr 25, 2012 at 01:23:35PM +0200, Christoph Hellwig wrote:
> > > On Wed, Apr 25, 2012 at 12:21:53PM +0100, Stefano Stabellini wrote:
> > > > That is true, in fact I couldn't figure out what I had to implement just
> > > > reading the comment. So I went through the blkback code and tried to
> > > > understand what I had to do, but I got it wrong.
> > > >
> > > > Reading the code again it seems to me that BLKIF_OP_FLUSH_DISKCACHE
> > > > is supposed to have the same semantics as REQ_FLUSH, that implies a
> > > > preflush if nr_segments > 0, not a postflush like I did.
> > >
> > > It's worse - blkfront translates both a REQ_FLUSH or a REQ_FUA
> > > into BLKIF_OP_FLUSH_DISKCACHE.
> >
> > I think that is what remained of the BARRIER request.
> > >
> > > REQ_FLUSH either is a pre flush or a pure flush without a data transfer,
> > > and REQ_FUA is a post flush. So to get the proper semantics you'll have
> > > to do both, _and_ sequence it so that no operation starts before the
> > > previous one finished.
> >
> > If I were to emulate the SCSI SYNC command which one would it be?
> >
> > I think REQ_FLUSH? In which I would think that the blkfront needs to
> > get rid of the REQ_FUA part?
> >
>
> ping?

And just shy of 7 months later I answer :-)

I think you are right. Getting rid of REQ_FUA looks like the
right way. Oh, and blkfront already does that!

1290 err = xenbus_gather(XBT_NIL, info->xbdev->otherend,
1291 "feature-flush-cache", "%d", &flush,
1292 NULL);
1293
1294 if (!err && flush) {
1295 info->feature_flush = REQ_FLUSH;
1296 info->flush_op = BLKIF_OP_FLUSH_DISKCACHE;
1297 }
1298

So what I am missing?
>
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [Qemu-devel] [PATCH] xen_disk: implement BLKIF_OP_FLUSH_DISKCACHE, remove BLKIF_OP_WRITE_BARRIER [ In reply to ]

stefano.stabellini at eu

Jan 9, 2013, 10:44 AM

Post #10 of 10 (403 views)

Permalink

On Wed, 19 Dec 2012, Konrad Rzeszutek Wilk wrote:
> On Wed, May 09, 2012 at 01:42:41PM +0100, Stefano Stabellini wrote:
> > On Thu, 26 Apr 2012, Konrad Rzeszutek Wilk wrote:
> > > On Wed, Apr 25, 2012 at 01:23:35PM +0200, Christoph Hellwig wrote:
> > > > On Wed, Apr 25, 2012 at 12:21:53PM +0100, Stefano Stabellini wrote:
> > > > > That is true, in fact I couldn't figure out what I had to implement just
> > > > > reading the comment. So I went through the blkback code and tried to
> > > > > understand what I had to do, but I got it wrong.
> > > > >
> > > > > Reading the code again it seems to me that BLKIF_OP_FLUSH_DISKCACHE
> > > > > is supposed to have the same semantics as REQ_FLUSH, that implies a
> > > > > preflush if nr_segments > 0, not a postflush like I did.
> > > >
> > > > It's worse - blkfront translates both a REQ_FLUSH or a REQ_FUA
> > > > into BLKIF_OP_FLUSH_DISKCACHE.
> > >
> > > I think that is what remained of the BARRIER request.
> > > >
> > > > REQ_FLUSH either is a pre flush or a pure flush without a data transfer,
> > > > and REQ_FUA is a post flush. So to get the proper semantics you'll have
> > > > to do both, _and_ sequence it so that no operation starts before the
> > > > previous one finished.
> > >
> > > If I were to emulate the SCSI SYNC command which one would it be?
> > >
> > > I think REQ_FLUSH? In which I would think that the blkfront needs to
> > > get rid of the REQ_FUA part?
> > >
> >
> > ping?
>
> And just shy of 7 months later I answer :-)
>
> I think you are right. Getting rid of REQ_FUA looks like the
> right way. Oh, and blkfront already does that!
>
> 1290 err = xenbus_gather(XBT_NIL, info->xbdev->otherend,
> 1291 "feature-flush-cache", "%d", &flush,
> 1292 NULL);
> 1293
> 1294 if (!err && flush) {
> 1295 info->feature_flush = REQ_FLUSH;
> 1296 info->flush_op = BLKIF_OP_FLUSH_DISKCACHE;
> 1297 }
> 1298
>
> So what I am missing?

Nothing, thanks. I have updated and resent the patch, fixing the
implementation of BLKIF_OP_FLUSH_DISKCACHE.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel