[PATCH 0/3] block/io: serialising request clean up and locking fix

[PATCH 0/3] block/io: serialising request clean up and locking fix

[Paolo Bonzini]

Peter Lieven noticed that reqs->overlap_offset and reqs->overlap_bytes
are written outside bs->reqs_lock.  Patch 3 fixes it, while patches 1
and 2 are preparatory cleanups.

Paolo Bonzini (3):
  block: eliminate BDRV_REQ_NO_SERIALISING
  block/io: wait for serialising requests when a request becomes
    serialising
  block/io: take bs->reqs_lock in bdrv_mark_request_serialising

 block/file-posix.c        |   1 -
 block/io.c                | 161 +++++++++++++++++++++++-----------------------
 include/block/block.h     |  12 ----
 include/block/block_int.h |   3 +-
 4 files changed, 80 insertions(+), 97 deletions(-)

-- 
1.8.3.1

[PATCH 1/3] block: eliminate BDRV_REQ_NO_SERIALISING

[Paolo Bonzini]

It is unused since commit 00e30f0 ("block/backup: use backup-top instead
of write notifiers", 2019-10-01), drop it to simplify the code.

While at it, drop redundant assertions on flags.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 block/io.c            | 18 ++++--------------
 include/block/block.h | 12 ------------
 2 files changed, 4 insertions(+), 26 deletions(-)

diff --git a/block/io.c b/block/io.c
index f75777f..b3a67fe 100644
--- a/block/io.c
+++ b/block/io.c
@@ -1445,8 +1445,7 @@ static int coroutine_fn bdrv_aligned_preadv(BdrvChild *child,
      * potential fallback support, if we ever implement any read flags
      * to pass through to drivers.  For now, there aren't any
      * passthrough flags.  */
-    assert(!(flags & ~(BDRV_REQ_NO_SERIALISING | BDRV_REQ_COPY_ON_READ |
-                       BDRV_REQ_PREFETCH)));
+    assert(!(flags & ~(BDRV_REQ_COPY_ON_READ | BDRV_REQ_PREFETCH)));
 
     /* Handle Copy on Read and associated serialisation */
     if (flags & BDRV_REQ_COPY_ON_READ) {
@@ -1458,12 +1457,7 @@ static int coroutine_fn bdrv_aligned_preadv(BdrvChild *child,
         bdrv_mark_request_serialising(req, bdrv_get_cluster_size(bs));
     }
 
-    /* BDRV_REQ_SERIALISING is only for write operation */
-    assert(!(flags & BDRV_REQ_SERIALISING));
-
-    if (!(flags & BDRV_REQ_NO_SERIALISING)) {
-        bdrv_wait_serialising_requests(req);
-    }
+    bdrv_wait_serialising_requests(req);
 
     if (flags & BDRV_REQ_COPY_ON_READ) {
         int64_t pnum;
@@ -1711,7 +1705,7 @@ int coroutine_fn bdrv_co_preadv_part(BdrvChild *child,
     bdrv_inc_in_flight(bs);
 
     /* Don't do copy-on-read if we read data before write operation */
-    if (atomic_read(&bs->copy_on_read) && !(flags & BDRV_REQ_NO_SERIALISING)) {
+    if (atomic_read(&bs->copy_on_read)) {
         flags |= BDRV_REQ_COPY_ON_READ;
     }
 
@@ -1852,8 +1846,6 @@ bdrv_co_write_req_prepare(BdrvChild *child, int64_t offset, uint64_t bytes,
         return -EPERM;
     }
 
-    /* BDRV_REQ_NO_SERIALISING is only for read operation */
-    assert(!(flags & BDRV_REQ_NO_SERIALISING));
     assert(!(bs->open_flags & BDRV_O_INACTIVE));
     assert((bs->open_flags & BDRV_O_NO_IO) == 0);
     assert(!(flags & ~BDRV_REQ_MASK));
@@ -3222,9 +3214,7 @@ static int coroutine_fn bdrv_co_copy_range_internal(
 
         /* BDRV_REQ_SERIALISING is only for write operation */
         assert(!(read_flags & BDRV_REQ_SERIALISING));
-        if (!(read_flags & BDRV_REQ_NO_SERIALISING)) {
-            bdrv_wait_serialising_requests(&req);
-        }
+        bdrv_wait_serialising_requests(&req);
 
         ret = src->bs->drv->bdrv_co_copy_range_from(src->bs,
                                                     src, src_offset,
diff --git a/include/block/block.h b/include/block/block.h
index 1df9848..9d43c02 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -51,18 +51,6 @@ typedef enum {
      */
     BDRV_REQ_MAY_UNMAP          = 0x4,
 
-    /*
-     * The BDRV_REQ_NO_SERIALISING flag is only valid for reads and means that
-     * we don't want wait_serialising_requests() during the read operation.
-     *
-     * This flag is used for backup copy-on-write operations, when we need to
-     * read old data before write (write notifier triggered). 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).
-     */
-    BDRV_REQ_NO_SERIALISING     = 0x8,
     BDRV_REQ_FUA                = 0x10,
     BDRV_REQ_WRITE_COMPRESSED   = 0x20,
 
-- 
1.8.3.1

[PATCH 2/3] block/io: wait for serialising requests when a request becomes serialising

[Paolo Bonzini]

Marking without waiting would not result in actual serialising behavior.
Thus, make a call bdrv_mark_request_serialising sufficient for
serialisation to happen.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 block/file-posix.c        |  1 -
 block/io.c                | 39 ++++++++++++++++-----------------------
 include/block/block_int.h |  3 +--
 3 files changed, 17 insertions(+), 26 deletions(-)

diff --git a/block/file-posix.c b/block/file-posix.c
index 1b805bd..2b08b02 100644
--- a/block/file-posix.c
+++ b/block/file-posix.c
@@ -2753,7 +2753,6 @@ raw_do_pwrite_zeroes(BlockDriverState *bs, int64_t offset, int bytes,
         req->overlap_bytes = req->bytes;
 
         bdrv_mark_request_serialising(req, bs->bl.request_alignment);
-        bdrv_wait_serialising_requests(req);
     }
 #endif
 
diff --git a/block/io.c b/block/io.c
index b3a67fe..e844c4b 100644
--- a/block/io.c
+++ b/block/io.c
@@ -41,6 +41,7 @@
 #define MAX_BOUNCE_BUFFER (32768 << BDRV_SECTOR_BITS)
 
 static void bdrv_parent_cb_resize(BlockDriverState *bs);
+static bool coroutine_fn bdrv_wait_serialising_requests(BdrvTrackedRequest *self);
 static int coroutine_fn bdrv_co_do_pwrite_zeroes(BlockDriverState *bs,
     int64_t offset, int bytes, BdrvRequestFlags flags);
 
@@ -715,7 +716,7 @@ static void tracked_request_begin(BdrvTrackedRequest *req,
     qemu_co_mutex_unlock(&bs->reqs_lock);
 }
 
-void bdrv_mark_request_serialising(BdrvTrackedRequest *req, uint64_t align)
+bool bdrv_mark_request_serialising(BdrvTrackedRequest *req, uint64_t align)
 {
     int64_t overlap_offset = req->offset & ~(align - 1);
     uint64_t overlap_bytes = ROUND_UP(req->offset + req->bytes, align)
@@ -728,18 +729,7 @@ void bdrv_mark_request_serialising(BdrvTrackedRequest *req, uint64_t align)
 
     req->overlap_offset = MIN(req->overlap_offset, overlap_offset);
     req->overlap_bytes = MAX(req->overlap_bytes, overlap_bytes);
-}
-
-static bool is_request_serialising_and_aligned(BdrvTrackedRequest *req)
-{
-    /*
-     * If the request is serialising, overlap_offset and overlap_bytes are set,
-     * so we can check if the request is aligned. Otherwise, don't care and
-     * return false.
-     */
-
-    return req->serialising && (req->offset == req->overlap_offset) &&
-           (req->bytes == req->overlap_bytes);
+    return bdrv_wait_serialising_requests(req);
 }
 
 /**
@@ -823,7 +813,7 @@ void bdrv_dec_in_flight(BlockDriverState *bs)
     bdrv_wakeup(bs);
 }
 
-bool coroutine_fn bdrv_wait_serialising_requests(BdrvTrackedRequest *self)
+static bool coroutine_fn bdrv_wait_serialising_requests(BdrvTrackedRequest *self)
 {
     BlockDriverState *bs = self->bs;
     BdrvTrackedRequest *req;
@@ -1455,10 +1445,10 @@ static int coroutine_fn bdrv_aligned_preadv(BdrvChild *child,
          * it ensures that the CoR read and write operations are atomic and
          * guest writes cannot interleave between them. */
         bdrv_mark_request_serialising(req, bdrv_get_cluster_size(bs));
+    } else {
+        bdrv_wait_serialising_requests(req);
     }
 
-    bdrv_wait_serialising_requests(req);
-
     if (flags & BDRV_REQ_COPY_ON_READ) {
         int64_t pnum;
 
@@ -1851,13 +1841,18 @@ bdrv_co_write_req_prepare(BdrvChild *child, int64_t offset, uint64_t bytes,
     assert(!(flags & ~BDRV_REQ_MASK));
 
     if (flags & BDRV_REQ_SERIALISING) {
-        bdrv_mark_request_serialising(req, bdrv_get_cluster_size(bs));
+        waited = bdrv_mark_request_serialising(req, bdrv_get_cluster_size(bs));
+        /*
+         * For a misaligned request, we should have already waited
+         * in bdrv_padding_rmw_read and locked out concurrent writers.
+         */
+        assert(!waited ||
+               (req->offset == req->overlap_offset &&
+                req->bytes == req->overlap_bytes));
+    } else {
+        bdrv_wait_serialising_requests(req);
     }
 
-    waited = bdrv_wait_serialising_requests(req);
-
-    assert(!waited || !req->serialising ||
-           is_request_serialising_and_aligned(req));
     assert(req->overlap_offset <= offset);
     assert(offset + bytes <= req->overlap_offset + req->overlap_bytes);
     assert(end_sector <= bs->total_sectors || child->perm & BLK_PERM_RESIZE);
@@ -2019,7 +2014,6 @@ static int coroutine_fn bdrv_co_do_zero_pwritev(BdrvChild *child,
     padding = bdrv_init_padding(bs, offset, bytes, &pad);
     if (padding) {
         bdrv_mark_request_serialising(req, align);
-        bdrv_wait_serialising_requests(req);
 
         bdrv_padding_rmw_read(child, req, &pad, true);
 
@@ -2122,7 +2116,6 @@ int coroutine_fn bdrv_co_pwritev_part(BdrvChild *child,
 
     if (bdrv_pad_request(bs, &qiov, &qiov_offset, &offset, &bytes, &pad)) {
         bdrv_mark_request_serialising(&req, align);
-        bdrv_wait_serialising_requests(&req);
         bdrv_padding_rmw_read(child, &req, &pad, false);
     }
 
diff --git a/include/block/block_int.h b/include/block/block_int.h
index dd033d0..640fb82 100644
--- a/include/block/block_int.h
+++ b/include/block/block_int.h
@@ -999,8 +999,7 @@ extern unsigned int bdrv_drain_all_count;
 void bdrv_apply_subtree_drain(BdrvChild *child, BlockDriverState *new_parent);
 void bdrv_unapply_subtree_drain(BdrvChild *child, BlockDriverState *old_parent);
 
-bool coroutine_fn bdrv_wait_serialising_requests(BdrvTrackedRequest *self);
-void bdrv_mark_request_serialising(BdrvTrackedRequest *req, uint64_t align);
+bool coroutine_fn bdrv_mark_request_serialising(BdrvTrackedRequest *req, uint64_t align);
 BdrvTrackedRequest *coroutine_fn bdrv_co_get_self_request(BlockDriverState *bs);
 
 int get_tmp_filename(char *filename, int size);
-- 
1.8.3.1

[PATCH 3/3] block/io: take bs->reqs_lock in bdrv_mark_request_serialising

[Paolo Bonzini]

bdrv_mark_request_serialising is writing the overlap_offset and
overlap_bytes fields of BdrvTrackedRequest.  Take bs->reqs_lock
for the whole direction of it, and not just when waiting for
serialising requests, so that tracked_request_overlaps does not
look at a half-updated request.

The new code does not unlock/relock around retries.  This is unnecessary
because a retry is always preceded by a CoQueue wait, which already
releases and reacquired bs->reqs_lock.

Reported-by: Peter Lieven <pl@kamp.de>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 block/io.c | 112 ++++++++++++++++++++++++++++++++++---------------------------
 1 file changed, 63 insertions(+), 49 deletions(-)

diff --git a/block/io.c b/block/io.c
index e844c4b..166c2c1 100644
--- a/block/io.c
+++ b/block/io.c
@@ -41,7 +41,6 @@
 #define MAX_BOUNCE_BUFFER (32768 << BDRV_SECTOR_BITS)
 
 static void bdrv_parent_cb_resize(BlockDriverState *bs);
-static bool coroutine_fn bdrv_wait_serialising_requests(BdrvTrackedRequest *self);
 static int coroutine_fn bdrv_co_do_pwrite_zeroes(BlockDriverState *bs,
     int64_t offset, int bytes, BdrvRequestFlags flags);
 
@@ -716,12 +715,69 @@ static void tracked_request_begin(BdrvTrackedRequest *req,
     qemu_co_mutex_unlock(&bs->reqs_lock);
 }
 
+static bool tracked_request_overlaps(BdrvTrackedRequest *req,
+                                     int64_t offset, uint64_t bytes)
+{
+    /*        aaaa   bbbb */
+    if (offset >= req->overlap_offset + req->overlap_bytes) {
+        return false;
+    }
+    /* bbbb   aaaa        */
+    if (req->overlap_offset >= offset + bytes) {
+        return false;
+    }
+    return true;
+}
+
+static bool coroutine_fn
+bdrv_wait_serialising_requests_locked(BlockDriverState *bs,
+                                      BdrvTrackedRequest *self)
+{
+    BdrvTrackedRequest *req;
+    bool retry;
+    bool waited = false;
+
+    do {
+        retry = false;
+        QLIST_FOREACH(req, &bs->tracked_requests, list) {
+            if (req == self || (!req->serialising && !self->serialising)) {
+                continue;
+            }
+            if (tracked_request_overlaps(req, self->overlap_offset,
+                                         self->overlap_bytes))
+            {
+                /* Hitting this means there was a reentrant request, for
+                 * example, a block driver issuing nested requests.  This must
+                 * never happen since it means deadlock.
+                 */
+                assert(qemu_coroutine_self() != req->co);
+
+                /* If the request is already (indirectly) waiting for us, or
+                 * will wait for us as soon as it wakes up, then just go on
+                 * (instead of producing a deadlock in the former case). */
+                if (!req->waiting_for) {
+                    self->waiting_for = req;
+                    qemu_co_queue_wait(&req->wait_queue, &bs->reqs_lock);
+                    self->waiting_for = NULL;
+                    retry = true;
+                    waited = true;
+                    break;
+                }
+            }
+        }
+    } while (retry);
+    return waited;
+}
+
 bool bdrv_mark_request_serialising(BdrvTrackedRequest *req, uint64_t align)
 {
+    BlockDriverState *bs = req->bs;
     int64_t overlap_offset = req->offset & ~(align - 1);
     uint64_t overlap_bytes = ROUND_UP(req->offset + req->bytes, align)
                                - overlap_offset;
+    bool waited;
 
+    qemu_co_mutex_lock(&bs->reqs_lock);
     if (!req->serialising) {
         atomic_inc(&req->bs->serialising_in_flight);
         req->serialising = true;
@@ -729,7 +785,9 @@ bool bdrv_mark_request_serialising(BdrvTrackedRequest *req, uint64_t align)
 
     req->overlap_offset = MIN(req->overlap_offset, overlap_offset);
     req->overlap_bytes = MAX(req->overlap_bytes, overlap_bytes);
-    return bdrv_wait_serialising_requests(req);
+    waited = bdrv_wait_serialising_requests_locked(bs, req);
+    qemu_co_mutex_unlock(&bs->reqs_lock);
+    return waited;
 }
 
 /**
@@ -783,20 +841,6 @@ static int bdrv_get_cluster_size(BlockDriverState *bs)
     }
 }
 
-static bool tracked_request_overlaps(BdrvTrackedRequest *req,
-                                     int64_t offset, uint64_t bytes)
-{
-    /*        aaaa   bbbb */
-    if (offset >= req->overlap_offset + req->overlap_bytes) {
-        return false;
-    }
-    /* bbbb   aaaa        */
-    if (req->overlap_offset >= offset + bytes) {
-        return false;
-    }
-    return true;
-}
-
 void bdrv_inc_in_flight(BlockDriverState *bs)
 {
     atomic_inc(&bs->in_flight);
@@ -816,45 +860,15 @@ void bdrv_dec_in_flight(BlockDriverState *bs)
 static bool coroutine_fn bdrv_wait_serialising_requests(BdrvTrackedRequest *self)
 {
     BlockDriverState *bs = self->bs;
-    BdrvTrackedRequest *req;
-    bool retry;
     bool waited = false;
 
     if (!atomic_read(&bs->serialising_in_flight)) {
         return false;
     }
 
-    do {
-        retry = false;
-        qemu_co_mutex_lock(&bs->reqs_lock);
-        QLIST_FOREACH(req, &bs->tracked_requests, list) {
-            if (req == self || (!req->serialising && !self->serialising)) {
-                continue;
-            }
-            if (tracked_request_overlaps(req, self->overlap_offset,
-                                         self->overlap_bytes))
-            {
-                /* Hitting this means there was a reentrant request, for
-                 * example, a block driver issuing nested requests.  This must
-                 * never happen since it means deadlock.
-                 */
-                assert(qemu_coroutine_self() != req->co);
-
-                /* If the request is already (indirectly) waiting for us, or
-                 * will wait for us as soon as it wakes up, then just go on
-                 * (instead of producing a deadlock in the former case). */
-                if (!req->waiting_for) {
-                    self->waiting_for = req;
-                    qemu_co_queue_wait(&req->wait_queue, &bs->reqs_lock);
-                    self->waiting_for = NULL;
-                    retry = true;
-                    waited = true;
-                    break;
-                }
-            }
-        }
-        qemu_co_mutex_unlock(&bs->reqs_lock);
-    } while (retry);
+    qemu_co_mutex_lock(&bs->reqs_lock);
+    waited = bdrv_wait_serialising_requests_locked(bs, self);
+    qemu_co_mutex_unlock(&bs->reqs_lock);
 
     return waited;
 }
-- 
1.8.3.1

Re: [PATCH 1/3] block: eliminate BDRV_REQ_NO_SERIALISING

[Kevin Wolf]

[ Fixing the qemu-block address ]

Am 18.12.2019 um 14:17 hat Paolo Bonzini geschrieben:
> It is unused since commit 00e30f0 ("block/backup: use backup-top instead
> of write notifiers", 2019-10-01), drop it to simplify the code.
> 
> While at it, drop redundant assertions on flags.
> 
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  block/io.c            | 18 ++++--------------
>  include/block/block.h | 12 ------------
>  2 files changed, 4 insertions(+), 26 deletions(-)
> 
> diff --git a/block/io.c b/block/io.c
> index f75777f..b3a67fe 100644
> --- a/block/io.c
> +++ b/block/io.c
> @@ -1445,8 +1445,7 @@ static int coroutine_fn bdrv_aligned_preadv(BdrvChild *child,
>       * potential fallback support, if we ever implement any read flags
>       * to pass through to drivers.  For now, there aren't any
>       * passthrough flags.  */
> -    assert(!(flags & ~(BDRV_REQ_NO_SERIALISING | BDRV_REQ_COPY_ON_READ |
> -                       BDRV_REQ_PREFETCH)));
> +    assert(!(flags & ~(BDRV_REQ_COPY_ON_READ | BDRV_REQ_PREFETCH)));
>  
>      /* Handle Copy on Read and associated serialisation */
>      if (flags & BDRV_REQ_COPY_ON_READ) {
> @@ -1458,12 +1457,7 @@ static int coroutine_fn bdrv_aligned_preadv(BdrvChild *child,
>          bdrv_mark_request_serialising(req, bdrv_get_cluster_size(bs));
>      }
>  
> -    /* BDRV_REQ_SERIALISING is only for write operation */
> -    assert(!(flags & BDRV_REQ_SERIALISING));

I think we shoud still keep this assertion as long as read requests
don't mark themselves as serialising when BDRV_REQ_SERIALISING is given.
Otherwise, someone might add the flag to a read request and will later
be surprised that it didn't work.

> -    if (!(flags & BDRV_REQ_NO_SERIALISING)) {
> -        bdrv_wait_serialising_requests(req);
> -    }
> +    bdrv_wait_serialising_requests(req);
>  
>      if (flags & BDRV_REQ_COPY_ON_READ) {
>          int64_t pnum;
> @@ -1711,7 +1705,7 @@ int coroutine_fn bdrv_co_preadv_part(BdrvChild *child,
>      bdrv_inc_in_flight(bs);
>  
>      /* Don't do copy-on-read if we read data before write operation */
> -    if (atomic_read(&bs->copy_on_read) && !(flags & BDRV_REQ_NO_SERIALISING)) {
> +    if (atomic_read(&bs->copy_on_read)) {

The comment wants an update, too (or maybe a removal).

>          flags |= BDRV_REQ_COPY_ON_READ;
>      }
>  
> @@ -1852,8 +1846,6 @@ bdrv_co_write_req_prepare(BdrvChild *child, int64_t offset, uint64_t bytes,
>          return -EPERM;
>      }
>  
> -    /* BDRV_REQ_NO_SERIALISING is only for read operation */
> -    assert(!(flags & BDRV_REQ_NO_SERIALISING));
>      assert(!(bs->open_flags & BDRV_O_INACTIVE));
>      assert((bs->open_flags & BDRV_O_NO_IO) == 0);
>      assert(!(flags & ~BDRV_REQ_MASK));
> @@ -3222,9 +3214,7 @@ static int coroutine_fn bdrv_co_copy_range_internal(
>  
>          /* BDRV_REQ_SERIALISING is only for write operation */
>          assert(!(read_flags & BDRV_REQ_SERIALISING));

Here you kept the assertion, so apart from making sense anyway, it would
also be more consistent to keep it above, too. :-)

> -        if (!(read_flags & BDRV_REQ_NO_SERIALISING)) {
> -            bdrv_wait_serialising_requests(&req);
> -        }
> +        bdrv_wait_serialising_requests(&req);
>  
>          ret = src->bs->drv->bdrv_co_copy_range_from(src->bs,

Kevin

Re: [PATCH 1/3] block: eliminate BDRV_REQ_NO_SERIALISING

[Paolo Bonzini]

On 18/12/19 17:37, Kevin Wolf wrote:
>>       * passthrough flags.  */
>> -    assert(!(flags & ~(BDRV_REQ_NO_SERIALISING | BDRV_REQ_COPY_ON_READ |
>> -                       BDRV_REQ_PREFETCH)));
>> +    assert(!(flags & ~(BDRV_REQ_COPY_ON_READ | BDRV_REQ_PREFETCH)));
>>  
>>      /* Handle Copy on Read and associated serialisation */
>>      if (flags & BDRV_REQ_COPY_ON_READ) {
>> @@ -1458,12 +1457,7 @@ static int coroutine_fn bdrv_aligned_preadv(BdrvChild *child,
>>          bdrv_mark_request_serialising(req, bdrv_get_cluster_size(bs));
>>      }
>>  
>> -    /* BDRV_REQ_SERIALISING is only for write operation */
>> -    assert(!(flags & BDRV_REQ_SERIALISING));
> I think we shoud still keep this assertion as long as read requests
> don't mark themselves as serialising when BDRV_REQ_SERIALISING is given.
> Otherwise, someone might add the flag to a read request and will later
> be surprised that it didn't work.

I'm removing it because it's anyway tested by the earlier

    assert(!(flags & ~(BDRV_REQ_COPY_ON_READ | BDRV_REQ_PREFETCH)));

>> @@ -3222,9 +3214,7 @@ static int coroutine_fn bdrv_co_copy_range_internal(
>>  
>>          /* BDRV_REQ_SERIALISING is only for write operation */
>>          assert(!(read_flags & BDRV_REQ_SERIALISING));
> Here you kept the assertion, so apart from making sense anyway, it would
> also be more consistent to keep it above, too. :-)

... which is not present here.

Paolo

Re: [PATCH 2/3] block/io: wait for serialising requests when a request becomes serialising

[Kevin Wolf]

Am 18.12.2019 um 14:17 hat Paolo Bonzini geschrieben:
> Marking without waiting would not result in actual serialising behavior.
> Thus, make a call bdrv_mark_request_serialising sufficient for
> serialisation to happen.
> 
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>

> @@ -1851,13 +1841,18 @@ bdrv_co_write_req_prepare(BdrvChild *child, int64_t offset, uint64_t bytes,
>      assert(!(flags & ~BDRV_REQ_MASK));
>  
>      if (flags & BDRV_REQ_SERIALISING) {
> -        bdrv_mark_request_serialising(req, bdrv_get_cluster_size(bs));
> +        waited = bdrv_mark_request_serialising(req, bdrv_get_cluster_size(bs));
> +        /*
> +         * For a misaligned request, we should have already waited
> +         * in bdrv_padding_rmw_read and locked out concurrent writers.

Actually, bdrv_padding_rmw_read() only asserts that the request is
already serialised. bdrv_mark_request_serialising() is already called by
the callers of bdrv_padding_rmw_read().

> +         */
> +        assert(!waited ||
> +               (req->offset == req->overlap_offset &&
> +                req->bytes == req->overlap_bytes));
> +    } else {
> +        bdrv_wait_serialising_requests(req);
>      }
>  
> -    waited = bdrv_wait_serialising_requests(req);
> -
> -    assert(!waited || !req->serialising ||
> -           is_request_serialising_and_aligned(req));
>      assert(req->overlap_offset <= offset);
>      assert(offset + bytes <= req->overlap_offset + req->overlap_bytes);
>      assert(end_sector <= bs->total_sectors || child->perm & BLK_PERM_RESIZE);

Other than the comment, the patch looks fine to me.

Kevin

Re: [PATCH 1/3] block: eliminate BDRV_REQ_NO_SERIALISING

[Kevin Wolf]

Am 18.12.2019 um 17:43 hat Paolo Bonzini geschrieben:
> On 18/12/19 17:37, Kevin Wolf wrote:
> >>       * passthrough flags.  */
> >> -    assert(!(flags & ~(BDRV_REQ_NO_SERIALISING | BDRV_REQ_COPY_ON_READ |
> >> -                       BDRV_REQ_PREFETCH)));
> >> +    assert(!(flags & ~(BDRV_REQ_COPY_ON_READ | BDRV_REQ_PREFETCH)));
> >>  
> >>      /* Handle Copy on Read and associated serialisation */
> >>      if (flags & BDRV_REQ_COPY_ON_READ) {
> >> @@ -1458,12 +1457,7 @@ static int coroutine_fn bdrv_aligned_preadv(BdrvChild *child,
> >>          bdrv_mark_request_serialising(req, bdrv_get_cluster_size(bs));
> >>      }
> >>  
> >> -    /* BDRV_REQ_SERIALISING is only for write operation */
> >> -    assert(!(flags & BDRV_REQ_SERIALISING));
> > I think we shoud still keep this assertion as long as read requests
> > don't mark themselves as serialising when BDRV_REQ_SERIALISING is given.
> > Otherwise, someone might add the flag to a read request and will later
> > be surprised that it didn't work.
> 
> I'm removing it because it's anyway tested by the earlier
> 
>     assert(!(flags & ~(BDRV_REQ_COPY_ON_READ | BDRV_REQ_PREFETCH)));

Ah, right. Never mind then.

Kevin

Re: [PATCH 3/3] block/io: take bs->reqs_lock in bdrv_mark_request_serialising

[Kevin Wolf]

Am 18.12.2019 um 14:17 hat Paolo Bonzini geschrieben:
> bdrv_mark_request_serialising is writing the overlap_offset and
> overlap_bytes fields of BdrvTrackedRequest.  Take bs->reqs_lock
> for the whole direction of it, and not just when waiting for

Is "direction" really the word you meant?

> serialising requests, so that tracked_request_overlaps does not
> look at a half-updated request.
> 
> The new code does not unlock/relock around retries.  This is unnecessary
> because a retry is always preceded by a CoQueue wait, which already
> releases and reacquired bs->reqs_lock.
> 
> Reported-by: Peter Lieven <pl@kamp.de>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>

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

Re: [PATCH 3/3] block/io: take bs->reqs_lock in bdrv_mark_request_serialising

[Paolo Bonzini]

On 18/12/19 17:59, Kevin Wolf wrote:
> Am 18.12.2019 um 14:17 hat Paolo Bonzini geschrieben:
>> bdrv_mark_request_serialising is writing the overlap_offset and
>> overlap_bytes fields of BdrvTrackedRequest.  Take bs->reqs_lock
>> for the whole direction of it, and not just when waiting for
> 
> Is "direction" really the word you meant?

No, "duration".  I'll send v2 with all the fixes then.

Paolo

> 
>> serialising requests, so that tracked_request_overlaps does not
>> look at a half-updated request.
>>
>> The new code does not unlock/relock around retries.  This is unnecessary
>> because a retry is always preceded by a CoQueue wait, which already
>> releases and reacquired bs->reqs_lock.
>>
>> Reported-by: Peter Lieven <pl@kamp.de>
>> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> 
> Reviewed-by: Kevin Wolf <kwolf@redhat.com>
>