qemu-block
[Top][All Lists]
Advanced

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

Re: [Qemu-block] [Qemu-devel] [PATCH v3 1/4] block: disallow BDRV_REQ_NO


From: Vladimir Sementsov-Ogievskiy
Subject: Re: [Qemu-block] [Qemu-devel] [PATCH v3 1/4] block: disallow BDRV_REQ_NO_SERIALISING for write
Date: Mon, 9 Jul 2018 19:08:50 +0300
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.6.0

07.07.2018 00:46, Eric Blake wrote:
On 07/06/2018 04:32 PM, Eric Blake wrote:
On 07/05/2018 02:46 AM, Vladimir Sementsov-Ogievskiy wrote:
Before commit 9ded4a01149 "backup: Use copy offloading",
BDRV_REQ_NO_SERIALISING was used for only one case: read in
copy-on-write operation during backup. Also, the flag was handled only
on read path (in bdrv_co_preadv and bdrv_aligned_preadv).

After 9ded4a01149, flag is used for not waiting serializing operations
on backup target (in same case of copy-on-write operation). This
behavior change is unsubstantiated and potentially dangerous, let's
drop it.

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


Commenting only on the grammar:

diff --git a/include/block/block.h b/include/block/block.h
index e5c7759a0c..a06a4d27de 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -50,6 +50,19 @@ typedef enum {
       * opened with BDRV_O_UNMAP.
       */
      BDRV_REQ_MAY_UNMAP          = 0x4,
+
+    /* The BDRV_REQ_NO_SERIALISING means that we don't want to
+     * wait_serialising_requests(), when reading.

Either:

/* BDRV_REQ_NO_SERALISING means that...

or

/* The BDRV_REQ_NO_SERIALISING flag means that...

s/want to/want/

Or, after reading patch 3/4,

BDRV_REQ_NO_SERAILISING bypasses request serialisation during reads.

(where the counterpart starts:

BDRV_REQ_SERIALISING forces request serialisation during writes.
)

hmm. "bypasses request serialisation during reads" - it's not quite right, I think. NO_SERIALISING means only that we will not wait for other requests. However, our request may be serialising itself, so other requests will wait for it (which, actually don't really make sense, because they will have to wait for original write operation too, which starts before read and finishes after it). So, it's funny, but currently, all "NO_SERIALISING" requests are strictly serialising in fact (except that they intersect with original writes (but in fact, are done before them).



+     *
+     * This flag is used for backup copy on write operation, when we need to +     * read old data before write (write notifier triggered). It is ok, due to +     * we already waited for serializing requests in initiative write (see +     * bdrv_aligned_pwritev), and it is necessary for the case when initiative
+     * write is serializing itself (we'll dead lock waiting it).

It is okay since we already waited for other serializing requests in the initiating write (see bdrv_aligned_pwritev), and it is necessary since the initiating write is already serializing (without the flag, the read would deadlock waiting for the write to complete).

+     *
+     * The described case is the only usage for the flag for now, so, it is
+     * supported only for read operation and restricted for write.

This last sentence is rather wordy; I'm fine with just:

The flag is only valid during read operations.

Or even just drop this last paragraph, since the first sentence of the comment already stated it was only for reads.


+     */
      BDRV_REQ_NO_SERIALISING     = 0x8,
      BDRV_REQ_FUA                = 0x10,
      BDRV_REQ_WRITE_COMPRESSED   = 0x20,

We're inconsistent on which flags we document; it might be nice to have a comment for each of them.  But not necessarily this patch's problem.




--
Best regards,
Vladimir




reply via email to

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