07.07.2018 00:32, 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 <vsement...@virtuozzo.com>
---
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/
+ *
+ * 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).
it is necessary only for the case ..., not always, isn't it?
I'll use
It is okay since we already waited for other serializing requests in the
initiating write (see bdrv_aligned_pwritev), and it is necessary if the
initiating write is already serializing (without the flag, the read
would deadlock waiting for the serialising 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.
ok
+ */
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
