(Sorry for the rather late reply... Thanks for the review!)
On 20.03.23 11:31, Vladimir Sementsov-Ogievskiy wrote:
On 17.03.23 20:50, Hanna Czenczek wrote:
[...]
diff --git a/block/io.c b/block/io.c
index 8974d46941..1e9cdba17a 100644
--- a/block/io.c
+++ b/block/io.c
[..]
+ pad->write = write;
+
return true;
}
@@ -1545,6 +1561,18 @@ zero_mem:
static void bdrv_padding_destroy(BdrvRequestPadding *pad)
Maybe, rename to _finalize, to stress that it's not only freeing
memory.
Sounds good!
[...]
@@ -1552,6 +1580,101 @@ static void
bdrv_padding_destroy(BdrvRequestPadding *pad)
memset(pad, 0, sizeof(*pad));
}
+/*
+ * Create pad->local_qiov by wrapping @iov in the padding head
and tail, while
+ * ensuring that the resulting vector will not exceed IOV_MAX
elements.
+ *
+ * To ensure this, when necessary, the first couple of elements
(up to three)
maybe, "first two-three elements"
Sure (here and...
[...]
+ /*
+ * If padded_niov > IOV_MAX, we cannot just concatenate
everything.
+ * Instead, merge the first couple of elements of @iov to
reduce the number
maybe, "first two-three elements"
...here).
+ * of vector elements as necessary.
+ */
+ if (padded_niov > IOV_MAX) {
[..]
@@ -1653,8 +1786,8 @@ int coroutine_fn
bdrv_co_preadv_part(BdrvChild *child,
flags |= BDRV_REQ_COPY_ON_READ;
}
- ret = bdrv_pad_request(bs, &qiov, &qiov_offset, &offset,
&bytes, &pad,
- NULL, &flags);
+ ret = bdrv_pad_request(bs, &qiov, &qiov_offset, &offset,
&bytes, false,
+ &pad, NULL, &flags);
if (ret < 0) {
goto fail;
}
a bit later:
tracked_request_end(&req);
bdrv_padding_destroy(&pad);
Now, the request is formally finished inside
bdrv_padding_destroy().. Not sure, does it really violate
something, but seems safer to swap these two calls.
I’d rather not, for two reasons: First, tracked requests are (as
far as I understand) only there to implement request serialization,
and so only care about metadata (offset, length, and type), which
is not changed by changes to the I/O vector.
Second, even if the state of the I/O vector were relevant to
tracked requests, I think it would actually be the other way
around, i.e. the tracked request must be ended before the padding
is finalized/destroyed. The tracked request is about the actual
request we submit to `child` (which is why tracked_request_begin()
is called after bdrv_pad_request()), and that request is done using
the modified I/O vector. So if the tracked request had any
connection to the request’s I/O vector (which it doesn’t), it would
be to this modified one, so we mustn’t invalidate it via
bdrv_padding_finalize() while the tracked request lives.
Or, said differently: I generally try to clean up things in the
inverse way they were set up, and because bdrv_pad_requests() comes
before tracked_request_begin(), I think tracked_request_end()
should come before bdrv_padding_finalize().