On 14.10.2020 14:59, Max Reitz wrote:
On 12.10.20 19:43, Andrey Shinkevich wrote:
Limit COR operations by the base node in the backing chain when the
overlay base node name is given. It will be useful for a block stream
job when the COR-filter is applied. The overlay base node is passed as
the base itself may change due to concurrent commit jobs on the same
backing chain.
Signed-off-by: Andrey Shinkevich <andrey.shinkevich@xxxxxxxxxxxxx>
---
block/copy-on-read.c | 39 +++++++++++++++++++++++++++++++++++++--
1 file changed, 37 insertions(+), 2 deletions(-)
diff --git a/block/copy-on-read.c b/block/copy-on-read.c
index c578b1b..dfbd6ad 100644
--- a/block/copy-on-read.c
+++ b/block/copy-on-read.c
@@ -122,8 +122,43 @@ static int coroutine_fn cor_co_preadv_part(BlockDriverState *bs,
size_t qiov_offset,
int flags)
{
- return bdrv_co_preadv_part(bs->file, offset, bytes, qiov, qiov_offset,
- flags | BDRV_REQ_COPY_ON_READ);
+ int64_t n = 0;
+ int64_t size = offset + bytes;
+ int local_flags;
+ int ret;
+ BDRVStateCOR *state = bs->opaque;
+
+ if (!state->base_overlay) {
+ return bdrv_co_preadv_part(bs->file, offset, bytes, qiov, qiov_offset,
+ flags | BDRV_REQ_COPY_ON_READ);
+ }
+
+ while (offset < size) {
+ local_flags = flags;
+
+ /* In case of failure, try to copy-on-read anyway */
+ ret = bdrv_is_allocated(bs->file->bs, offset, bytes, &n);
+ if (!ret) {
In case of failure, a negative value is going to be returned, we won’t
go into this conditional block, and local_flags isn’t going to contain
BDRV_REQ_COPY_ON_READ.
So the idea of CORing in case of failure sounds sound to me, but it
doesn’t look like that’s done.
Yes, it's obvious. That was just my fault to miss setting the additional
condition for "ret < 0". Thank you for noticing that.
Andrey
+ ret = bdrv_is_allocated_above(bdrv_cow_bs(bs->file->bs),
I think this should either be bdrv_backing_chain_next() or we must rule
out the possibility of bs->file->bs being a filter somewhere. I think
I’d prefer the former.
+ state->base_overlay, true, offset,
+ n, &n);
+ if (ret) {
“ret == 1 || ret < 0” would be more explicit (and in line with the “!ret
|| ret < 0” probably needed above), but correct either way.
Max
