qemu-devel.nongnu.org archive mirror
 help / color / mirror / Atom feed
* [Qemu-devel] [PATCHv2 0/3] Better 'Force Unit Access' handling
@ 2016-05-03 22:39 Eric Blake
  2016-05-03 22:39 ` [Qemu-devel] [PATCHv2 1/3] block: Make supported_write_flags a per-bds property Eric Blake
                   ` (4 more replies)
  0 siblings, 5 replies; 8+ messages in thread
From: Eric Blake @ 2016-05-03 22:39 UTC (permalink / raw)
  To: qemu-devel; +Cc: qemu-block, kwolf

I noticed some inconsistencies in FUA handling while working
with NBD, then Kevin pointed out that my initial attempt wasn't
quite right for iscsi which also had problems, so this has
expanded into a series rather than a single patch.

I'm not sure if this is qemu-stable material at this point.

Depends on Kevin's block-next branch.

Also available as a tag at this location:
git fetch git://repo.or.cz/qemu/ericb.git nbd-fua-v2

v1 was here:
https://lists.gnu.org/archive/html/qemu-devel/2016-04/msg04674.html

diffstat not worth posting, as the series is basically rewritten

Eric Blake (3):
  block: Make supported_write_flags a per-bds property
  block: Honor BDRV_REQ_FUA during write_zeroes
  nbd: Simplify client FUA handling

 block/nbd-client.h        |  2 +-
 include/block/block_int.h | 11 +++++++----
 block/io.c                | 39 ++++++++++++++++++++++++++++++---------
 block/iscsi.c             | 11 ++++++++---
 block/nbd-client.c        | 11 +++++++----
 block/nbd.c               | 28 +++-------------------------
 block/raw-posix.c         |  1 +
 block/raw_bsd.c           |  3 ++-
 8 files changed, 59 insertions(+), 47 deletions(-)

-- 
2.5.5

^ permalink raw reply	[flat|nested] 8+ messages in thread

* [Qemu-devel] [PATCHv2 1/3] block: Make supported_write_flags a per-bds property
  2016-05-03 22:39 [Qemu-devel] [PATCHv2 0/3] Better 'Force Unit Access' handling Eric Blake
