Re: [PATCH v11 05/13] copy-on-read: limit COR operations to base in COR driver

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(a)virtuozzo.com&gt; > --- > 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.

> + 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