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
[ 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
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
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
© 2016 - 2026 Red Hat, Inc.