@ 2016-05-03 22:39 ` Eric Blake
  2016-05-03 22:39 ` [Qemu-devel] [PATCHv2 2/3] block: Honor BDRV_REQ_FUA during write_zeroes Eric Blake
                   ` (3 subsequent siblings)
  4 siblings, 0 replies; 8+ messages in thread
From: Eric Blake @ 2016-05-03 22:39 UTC (permalink / raw)
  To: qemu-devel
  Cc: qemu-block, kwolf, Stefan Hajnoczi, Fam Zheng, Max Reitz,
	Ronnie Sahlberg, Paolo Bonzini, Peter Lieven

Pre-patch, .supported_write_flags lives at the driver level, which
means we are blindly declaring that all block devices using a
given driver will either equally support FUA, or that we need a
fallback at the block layer.  But there are drivers where FUA
support is a per-block decision: the NBD block driver is dependent
on the remote server advertising NBD_FLAG_SEND_FUA (and has
fallback code to duplicate the flush that the block layer would do
if NBD had not set .supported_write_flags); and the iscsi block
driver is dependent on the mode sense bits advertised by the
underlying device (and is currently silently ignoring FUA requests
if the underlying device does not support FUA).

The fix is to make supported flags as a per-BDS option, set during
.bdrv_open().  This patch moves the variable and fixes NBD and iscsi
to set it only conditionally; later patches will then further
simplify the NBD driver to quit duplicating work done at the block
layer, as well as tackle the fact that SCSI does not support FUA
semantics on WRITESAME(10/16) but only on WRITE(10/16).

Signed-off-by: Eric Blake <eblake@redhat.com>
---
 include/block/block_int.h |  4 ++--
 block/io.c                |  9 ++++-----
 block/iscsi.c             | 10 +++++++---
 block/nbd-client.c        |  3 +++
 block/nbd.c               |  3 ---
 block/raw_bsd.c           |  2 +-
 6 files changed, 17 insertions(+), 14 deletions(-)

diff --git a/include/block/block_int.h b/include/block/block_int.h
index c512074..63e49ba 100644
--- a/include/block/block_int.h
+++ b/include/block/block_int.h
@@ -158,8 +158,6 @@ struct BlockDriver {
     int coroutine_fn (*bdrv_co_pwritev)(BlockDriverState *bs,
         uint64_t offset, uint64_t bytes, QEMUIOVector *qiov, int flags);

-    int supported_write_flags;
-
     /*
      * Efficiently zero a region of the disk image.  Typically an image format
      * would use a compact metadata representation to implement this.  This
@@ -445,6 +443,8 @@ struct BlockDriverState {

     /* Alignment requirement for offset/length of I/O requests */
     unsigned int request_alignment;
+    /* Flags honored during pwrite (so far: BDRV_REQ_FUA) */
+    unsigned int supported_write_flags;

     /* the following member gives a name to every node on the bs graph. */
     char node_name[32];
diff --git a/block/io.c b/block/io.c
index 0db1146..1fb7afe 100644
--- a/block/io.c
+++ b/block/io.c
@@ -841,9 +841,10 @@ static int coroutine_fn bdrv_driver_pwritev(BlockDriverState *bs,

     if (drv->bdrv_co_writev_flags) {
         ret = drv->bdrv_co_writev_flags(bs, sector_num, nb_sectors, qiov,
-                                        flags);
+                                        flags & bs->supported_write_flags);
+        flags &= ~bs->supported_write_flags;
     } else if (drv->bdrv_co_writev) {
-        assert(drv->supported_write_flags == 0);
+        assert(!bs->supported_write_flags);
         ret = drv->bdrv_co_writev(bs, sector_num, nb_sectors, qiov);
     } else {
         BlockAIOCB *acb;
@@ -862,9 +863,7 @@ static int coroutine_fn bdrv_driver_pwritev(BlockDriverState *bs,
     }

 emulate_flags:
-    if (ret == 0 && (flags & BDRV_REQ_FUA) &&
-        !(drv->supported_write_flags & BDRV_REQ_FUA))
-    {
+    if (ret == 0 && (flags & BDRV_REQ_FUA)) {
         ret = bdrv_co_flush(bs);
     }

diff --git a/block/iscsi.c b/block/iscsi.c
index 4f75204..6d5c1f6 100644
--- a/block/iscsi.c
+++ b/block/iscsi.c
@@ -456,8 +456,11 @@ iscsi_co_writev_flags(BlockDriverState *bs, int64_t sector_num, int nb_sectors,
     struct IscsiTask iTask;
     uint64_t lba;
     uint32_t num_sectors;
-    bool fua;
+    bool fua = flags & BDRV_REQ_FUA;

+    if (fua) {
+        assert(iscsilun->dpofua);
+    }
     if (!is_request_lun_aligned(sector_num, nb_sectors, iscsilun)) {
         return -EINVAL;
     }
@@ -472,7 +475,6 @@ iscsi_co_writev_flags(BlockDriverState *bs, int64_t sector_num, int nb_sectors,
     num_sectors = sector_qemu2lun(nb_sectors, iscsilun);
     iscsi_co_init_iscsitask(iscsilun, &iTask);
 retry:
-    fua = iscsilun->dpofua && (flags & BDRV_REQ_FUA);
     if (iscsilun->use_16_for_rw) {
         iTask.task = iscsi_write16_task(iscsilun->iscsi, iscsilun->lun, lba,
                                         NULL, num_sectors * iscsilun->block_size,
@@ -1548,6 +1550,9 @@ static int iscsi_open(BlockDriverState *bs, QDict *options, int flags,
     task = NULL;

     iscsi_modesense_sync(iscsilun);
+    if (iscsilun->dpofua) {
+        bs->supported_write_flags = BDRV_REQ_FUA;
+    }

     /* Check the write protect flag of the LUN if we want to write */
     if (iscsilun->type == TYPE_DISK && (flags & BDRV_O_RDWR) &&
@@ -1841,7 +1846,6 @@ static BlockDriver bdrv_iscsi = {
     .bdrv_co_write_zeroes = iscsi_co_write_zeroes,
     .bdrv_co_readv         = iscsi_co_readv,
     .bdrv_co_writev_flags  = iscsi_co_writev_flags,
-    .supported_write_flags = BDRV_REQ_FUA,
     .bdrv_co_flush_to_disk = iscsi_co_flush,

 #ifdef __linux__
diff --git a/block/nbd-client.c b/block/nbd-client.c
index 878e879..5fc96e9 100644
--- a/block/nbd-client.c
+++ b/block/nbd-client.c
@@ -414,6 +414,9 @@ int nbd_client_init(BlockDriverState *bs,
         logout("Failed to negotiate with the NBD server\n");
         return ret;
     }
+    if (client->nbdflags & NBD_FLAG_SEND_FUA) {
+        bs->supported_write_flags = BDRV_REQ_FUA;
+    }

     qemu_co_mutex_init(&client->send_mutex);
     qemu_co_mutex_init(&client->free_sema);
diff --git a/block/nbd.c b/block/nbd.c
index fccbfef..a4fba91 100644
--- a/block/nbd.c
+++ b/block/nbd.c
@@ -471,7 +471,6 @@ static BlockDriver bdrv_nbd = {
     .bdrv_file_open             = nbd_open,
     .bdrv_co_readv              = nbd_co_readv,
     .bdrv_co_writev_flags       = nbd_co_writev_flags,
-    .supported_write_flags      = BDRV_REQ_FUA,
     .bdrv_close                 = nbd_close,
     .bdrv_co_flush_to_os        = nbd_co_flush,
     .bdrv_co_discard            = nbd_co_discard,
@@ -490,7 +489,6 @@ static BlockDriver bdrv_nbd_tcp = {
     .bdrv_file_open             = nbd_open,
     .bdrv_co_readv              = nbd_co_readv,
     .bdrv_co_writev_flags       = nbd_co_writev_flags,
-    .supported_write_flags      = BDRV_REQ_FUA,
     .bdrv_close                 = nbd_close,
     .bdrv_co_flush_to_os        = nbd_co_flush,
     .bdrv_co_discard            = nbd_co_discard,
@@ -509,7 +507,6 @@ static BlockDriver bdrv_nbd_unix = {
     .bdrv_file_open             = nbd_open,
     .bdrv_co_readv              = nbd_co_readv,
     .bdrv_co_writev_flags       = nbd_co_writev_flags,
-    .supported_write_flags      = BDRV_REQ_FUA,
     .bdrv_close                 = nbd_close,
     .bdrv_co_flush_to_os        = nbd_co_flush,
     .bdrv_co_discard            = nbd_co_discard,
diff --git a/block/raw_bsd.c b/block/raw_bsd.c
index 5e65fb0..1a1618e 100644
--- a/block/raw_bsd.c
+++ b/block/raw_bsd.c
@@ -204,6 +204,7 @@ static int raw_open(BlockDriverState *bs, QDict *options, int flags,
                     Error **errp)
 {
     bs->sg = bs->file->bs->sg;
+    bs->supported_write_flags = BDRV_REQ_FUA;

     if (bs->probed && !bdrv_is_read_only(bs)) {
         fprintf(stderr,
@@ -250,7 +251,6 @@ BlockDriver bdrv_raw = {
     .bdrv_create          = &raw_create,
     .bdrv_co_readv        = &raw_co_readv,
     .bdrv_co_writev_flags = &raw_co_writev_flags,
-    .supported_write_flags = BDRV_REQ_FUA,
     .bdrv_co_write_zeroes = &raw_co_write_zeroes,
     .bdrv_co_discard      = &raw_co_discard,
     .bdrv_co_get_block_status = &raw_co_get_block_status,
-- 
2.5.5

^ permalink raw reply related	[flat|nested] 8+ messages in thread

* [Qemu-devel] [PATCHv2 2/3] block: Honor BDRV_REQ_FUA during write_zeroes
  2016-05-03 22:39 [Qemu-devel] [PATCHv2 0/3] Better 'Force Unit Access' handling Eric Blake
  2016-05-03 22:39 ` [Qemu-devel] [PATCHv2 1/3] block: Make supported_write_flags a per-bds property Eric Blake
