[PATCH v11 05/13] copy-on-read: limit COR operations to base in COR driver
Andrey Shinkevich
andrey.shinkevich at virtuozzo.com
Wed Oct 14 17:43:35 UTC 2020
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 at virtuozzo.com>
>> ---
>> 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
>
More information about the libvir-list
mailing list