| Message ID | 1602524605-481160-5-git-send-email-andrey.shinkevich@virtuozzo.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Apply COR-filter to the block-stream permanently | expand |
On 12.10.20 19:43, Andrey Shinkevich wrote: > We are going to use the COR-filter for a block-stream job. > To limit COR operations by the base node in the backing chain during > stream job, pass the name of overlay base node to the copy-on-read > driver as base node itself may change due to possible concurrent jobs. > The rest of the functionality will be implemented in the patch that > follows. > > Signed-off-by: Andrey Shinkevich <andrey.shinkevich@virtuozzo.com> > --- > block/copy-on-read.c | 14 ++++++++++++++ > 1 file changed, 14 insertions(+) Is there a reason why you didn’t add this option to QAPI (as part of a yet-to-be-created BlockdevOptionsCor)? Because I’d really like it there. > diff --git a/block/copy-on-read.c b/block/copy-on-read.c > index bcccf0f..c578b1b 100644 > --- a/block/copy-on-read.c > +++ b/block/copy-on-read.c > @@ -24,19 +24,24 @@ > #include "block/block_int.h" > #include "qemu/module.h" > #include "qapi/error.h" > +#include "qapi/qmp/qerror.h" > #include "qapi/qmp/qdict.h" > #include "block/copy-on-read.h" > > > typedef struct BDRVStateCOR { > bool active; > + BlockDriverState *base_overlay; > } BDRVStateCOR; > > > static int cor_open(BlockDriverState *bs, QDict *options, int flags, > Error **errp) > { > + BlockDriverState *base_overlay = NULL; > BDRVStateCOR *state = bs->opaque; > + /* We need the base overlay node rather than the base itself */ > + const char *base_overlay_node = qdict_get_try_str(options, "base"); Shouldn’t it be called base-overlay or above-base then? > > bs->file = bdrv_open_child(NULL, options, "file", bs, &child_of_bds, > BDRV_CHILD_FILTERED | BDRV_CHILD_PRIMARY, > @@ -52,7 +57,16 @@ 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 (base_overlay_node) { > + qdict_del(options, "base"); > + base_overlay = bdrv_lookup_bs(NULL, base_overlay_node, errp); I think this is a use-after-free. The storage @base_overlay_node points to belongs to a QString, which is referenced only by @options; so deleting that element of @options should free that string. Max > + if (!base_overlay) { > + error_setg(errp, QERR_BASE_NOT_FOUND, base_overlay_node); > + return -EINVAL; > + } > + } > state->active = true; > + state->base_overlay = base_overlay; > > /* > * We don't need to call bdrv_child_refresh_perms() now as the permissions >
14.10.2020 14:57, Max Reitz wrote: > On 14.10.20 13:09, Max Reitz wrote: >> On 12.10.20 19:43, Andrey Shinkevich wrote: >>> We are going to use the COR-filter for a block-stream job. >>> To limit COR operations by the base node in the backing chain during >>> stream job, pass the name of overlay base node to the copy-on-read >>> driver as base node itself may change due to possible concurrent jobs. >>> The rest of the functionality will be implemented in the patch that >>> follows. >>> >>> Signed-off-by: Andrey Shinkevich <andrey.shinkevich@virtuozzo.com> >>> --- >>> block/copy-on-read.c | 14 ++++++++++++++ >>> 1 file changed, 14 insertions(+) >> >> Is there a reason why you didn’t add this option to QAPI (as part of a >> yet-to-be-created BlockdevOptionsCor)? Because I’d really like it there. >> >>> diff --git a/block/copy-on-read.c b/block/copy-on-read.c >>> index bcccf0f..c578b1b 100644 >>> --- a/block/copy-on-read.c >>> +++ b/block/copy-on-read.c >>> @@ -24,19 +24,24 @@ >>> #include "block/block_int.h" >>> #include "qemu/module.h" >>> #include "qapi/error.h" >>> +#include "qapi/qmp/qerror.h" >>> #include "qapi/qmp/qdict.h" >>> #include "block/copy-on-read.h" >>> >>> >>> typedef struct BDRVStateCOR { >>> bool active; >>> + BlockDriverState *base_overlay; >>> } BDRVStateCOR; >>> >>> >>> static int cor_open(BlockDriverState *bs, QDict *options, int flags, >>> Error **errp) >>> { >>> + BlockDriverState *base_overlay = NULL; >>> BDRVStateCOR *state = bs->opaque; >>> + /* We need the base overlay node rather than the base itself */ >>> + const char *base_overlay_node = qdict_get_try_str(options, "base"); >> >> Shouldn’t it be called base-overlay or above-base then? > > While reviewing patch 5, I realized this sould indeed be base-overlay > (i.e. the next allocation-bearing layer above the base, not just a > filter on top of base), but that should be noted somewhere, of course. > Best would be the QAPI documentation for this option. O:) > What about naming it just "bottom" or "bottom-node"? If we don't have base, it's strange to have something "relative to base". And we can document, that "bottom" must be a non-filter node in a backing chain of "top".
14.10.2020 19:08, Andrey Shinkevich wrote: > On 14.10.2020 14:09, Max Reitz wrote: >> On 12.10.20 19:43, Andrey Shinkevich wrote: >>> We are going to use the COR-filter for a block-stream job. >>> To limit COR operations by the base node in the backing chain during >>> stream job, pass the name of overlay base node to the copy-on-read >>> driver as base node itself may change due to possible concurrent jobs. >>> The rest of the functionality will be implemented in the patch that >>> follows. >>> >>> Signed-off-by: Andrey Shinkevich <andrey.shinkevich@virtuozzo.com> >>> --- >>> block/copy-on-read.c | 14 ++++++++++++++ >>> 1 file changed, 14 insertions(+) >> >> Is there a reason why you didn’t add this option to QAPI (as part of a >> yet-to-be-created BlockdevOptionsCor)? Because I’d really like it there. >> > > I agree that passing a base overlay under the base option looks clumsy. We could pass the base node name and find its overlay ourselves here in cor_open(). In that case, we can use the existing QAPI. Actually, there is no existing QAPI: if you don't modify qapi/*.json, user is not able to pass the option through QAPI. It's still possible to pass the option through command-line, or when create the filter internally (like we are going to do in block-stream), but not through QAPI. So, it's better to make a new QAPI parameter, to make the new option available for QMP interface. > The reason I used the existing QAPI is to make it easier for a user to operate with the traditional options and to keep things simple. So, the user shouldn't think what overlay or above-base node to pass. > If we introduce the specific BlockdevOptionsCor, what other options may come with? > >>> diff --git a/block/copy-on-read.c b/block/copy-on-read.c >>> index bcccf0f..c578b1b 100644 >>> --- a/block/copy-on-read.c >>> +++ b/block/copy-on-read.c >>> @@ -24,19 +24,24 @@ >>> #include "block/block_int.h" >>> #include "qemu/module.h" >>> #include "qapi/error.h" >>> +#include "qapi/qmp/qerror.h" >>> #include "qapi/qmp/qdict.h" >>> #include "block/copy-on-read.h" >>> typedef struct BDRVStateCOR { >>> bool active; >>> + BlockDriverState *base_overlay; >>> } BDRVStateCOR; >>> static int cor_open(BlockDriverState *bs, QDict *options, int flags, >>> Error **errp) >>> { >>> + BlockDriverState *base_overlay = NULL; >>> BDRVStateCOR *state = bs->opaque; >>> + /* We need the base overlay node rather than the base itself */ >>> + const char *base_overlay_node = qdict_get_try_str(options, "base"); >> >> Shouldn’t it be called base-overlay or above-base then? >> > > The base_overlay identifier is used below as the pointer to BS. The base_overlay_node stands for the name of the node. I used that identifier to differ between the types. And the above_base has another meaning per block/stream.c - it can be a temporary filter with a JSON-name. > >>> bs->file = bdrv_open_child(NULL, options, "file", bs, &child_of_bds, >>> BDRV_CHILD_FILTERED | BDRV_CHILD_PRIMARY, >>> @@ -52,7 +57,16 @@ 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 (base_overlay_node) { >>> + qdict_del(options, "base"); >>> + base_overlay = bdrv_lookup_bs(NULL, base_overlay_node, errp); >> >> I think this is a use-after-free. The storage @base_overlay_node points >> to belongs to a QString, which is referenced only by @options; so >> deleting that element of @options should free that string. >> >> Max >> > > I will swap those two function calls (bdrv_lookup_bs(); qdict_del();). > Thank you. > > Andrey > >>> + if (!base_overlay) { >>> + error_setg(errp, QERR_BASE_NOT_FOUND, base_overlay_node); >>> + return -EINVAL; >>> + } >>> + } >>> state->active = true; >>> + state->base_overlay = base_overlay; >>> /* >>> * We don't need to call bdrv_child_refresh_perms() now as the permissions >>> >> >>
On 14.10.20 16:56, Vladimir Sementsov-Ogievskiy wrote: > 14.10.2020 14:57, Max Reitz wrote: >> On 14.10.20 13:09, Max Reitz wrote: >>> On 12.10.20 19:43, Andrey Shinkevich wrote: >>>> We are going to use the COR-filter for a block-stream job. >>>> To limit COR operations by the base node in the backing chain during >>>> stream job, pass the name of overlay base node to the copy-on-read >>>> driver as base node itself may change due to possible concurrent jobs. >>>> The rest of the functionality will be implemented in the patch that >>>> follows. >>>> >>>> Signed-off-by: Andrey Shinkevich <andrey.shinkevich@virtuozzo.com> >>>> --- >>>> block/copy-on-read.c | 14 ++++++++++++++ >>>> 1 file changed, 14 insertions(+) >>> >>> Is there a reason why you didn’t add this option to QAPI (as part of a >>> yet-to-be-created BlockdevOptionsCor)? Because I’d really like it >>> there. >>> >>>> diff --git a/block/copy-on-read.c b/block/copy-on-read.c >>>> index bcccf0f..c578b1b 100644 >>>> --- a/block/copy-on-read.c >>>> +++ b/block/copy-on-read.c >>>> @@ -24,19 +24,24 @@ >>>> #include "block/block_int.h" >>>> #include "qemu/module.h" >>>> #include "qapi/error.h" >>>> +#include "qapi/qmp/qerror.h" >>>> #include "qapi/qmp/qdict.h" >>>> #include "block/copy-on-read.h" >>>> typedef struct BDRVStateCOR { >>>> bool active; >>>> + BlockDriverState *base_overlay; >>>> } BDRVStateCOR; >>>> static int cor_open(BlockDriverState *bs, QDict *options, int >>>> flags, >>>> Error **errp) >>>> { >>>> + BlockDriverState *base_overlay = NULL; >>>> BDRVStateCOR *state = bs->opaque; >>>> + /* We need the base overlay node rather than the base itself */ >>>> + const char *base_overlay_node = qdict_get_try_str(options, >>>> "base"); >>> >>> Shouldn’t it be called base-overlay or above-base then? >> >> While reviewing patch 5, I realized this sould indeed be base-overlay (Just realized that sounds different from how I meant it. I meant that “above-base” would be wrong, so from the two, if any, it should be “base-overlay”.) >> (i.e. the next allocation-bearing layer above the base, not just a >> filter on top of base), but that should be noted somewhere, of course. >> Best would be the QAPI documentation for this option. O:) >> > > What about naming it just "bottom" or "bottom-node"? If we don't have > base, it's strange to have something "relative to base". And we can > document, that "bottom" must be a non-filter node in a backing chain of > "top". Works for me, too. Max
diff --git a/block/copy-on-read.c b/block/copy-on-read.c index bcccf0f..c578b1b 100644 --- a/block/copy-on-read.c +++ b/block/copy-on-read.c @@ -24,19 +24,24 @@ #include "block/block_int.h" #include "qemu/module.h" #include "qapi/error.h" +#include "qapi/qmp/qerror.h" #include "qapi/qmp/qdict.h" #include "block/copy-on-read.h" typedef struct BDRVStateCOR { bool active; + BlockDriverState *base_overlay; } BDRVStateCOR; static int cor_open(BlockDriverState *bs, QDict *options, int flags, Error **errp) { + BlockDriverState *base_overlay = NULL; BDRVStateCOR *state = bs->opaque; + /* We need the base overlay node rather than the base itself */ + const char *base_overlay_node = qdict_get_try_str(options, "base"); bs->file = bdrv_open_child(NULL, options, "file", bs, &child_of_bds, BDRV_CHILD_FILTERED | BDRV_CHILD_PRIMARY, @@ -52,7 +57,16 @@ 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 (base_overlay_node) { + qdict_del(options, "base"); + base_overlay = bdrv_lookup_bs(NULL, base_overlay_node, errp); + if (!base_overlay) { + error_setg(errp, QERR_BASE_NOT_FOUND, base_overlay_node); + return -EINVAL; + } + } state->active = true; + state->base_overlay = base_overlay; /* * We don't need to call bdrv_child_refresh_perms() now as the permissions
We are going to use the COR-filter for a block-stream job. To limit COR operations by the base node in the backing chain during stream job, pass the name of overlay base node to the copy-on-read driver as base node itself may change due to possible concurrent jobs. The rest of the functionality will be implemented in the patch that follows. Signed-off-by: Andrey Shinkevich <andrey.shinkevich@virtuozzo.com> --- block/copy-on-read.c | 14 ++++++++++++++ 1 file changed, 14 insertions(+)