@ 2016-05-03 22:39 ` Eric Blake
  2016-05-03 22:39 ` [Qemu-devel] [PATCHv2 3/3] nbd: Simplify client FUA handling Eric Blake
                   ` (2 subsequent siblings)
  4 siblings, 0 replies; 8+ messages in thread
From: Eric Blake @ 2016-05-03 22:39 UTC (permalink / raw)
  To: qemu-devel
  Cc: qemu-block, kwolf, Stefan Hajnoczi, Fam Zheng, Max Reitz,
	Ronnie Sahlberg, Paolo Bonzini, Peter Lieven

The block layer has a couple of cases where it can lose
Force Unit Access semantics when writing a large block of
zeroes, such that the request returns before the zeroes
have been guaranteed to land on underlying media.

SCSI does not support FUA during WRITESAME(10/16); FUA is only
supported if it falls back to WRITE(10/16).  But where the
underlying device is new enough to not need a fallback, it
means that any upper layer request with FUA semantics was
silently ignoring BDRV_REQ_FUA.

Conversely, NBD has situations where it can support FUA but not
ZERO_WRITE; when that happens, the generic block layer fallback
to bdrv_driver_pwritev() (or the older bdrv_co_writev() in qemu
2.6) was losing the FUA flag.

The problem of losing flags unrelated to ZERO_WRITE has been
latent in bdrv_co_do_write_zeroes() since commit aa7bfbff, but
back then, it did not matter because there was no FUA flag.  It
became observable when commit 93f5e6d8 paved the way for flags
that can impact correctness, when we should have been using
bdrv_co_writev_flags() with modified flags.  Compare to commit
9eeb6dd, which got flag manipulation right in
bdrv_co_do_zero_pwritev().

Symptoms: I tested with qemu-io with default writethrough cache
(which is supposed to use FUA semantics on every write), and
targetted an NBD client connected to a server that intentionally
did not advertise NBD_FLAG_SEND_FUA.  When doing 'write 0 512',
the NBD client sent two operations (NBD_CMD_WRITE then
NBD_CMD_FLUSH) to get the fallback FUA semantics; but when doing
'write -z 0 512', the NBD client sent only NBD_CMD_WRITE.

The fix is do to a cleanup bdrv_co_flush() at the end of the
operation if any step in the middle relied on a BDS that does
not natively support FUA for that step (note that we don't
need to flush after every operation, if the operation is broken
into chunks based on bounce-buffer sizing).  Each BDS gains a
new flag .supported_zero_flags, which parallels the use of
.supported_write_flags but only when accessing a zero write
operation (the flags MUST be different, because of SCSI having
different semantics based on WRITE vs. WRITESAME; and also
because BDRV_REQ_MAY_UNMAP only makes sense on zero writes).

Also fix some documentation to describe -ENOTSUP semantics,
particularly since iscsi depends on those semantics.

Down the road, we may want to add a driver where its
.bdrv_co_pwritev() honors all three of BDRV_REQ_FUA,
BDRV_REQ_ZERO_WRITE, and BDRV_REQ_MAY_UNMAP, and advertise
this via bs->supported_write_flags for blocks opened by that
driver; such a driver should NOT supply .bdrv_co_write_zeroes
nor .supported_zero_flags.  But none of the drivers touched
in this patch want to do that (the act of writing zeroes is
different enough from normal writes to deserve a second
callback).

Signed-off-by: Eric Blake <eblake@redhat.com>
---
 include/block/block_int.h |  7 +++++--
 block/io.c                | 30 ++++++++++++++++++++++++++----
 block/iscsi.c             |  1 +
 block/raw-posix.c         |  1 +
 block/raw_bsd.c           |  1 +
 5 files changed, 34 insertions(+), 6 deletions(-)

diff --git a/include/block/block_int.h b/include/block/block_int.h
index 63e49ba..e201b92 100644
--- a/include/block/block_int.h
+++ b/include/block/block_int.h
@@ -161,8 +161,8 @@ struct BlockDriver {
     /*
      * Efficiently zero a region of the disk image.  Typically an image format
      * would use a compact metadata representation to implement this.  This
-     * function pointer may be NULL and .bdrv_co_writev() will be called
-     * instead.
+     * function pointer may be NULL or return -ENOSUP and .bdrv_co_writev()
+     * will be called instead.
      */
     int coroutine_fn (*bdrv_co_write_zeroes)(BlockDriverState *bs,
         int64_t sector_num, int nb_sectors, BdrvRequestFlags flags);
@@ -445,6 +445,9 @@ struct BlockDriverState {
     unsigned int request_alignment;
     /* Flags honored during pwrite (so far: BDRV_REQ_FUA) */
     unsigned int supported_write_flags;
+    /* Flags honored during write_zeroes (so far: BDRV_REQ_FUA,
+     * BDRV_REQ_MAY_UNMAP) */
+    unsigned int supported_zero_flags;

     /* the following member gives a name to every node on the bs graph. */
     char node_name[32];
diff --git a/block/io.c b/block/io.c
index 1fb7afe..8b65127 100644
--- a/block/io.c
+++ b/block/io.c
@@ -652,7 +652,8 @@ int bdrv_write_zeroes(BlockDriverState *bs, int64_t sector_num,
  * Completely zero out a block device with the help of bdrv_write_zeroes.
  * The operation is sped up by checking the block status and only writing
  * zeroes to the device if they currently do not return zeroes. Optional
- * flags are passed through to bdrv_write_zeroes (e.g. BDRV_REQ_MAY_UNMAP).
+ * flags are passed through to bdrv_write_zeroes (e.g. BDRV_REQ_MAY_UNMAP,
+ * BDRV_REQ_FUA).
  *
  * Returns < 0 on error, 0 on success. For error codes see bdrv_write().
  */
@@ -1160,6 +1161,7 @@ static int coroutine_fn bdrv_co_do_write_zeroes(BlockDriverState *bs,
     QEMUIOVector qiov;
     struct iovec iov = {0};
     int ret = 0;
+    bool need_flush = false;

     int max_write_zeroes = MIN_NON_ZERO(bs->bl.max_write_zeroes,
                                         BDRV_REQUEST_MAX_SECTORS);
@@ -1192,13 +1194,29 @@ static int coroutine_fn bdrv_co_do_write_zeroes(BlockDriverState *bs,
         ret = -ENOTSUP;
         /* First try the efficient write zeroes operation */
         if (drv->bdrv_co_write_zeroes) {
-            ret = drv->bdrv_co_write_zeroes(bs, sector_num, num, flags);
+            ret = drv->bdrv_co_write_zeroes(bs, sector_num, num,
+                                            flags & bs->supported_zero_flags);
+            if (ret != -ENOTSUP && (flags & BDRV_REQ_FUA) &&
+                !(bs->supported_zero_flags & BDRV_REQ_FUA)) {
+                need_flush = true;
+            }
+        } else {
+            assert(!bs->supported_zero_flags);
         }

         if (ret == -ENOTSUP) {
             /* Fall back to bounce buffer if write zeroes is unsupported */
             int max_xfer_len = MIN_NON_ZERO(bs->bl.max_transfer_length,
                                             MAX_WRITE_ZEROES_BOUNCE_BUFFER);
+            BdrvRequestFlags write_flags = flags & ~BDRV_REQ_ZERO_WRITE;
+
+            if ((flags & BDRV_REQ_FUA) &&
+                !(bs->supported_write_flags & BDRV_REQ_FUA)) {
+                /* No need for bdrv_driver_pwrite() to do a fallback
+                 * flush on each chunk; use just one at the end */
+                write_flags &= ~BDRV_REQ_FUA;
+                need_flush = true;
+            }
             num = MIN(num, max_xfer_len);
             iov.iov_len = num * BDRV_SECTOR_SIZE;
             if (iov.iov_base == NULL) {
@@ -1212,7 +1230,8 @@ static int coroutine_fn bdrv_co_do_write_zeroes(BlockDriverState *bs,
             qemu_iovec_init_external(&qiov, &iov, 1);

             ret = bdrv_driver_pwritev(bs, sector_num * BDRV_SECTOR_SIZE,
-                                      num * BDRV_SECTOR_SIZE, &qiov, 0);
+                                      num * BDRV_SECTOR_SIZE, &qiov,
+                                      write_flags);

             /* Keep bounce buffer around if it is big enough for all
              * all future requests.
@@ -1228,6 +1247,9 @@ static int coroutine_fn bdrv_co_do_write_zeroes(BlockDriverState *bs,
     }

 fail:
+    if (ret == 0 && need_flush) {
+        ret = bdrv_co_flush(bs);
+    }
     qemu_vfree(iov.iov_base);
     return ret;
 }
diff --git a/block/iscsi.c b/block/iscsi.c
index 6d5c1f6..10f3906 100644
--- a/block/iscsi.c
+++ b/block/iscsi.c
@@ -1553,6 +1553,7 @@ static int iscsi_open(BlockDriverState *bs, QDict *options, int flags,
     if (iscsilun->dpofua) {
         bs->supported_write_flags = BDRV_REQ_FUA;
     }
+    bs->supported_zero_flags = BDRV_REQ_MAY_UNMAP;

     /* Check the write protect flag of the LUN if we want to write */
     if (iscsilun->type == TYPE_DISK && (flags & BDRV_O_RDWR) &&
diff --git a/block/raw-posix.c b/block/raw-posix.c
index 71ec463..a4f5a1b 100644
--- a/block/raw-posix.c
+++ b/block/raw-posix.c
@@ -517,6 +517,7 @@ static int raw_open_common(BlockDriverState *bs, QDict *options,

     s->has_discard = true;
     s->has_write_zeroes = true;
+    bs->supported_zero_flags = BDRV_REQ_MAY_UNMAP;
     if ((bs->open_flags & BDRV_O_NOCACHE) != 0) {
         s->needs_alignment = true;
     }
diff --git a/block/raw_bsd.c b/block/raw_bsd.c
index 1a1618e..3385ed4 100644
--- a/block/raw_bsd.c
+++ b/block/raw_bsd.c
@@ -205,6 +205,7 @@ static int raw_open(BlockDriverState *bs, QDict *options, int flags,
 {
     bs->sg = bs->file->bs->sg;
     bs->supported_write_flags = BDRV_REQ_FUA;
+    bs->supported_zero_flags = BDRV_REQ_FUA | BDRV_REQ_MAY_UNMAP;

     if (bs->probed && !bdrv_is_read_only(bs)) {
         fprintf(stderr,
-- 
2.5.5

^ permalink raw reply related	[flat|nested] 8+ messages in thread

* [Qemu-devel] [PATCHv2 3/3] nbd: Simplify client FUA handling
  2016-05-03 22:39 [Qemu-devel] [PATCHv2 0/3] Better 'Force Unit Access' handling Eric Blake
  2016-05-03 22:39 ` [Qemu-devel] [PATCHv2 1/3] block: Make supported_write_flags a per-bds property Eric Blake
  2016-05-03 22:39 ` [Qemu-devel] [PATCHv2 2/3] block: Honor BDRV_REQ_FUA during write_zeroes Eric Blake
@ 2016-05-03 22:39 ` Eric Blake
  2016-05-10 12:38 ` [Qemu-devel] [PATCHv2 0/3] Better 'Force Unit Access' handling Kevin Wolf
  2016-05-11  1:34 ` Fam Zheng
  4 siblings, 0 replies; 8+ messages in thread
