Message ID | 20190627223255.3789-2-mreitz@redhat.com |
---|---|
State | New |
Headers | show |
Series | block: Add BDS.never_freeze | expand |
On 28/06/2019 01:32, Max Reitz wrote: > The commit and the mirror block job must be able to drop their filter > node at any point. However, this will not be possible if any of the > BdrvChild links to them is frozen. Therefore, we need to prevent them > from ever becoming frozen. > > Signed-off-by: Max Reitz <mreitz@redhat.com> > --- > include/block/block_int.h | 3 +++ > block.c | 8 ++++++++ > block/commit.c | 4 ++++ > block/mirror.c | 4 ++++ > 4 files changed, 19 insertions(+) > > diff --git a/include/block/block_int.h b/include/block/block_int.h > index d6415b53c1..50902531b7 100644 > --- a/include/block/block_int.h > +++ b/include/block/block_int.h > @@ -885,6 +885,9 @@ struct BlockDriverState { > > /* Only read/written by whoever has set active_flush_req to true. */ > unsigned int flushed_gen; /* Flushed write generation */ > + > + /* BdrvChild links to this node may never be frozen */ > + bool never_freeze; > }; > > struct BlockBackendRootState { > diff --git a/block.c b/block.c > index c139540f2b..6565192b91 100644 > --- a/block.c > +++ b/block.c > @@ -4416,6 +4416,14 @@ int bdrv_freeze_backing_chain(BlockDriverState *bs, BlockDriverState *base, > return -EPERM; > } > > + for (i = bs; i != base; i = backing_bs(i)) { > + if (i->backing && backing_bs(i)->never_freeze) { > + error_setg(errp, "Cannot freeze '%s' link to '%s'", > + i->backing->name, backing_bs(i)->node_name); > + return -EPERM; > + } > + } > + > for (i = bs; i != base; i = backing_bs(i)) { > if (i->backing) { > i->backing->frozen = true; > diff --git a/block/commit.c b/block/commit.c > index ca7e408b26..2c5a6d4ebc 100644 > --- a/block/commit.c > +++ b/block/commit.c > @@ -298,6 +298,10 @@ void commit_start(const char *job_id, BlockDriverState *bs, > if (!filter_node_name) { > commit_top_bs->implicit = true; > } > + > + /* So that we can always drop this node */ > + commit_top_bs->never_freeze = true; > + > commit_top_bs->total_sectors = top->total_sectors; > > bdrv_append(commit_top_bs, top, &local_err); > diff --git a/block/mirror.c b/block/mirror.c > index 2fcec70e35..8cb75fb409 100644 > --- a/block/mirror.c > +++ b/block/mirror.c > @@ -1551,6 +1551,10 @@ static BlockJob *mirror_start_job( > if (!filter_node_name) { > mirror_top_bs->implicit = true; > } > + > + /* So that we can always drop this node */ > + mirror_top_bs->never_freeze = true; > + > mirror_top_bs->total_sectors = bs->total_sectors; > mirror_top_bs->supported_write_flags = BDRV_REQ_WRITE_UNCHANGED; > mirror_top_bs->supported_zero_flags = BDRV_REQ_WRITE_UNCHANGED | > Reviewed-by: Andrey Shinkevich <andrey.shinkevich@virtuozzo.com>
On Fri 28 Jun 2019 12:32:51 AM CEST, Max Reitz wrote: > @@ -4416,6 +4416,14 @@ int bdrv_freeze_backing_chain(BlockDriverState *bs, BlockDriverState *base, > return -EPERM; > } > > + for (i = bs; i != base; i = backing_bs(i)) { > + if (i->backing && backing_bs(i)->never_freeze) { > + error_setg(errp, "Cannot freeze '%s' link to '%s'", > + i->backing->name, backing_bs(i)->node_name); > + return -EPERM; > + } > + } How about adding this to bdrv_is_backing_chain_frozen() instead? Berto
On 02.07.19 16:02, Alberto Garcia wrote: > On Fri 28 Jun 2019 12:32:51 AM CEST, Max Reitz wrote: >> @@ -4416,6 +4416,14 @@ int bdrv_freeze_backing_chain(BlockDriverState *bs, BlockDriverState *base, >> return -EPERM; >> } >> >> + for (i = bs; i != base; i = backing_bs(i)) { >> + if (i->backing && backing_bs(i)->never_freeze) { >> + error_setg(errp, "Cannot freeze '%s' link to '%s'", >> + i->backing->name, backing_bs(i)->node_name); >> + return -EPERM; >> + } >> + } > > How about adding this to bdrv_is_backing_chain_frozen() instead? But that’s the wrong place. For example, that function is called by bdrv_set_backing_hd() to check whether the backing BDS can be changed. But the point of never_freeze is to ensure that links to the BDS can be changed. never_freeze only becomes relevant when trying to freeze the backing chain, in that it should prevent it. So I think putting the check here is correct. Max
On Fri 28 Jun 2019 12:32:51 AM CEST, Max Reitz wrote: > The commit and the mirror block job must be able to drop their filter > node at any point. However, this will not be possible if any of the > BdrvChild links to them is frozen. Therefore, we need to prevent them > from ever becoming frozen. > > Signed-off-by: Max Reitz <mreitz@redhat.com> Reviewed-by: Alberto Garcia <berto@igalia.com> Berto
On 02.07.19 17:36, Alberto Garcia wrote: > On Fri 28 Jun 2019 12:32:51 AM CEST, Max Reitz wrote: >> The commit and the mirror block job must be able to drop their filter >> node at any point. However, this will not be possible if any of the >> BdrvChild links to them is frozen. Therefore, we need to prevent them >> from ever becoming frozen. >> >> Signed-off-by: Max Reitz <mreitz@redhat.com> > > Reviewed-by: Alberto Garcia <berto@igalia.com> Thanks! :-) Max
diff --git a/include/block/block_int.h b/include/block/block_int.h index d6415b53c1..50902531b7 100644 --- a/include/block/block_int.h +++ b/include/block/block_int.h @@ -885,6 +885,9 @@ struct BlockDriverState { /* Only read/written by whoever has set active_flush_req to true. */ unsigned int flushed_gen; /* Flushed write generation */ + + /* BdrvChild links to this node may never be frozen */ + bool never_freeze; }; struct BlockBackendRootState { diff --git a/block.c b/block.c index c139540f2b..6565192b91 100644 --- a/block.c +++ b/block.c @@ -4416,6 +4416,14 @@ int bdrv_freeze_backing_chain(BlockDriverState *bs, BlockDriverState *base, return -EPERM; } + for (i = bs; i != base; i = backing_bs(i)) { + if (i->backing && backing_bs(i)->never_freeze) { + error_setg(errp, "Cannot freeze '%s' link to '%s'", + i->backing->name, backing_bs(i)->node_name); + return -EPERM; + } + } + for (i = bs; i != base; i = backing_bs(i)) { if (i->backing) { i->backing->frozen = true; diff --git a/block/commit.c b/block/commit.c index ca7e408b26..2c5a6d4ebc 100644 --- a/block/commit.c +++ b/block/commit.c @@ -298,6 +298,10 @@ void commit_start(const char *job_id, BlockDriverState *bs, if (!filter_node_name) { commit_top_bs->implicit = true; } + + /* So that we can always drop this node */ + commit_top_bs->never_freeze = true; + commit_top_bs->total_sectors = top->total_sectors; bdrv_append(commit_top_bs, top, &local_err); diff --git a/block/mirror.c b/block/mirror.c index 2fcec70e35..8cb75fb409 100644 --- a/block/mirror.c +++ b/block/mirror.c @@ -1551,6 +1551,10 @@ static BlockJob *mirror_start_job( if (!filter_node_name) { mirror_top_bs->implicit = true; } + + /* So that we can always drop this node */ + mirror_top_bs->never_freeze = true; + mirror_top_bs->total_sectors = bs->total_sectors; mirror_top_bs->supported_write_flags = BDRV_REQ_WRITE_UNCHANGED; mirror_top_bs->supported_zero_flags = BDRV_REQ_WRITE_UNCHANGED |
The commit and the mirror block job must be able to drop their filter node at any point. However, this will not be possible if any of the BdrvChild links to them is frozen. Therefore, we need to prevent them from ever becoming frozen. Signed-off-by: Max Reitz <mreitz@redhat.com> --- include/block/block_int.h | 3 +++ block.c | 8 ++++++++ block/commit.c | 4 ++++ block/mirror.c | 4 ++++ 4 files changed, 19 insertions(+)