| Message ID | 20201204220758.2879-6-vsementsov@virtuozzo.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Apply COR-filter to the block-stream permanently | expand |
I don’t like this patch’s subject very much, because I find the implementation of the @bottom option to be more noteworthy than the addition of the QAPI structure. On 04.12.20 23:07, Vladimir Sementsov-Ogievskiy wrote: > From: Andrey Shinkevich <andrey.shinkevich@virtuozzo.com> > > Create the BlockdevOptionsCor structure for COR driver specific options > splitting it off form the BlockdevOptionsGenericFormat. The only option > 'bottom' node in the structure denotes an image file that limits the > COR operations in the backing chain. > We are going to use the COR-filter for a block-stream job and will pass > a bottom node name to the COR driver. The bottom node is the first > non-filter overlay of the base. It was introduced because the base node > itself may change due to possible concurrent jobs. > > Suggested-by: Max Reitz <mreitz@redhat.com> > Suggested-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> > Signed-off-by: Andrey Shinkevich <andrey.shinkevich@virtuozzo.com> > [vsementsov: fix bdrv_is_allocated_above() usage] > Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> > --- > qapi/block-core.json | 21 +++++++++++++++- > block/copy-on-read.c | 57 ++++++++++++++++++++++++++++++++++++++++++-- > 2 files changed, 75 insertions(+), 3 deletions(-) > > diff --git a/qapi/block-core.json b/qapi/block-core.json > index 8ef3df6767..04055ef50c 100644 > --- a/qapi/block-core.json > +++ b/qapi/block-core.json > @@ -3942,6 +3942,25 @@ > 'data': { 'throttle-group': 'str', > 'file' : 'BlockdevRef' > } } > + > +## > +# @BlockdevOptionsCor: > +# > +# Driver specific block device options for the copy-on-read driver. > +# > +# @bottom: the name of a non-filter node (allocation-bearing layer) that limits > +# the COR operations in the backing chain (inclusive). This seems to me like something’s missing. Perhaps technically there isn’t, but “limits the COR operations” begs the question (to me) “Limits them in what way?” (to which the answer is: No data below @bottom is copied). Could you make it more verbose? Perhaps something like “The name of a non-filter node (allocation-bearing layer) that limits the COR operations in the backing chain (inclusive), so that no data below this node will be copied by this filter”? > +# For the block-stream job, it will be the first non-filter overlay of > +# the base node. We do not involve the base node into the COR > +# operations because the base may change due to a concurrent > +# block-commit job on the same backing chain. I think the default behavior should be mentioned here somewhere, i.e. that no limit is applied, so that data from all backing layers may be copied. > +# > +# Since: 5.2 *6.0 > +## > +{ 'struct': 'BlockdevOptionsCor', > + 'base': 'BlockdevOptionsGenericFormat', > + 'data': { '*bottom': 'str' } } > + > ## > # @BlockdevOptions: > # [...] > diff --git a/block/copy-on-read.c b/block/copy-on-read.c > index 618c4c4f43..67f61983c0 100644 > --- a/block/copy-on-read.c > +++ b/block/copy-on-read.c [...] > @@ -51,7 +56,17 @@ static int cor_open(BlockDriverState *bs, QDict *options, int flags, > ((BDRV_REQ_FUA | BDRV_REQ_MAY_UNMAP | BDRV_REQ_NO_FALLBACK) & > bs->file->bs->supported_zero_flags); > > + if (bottom_node) { > + bottom_bs = bdrv_lookup_bs(NULL, bottom_node, errp); > + if (!bottom_bs) { > + error_setg(errp, "Bottom node '%s' not found", bottom_node); > + qdict_del(options, "bottom"); > + return -EINVAL; > + } Should we verify that bottom_bs is not a filter, as required by the schema? Max
10.12.2020 20:43, Max Reitz wrote: > I don’t like this patch’s subject very much, because I find the implementation of the @bottom option to be more noteworthy than the addition of the QAPI structure. > > > On 04.12.20 23:07, Vladimir Sementsov-Ogievskiy wrote: >> From: Andrey Shinkevich <andrey.shinkevich@virtuozzo.com> >> >> Create the BlockdevOptionsCor structure for COR driver specific options >> splitting it off form the BlockdevOptionsGenericFormat. The only option >> 'bottom' node in the structure denotes an image file that limits the >> COR operations in the backing chain. >> We are going to use the COR-filter for a block-stream job and will pass >> a bottom node name to the COR driver. The bottom node is the first >> non-filter overlay of the base. It was introduced because the base node >> itself may change due to possible concurrent jobs. >> >> Suggested-by: Max Reitz <mreitz@redhat.com> >> Suggested-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> >> Signed-off-by: Andrey Shinkevich <andrey.shinkevich@virtuozzo.com> >> [vsementsov: fix bdrv_is_allocated_above() usage] >> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> >> --- >> qapi/block-core.json | 21 +++++++++++++++- >> block/copy-on-read.c | 57 ++++++++++++++++++++++++++++++++++++++++++-- >> 2 files changed, 75 insertions(+), 3 deletions(-) >> >> diff --git a/qapi/block-core.json b/qapi/block-core.json >> index 8ef3df6767..04055ef50c 100644 >> --- a/qapi/block-core.json >> +++ b/qapi/block-core.json >> @@ -3942,6 +3942,25 @@ >> 'data': { 'throttle-group': 'str', >> 'file' : 'BlockdevRef' >> } } >> + >> +## >> +# @BlockdevOptionsCor: >> +# >> +# Driver specific block device options for the copy-on-read driver. >> +# >> +# @bottom: the name of a non-filter node (allocation-bearing layer) that limits >> +# the COR operations in the backing chain (inclusive). > > This seems to me like something’s missing. Perhaps technically there isn’t, but “limits the COR operations” begs the question (to me) “Limits them in what way?” (to which the answer is: No data below @bottom is copied). > > Could you make it more verbose? Perhaps something like “The name of a non-filter node (allocation-bearing layer) that limits the COR operations in the backing chain (inclusive), so that no data below this node will be copied by this filter”? Sounds good for me. > >> +# For the block-stream job, it will be the first non-filter overlay of >> +# the base node. We do not involve the base node into the COR >> +# operations because the base may change due to a concurrent >> +# block-commit job on the same backing chain. > I now see that paragraph conflicts with further introduce of "bottom" for stream job itself. I think it may be safely dropped. It's a wrong place to describe how block-stream works. > I think the default behavior should be mentioned here somewhere, i.e. that no limit is applied, so that data from all backing layers may be copied. agree > >> +# >> +# Since: 5.2 > > *6.0 > >> +## >> +{ 'struct': 'BlockdevOptionsCor', >> + 'base': 'BlockdevOptionsGenericFormat', >> + 'data': { '*bottom': 'str' } } >> + >> ## >> # @BlockdevOptions: >> # > > [...] > >> diff --git a/block/copy-on-read.c b/block/copy-on-read.c >> index 618c4c4f43..67f61983c0 100644 >> --- a/block/copy-on-read.c >> +++ b/block/copy-on-read.c > > [...] > >> @@ -51,7 +56,17 @@ static int cor_open(BlockDriverState *bs, QDict *options, int flags, >> ((BDRV_REQ_FUA | BDRV_REQ_MAY_UNMAP | BDRV_REQ_NO_FALLBACK) & >> bs->file->bs->supported_zero_flags); >> + if (bottom_node) { >> + bottom_bs = bdrv_lookup_bs(NULL, bottom_node, errp); >> + if (!bottom_bs) { >> + error_setg(errp, "Bottom node '%s' not found", bottom_node); >> + qdict_del(options, "bottom"); >> + return -EINVAL; >> + } > > Should we verify that bottom_bs is not a filter, as required by the schema? > yes, thanks for the catch! Hmm.. Interesting, we don't freeze the backing chain in cor filter open. And I think we shouldn't. But then, bottom node may disappear. We should handle it without a crash. I suggest: 1. document, that if bottom node disappear from the backing chain of the filter, it continues to work like without any specified "bottom" node 2. do bdrv_ref/bdrv_unref of bottom_bs, to not work with dead pointer 3. check in cor_co_preadv_part() is bottom_bs is still in backing chain or not Haha, bottom node may return into backing chain at some moment and we can continue to handle it:)
On 10.12.20 19:30, Vladimir Sementsov-Ogievskiy wrote: > 10.12.2020 20:43, Max Reitz wrote: >> I don’t like this patch’s subject very much, because I find the >> implementation of the @bottom option to be more noteworthy than the >> addition of the QAPI structure. >> >> >> On 04.12.20 23:07, Vladimir Sementsov-Ogievskiy wrote: >>> From: Andrey Shinkevich <andrey.shinkevich@virtuozzo.com> >>> >>> Create the BlockdevOptionsCor structure for COR driver specific options >>> splitting it off form the BlockdevOptionsGenericFormat. The only option >>> 'bottom' node in the structure denotes an image file that limits the >>> COR operations in the backing chain. >>> We are going to use the COR-filter for a block-stream job and will pass >>> a bottom node name to the COR driver. The bottom node is the first >>> non-filter overlay of the base. It was introduced because the base node >>> itself may change due to possible concurrent jobs. >>> >>> Suggested-by: Max Reitz <mreitz@redhat.com> >>> Suggested-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> >>> Signed-off-by: Andrey Shinkevich <andrey.shinkevich@virtuozzo.com> >>> [vsementsov: fix bdrv_is_allocated_above() usage] >>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> >>> --- >>> qapi/block-core.json | 21 +++++++++++++++- >>> block/copy-on-read.c | 57 ++++++++++++++++++++++++++++++++++++++++++-- >>> 2 files changed, 75 insertions(+), 3 deletions(-) >>> >>> diff --git a/qapi/block-core.json b/qapi/block-core.json >>> index 8ef3df6767..04055ef50c 100644 >>> --- a/qapi/block-core.json >>> +++ b/qapi/block-core.json >>> @@ -3942,6 +3942,25 @@ >>> 'data': { 'throttle-group': 'str', >>> 'file' : 'BlockdevRef' >>> } } >>> + >>> +## >>> +# @BlockdevOptionsCor: >>> +# >>> +# Driver specific block device options for the copy-on-read driver. >>> +# >>> +# @bottom: the name of a non-filter node (allocation-bearing layer) >>> that limits >>> +# the COR operations in the backing chain (inclusive). >> >> This seems to me like something’s missing. Perhaps technically there >> isn’t, but “limits the COR operations” begs the question (to me) >> “Limits them in what way?” (to which the answer is: No data below >> @bottom is copied). >> >> Could you make it more verbose? Perhaps something like “The name of a >> non-filter node (allocation-bearing layer) that limits the COR >> operations in the backing chain (inclusive), so that no data below >> this node will be copied by this filter”? > > Sounds good for me. > >> >>> +# For the block-stream job, it will be the first non-filter >>> overlay of >>> +# the base node. We do not involve the base node into the COR >>> +# operations because the base may change due to a concurrent >>> +# block-commit job on the same backing chain. >> > > I now see that paragraph conflicts with further introduce of "bottom" > for stream job itself. I think it may be safely dropped. It's a wrong > place to describe how block-stream works. > >> I think the default behavior should be mentioned here somewhere, i.e. >> that no limit is applied, so that data from all backing layers may be >> copied. > > agree > >> >>> +# >>> +# Since: 5.2 >> >> *6.0 >> >>> +## >>> +{ 'struct': 'BlockdevOptionsCor', >>> + 'base': 'BlockdevOptionsGenericFormat', >>> + 'data': { '*bottom': 'str' } } >>> + >>> ## >>> # @BlockdevOptions: >>> # >> >> [...] >> >>> diff --git a/block/copy-on-read.c b/block/copy-on-read.c >>> index 618c4c4f43..67f61983c0 100644 >>> --- a/block/copy-on-read.c >>> +++ b/block/copy-on-read.c >> >> [...] >> >>> @@ -51,7 +56,17 @@ static int cor_open(BlockDriverState *bs, QDict >>> *options, int flags, >>> ((BDRV_REQ_FUA | BDRV_REQ_MAY_UNMAP | BDRV_REQ_NO_FALLBACK) & >>> bs->file->bs->supported_zero_flags); >>> + if (bottom_node) { >>> + bottom_bs = bdrv_lookup_bs(NULL, bottom_node, errp); >>> + if (!bottom_bs) { >>> + error_setg(errp, "Bottom node '%s' not found", >>> bottom_node); >>> + qdict_del(options, "bottom"); >>> + return -EINVAL; >>> + } >> >> Should we verify that bottom_bs is not a filter, as required by the >> schema? >> > > yes, thanks for the catch! > > > Hmm.. Interesting, we don't freeze the backing chain in cor filter open. > And I think we shouldn't. But then, bottom node may disappear. We should > handle it without a crash. > > I suggest: > > 1. document, that if bottom node disappear from the backing chain of the > filter, it continues to work like without any specified "bottom" node > > 2. do bdrv_ref/bdrv_unref of bottom_bs, to not work with dead pointer > > 3. check in cor_co_preadv_part() is bottom_bs is still in backing chain > or not Hm, right. Alternatively, we could also freeze the chain until the bottom node and then allow changing the @bottom node through reopen. Then it would have to be manually unset before the bottom node is allowed to disappear from the chain. Would freezing the chain pose a problem? Max
11.12.2020 11:54, Max Reitz wrote: > On 10.12.20 19:30, Vladimir Sementsov-Ogievskiy wrote: >> 10.12.2020 20:43, Max Reitz wrote: >>> I don’t like this patch’s subject very much, because I find the implementation of the @bottom option to be more noteworthy than the addition of the QAPI structure. >>> >>> >>> On 04.12.20 23:07, Vladimir Sementsov-Ogievskiy wrote: >>>> From: Andrey Shinkevich <andrey.shinkevich@virtuozzo.com> >>>> >>>> Create the BlockdevOptionsCor structure for COR driver specific options >>>> splitting it off form the BlockdevOptionsGenericFormat. The only option >>>> 'bottom' node in the structure denotes an image file that limits the >>>> COR operations in the backing chain. >>>> We are going to use the COR-filter for a block-stream job and will pass >>>> a bottom node name to the COR driver. The bottom node is the first >>>> non-filter overlay of the base. It was introduced because the base node >>>> itself may change due to possible concurrent jobs. >>>> >>>> Suggested-by: Max Reitz <mreitz@redhat.com> >>>> Suggested-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> >>>> Signed-off-by: Andrey Shinkevich <andrey.shinkevich@virtuozzo.com> >>>> [vsementsov: fix bdrv_is_allocated_above() usage] >>>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> >>>> --- >>>> qapi/block-core.json | 21 +++++++++++++++- >>>> block/copy-on-read.c | 57 ++++++++++++++++++++++++++++++++++++++++++-- >>>> 2 files changed, 75 insertions(+), 3 deletions(-) >>>> >>>> diff --git a/qapi/block-core.json b/qapi/block-core.json >>>> index 8ef3df6767..04055ef50c 100644 >>>> --- a/qapi/block-core.json >>>> +++ b/qapi/block-core.json >>>> @@ -3942,6 +3942,25 @@ >>>> 'data': { 'throttle-group': 'str', >>>> 'file' : 'BlockdevRef' >>>> } } >>>> + >>>> +## >>>> +# @BlockdevOptionsCor: >>>> +# >>>> +# Driver specific block device options for the copy-on-read driver. >>>> +# >>>> +# @bottom: the name of a non-filter node (allocation-bearing layer) that limits >>>> +# the COR operations in the backing chain (inclusive). >>> >>> This seems to me like something’s missing. Perhaps technically there isn’t, but “limits the COR operations” begs the question (to me) “Limits them in what way?” (to which the answer is: No data below @bottom is copied). >>> >>> Could you make it more verbose? Perhaps something like “The name of a non-filter node (allocation-bearing layer) that limits the COR operations in the backing chain (inclusive), so that no data below this node will be copied by this filter”? >> >> Sounds good for me. >> >>> >>>> +# For the block-stream job, it will be the first non-filter overlay of >>>> +# the base node. We do not involve the base node into the COR >>>> +# operations because the base may change due to a concurrent >>>> +# block-commit job on the same backing chain. >>> >> >> I now see that paragraph conflicts with further introduce of "bottom" for stream job itself. I think it may be safely dropped. It's a wrong place to describe how block-stream works. >> >>> I think the default behavior should be mentioned here somewhere, i.e. that no limit is applied, so that data from all backing layers may be copied. >> >> agree >> >>> >>>> +# >>>> +# Since: 5.2 >>> >>> *6.0 >>> >>>> +## >>>> +{ 'struct': 'BlockdevOptionsCor', >>>> + 'base': 'BlockdevOptionsGenericFormat', >>>> + 'data': { '*bottom': 'str' } } >>>> + >>>> ## >>>> # @BlockdevOptions: >>>> # >>> >>> [...] >>> >>>> diff --git a/block/copy-on-read.c b/block/copy-on-read.c >>>> index 618c4c4f43..67f61983c0 100644 >>>> --- a/block/copy-on-read.c >>>> +++ b/block/copy-on-read.c >>> >>> [...] >>> >>>> @@ -51,7 +56,17 @@ static int cor_open(BlockDriverState *bs, QDict *options, int flags, >>>> ((BDRV_REQ_FUA | BDRV_REQ_MAY_UNMAP | BDRV_REQ_NO_FALLBACK) & >>>> bs->file->bs->supported_zero_flags); >>>> + if (bottom_node) { >>>> + bottom_bs = bdrv_lookup_bs(NULL, bottom_node, errp); >>>> + if (!bottom_bs) { >>>> + error_setg(errp, "Bottom node '%s' not found", bottom_node); >>>> + qdict_del(options, "bottom"); >>>> + return -EINVAL; >>>> + } >>> >>> Should we verify that bottom_bs is not a filter, as required by the schema? >>> >> >> yes, thanks for the catch! >> >> >> Hmm.. Interesting, we don't freeze the backing chain in cor filter open. And I think we shouldn't. But then, bottom node may disappear. We should handle it without a crash. >> >> I suggest: >> >> 1. document, that if bottom node disappear from the backing chain of the filter, it continues to work like without any specified "bottom" node >> >> 2. do bdrv_ref/bdrv_unref of bottom_bs, to not work with dead pointer >> >> 3. check in cor_co_preadv_part() is bottom_bs is still in backing chain or not > > Hm, right. > > Alternatively, we could also freeze the chain until the bottom node and then allow changing the @bottom node through reopen. Then it would have to be manually unset before the bottom node is allowed to disappear from the chain. > > Would freezing the chain pose a problem? > Hmm. Then we'll just need not freeze it in block-stream, so freezing is done by filter. It's just more restrictive.. But I can't imagine reasonable cases where user specify bottom node and than remove it. Forcing user to reopen the filter to change the bottom node may be more clean then "just don't care". OK, I think we can freeze the chain in the filter.
diff --git a/qapi/block-core.json b/qapi/block-core.json index 8ef3df6767..04055ef50c 100644 --- a/qapi/block-core.json +++ b/qapi/block-core.json @@ -3942,6 +3942,25 @@ 'data': { 'throttle-group': 'str', 'file' : 'BlockdevRef' } } + +## +# @BlockdevOptionsCor: +# +# Driver specific block device options for the copy-on-read driver. +# +# @bottom: the name of a non-filter node (allocation-bearing layer) that limits +# the COR operations in the backing chain (inclusive). +# For the block-stream job, it will be the first non-filter overlay of +# the base node. We do not involve the base node into the COR +# operations because the base may change due to a concurrent +# block-commit job on the same backing chain. +# +# Since: 5.2 +## +{ 'struct': 'BlockdevOptionsCor', + 'base': 'BlockdevOptionsGenericFormat', + 'data': { '*bottom': 'str' } } + ## # @BlockdevOptions: # @@ -3994,7 +4013,7 @@ 'bochs': 'BlockdevOptionsGenericFormat', 'cloop': 'BlockdevOptionsGenericFormat', 'compress': 'BlockdevOptionsGenericFormat', - 'copy-on-read':'BlockdevOptionsGenericFormat', + 'copy-on-read':'BlockdevOptionsCor', 'dmg': 'BlockdevOptionsGenericFormat', 'file': 'BlockdevOptionsFile', 'ftp': 'BlockdevOptionsCurlFtp', diff --git a/block/copy-on-read.c b/block/copy-on-read.c index 618c4c4f43..67f61983c0 100644 --- a/block/copy-on-read.c +++ b/block/copy-on-read.c @@ -24,18 +24,23 @@ #include "block/block_int.h" #include "qemu/module.h" #include "qapi/error.h" +#include "qapi/qmp/qdict.h" #include "block/copy-on-read.h" typedef struct BDRVStateCOR { bool active; + BlockDriverState *bottom_bs; } BDRVStateCOR; static int cor_open(BlockDriverState *bs, QDict *options, int flags, Error **errp) { + BlockDriverState *bottom_bs = NULL; BDRVStateCOR *state = bs->opaque; + /* Find a bottom node name, if any */ + const char *bottom_node = qdict_get_try_str(options, "bottom"); bs->file = bdrv_open_child(NULL, options, "file", bs, &child_of_bds, BDRV_CHILD_FILTERED | BDRV_CHILD_PRIMARY, @@ -51,7 +56,17 @@ static int cor_open(BlockDriverState *bs, QDict *options, int flags, ((BDRV_REQ_FUA | BDRV_REQ_MAY_UNMAP | BDRV_REQ_NO_FALLBACK) & bs->file->bs->supported_zero_flags); + if (bottom_node) { + bottom_bs = bdrv_lookup_bs(NULL, bottom_node, errp); + if (!bottom_bs) { + error_setg(errp, "Bottom node '%s' not found", bottom_node); + qdict_del(options, "bottom"); + return -EINVAL; + } + qdict_del(options, "bottom"); + } state->active = true; + state->bottom_bs = bottom_bs; /* * We don't need to call bdrv_child_refresh_perms() now as the permissions @@ -107,8 +122,46 @@ 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; + int local_flags; + int ret; + BDRVStateCOR *state = bs->opaque; + + if (!state->bottom_bs) { + return bdrv_co_preadv_part(bs->file, offset, bytes, qiov, qiov_offset, + flags | BDRV_REQ_COPY_ON_READ); + } + + while (bytes) { + 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 <= 0) { + ret = bdrv_is_allocated_above(bdrv_backing_chain_next(bs->file->bs), + state->bottom_bs, true, offset, + n, &n); + if (ret > 0 || ret < 0) { + local_flags |= BDRV_REQ_COPY_ON_READ; + } + /* Finish earlier if the end of a backing file has been reached */ + if (n == 0) { + break; + } + } + + ret = bdrv_co_preadv_part(bs->file, offset, n, qiov, qiov_offset, + local_flags); + if (ret < 0) { + return ret; + } + + offset += n; + qiov_offset += n; + bytes -= n; + } + + return 0; }