Re: [PATCH v2 26/36] block/backup-top: drop .active
Am 04.02.2021 um 14:46 hat Vladimir Sementsov-Ogievskiy geschrieben:
> 04.02.2021 16:25, Kevin Wolf wrote:
> > Am 04.02.2021 um 13:33 hat Vladimir Sementsov-Ogievskiy geschrieben:
> > > 04.02.2021 15:26, Kevin Wolf wrote:
> > > > Am 27.11.2020 um 15:45 hat Vladimir Sementsov-Ogievskiy geschrieben:
> > > > > We don't need this workaround anymore: bdrv_append is already smart
> > > > > enough and we can use new bdrv_drop_filter().
> > > > >
> > > > > Signed-off-by: Vladimir Sementsov-Ogievskiy
> > > > > ---
> > > > >block/backup-top.c | 38
> > > > > +-
> > > > >tests/qemu-iotests/283.out | 2 +-
> > > > >2 files changed, 2 insertions(+), 38 deletions(-)
> > > > >
> > > > > diff --git a/block/backup-top.c b/block/backup-top.c
> > > > > index 650ed6195c..84eb73aeb7 100644
> > > > > --- a/block/backup-top.c
> > > > > +++ b/block/backup-top.c
> > > > > @@ -37,7 +37,6 @@
> > > > >typedef struct BDRVBackupTopState {
> > > > >BlockCopyState *bcs;
> > > > >BdrvChild *target;
> > > > > -bool active;
> > > > >int64_t cluster_size;
> > > > >} BDRVBackupTopState;
> > > > > @@ -127,21 +126,6 @@ static void
> > > > > backup_top_child_perm(BlockDriverState *bs, BdrvChild *c,
> > > > > uint64_t perm, uint64_t shared,
> > > > > uint64_t *nperm, uint64_t
> > > > > *nshared)
> > > > >{
> > > > > -BDRVBackupTopState *s = bs->opaque;
> > > > > -
> > > > > -if (!s->active) {
> > > > > -/*
> > > > > - * The filter node may be in process of bdrv_append(), which
> > > > > firstly do
> > > > > - * bdrv_set_backing_hd() and then bdrv_replace_node(). This
> > > > > means that
> > > > > - * we can't unshare BLK_PERM_WRITE during bdrv_append()
> > > > > operation. So,
> > > > > - * let's require nothing during bdrv_append() and refresh
> > > > > permissions
> > > > > - * after it (see bdrv_backup_top_append()).
> > > > > - */
> > > > > -*nperm = 0;
> > > > > -*nshared = BLK_PERM_ALL;
> > > > > -return;
> > > > > -}
> > > > > -
> > > > >if (!(role & BDRV_CHILD_FILTERED)) {
> > > > >/*
> > > > > * Target child
> > > > > @@ -229,18 +213,6 @@ BlockDriverState
> > > > > *bdrv_backup_top_append(BlockDriverState *source,
> > > > >}
> > > > >appended = true;
> > > > > -/*
> > > > > - * bdrv_append() finished successfully, now we can require
> > > > > permissions
> > > > > - * we want.
> > > > > - */
> > > > > -state->active = true;
> > > > > -bdrv_child_refresh_perms(top, top->backing, _err);
> > > >
> > > > bdrv_append() uses bdrv_refresh_perms() for the whole node. Is it doing
> > > > unnecessary extra work there and should really do the same as backup-top
> > > > did here, i.e. bdrv_child_refresh_perms(bs_new->backing)?
> > > >
> > > > (Really a comment for an earlier patch. This patch itself looks fine.)
> > > >
> > >
> > > You mean how backup-top code works at the point when we modified
> > > bdrv_append()? Actually all works, as we use state->active. We set it
> > > to true and should call refresh_perms. Now we drop _refresh_perms
> > > _together_ with state->active variable, so filter is always "active",
> > > but new bdrv_append can handle it now. I.e., before this patch
> > > backup-top.c code is correct but over-complicated with logic which is
> > > not necessary after bdrv_append() improvement (and of-course we need
> > > also bdrv_drop_filter() to drop the whole state->active related
> > > logic).
> >
> > No, I just mean that bdrv_child_refresh_perms(bs, bs->backing) is enough
> > when adding a new image to the chain. A full bdrv_child_refresh_perms()
> > like we now have in bdrv_append() is doing more work than is necessary.
> >
> > It doesn't make a difference for backup-top (because the filter has only
> > a single child), but if you append a new qcow2 snapshot, you would also
> > recalculate permissions for the bs->file subtree even though nothing has
> > changed there.
> >
> > It's only a small detail anyway, not very important in a slow path.
>
> Understand now. I think bdrv_append() do correct things: bs_new gets
> new parents, so we refresh the whole subtree.. So for appending qcow2
> we should refresh its file child as well. Probably new permissions of
> new bs_new parents will influence what qcow2 wants to do with it file
> node..
You mean the parents that move from bs_top to bs_new and that they could
change the permissions that bs_new needs?
Good point, yes.
Kevin
Re: [PATCH v2 26/36] block/backup-top: drop .active
04.02.2021 16:25, Kevin Wolf wrote:
Am 04.02.2021 um 13:33 hat Vladimir Sementsov-Ogievskiy geschrieben:
04.02.2021 15:26, Kevin Wolf wrote:
Am 27.11.2020 um 15:45 hat Vladimir Sementsov-Ogievskiy geschrieben:
We don't need this workaround anymore: bdrv_append is already smart
enough and we can use new bdrv_drop_filter().
Signed-off-by: Vladimir Sementsov-Ogievskiy
---
block/backup-top.c | 38 +-
tests/qemu-iotests/283.out | 2 +-
2 files changed, 2 insertions(+), 38 deletions(-)
diff --git a/block/backup-top.c b/block/backup-top.c
index 650ed6195c..84eb73aeb7 100644
--- a/block/backup-top.c
+++ b/block/backup-top.c
@@ -37,7 +37,6 @@
typedef struct BDRVBackupTopState {
BlockCopyState *bcs;
BdrvChild *target;
-bool active;
int64_t cluster_size;
} BDRVBackupTopState;
@@ -127,21 +126,6 @@ static void backup_top_child_perm(BlockDriverState *bs,
BdrvChild *c,
uint64_t perm, uint64_t shared,
uint64_t *nperm, uint64_t *nshared)
{
-BDRVBackupTopState *s = bs->opaque;
-
-if (!s->active) {
-/*
- * The filter node may be in process of bdrv_append(), which firstly do
- * bdrv_set_backing_hd() and then bdrv_replace_node(). This means that
- * we can't unshare BLK_PERM_WRITE during bdrv_append() operation. So,
- * let's require nothing during bdrv_append() and refresh permissions
- * after it (see bdrv_backup_top_append()).
- */
-*nperm = 0;
-*nshared = BLK_PERM_ALL;
-return;
-}
-
if (!(role & BDRV_CHILD_FILTERED)) {
/*
* Target child
@@ -229,18 +213,6 @@ BlockDriverState *bdrv_backup_top_append(BlockDriverState
*source,
}
appended = true;
-/*
- * bdrv_append() finished successfully, now we can require permissions
- * we want.
- */
-state->active = true;
-bdrv_child_refresh_perms(top, top->backing, _err);
bdrv_append() uses bdrv_refresh_perms() for the whole node. Is it doing
unnecessary extra work there and should really do the same as backup-top
did here, i.e. bdrv_child_refresh_perms(bs_new->backing)?
(Really a comment for an earlier patch. This patch itself looks fine.)
You mean how backup-top code works at the point when we modified
bdrv_append()? Actually all works, as we use state->active. We set it
to true and should call refresh_perms. Now we drop _refresh_perms
_together_ with state->active variable, so filter is always "active",
but new bdrv_append can handle it now. I.e., before this patch
backup-top.c code is correct but over-complicated with logic which is
not necessary after bdrv_append() improvement (and of-course we need
also bdrv_drop_filter() to drop the whole state->active related
logic).
No, I just mean that bdrv_child_refresh_perms(bs, bs->backing) is enough
when adding a new image to the chain. A full bdrv_child_refresh_perms()
like we now have in bdrv_append() is doing more work than is necessary.
It doesn't make a difference for backup-top (because the filter has only
a single child), but if you append a new qcow2 snapshot, you would also
recalculate permissions for the bs->file subtree even though nothing has
changed there.
It's only a small detail anyway, not very important in a slow path.
Understand now. I think bdrv_append() do correct things: bs_new gets new
parents, so we refresh the whole subtree.. So for appending qcow2 we should
refresh its file child as well. Probably new permissions of new bs_new parents
will influence what qcow2 wants to do with it file node..
--
Best regards,
Vladimir
Re: [PATCH v2 26/36] block/backup-top: drop .active
Am 04.02.2021 um 13:33 hat Vladimir Sementsov-Ogievskiy geschrieben:
> 04.02.2021 15:26, Kevin Wolf wrote:
> > Am 27.11.2020 um 15:45 hat Vladimir Sementsov-Ogievskiy geschrieben:
> > > We don't need this workaround anymore: bdrv_append is already smart
> > > enough and we can use new bdrv_drop_filter().
> > >
> > > Signed-off-by: Vladimir Sementsov-Ogievskiy
> > > ---
> > > block/backup-top.c | 38 +-
> > > tests/qemu-iotests/283.out | 2 +-
> > > 2 files changed, 2 insertions(+), 38 deletions(-)
> > >
> > > diff --git a/block/backup-top.c b/block/backup-top.c
> > > index 650ed6195c..84eb73aeb7 100644
> > > --- a/block/backup-top.c
> > > +++ b/block/backup-top.c
> > > @@ -37,7 +37,6 @@
> > > typedef struct BDRVBackupTopState {
> > > BlockCopyState *bcs;
> > > BdrvChild *target;
> > > -bool active;
> > > int64_t cluster_size;
> > > } BDRVBackupTopState;
> > > @@ -127,21 +126,6 @@ static void backup_top_child_perm(BlockDriverState
> > > *bs, BdrvChild *c,
> > > uint64_t perm, uint64_t shared,
> > > uint64_t *nperm, uint64_t *nshared)
> > > {
> > > -BDRVBackupTopState *s = bs->opaque;
> > > -
> > > -if (!s->active) {
> > > -/*
> > > - * The filter node may be in process of bdrv_append(), which
> > > firstly do
> > > - * bdrv_set_backing_hd() and then bdrv_replace_node(). This
> > > means that
> > > - * we can't unshare BLK_PERM_WRITE during bdrv_append()
> > > operation. So,
> > > - * let's require nothing during bdrv_append() and refresh
> > > permissions
> > > - * after it (see bdrv_backup_top_append()).
> > > - */
> > > -*nperm = 0;
> > > -*nshared = BLK_PERM_ALL;
> > > -return;
> > > -}
> > > -
> > > if (!(role & BDRV_CHILD_FILTERED)) {
> > > /*
> > >* Target child
> > > @@ -229,18 +213,6 @@ BlockDriverState
> > > *bdrv_backup_top_append(BlockDriverState *source,
> > > }
> > > appended = true;
> > > -/*
> > > - * bdrv_append() finished successfully, now we can require
> > > permissions
> > > - * we want.
> > > - */
> > > -state->active = true;
> > > -bdrv_child_refresh_perms(top, top->backing, _err);
> >
> > bdrv_append() uses bdrv_refresh_perms() for the whole node. Is it doing
> > unnecessary extra work there and should really do the same as backup-top
> > did here, i.e. bdrv_child_refresh_perms(bs_new->backing)?
> >
> > (Really a comment for an earlier patch. This patch itself looks fine.)
> >
>
> You mean how backup-top code works at the point when we modified
> bdrv_append()? Actually all works, as we use state->active. We set it
> to true and should call refresh_perms. Now we drop _refresh_perms
> _together_ with state->active variable, so filter is always "active",
> but new bdrv_append can handle it now. I.e., before this patch
> backup-top.c code is correct but over-complicated with logic which is
> not necessary after bdrv_append() improvement (and of-course we need
> also bdrv_drop_filter() to drop the whole state->active related
> logic).
No, I just mean that bdrv_child_refresh_perms(bs, bs->backing) is enough
when adding a new image to the chain. A full bdrv_child_refresh_perms()
like we now have in bdrv_append() is doing more work than is necessary.
It doesn't make a difference for backup-top (because the filter has only
a single child), but if you append a new qcow2 snapshot, you would also
recalculate permissions for the bs->file subtree even though nothing has
changed there.
It's only a small detail anyway, not very important in a slow path.
Kevin
Re: [PATCH v2 26/36] block/backup-top: drop .active
04.02.2021 15:26, Kevin Wolf wrote:
Am 27.11.2020 um 15:45 hat Vladimir Sementsov-Ogievskiy geschrieben:
We don't need this workaround anymore: bdrv_append is already smart
enough and we can use new bdrv_drop_filter().
Signed-off-by: Vladimir Sementsov-Ogievskiy
---
block/backup-top.c | 38 +-
tests/qemu-iotests/283.out | 2 +-
2 files changed, 2 insertions(+), 38 deletions(-)
diff --git a/block/backup-top.c b/block/backup-top.c
index 650ed6195c..84eb73aeb7 100644
--- a/block/backup-top.c
+++ b/block/backup-top.c
@@ -37,7 +37,6 @@
typedef struct BDRVBackupTopState {
BlockCopyState *bcs;
BdrvChild *target;
-bool active;
int64_t cluster_size;
} BDRVBackupTopState;
@@ -127,21 +126,6 @@ static void backup_top_child_perm(BlockDriverState *bs, BdrvChild *c,
uint64_t perm, uint64_t shared,
uint64_t *nperm, uint64_t *nshared)
{
-BDRVBackupTopState *s = bs->opaque;
-
-if (!s->active) {
-/*
- * The filter node may be in process of bdrv_append(), which firstly do
- * bdrv_set_backing_hd() and then bdrv_replace_node(). This means that
- * we can't unshare BLK_PERM_WRITE during bdrv_append() operation. So,
- * let's require nothing during bdrv_append() and refresh permissions
- * after it (see bdrv_backup_top_append()).
- */
-*nperm = 0;
-*nshared = BLK_PERM_ALL;
-return;
-}
-
if (!(role & BDRV_CHILD_FILTERED)) {
/*
* Target child
@@ -229,18 +213,6 @@ BlockDriverState *bdrv_backup_top_append(BlockDriverState
*source,
}
appended = true;
-/*
- * bdrv_append() finished successfully, now we can require permissions
- * we want.
- */
-state->active = true;
-bdrv_child_refresh_perms(top, top->backing, _err);
bdrv_append() uses bdrv_refresh_perms() for the whole node. Is it doing
unnecessary extra work there and should really do the same as backup-top
did here, i.e. bdrv_child_refresh_perms(bs_new->backing)?
(Really a comment for an earlier patch. This patch itself looks fine.)
You mean how backup-top code works at the point when we modified bdrv_append()? Actually all
works, as we use state->active. We set it to true and should call refresh_perms. Now we drop
_refresh_perms _together_ with state->active variable, so filter is always "active",
but new bdrv_append can handle it now. I.e., before this patch backup-top.c code is correct but
over-complicated with logic which is not necessary after bdrv_append() improvement (and of-course
we need also bdrv_drop_filter() to drop the whole state->active related logic).
--
Best regards,
Vladimir
Re: [PATCH v2 26/36] block/backup-top: drop .active
Am 27.11.2020 um 15:45 hat Vladimir Sementsov-Ogievskiy geschrieben:
> We don't need this workaround anymore: bdrv_append is already smart
> enough and we can use new bdrv_drop_filter().
>
> Signed-off-by: Vladimir Sementsov-Ogievskiy
> ---
> block/backup-top.c | 38 +-
> tests/qemu-iotests/283.out | 2 +-
> 2 files changed, 2 insertions(+), 38 deletions(-)
>
> diff --git a/block/backup-top.c b/block/backup-top.c
> index 650ed6195c..84eb73aeb7 100644
> --- a/block/backup-top.c
> +++ b/block/backup-top.c
> @@ -37,7 +37,6 @@
> typedef struct BDRVBackupTopState {
> BlockCopyState *bcs;
> BdrvChild *target;
> -bool active;
> int64_t cluster_size;
> } BDRVBackupTopState;
>
> @@ -127,21 +126,6 @@ static void backup_top_child_perm(BlockDriverState *bs,
> BdrvChild *c,
>uint64_t perm, uint64_t shared,
>uint64_t *nperm, uint64_t *nshared)
> {
> -BDRVBackupTopState *s = bs->opaque;
> -
> -if (!s->active) {
> -/*
> - * The filter node may be in process of bdrv_append(), which firstly
> do
> - * bdrv_set_backing_hd() and then bdrv_replace_node(). This means
> that
> - * we can't unshare BLK_PERM_WRITE during bdrv_append() operation.
> So,
> - * let's require nothing during bdrv_append() and refresh permissions
> - * after it (see bdrv_backup_top_append()).
> - */
> -*nperm = 0;
> -*nshared = BLK_PERM_ALL;
> -return;
> -}
> -
> if (!(role & BDRV_CHILD_FILTERED)) {
> /*
> * Target child
> @@ -229,18 +213,6 @@ BlockDriverState
> *bdrv_backup_top_append(BlockDriverState *source,
> }
> appended = true;
>
> -/*
> - * bdrv_append() finished successfully, now we can require permissions
> - * we want.
> - */
> -state->active = true;
> -bdrv_child_refresh_perms(top, top->backing, _err);
bdrv_append() uses bdrv_refresh_perms() for the whole node. Is it doing
unnecessary extra work there and should really do the same as backup-top
did here, i.e. bdrv_child_refresh_perms(bs_new->backing)?
(Really a comment for an earlier patch. This patch itself looks fine.)
Kevin
