| Message ID | 20170801134907.31253-3-el13635@mail.ntua.gr |
|---|---|
| State | New |
| Headers | show |
On Tue, Aug 01, 2017 at 04:49:06PM +0300, Manos Pitsidianakis wrote: > diff --git a/block.c b/block.c > index 886a457ab0..9ebdba28b0 100644 > --- a/block.c > +++ b/block.c > @@ -4947,3 +4947,20 @@ bool bdrv_can_store_new_dirty_bitmap(BlockDriverState *bs, const char *name, > > return drv->bdrv_can_store_new_dirty_bitmap(bs, name, granularity, errp); > } > + > +/* Get first non-implicit node down a bs chain. */ > +BlockDriverState *bdrv_get_first_non_implicit(BlockDriverState *bs) Linguistically non-implicit == explicit and that would be simpler. Alternatives to implicit/explicit are: transient/permanent internal/external self-created/user-created worker/user Let's agree on one and use it consistently. I like the worker nodes vs user nodes because it reflects their function. implicit/explicit is fine by me too. I just think non-implicit is clunky.
Am 01.08.2017 um 15:49 hat Manos Pitsidianakis geschrieben: > Implicit filter nodes added at the top of nodes can interfere with block > jobs. This is not a problem when they are added by other jobs since > adding another job will issue a QERR_DEVICE_IN_USE, but it can happen in > the next commit which introduces an implicitly created throttle filter > node below BlockBackend, which we want to be skipped during automatic > operations on the graph since the user does not necessarily know about > their existence. > > Signed-off-by: Manos Pitsidianakis <el13635@mail.ntua.gr> > --- > block.c | 17 +++++++++++++++++ > blockdev.c | 12 ++++++++++++ > include/block/block_int.h | 2 ++ > 3 files changed, 31 insertions(+) > > diff --git a/block.c b/block.c > index 886a457ab0..9ebdba28b0 100644 > --- a/block.c > +++ b/block.c > @@ -4947,3 +4947,20 @@ bool bdrv_can_store_new_dirty_bitmap(BlockDriverState *bs, const char *name, > > return drv->bdrv_can_store_new_dirty_bitmap(bs, name, granularity, errp); > } > + > +/* Get first non-implicit node down a bs chain. */ > +BlockDriverState *bdrv_get_first_non_implicit(BlockDriverState *bs) > +{ > + if (!bs) { > + return NULL; > + } > + > + if (!bs->implicit) { > + return bs; > + } > + > + /* at this point bs is implicit and must have a child */ > + assert(bs->file); > + > + return bdrv_get_first_non_implicit(bs->file->bs); > +} mirror_top/commit_top use bs->backing instead of bs->file, so this assertion would fail for them. It's probably better to assert that the implicit node has only one child and then use that child, whatever it may be. I'd also prefer the function to work iteratively rather than recursively because chains can be rather long. (I know that we recurse in many places anyway, so it's not a strong argument, but I think it would also look a bit nicer.) Kevin
diff --git a/block.c b/block.c index 886a457ab0..9ebdba28b0 100644 --- a/block.c +++ b/block.c @@ -4947,3 +4947,20 @@ bool bdrv_can_store_new_dirty_bitmap(BlockDriverState *bs, const char *name, return drv->bdrv_can_store_new_dirty_bitmap(bs, name, granularity, errp); } + +/* Get first non-implicit node down a bs chain. */ +BlockDriverState *bdrv_get_first_non_implicit(BlockDriverState *bs) +{ + if (!bs) { + return NULL; + } + + if (!bs->implicit) { + return bs; + } + + /* at this point bs is implicit and must have a child */ + assert(bs->file); + + return bdrv_get_first_non_implicit(bs->file->bs); +} diff --git a/blockdev.c b/blockdev.c index 23475abb72..d903a23786 100644 --- a/blockdev.c +++ b/blockdev.c @@ -1664,6 +1664,9 @@ static void external_snapshot_prepare(BlkActionState *common, return; } + /* Skip implicit filter nodes */ + state->old_bs = bdrv_get_first_non_implicit(state->old_bs); + /* Acquire AioContext now so any threads operating on old_bs stop */ state->aio_context = bdrv_get_aio_context(state->old_bs); aio_context_acquire(state->aio_context); @@ -3095,6 +3098,9 @@ void qmp_block_commit(bool has_job_id, const char *job_id, const char *device, return; } + /* Skip implicit filter nodes */ + bs = bdrv_get_first_non_implicit(bs); + aio_context = bdrv_get_aio_context(bs); aio_context_acquire(aio_context); @@ -3209,6 +3215,9 @@ static BlockJob *do_drive_backup(DriveBackup *backup, BlockJobTxn *txn, return NULL; } + /* Skip implicit filter nodes */ + bs = bdrv_get_first_non_implicit(bs); + aio_context = bdrv_get_aio_context(bs); aio_context_acquire(aio_context); @@ -3484,6 +3493,9 @@ void qmp_drive_mirror(DriveMirror *arg, Error **errp) return; } + /* Skip implicit filter nodes */ + bs = bdrv_get_first_non_implicit(bs); + aio_context = bdrv_get_aio_context(bs); aio_context_acquire(aio_context); diff --git a/include/block/block_int.h b/include/block/block_int.h index d4f4ea7584..9eeae490f0 100644 --- a/include/block/block_int.h +++ b/include/block/block_int.h @@ -981,4 +981,6 @@ void bdrv_dec_in_flight(BlockDriverState *bs); void blockdev_close_all_bdrv_states(void); +BlockDriverState *bdrv_get_first_non_implicit(BlockDriverState *bs); + #endif /* BLOCK_INT_H */
Implicit filter nodes added at the top of nodes can interfere with block jobs. This is not a problem when they are added by other jobs since adding another job will issue a QERR_DEVICE_IN_USE, but it can happen in the next commit which introduces an implicitly created throttle filter node below BlockBackend, which we want to be skipped during automatic operations on the graph since the user does not necessarily know about their existence. Signed-off-by: Manos Pitsidianakis <el13635@mail.ntua.gr> --- block.c | 17 +++++++++++++++++ blockdev.c | 12 ++++++++++++ include/block/block_int.h | 2 ++ 3 files changed, 31 insertions(+)