qemu-block
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Qemu-block] [PATCH 1/2] block: add BDRV_REQ_SERIALISING flag


From: Vladimir Sementsov-Ogievskiy
Subject: Re: [Qemu-block] [PATCH 1/2] block: add BDRV_REQ_SERIALISING flag
Date: Wed, 4 Jul 2018 19:11:00 +0300
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.6.0

04.07.2018 18:08, Kevin Wolf wrote:
Am 04.07.2018 um 16:44 hat Vladimir Sementsov-Ogievskiy geschrieben:
03.07.2018 21:07, Vladimir Sementsov-Ogievskiy wrote:
Serialized writes should be used in copy-on-write of backup(sync=none)
for image fleecing scheme.

Signed-off-by: Vladimir Sementsov-Ogievskiy <address@hidden>
---
   include/block/block.h | 5 ++++-
   block/io.c            | 4 ++++
   2 files changed, 8 insertions(+), 1 deletion(-)

diff --git a/include/block/block.h b/include/block/block.h
index e5c7759a0c..107113aad5 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -58,8 +58,11 @@ typedef enum {
        * content. */
       BDRV_REQ_WRITE_UNCHANGED    = 0x40,
+    /* Force request serializing. Only for writes. */
+    BDRV_REQ_SERIALISING        = 0x80,
+
       /* Mask of valid flags */
-    BDRV_REQ_MASK               = 0x7f,
+    BDRV_REQ_MASK               = 0xff,
   } BdrvRequestFlags;
   typedef struct BlockSizes {
diff --git a/block/io.c b/block/io.c
index 1a2272fad3..d5ba078514 100644
--- a/block/io.c
+++ b/block/io.c
@@ -1572,6 +1572,10 @@ static int coroutine_fn bdrv_aligned_pwritev(BdrvChild 
*child,
       max_transfer = QEMU_ALIGN_DOWN(MIN_NON_ZERO(bs->bl.max_transfer, 
INT_MAX),
                                      align);
+    if (flags & BDRV_REQ_SERIALISING) {
+        mark_request_serialising(req, bdrv_get_cluster_size(bs));
+    }
+
       waited = wait_serialising_requests(req);
       assert(!waited || !req->serialising);
Kevin, about this assertion, introduced in 28de2dcd88de "block: Assert
serialisation assumptions in pwritev"? Will not it fail with fleecing
scheme? I'm afraid it will, when we will wait for client read with our
request, marked serializing a moment ago...
Hm, looks like it yes.

Can we just switch it to assert(!waited || !req->partial);, setting
req->partial in bdrv_co_pwritev for parts of unaligned requests? And allow
new flag only for aligned requests?

Other ideas?
The commit message of 28de2dcd88de tells you what we need to do (and
that just changing the assertion is wrong):

     If a request calls wait_serialising_requests() and actually has to wait
     in this function (i.e. a coroutine yield), other requests can run and
     previously read data (like the head or tail buffer) could become
     outdated. In this case, we would have to restart from the beginning to
     read in the updated data.

     However, we're lucky and don't actually need to do that: A request can
     only wait in the first call of wait_serialising_requests() because we
     mark it as serialising before that call, so any later requests would
     wait. So as we don't wait in practice, we don't have to reload the data.

     This is an important assumption that may not be broken or data
     corruption will happen. Document it with some assertions.

So we may need to return -EAGAIN here, check that in the caller and
repeat the write request from the very start.

But in case of aligned request, there no previously read data, and we can safely continue. And actually it's our case (backup writes are aligned).


Kevin


--
Best regards,
Vladimir




reply via email to

[Prev in Thread] Current Thread [Next in Thread]