Xen devel - Apr 2012 - [PATCH] xen_disk: implement BLKIF_OP_FLUSH_DISKCACHE, remove BLKIF_OP_WRITE_BARRIER

Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
---
 hw/xen_disk.c |    8 ++++----
 1 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/hw/xen_disk.c b/hw/xen_disk.c
index 015d2af..3e4a47b 100644
--- a/hw/xen_disk.c
+++ b/hw/xen_disk.c
@@ -183,12 +183,12 @@ static int ioreq_parse(struct ioreq *ioreq)
     case BLKIF_OP_READ:
         ioreq->prot = PROT_WRITE; /* to memory */
         break;
-    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 */
     case BLKIF_OP_WRITE:
         ioreq->prot = PROT_READ; /* from memory */
@@ -354,7 +354,7 @@ static int ioreq_runio_qemu_aio(struct ioreq *ioreq)
                        qemu_aio_complete, ioreq);
         break;
     case BLKIF_OP_WRITE:
-    case BLKIF_OP_WRITE_BARRIER:
+    case BLKIF_OP_FLUSH_DISKCACHE:
         if (!ioreq->req.nr_segments) {
             break;
         }
@@ -627,7 +627,7 @@ static int blk_init(struct XenDevice *xendev)
                   blkdev->file_size, blkdev->file_size >> 20);
 
     /* fill info */
-    xenstore_write_be_int(&blkdev->xendev, "feature-barrier",
1);
+    xenstore_write_be_int(&blkdev->xendev,
"feature-flush-cache", 1);
     xenstore_write_be_int(&blkdev->xendev, "info",           
info);
     xenstore_write_be_int(&blkdev->xendev, "sector-size",    
blkdev->file_blk);
     xenstore_write_be_int(&blkdev->xendev, "sectors",
-- 
1.7.2.5

On Mon, Apr 16, 2012 at 06:05:57PM +0100, Stefano Stabellini
wrote:
> Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
Reviewed-by: Konrad Rzeszutek Wilk
<konrad.wilk@oracle.com>
> ---
>  hw/xen_disk.c |    8 ++++----
>  1 files changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/hw/xen_disk.c b/hw/xen_disk.c
> index 015d2af..3e4a47b 100644
> --- a/hw/xen_disk.c
> +++ b/hw/xen_disk.c
> @@ -183,12 +183,12 @@ static int ioreq_parse(struct ioreq *ioreq)
>      case BLKIF_OP_READ:
>          ioreq->prot = PROT_WRITE; /* to memory */
>          break;
> -    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 */
>      case BLKIF_OP_WRITE:
>          ioreq->prot = PROT_READ; /* from memory */
> @@ -354,7 +354,7 @@ static int ioreq_runio_qemu_aio(struct ioreq *ioreq)
>                         qemu_aio_complete, ioreq);
>          break;
>      case BLKIF_OP_WRITE:
> -    case BLKIF_OP_WRITE_BARRIER:
> +    case BLKIF_OP_FLUSH_DISKCACHE:
>          if (!ioreq->req.nr_segments) {
>              break;
>          }
> @@ -627,7 +627,7 @@ static int blk_init(struct XenDevice *xendev)
>                    blkdev->file_size, blkdev->file_size >> 20);
>  
>      /* fill info */
> -    xenstore_write_be_int(&blkdev->xendev,
"feature-barrier", 1);
> +    xenstore_write_be_int(&blkdev->xendev,
"feature-flush-cache", 1);
>      xenstore_write_be_int(&blkdev->xendev, "info",       
info);
>      xenstore_write_be_int(&blkdev->xendev, "sector-size",
blkdev->file_blk);
>      xenstore_write_be_int(&blkdev->xendev, "sectors",
> -- 
> 1.7.2.5

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

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

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

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.

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?

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.

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?

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?

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

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.