From: Eric Blake @ 2016-05-03 22:39 UTC (permalink / raw)
  To: qemu-devel; +Cc: qemu-block, kwolf, Paolo Bonzini, Max Reitz

Now that the block layer honors per-bds FUA support, we don't
have to duplicate the fallback flush at the NBD layer.  The
static function nbd_co_writev_flags() is no longer needed, and
the driver can just directly use nbd_client_co_writev().

Signed-off-by: Eric Blake <eblake@redhat.com>
---
 block/nbd-client.h |  2 +-
 block/nbd-client.c |  8 ++++----
 block/nbd.c        | 25 +++----------------------
 3 files changed, 8 insertions(+), 27 deletions(-)

diff --git a/block/nbd-client.h b/block/nbd-client.h
index bc7aec0..c618dad 100644
--- a/block/nbd-client.h
+++ b/block/nbd-client.h
@@ -48,7 +48,7 @@ int nbd_client_co_discard(BlockDriverState *bs, int64_t sector_num,
                           int nb_sectors);
 int nbd_client_co_flush(BlockDriverState *bs);
 int nbd_client_co_writev(BlockDriverState *bs, int64_t sector_num,
-                         int nb_sectors, QEMUIOVector *qiov, int *flags);
+                         int nb_sectors, QEMUIOVector *qiov, int flags);
 int nbd_client_co_readv(BlockDriverState *bs, int64_t sector_num,
                         int nb_sectors, QEMUIOVector *qiov);

