[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH v11 05/13] copy-on-read: limit COR operations to base in COR
From: |
Max Reitz |
Subject: |
Re: [PATCH v11 05/13] copy-on-read: limit COR operations to base in COR driver |
Date: |
Wed, 14 Oct 2020 13:59:22 +0200 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101 Thunderbird/68.11.0 |
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@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.
> + 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
signature.asc
Description: OpenPGP digital signature
- [PATCH v11 03/13] qapi: add filter-node-name to block-stream, (continued)
- [PATCH v11 03/13] qapi: add filter-node-name to block-stream, Andrey Shinkevich, 2020/10/12
- [PATCH v11 04/13] copy-on-read: pass overlay base node name to COR driver, Andrey Shinkevich, 2020/10/12
- Re: [PATCH v11 04/13] copy-on-read: pass overlay base node name to COR driver, Andrey Shinkevich, 2020/10/14
- Re: [PATCH v11 04/13] copy-on-read: pass overlay base node name to COR driver, Vladimir Sementsov-Ogievskiy, 2020/10/14
- Re: [PATCH v11 04/13] copy-on-read: pass overlay base node name to COR driver, Max Reitz, 2020/10/14
[PATCH v11 05/13] copy-on-read: limit COR operations to base in COR driver, Andrey Shinkevich, 2020/10/12
[PATCH v11 06/13] block: modify the comment for BDRV_REQ_PREFETCH flag, Andrey Shinkevich, 2020/10/12