| Message ID | 1597785880-431103-3-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 |
19.08.2020 00:24, Andrey Shinkevich wrote: > Provide API for the COR-filter insertion/removal. > Also, drop the filter child permissions for an inactive state when the > filter node is being removed. > > Signed-off-by: Andrey Shinkevich <andrey.shinkevich@virtuozzo.com> > --- > block/copy-on-read.c | 103 +++++++++++++++++++++++++++++++++++++++++++++++++++ > block/copy-on-read.h | 36 ++++++++++++++++++ > 2 files changed, 139 insertions(+) > create mode 100644 block/copy-on-read.h > > diff --git a/block/copy-on-read.c b/block/copy-on-read.c > index cb03e0f..150d9b7 100644 > --- a/block/copy-on-read.c > +++ b/block/copy-on-read.c > @@ -23,11 +23,21 @@ > #include "qemu/osdep.h" > #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; > +} BDRVStateCOR; > > > static int cor_open(BlockDriverState *bs, QDict *options, int flags, > Error **errp) > { > + BDRVStateCOR *state = bs->opaque; > + > bs->file = bdrv_open_child(NULL, options, "file", bs, &child_of_bds, > BDRV_CHILD_FILTERED | BDRV_CHILD_PRIMARY, > false, errp); > @@ -42,6 +52,8 @@ 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); > > + state->active = true; We don't need to call bdrv_child_refresh_perms() here, as permissions will be updated soon, when the filter node get its parent, yes? Let's add at least a comment on this. > + > return 0; > } > > @@ -57,6 +69,17 @@ static void cor_child_perm(BlockDriverState *bs, BdrvChild *c, > uint64_t perm, uint64_t shared, > uint64_t *nperm, uint64_t *nshared) > { > + BDRVStateCOR *s = bs->opaque; > + > + if (!s->active) { > + /* > + * While the filter is being removed > + */ > + *nperm = 0; > + *nshared = BLK_PERM_ALL; > + return; > + } > + > *nperm = perm & PERM_PASSTHROUGH; > *nshared = (shared & PERM_PASSTHROUGH) | PERM_UNCHANGED; > > @@ -135,6 +158,7 @@ static void cor_lock_medium(BlockDriverState *bs, bool locked) > > static BlockDriver bdrv_copy_on_read = { > .format_name = "copy-on-read", > + .instance_size = sizeof(BDRVStateCOR), > > .bdrv_open = cor_open, > .bdrv_child_perm = cor_child_perm, > @@ -159,4 +183,83 @@ static void bdrv_copy_on_read_init(void) > bdrv_register(&bdrv_copy_on_read); > } > > + > +static BlockDriverState *create_filter_node(BlockDriverState *bs, > + const char *filter_node_name, > + Error **errp) > +{ > + QDict *opts = qdict_new(); > + > + qdict_put_str(opts, "driver", "copy-on-read"); > + qdict_put_str(opts, "file", bdrv_get_node_name(bs)); > + if (filter_node_name) { > + qdict_put_str(opts, "node-name", filter_node_name); > + } > + > + return bdrv_open(NULL, NULL, opts, BDRV_O_RDWR, errp); > +} > + > + > +BlockDriverState *bdrv_cor_filter_append(BlockDriverState *bs, > + const char *filter_node_name, > + Error **errp) > +{ > + BlockDriverState *cor_filter_bs; > + BDRVStateCOR *state; > + Error *local_err = NULL; > + > + cor_filter_bs = create_filter_node(bs, filter_node_name, errp); > + if (cor_filter_bs == NULL) { > + error_prepend(errp, "Could not create filter node: "); > + return NULL; > + } > + > + if (!filter_node_name) { > + cor_filter_bs->implicit = true; > + } > + > + bdrv_drained_begin(bs); > + bdrv_replace_node(bs, cor_filter_bs, &local_err); > + bdrv_drained_end(bs); > + > + if (local_err) { > + bdrv_unref(cor_filter_bs); > + error_propagate(errp, local_err); > + return NULL; > + } > + > + state = cor_filter_bs->opaque; > + state->active = true; hmm stop. it's already active, after create_filter_node, so you don't need to set it again, isn't it? > + > + return cor_filter_bs; > +} > + > + > +void bdrv_cor_filter_drop(BlockDriverState *cor_filter_bs) > +{ > + BdrvChild *child; > + BlockDriverState *bs; > + BDRVStateCOR *s = cor_filter_bs->opaque; > + > + child = bdrv_filter_child(cor_filter_bs); > + if (!child) { > + return; > + } > + bs = child->bs; > + > + /* Retain the BDS until we complete the graph change. */ > + bdrv_ref(bs); > + /* Hold a guest back from writing while permissions are being reset. */ > + bdrv_drained_begin(bs); > + /* Drop permissions before the graph change. */ > + s->active = false; > + bdrv_child_refresh_perms(cor_filter_bs, child, &error_abort); > + bdrv_replace_node(cor_filter_bs, bs, &error_abort); > + > + bdrv_drained_end(bs); > + bdrv_unref(bs); > + bdrv_unref(cor_filter_bs); > +} > + > + > block_init(bdrv_copy_on_read_init); > diff --git a/block/copy-on-read.h b/block/copy-on-read.h > new file mode 100644 > index 0000000..db03c6c > --- /dev/null > +++ b/block/copy-on-read.h > @@ -0,0 +1,36 @@ > +/* > + * Copy-on-read filter block driver > + * > + * The filter driver performs Copy-On-Read (COR) operations > + * > + * Copyright (c) 2018-2020 Virtuozzo International GmbH. > + * > + * Author: > + * Andrey Shinkevich <andrey.shinkevich@virtuozzo.com> > + * > + * This program is free software; you can redistribute it and/or modify > + * it under the terms of the GNU General Public License as published by > + * the Free Software Foundation; either version 2 of the License, or > + * (at your option) any later version. > + * > + * This program is distributed in the hope that it will be useful, > + * but WITHOUT ANY WARRANTY; without even the implied warranty of > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the > + * GNU General Public License for more details. > + * > + * You should have received a copy of the GNU General Public License > + * along with this program. If not, see <http://www.gnu.org/licenses/>. > + */ > + > +#ifndef COPY_ON_READ_FILTER > +#define COPY_ON_READ_FILTER > + > +#include "block/block_int.h" > +#include "block/block-copy.h" do you really need block-copy.h ? > + > +BlockDriverState *bdrv_cor_filter_append(BlockDriverState *bs, > + const char *filter_node_name, > + Error **errp); > +void bdrv_cor_filter_drop(BlockDriverState *cor_filter_bs); > + > +#endif /* COPY_ON_READ_FILTER */ > Code seems correct to me, with extra "state->active = true;" dropped: Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
On 19.08.2020 13:21, Vladimir Sementsov-Ogievskiy wrote: > 19.08.2020 00:24, Andrey Shinkevich wrote: >> Provide API for the COR-filter insertion/removal. >> Also, drop the filter child permissions for an inactive state when the >> filter node is being removed. >> >> Signed-off-by: Andrey Shinkevich <andrey.shinkevich@virtuozzo.com> >> --- >> block/copy-on-read.c | 103 >> +++++++++++++++++++++++++++++++++++++++++++++++++++ >> block/copy-on-read.h | 36 ++++++++++++++++++ >> 2 files changed, 139 insertions(+) >> create mode 100644 block/copy-on-read.h >> >> diff --git a/block/copy-on-read.c b/block/copy-on-read.c >> index cb03e0f..150d9b7 100644 >> --- a/block/copy-on-read.c >> +++ b/block/copy-on-read.c >> @@ -23,11 +23,21 @@ >> #include "qemu/osdep.h" >> #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; >> +} BDRVStateCOR; >> static int cor_open(BlockDriverState *bs, QDict *options, int >> flags, >> Error **errp) >> { >> + BDRVStateCOR *state = bs->opaque; >> + >> bs->file = bdrv_open_child(NULL, options, "file", bs, >> &child_of_bds, >> BDRV_CHILD_FILTERED | >> BDRV_CHILD_PRIMARY, >> false, errp); >> @@ -42,6 +52,8 @@ 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); >> + state->active = true; > > We don't need to call bdrv_child_refresh_perms() here, as permissions > will be > updated soon, when the filter node get its parent, yes? > > Let's add at least a comment on this. > >> + >> return 0; >> } >> @@ -57,6 +69,17 @@ static void cor_child_perm(BlockDriverState *bs, >> BdrvChild *c, >> uint64_t perm, uint64_t shared, >> uint64_t *nperm, uint64_t *nshared) >> { >> + BDRVStateCOR *s = bs->opaque; >> + >> + if (!s->active) { >> + /* >> + * While the filter is being removed >> + */ >> + *nperm = 0; >> + *nshared = BLK_PERM_ALL; >> + return; >> + } >> + >> *nperm = perm & PERM_PASSTHROUGH; >> *nshared = (shared & PERM_PASSTHROUGH) | PERM_UNCHANGED; >> @@ -135,6 +158,7 @@ static void cor_lock_medium(BlockDriverState >> *bs, bool locked) >> static BlockDriver bdrv_copy_on_read = { >> .format_name = "copy-on-read", >> + .instance_size = sizeof(BDRVStateCOR), >> .bdrv_open = cor_open, >> .bdrv_child_perm = cor_child_perm, >> @@ -159,4 +183,83 @@ static void bdrv_copy_on_read_init(void) >> bdrv_register(&bdrv_copy_on_read); >> } >> + >> +static BlockDriverState *create_filter_node(BlockDriverState *bs, >> + const char >> *filter_node_name, >> + Error **errp) >> +{ >> + QDict *opts = qdict_new(); >> + >> + qdict_put_str(opts, "driver", "copy-on-read"); >> + qdict_put_str(opts, "file", bdrv_get_node_name(bs)); >> + if (filter_node_name) { >> + qdict_put_str(opts, "node-name", filter_node_name); >> + } >> + >> + return bdrv_open(NULL, NULL, opts, BDRV_O_RDWR, errp); >> +} >> + >> + >> +BlockDriverState *bdrv_cor_filter_append(BlockDriverState *bs, >> + const char *filter_node_name, >> + Error **errp) >> +{ >> + BlockDriverState *cor_filter_bs; >> + BDRVStateCOR *state; >> + Error *local_err = NULL; >> + >> + cor_filter_bs = create_filter_node(bs, filter_node_name, errp); >> + if (cor_filter_bs == NULL) { >> + error_prepend(errp, "Could not create filter node: "); >> + return NULL; >> + } >> + >> + if (!filter_node_name) { >> + cor_filter_bs->implicit = true; >> + } >> + >> + bdrv_drained_begin(bs); >> + bdrv_replace_node(bs, cor_filter_bs, &local_err); >> + bdrv_drained_end(bs); >> + >> + if (local_err) { >> + bdrv_unref(cor_filter_bs); >> + error_propagate(errp, local_err); >> + return NULL; >> + } >> + >> + state = cor_filter_bs->opaque; >> + state->active = true; > > hmm stop. it's already active, after create_filter_node, so you don't > need to set it again, isn't it? I will remove the extra assignment from the cor_open() for consistancy. Andrey > >> + >> + return cor_filter_bs; >> +} >> + >> + >> +void bdrv_cor_filter_drop(BlockDriverState *cor_filter_bs) >> +{ >> + BdrvChild *child; >> + BlockDriverState *bs; >> + BDRVStateCOR *s = cor_filter_bs->opaque; >> + >> + child = bdrv_filter_child(cor_filter_bs); >> + if (!child) { >> + return; >> + } >> + bs = child->bs; >> + >> + /* Retain the BDS until we complete the graph change. */ >> + bdrv_ref(bs); >> + /* Hold a guest back from writing while permissions are being >> reset. */ >> + bdrv_drained_begin(bs); >> + /* Drop permissions before the graph change. */ >> + s->active = false; >> + bdrv_child_refresh_perms(cor_filter_bs, child, &error_abort); >> + bdrv_replace_node(cor_filter_bs, bs, &error_abort); >> + >> + bdrv_drained_end(bs); >> + bdrv_unref(bs); >> + bdrv_unref(cor_filter_bs); >> +} >> + >> + >> block_init(bdrv_copy_on_read_init); >> diff --git a/block/copy-on-read.h b/block/copy-on-read.h >> new file mode 100644 >> index 0000000..db03c6c >> --- /dev/null >> +++ b/block/copy-on-read.h >> @@ -0,0 +1,36 @@ >> +/* >> + * Copy-on-read filter block driver >> + * >> + * The filter driver performs Copy-On-Read (COR) operations >> + * >> + * Copyright (c) 2018-2020 Virtuozzo International GmbH. >> + * >> + * Author: >> + * Andrey Shinkevich <andrey.shinkevich@virtuozzo.com> >> + * >> + * This program is free software; you can redistribute it and/or modify >> + * it under the terms of the GNU General Public License as published by >> + * the Free Software Foundation; either version 2 of the License, or >> + * (at your option) any later version. >> + * >> + * This program is distributed in the hope that it will be useful, >> + * but WITHOUT ANY WARRANTY; without even the implied warranty of >> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the >> + * GNU General Public License for more details. >> + * >> + * You should have received a copy of the GNU General Public License >> + * along with this program. If not, see <http://www.gnu.org/licenses/>. >> + */ >> + >> +#ifndef COPY_ON_READ_FILTER >> +#define COPY_ON_READ_FILTER >> + >> +#include "block/block_int.h" >> +#include "block/block-copy.h" > > do you really need block-copy.h ? > >> + >> +BlockDriverState *bdrv_cor_filter_append(BlockDriverState *bs, >> + const char *filter_node_name, >> + Error **errp); >> +void bdrv_cor_filter_drop(BlockDriverState *cor_filter_bs); >> + >> +#endif /* COPY_ON_READ_FILTER */ >> > > Code seems correct to me, with extra "state->active = true;" dropped: > Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> >
On 23.08.2020 22:35, Andrey Shinkevich wrote: > On 19.08.2020 13:21, Vladimir Sementsov-Ogievskiy wrote: >> 19.08.2020 00:24, Andrey Shinkevich wrote: >>> Provide API for the COR-filter insertion/removal. >>> Also, drop the filter child permissions for an inactive state when the >>> filter node is being removed. >>> >>> Signed-off-by: Andrey Shinkevich <andrey.shinkevich@virtuozzo.com> >>> --- >>> block/copy-on-read.c | 103 >>> +++++++++++++++++++++++++++++++++++++++++++++++++++ >>> block/copy-on-read.h | 36 ++++++++++++++++++ >>> 2 files changed, 139 insertions(+) >>> create mode 100644 block/copy-on-read.h >>> >>> diff --git a/block/copy-on-read.c b/block/copy-on-read.c >>> index cb03e0f..150d9b7 100644 >>> --- a/block/copy-on-read.c >>> +++ b/block/copy-on-read.c >>> @@ -23,11 +23,21 @@ ... >>> +BlockDriverState *bdrv_cor_filter_append(BlockDriverState *bs, >>> + const char *filter_node_name, >>> + Error **errp) >>> +{ >>> + BlockDriverState *cor_filter_bs; >>> + BDRVStateCOR *state; >>> + Error *local_err = NULL; >>> + >>> + cor_filter_bs = create_filter_node(bs, filter_node_name, errp); >>> + if (cor_filter_bs == NULL) { >>> + error_prepend(errp, "Could not create filter node: "); >>> + return NULL; >>> + } >>> + >>> + if (!filter_node_name) { >>> + cor_filter_bs->implicit = true; >>> + } >>> + >>> + bdrv_drained_begin(bs); >>> + bdrv_replace_node(bs, cor_filter_bs, &local_err); >>> + bdrv_drained_end(bs); >>> + >>> + if (local_err) { >>> + bdrv_unref(cor_filter_bs); >>> + error_propagate(errp, local_err); >>> + return NULL; >>> + } >>> + >>> + state = cor_filter_bs->opaque; >>> + state->active = true; >> >> hmm stop. it's already active, after create_filter_node, so you don't >> need to set it again, isn't it? > > > I will remove the extra assignment from the cor_open() for consistancy. > > Andrey > > No, I won't. It is wrong to do that. COR-operations wouldn't work. Andrey >> >>> + >>> + return cor_filter_bs; >>> +} >>> + ...
diff --git a/block/copy-on-read.c b/block/copy-on-read.c index cb03e0f..150d9b7 100644 --- a/block/copy-on-read.c +++ b/block/copy-on-read.c @@ -23,11 +23,21 @@ #include "qemu/osdep.h" #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; +} BDRVStateCOR; static int cor_open(BlockDriverState *bs, QDict *options, int flags, Error **errp) { + BDRVStateCOR *state = bs->opaque; + bs->file = bdrv_open_child(NULL, options, "file", bs, &child_of_bds, BDRV_CHILD_FILTERED | BDRV_CHILD_PRIMARY, false, errp); @@ -42,6 +52,8 @@ 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); + state->active = true; + return 0; } @@ -57,6 +69,17 @@ static void cor_child_perm(BlockDriverState *bs, BdrvChild *c, uint64_t perm, uint64_t shared, uint64_t *nperm, uint64_t *nshared) { + BDRVStateCOR *s = bs->opaque; + + if (!s->active) { + /* + * While the filter is being removed + */ + *nperm = 0; + *nshared = BLK_PERM_ALL; + return; + } + *nperm = perm & PERM_PASSTHROUGH; *nshared = (shared & PERM_PASSTHROUGH) | PERM_UNCHANGED; @@ -135,6 +158,7 @@ static void cor_lock_medium(BlockDriverState *bs, bool locked) static BlockDriver bdrv_copy_on_read = { .format_name = "copy-on-read", + .instance_size = sizeof(BDRVStateCOR), .bdrv_open = cor_open, .bdrv_child_perm = cor_child_perm, @@ -159,4 +183,83 @@ static void bdrv_copy_on_read_init(void) bdrv_register(&bdrv_copy_on_read); } + +static BlockDriverState *create_filter_node(BlockDriverState *bs, + const char *filter_node_name, + Error **errp) +{ + QDict *opts = qdict_new(); + + qdict_put_str(opts, "driver", "copy-on-read"); + qdict_put_str(opts, "file", bdrv_get_node_name(bs)); + if (filter_node_name) { + qdict_put_str(opts, "node-name", filter_node_name); + } + + return bdrv_open(NULL, NULL, opts, BDRV_O_RDWR, errp); +} + + +BlockDriverState *bdrv_cor_filter_append(BlockDriverState *bs, + const char *filter_node_name, + Error **errp) +{ + BlockDriverState *cor_filter_bs; + BDRVStateCOR *state; + Error *local_err = NULL; + + cor_filter_bs = create_filter_node(bs, filter_node_name, errp); + if (cor_filter_bs == NULL) { + error_prepend(errp, "Could not create filter node: "); + return NULL; + } + + if (!filter_node_name) { + cor_filter_bs->implicit = true; + } + + bdrv_drained_begin(bs); + bdrv_replace_node(bs, cor_filter_bs, &local_err); + bdrv_drained_end(bs); + + if (local_err) { + bdrv_unref(cor_filter_bs); + error_propagate(errp, local_err); + return NULL; + } + + state = cor_filter_bs->opaque; + state->active = true; + + return cor_filter_bs; +} + + +void bdrv_cor_filter_drop(BlockDriverState *cor_filter_bs) +{ + BdrvChild *child; + BlockDriverState *bs; + BDRVStateCOR *s = cor_filter_bs->opaque; + + child = bdrv_filter_child(cor_filter_bs); + if (!child) { + return; + } + bs = child->bs; + + /* Retain the BDS until we complete the graph change. */ + bdrv_ref(bs); + /* Hold a guest back from writing while permissions are being reset. */ + bdrv_drained_begin(bs); + /* Drop permissions before the graph change. */ + s->active = false; + bdrv_child_refresh_perms(cor_filter_bs, child, &error_abort); + bdrv_replace_node(cor_filter_bs, bs, &error_abort); + + bdrv_drained_end(bs); + bdrv_unref(bs); + bdrv_unref(cor_filter_bs); +} + + block_init(bdrv_copy_on_read_init); diff --git a/block/copy-on-read.h b/block/copy-on-read.h new file mode 100644 index 0000000..db03c6c --- /dev/null +++ b/block/copy-on-read.h @@ -0,0 +1,36 @@ +/* + * Copy-on-read filter block driver + * + * The filter driver performs Copy-On-Read (COR) operations + * + * Copyright (c) 2018-2020 Virtuozzo International GmbH. + * + * Author: + * Andrey Shinkevich <andrey.shinkevich@virtuozzo.com> + * + * This program is free software; you can redistribute it and/or modify + * it under the terms of the GNU General Public License as published by + * the Free Software Foundation; either version 2 of the License, or + * (at your option) any later version. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + * + * You should have received a copy of the GNU General Public License + * along with this program. If not, see <http://www.gnu.org/licenses/>. + */ + +#ifndef COPY_ON_READ_FILTER +#define COPY_ON_READ_FILTER + +#include "block/block_int.h" +#include "block/block-copy.h" + +BlockDriverState *bdrv_cor_filter_append(BlockDriverState *bs, + const char *filter_node_name, + Error **errp); +void bdrv_cor_filter_drop(BlockDriverState *cor_filter_bs); + +#endif /* COPY_ON_READ_FILTER */
Provide API for the COR-filter insertion/removal. Also, drop the filter child permissions for an inactive state when the filter node is being removed. Signed-off-by: Andrey Shinkevich <andrey.shinkevich@virtuozzo.com> --- block/copy-on-read.c | 103 +++++++++++++++++++++++++++++++++++++++++++++++++++ block/copy-on-read.h | 36 ++++++++++++++++++ 2 files changed, 139 insertions(+) create mode 100644 block/copy-on-read.h