diff --git a/block/nbd-client.c b/block/nbd-client.c
index 5fc96e9..4d13444 100644
--- a/block/nbd-client.c
+++ b/block/nbd-client.c
@@ -243,15 +243,15 @@ static int nbd_co_readv_1(BlockDriverState *bs, int64_t sector_num,

 static int nbd_co_writev_1(BlockDriverState *bs, int64_t sector_num,
                            int nb_sectors, QEMUIOVector *qiov,
-                           int offset, int *flags)
+                           int offset, int flags)
 {
     NbdClientSession *client = nbd_get_client_session(bs);
     struct nbd_request request = { .type = NBD_CMD_WRITE };
     struct nbd_reply reply;
     ssize_t ret;

-    if ((*flags & BDRV_REQ_FUA) && (client->nbdflags & NBD_FLAG_SEND_FUA)) {
-        *flags &= ~BDRV_REQ_FUA;
+    if (flags & BDRV_REQ_FUA) {
+        assert(client->nbdflags & NBD_FLAG_SEND_FUA);
         request.type |= NBD_CMD_FLAG_FUA;
     }

@@ -291,7 +291,7 @@ int nbd_client_co_readv(BlockDriverState *bs, int64_t sector_num,
 }

 int nbd_client_co_writev(BlockDriverState *bs, int64_t sector_num,
-                         int nb_sectors, QEMUIOVector *qiov, int *flags)
+                         int nb_sectors, QEMUIOVector *qiov, int flags)
 {
     int offset = 0;
     int ret;
diff --git a/block/nbd.c b/block/nbd.c
index a4fba91..6015e8b 100644
--- a/block/nbd.c
+++ b/block/nbd.c
@@ -355,25 +355,6 @@ static int nbd_co_readv(BlockDriverState *bs, int64_t sector_num,
     return nbd_client_co_readv(bs, sector_num, nb_sectors, qiov);
 }

-static int nbd_co_writev_flags(BlockDriverState *bs, int64_t sector_num,
-                               int nb_sectors, QEMUIOVector *qiov, int flags)
-{
-    int ret;
-
-    ret = nbd_client_co_writev(bs, sector_num, nb_sectors, qiov, &flags);
-    if (ret < 0) {
-        return ret;
-    }
-
-    /* The flag wasn't sent to the server, so we need to emulate it with an
-     * explicit flush */
-    if (flags & BDRV_REQ_FUA) {
-        ret = nbd_client_co_flush(bs);
-    }
-
-    return ret;
-}
-
 static int nbd_co_flush(BlockDriverState *bs)
 {
     return nbd_client_co_flush(bs);
@@ -470,7 +451,7 @@ static BlockDriver bdrv_nbd = {
     .bdrv_parse_filename        = nbd_parse_filename,
     .bdrv_file_open             = nbd_open,
     .bdrv_co_readv              = nbd_co_readv,
-    .bdrv_co_writev_flags       = nbd_co_writev_flags,
+    .bdrv_co_writev_flags       = nbd_client_co_writev,
     .bdrv_close                 = nbd_close,
     .bdrv_co_flush_to_os        = nbd_co_flush,
     .bdrv_co_discard            = nbd_co_discard,
@@ -488,7 +469,7 @@ static BlockDriver bdrv_nbd_tcp = {
     .bdrv_parse_filename        = nbd_parse_filename,
     .bdrv_file_open             = nbd_open,
     .bdrv_co_readv              = nbd_co_readv,
-    .bdrv_co_writev_flags       = nbd_co_writev_flags,
+    .bdrv_co_writev_flags       = nbd_client_co_writev,
     .bdrv_close                 = nbd_close,
     .bdrv_co_flush_to_os        = nbd_co_flush,
     .bdrv_co_discard            = nbd_co_discard,
@@ -506,7 +487,7 @@ static BlockDriver bdrv_nbd_unix = {
     .bdrv_parse_filename        = nbd_parse_filename,
     .bdrv_file_open             = nbd_open,
     .bdrv_co_readv              = nbd_co_readv,
-    .bdrv_co_writev_flags       = nbd_co_writev_flags,
+    .bdrv_co_writev_flags       = nbd_client_co_writev,
     .bdrv_close                 = nbd_close,
     .bdrv_co_flush_to_os        = nbd_co_flush,
     .bdrv_co_discard            = nbd_co_discard,
-- 
2.5.5

^ permalink raw reply related	[flat|nested] 8+ messages in thread

* Re: [Qemu-devel] [PATCHv2 0/3] Better 'Force Unit Access' handling
  2016-05-03 22:39 [Qemu-devel] [PATCHv2 0/3] Better 'Force Unit Access' handling Eric Blake
                   ` (2 preceding siblings ...)
  2016-05-03 22:39 ` [Qemu-devel] [PATCHv2 3/3] nbd: Simplify client FUA handling Eric Blake
@ 2016-05-10 12:38 ` Kevin Wolf
  2016-05-11 13:30   ` Stefan Hajnoczi
  2016-05-11  1:34 ` Fam Zheng
  4 siblings, 1 reply; 8+ messages in thread
From: Kevin Wolf @ 2016-05-10 12:38 UTC (permalink / raw)
  To: Eric Blake; +Cc: qemu-devel, qemu-block, stefanha

Am 04.05.2016 um 00:39 hat Eric Blake geschrieben:
> I noticed some inconsistencies in FUA handling while working
> with NBD, then Kevin pointed out that my initial attempt wasn't
> quite right for iscsi which also had problems, so this has
> expanded into a series rather than a single patch.
> 
> I'm not sure if this is qemu-stable material at this point.
> 
> Depends on Kevin's block-next branch.
> 
> Also available as a tag at this location:
> git fetch git://repo.or.cz/qemu/ericb.git nbd-fua-v2

Reviewed-by: Kevin Wolf <kwolf@redhat.com>

Stefan, if you're happy with the series, I can merge it into block-next.

Kevin

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [Qemu-devel] [PATCHv2 0/3] Better 'Force Unit Access' handling
  2016-05-03 22:39 [Qemu-devel] [PATCHv2 0/3] Better 'Force Unit Access' handling Eric Blake
                   ` (3 preceding siblings ...)
  2016-05-10 12:38 ` [Qemu-devel] [PATCHv2 0/3] Better 'Force Unit Access' handling Kevin Wolf
@ 2016-05-11  1:34 ` Fam Zheng
  2016-05-11 11:50   ` Kevin Wolf
  4 siblings, 1 reply; 8+ messages in thread
From: Fam Zheng @ 2016-05-11  1:34 UTC (permalink / raw)
  To: Eric Blake; +Cc: qemu-devel, kwolf, qemu-block

On Tue, 05/03 16:39, Eric Blake wrote:
> I noticed some inconsistencies in FUA handling while working
> with NBD, then Kevin pointed out that my initial attempt wasn't
> quite right for iscsi which also had problems, so this has
> expanded into a series rather than a single patch.
> 
> I'm not sure if this is qemu-stable material at this point.
> 
> Depends on Kevin's block-next branch.
> 
> Also available as a tag at this location:
> git fetch git://repo.or.cz/qemu/ericb.git nbd-fua-v2
> 
> v1 was here:
> https://lists.gnu.org/archive/html/qemu-devel/2016-04/msg04674.html
> 
> diffstat not worth posting, as the series is basically rewritten
> 
> Eric Blake (3):
>   block: Make supported_write_flags a per-bds property
>   block: Honor BDRV_REQ_FUA during write_zeroes
>   nbd: Simplify client FUA handling

Reviewed-by: Fam Zheng <famz@redhat.com>

> 
>  block/nbd-client.h        |  2 +-
>  include/block/block_int.h | 11 +++++++----
>  block/io.c                | 39 ++++++++++++++++++++++++++++++---------
>  block/iscsi.c             | 11 ++++++++---
>  block/nbd-client.c        | 11 +++++++----
>  block/nbd.c               | 28 +++-------------------------
>  block/raw-posix.c         |  1 +
>  block/raw_bsd.c           |  3 ++-
>  8 files changed, 59 insertions(+), 47 deletions(-)
> 
> -- 
> 2.5.5
> 
> 

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [Qemu-devel] [PATCHv2 0/3] Better 'Force Unit Access' handling
  2016-05-11  1:34 ` Fam Zheng
@ 2016-05-11 11:50   ` Kevin Wolf
  0 siblings, 0 replies; 8+ messages in thread
From: Kevin Wolf @ 2016-05-11 11:50 UTC (permalink / raw)
  To: Fam Zheng; +Cc: Eric Blake, qemu-devel, qemu-block

Am 11.05.2016 um 03:34 hat Fam Zheng geschrieben:
> On Tue, 05/03 16:39, Eric Blake wrote:
> > I noticed some inconsistencies in FUA handling while working
> > with NBD, then Kevin pointed out that my initial attempt wasn't
> > quite right for iscsi which also had problems, so this has
> > expanded into a series rather than a single patch.
> > 
> > I'm not sure if this is qemu-stable material at this point.
> > 
> > Depends on Kevin's block-next branch.
> > 
> > Also available as a tag at this location:
> > git fetch git://repo.or.cz/qemu/ericb.git nbd-fua-v2
> > 
> > v1 was here:
> > https://lists.gnu.org/archive/html/qemu-devel/2016-04/msg04674.html
> > 
> > diffstat not worth posting, as the series is basically rewritten
> > 
> > Eric Blake (3):
> >   block: Make supported_write_flags a per-bds property
> >   block: Honor BDRV_REQ_FUA during write_zeroes
> >   nbd: Simplify client FUA handling
> 
> Reviewed-by: Fam Zheng <famz@redhat.com>

Ah, sorry, forgot that you co-maintain this code now. So this is good
enough to merge the series. :-)

Thanks, applied to block-next.

Kevin

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [Qemu-devel] [PATCHv2 0/3] Better 'Force Unit Access' handling
  2016-05-10 12:38 ` [Qemu-devel] [PATCHv2 0/3] Better 'Force Unit Access' handling Kevin Wolf
@ 2016-05-11 13:30   ` Stefan Hajnoczi
  0 siblings, 0 replies; 8+ messages in thread
From: Stefan Hajnoczi @ 2016-05-11 13:30 UTC (permalink / raw)
  To: Kevin Wolf; +Cc: Eric Blake, qemu-devel, qemu-block

[-- Attachment #1: Type: text/plain, Size: 795 bytes --]

On Tue, May 10, 2016 at 02:38:29PM +0200, Kevin Wolf wrote:
> Am 04.05.2016 um 00:39 hat Eric Blake geschrieben:
> > I noticed some inconsistencies in FUA handling while working
> > with NBD, then Kevin pointed out that my initial attempt wasn't
> > quite right for iscsi which also had problems, so this has
> > expanded into a series rather than a single patch.
> > 
> > I'm not sure if this is qemu-stable material at this point.
> > 
> > Depends on Kevin's block-next branch.
> > 
> > Also available as a tag at this location:
> > git fetch git://repo.or.cz/qemu/ericb.git nbd-fua-v2
> 
> Reviewed-by: Kevin Wolf <kwolf@redhat.com>
> 
> Stefan, if you're happy with the series, I can merge it into block-next.
> 
> Kevin

Acked-by: Stefan Hajnoczi <stefanha@redhat.com>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

^ permalink raw reply	[flat|nested] 8+ messages in thread

end of thread, other threads:[~2016-05-11 13:31 UTC | newest]

Thread overview: 8+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2016-05-03 22:39 [Qemu-devel] [PATCHv2 0/3] Better 'Force Unit Access' handling Eric Blake
2016-05-03 22:39 ` [Qemu-devel] [PATCHv2 1/3] block: Make supported_write_flags a per-bds property Eric Blake
2016-05-03 22:39 ` [Qemu-devel] [PATCHv2 2/3] block: Honor BDRV_REQ_FUA during write_zeroes Eric Blake
2016-05-03 22:39 ` [Qemu-devel] [PATCHv2 3/3] nbd: Simplify client FUA handling Eric Blake
2016-05-10 12:38 ` [Qemu-devel] [PATCHv2 0/3] Better 'Force Unit Access' handling Kevin Wolf
2016-05-11 13:30   ` Stefan Hajnoczi
2016-05-11  1:34 ` Fam Zheng
2016-05-11 11:50   ` Kevin Wolf

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).