On 04.04.23 10:10, Vladimir Sementsov-Ogievskiy wrote:
On 03.04.23 16:33, Hanna Czenczek wrote:
(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().
Note, that it's wise-versa in bdrv_co_pwritev_part().
Well, and it’s this way here. We agree that for clean-up, the order
doesn’t functionally matter, so either way is actually fine.
For me it's just simpler to think that the whole request, including
filling user-given qiov with data on read part is inside
tracked_request_begin() / tracked_request_end().
It isn’t, though, because padding must be done before the tracked
request is created. The tracked request uses the request’s actual
offset and length, after padding, so bdrv_pad_request() must always be
done before (i.e., outside) tracked_request_begin().
And moving the last manipulation with qiov out of it breaks this
simple thought.
Guest should not care of it, as it doesn't know about request
tracking.. But what about internal code? Some code may depend on some
requests be finished after bdrv_drained_begin() call, but now they may
be not fully finished, and some data may be not copied back to
original qiov.
I agree with your point about sequence of objects finalization, but
maybe, that just shows that copying data back to qiov should not be a
part of bdrv_padding_finalize(), but instead be a separate function,
called before tracked_request_end().
But my thought is that copying back shouldn’t be done before
tracked_request_end(), because copying back is not part of the tracked
request. What we track is the padded request, which uses a modified I/O
vector, so undoing that modification shouldn’t be done while the tracked
request lives.
I know I’m inconsistent with regards to bdrv_co_pwritev_part(), which is
because it doesn’t matter. My actual position is that tracked requests
are about metadata, so undoing/finalizing the padding (including
potentially copying data back) has nothing to do with a tracked request,
so the order cannot of finalizing both cannot matter.
But you’re arguing for consistency, and my position on that is, if we
want consistency, I’d finalize the tracked request first, and the
padding second. This is also because tracking is done for
serialization, so we should end it as soon as possible, so that
concurrent requests are resumed quickly. (Though I’m not sure if
delaying it by a memcpy() matters for an essentially single-threaded
block layer at this time.)
Hanna
