| Message ID | 1602524605-481160-5-git-send-email-andrey.shinkevich@virtuozzo.com (mailing list archive) |
|---|---|
| State | New, archived |
| 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 >
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:) Max
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".
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. 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 >> > >
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
On 14.10.20 18: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. Sounds more complicated than to just split off BlockdevOptionsCor (from BlockdevOptionsGenericFormat, like e.g. BlockdevOptionsRaw does it). > 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. Well, they don’t have to pass anything if they don’t need a bottom node. So unless they want to use a bottom/base-overlay node, it won’t become more complicated. > If we introduce the specific BlockdevOptionsCor, what other options may > come with? Nothing yet, I think. >>> 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. Yes, agreed; I’m not talking about the variable identifier though. I’m talking about the option name, which in this version is just “base”. (And whenever that option is used, once here, once later in block/stream.c, there is an explanatory comment why we don’t pass the base node through it, but the first overlay above the base. Which makes me believe perhaps it shouldn’t be called “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); >> >> 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. Don’t forget that the error_setg() below also makes use of it: >>> + if (!base_overlay) { >>> + error_setg(errp, QERR_BASE_NOT_FOUND, base_overlay_node); >>> + return -EINVAL; >>> + } >>> + } 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(+)