| Message ID | 20201127144522.29991-7-vsementsov@virtuozzo.com |
|---|---|
| State | New |
| Headers | show |
| Series | block: update graph permissions update | expand |
Am 27.11.2020 um 15:44 hat Vladimir Sementsov-Ogievskiy geschrieben: > Add new handler to get aio context and implement it in all child > classes. Add corresponding public interface to be used soon. > > Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> Hm, are you going to introduce cases where parent and child context don't match, or why is this a useful function? Even if so, I feel it shouldn't be any of the child's business what AioContext the parent uses. Well, maybe the rest of the series will answer this. Kevin
18.01.2021 18:13, Kevin Wolf wrote: > Am 27.11.2020 um 15:44 hat Vladimir Sementsov-Ogievskiy geschrieben: >> Add new handler to get aio context and implement it in all child >> classes. Add corresponding public interface to be used soon. >> >> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> > > Hm, are you going to introduce cases where parent and child context > don't match, or why is this a useful function? > > Even if so, I feel it shouldn't be any of the child's business what > AioContext the parent uses. > > Well, maybe the rest of the series will answer this. > It's for the following patch, to not pass parent (as opaque) with it's class, and with its ctx in separate. Just to simplify the interface of the function, we are going to work with a lot. Hm. I'll take this opportunity to say, that the terminology that calls graph edge "BdrvChild" always leads to the mess between parents and children.. "child_class" is a class of parent.. list of parents is list of children... It all would be a lot simpler to understand if BdrvChild was BdrvEdge or something like this.
Am 18.01.2021 um 18:36 hat Vladimir Sementsov-Ogievskiy geschrieben: > 18.01.2021 18:13, Kevin Wolf wrote: > > Am 27.11.2020 um 15:44 hat Vladimir Sementsov-Ogievskiy geschrieben: > > > Add new handler to get aio context and implement it in all child > > > classes. Add corresponding public interface to be used soon. > > > > > > Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> > > > > Hm, are you going to introduce cases where parent and child context > > don't match, or why is this a useful function? > > > > Even if so, I feel it shouldn't be any of the child's business what > > AioContext the parent uses. > > > > Well, maybe the rest of the series will answer this. > > It's for the following patch, to not pass parent (as opaque) with it's > class, and with its ctx in separate. Just to simplify the interface of > the function, we are going to work with a lot. In a way, the result is nicer because we already assume that ctx is the parent context when we move things to different AioContexts. So it's more consistent to just directly take it from the parent. At the same time, it also complicates things a bit because now we need a defined state in the middle of an operation that adds, removes or replaces a child. I guess I still to make more progress in the review of this series until I see how you're using the interface. > Hm. I'll take this opportunity to say, that the terminology that calls > graph edge "BdrvChild" always leads to the mess between parents and > children.. "child_class" is a class of parent.. list of parents is > list of children... It all would be a lot simpler to understand if > BdrvChild was BdrvEdge or something like this. Yeah, that would probably be clearer now that we actually use it to work with both ends of the edge. And BdrvNode instead of BlockDriverState, I guess. Kevin
19.01.2021 19:38, Kevin Wolf wrote: > Am 18.01.2021 um 18:36 hat Vladimir Sementsov-Ogievskiy geschrieben: >> 18.01.2021 18:13, Kevin Wolf wrote: >>> Am 27.11.2020 um 15:44 hat Vladimir Sementsov-Ogievskiy geschrieben: >>>> Add new handler to get aio context and implement it in all child >>>> classes. Add corresponding public interface to be used soon. >>>> >>>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> >>> >>> Hm, are you going to introduce cases where parent and child context >>> don't match, or why is this a useful function? >>> >>> Even if so, I feel it shouldn't be any of the child's business what >>> AioContext the parent uses. >>> >>> Well, maybe the rest of the series will answer this. >> >> It's for the following patch, to not pass parent (as opaque) with it's >> class, and with its ctx in separate. Just to simplify the interface of >> the function, we are going to work with a lot. > > In a way, the result is nicer because we already assume that ctx is the > parent context when we move things to different AioContexts. So it's > more consistent to just directly take it from the parent. > > At the same time, it also complicates things a bit because now we need a > defined state in the middle of an operation that adds, removes or > replaces a child. > > I guess I still to make more progress in the review of this series until > I see how you're using the interface. > >> Hm. I'll take this opportunity to say, that the terminology that calls >> graph edge "BdrvChild" always leads to the mess between parents and >> children.. "child_class" is a class of parent.. list of parents is >> list of children... It all would be a lot simpler to understand if >> BdrvChild was BdrvEdge or something like this. > > Yeah, that would probably be clearer now that we actually use it to > work with both ends of the edge. And BdrvNode instead of > BlockDriverState, I guess. Do you think, we can just rename them? Or it would be too painful for developers, who get used to current names? I can make patches
Am 22.01.2021 um 12:04 hat Vladimir Sementsov-Ogievskiy geschrieben: > 19.01.2021 19:38, Kevin Wolf wrote: > > Am 18.01.2021 um 18:36 hat Vladimir Sementsov-Ogievskiy geschrieben: > > > 18.01.2021 18:13, Kevin Wolf wrote: > > > > Am 27.11.2020 um 15:44 hat Vladimir Sementsov-Ogievskiy geschrieben: > > > > > Add new handler to get aio context and implement it in all child > > > > > classes. Add corresponding public interface to be used soon. > > > > > > > > > > Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> > > > > > > > > Hm, are you going to introduce cases where parent and child context > > > > don't match, or why is this a useful function? > > > > > > > > Even if so, I feel it shouldn't be any of the child's business what > > > > AioContext the parent uses. > > > > > > > > Well, maybe the rest of the series will answer this. > > > > > > It's for the following patch, to not pass parent (as opaque) with it's > > > class, and with its ctx in separate. Just to simplify the interface of > > > the function, we are going to work with a lot. > > > > In a way, the result is nicer because we already assume that ctx is the > > parent context when we move things to different AioContexts. So it's > > more consistent to just directly take it from the parent. > > > > At the same time, it also complicates things a bit because now we need a > > defined state in the middle of an operation that adds, removes or > > replaces a child. > > > > I guess I still to make more progress in the review of this series until > > I see how you're using the interface. > > > > > Hm. I'll take this opportunity to say, that the terminology that calls > > > graph edge "BdrvChild" always leads to the mess between parents and > > > children.. "child_class" is a class of parent.. list of parents is > > > list of children... It all would be a lot simpler to understand if > > > BdrvChild was BdrvEdge or something like this. > > > > Yeah, that would probably be clearer now that we actually use it to > > work with both ends of the edge. And BdrvNode instead of > > BlockDriverState, I guess. > > Do you think, we can just rename them? Or it would be too painful for developers, > who get used to current names? I can make patches I think getting used to new names wouldn't be too bad. I would be more afraid of the merge conflicts. Not sure, but it might in the category that we would do it differently if we were starting over, but maybe not worth changing now. Kevin
22.01.2021 14:18, Kevin Wolf wrote: > Am 22.01.2021 um 12:04 hat Vladimir Sementsov-Ogievskiy geschrieben: >> 19.01.2021 19:38, Kevin Wolf wrote: >>> Am 18.01.2021 um 18:36 hat Vladimir Sementsov-Ogievskiy geschrieben: >>>> 18.01.2021 18:13, Kevin Wolf wrote: >>>>> Am 27.11.2020 um 15:44 hat Vladimir Sementsov-Ogievskiy geschrieben: >>>>>> Add new handler to get aio context and implement it in all child >>>>>> classes. Add corresponding public interface to be used soon. >>>>>> >>>>>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> >>>>> >>>>> Hm, are you going to introduce cases where parent and child context >>>>> don't match, or why is this a useful function? >>>>> >>>>> Even if so, I feel it shouldn't be any of the child's business what >>>>> AioContext the parent uses. >>>>> >>>>> Well, maybe the rest of the series will answer this. >>>> >>>> It's for the following patch, to not pass parent (as opaque) with it's >>>> class, and with its ctx in separate. Just to simplify the interface of >>>> the function, we are going to work with a lot. >>> >>> In a way, the result is nicer because we already assume that ctx is the >>> parent context when we move things to different AioContexts. So it's >>> more consistent to just directly take it from the parent. >>> >>> At the same time, it also complicates things a bit because now we need a >>> defined state in the middle of an operation that adds, removes or >>> replaces a child. >>> >>> I guess I still to make more progress in the review of this series until >>> I see how you're using the interface. >>> >>>> Hm. I'll take this opportunity to say, that the terminology that calls >>>> graph edge "BdrvChild" always leads to the mess between parents and >>>> children.. "child_class" is a class of parent.. list of parents is >>>> list of children... It all would be a lot simpler to understand if >>>> BdrvChild was BdrvEdge or something like this. >>> >>> Yeah, that would probably be clearer now that we actually use it to >>> work with both ends of the edge. And BdrvNode instead of >>> BlockDriverState, I guess. >> >> Do you think, we can just rename them? Or it would be too painful for developers, >> who get used to current names? I can make patches > > I think getting used to new names wouldn't be too bad. I would be more > afraid of the merge conflicts. > > Not sure, but it might in the category that we would do it differently > if we were starting over, but maybe not worth changing now. > Hmm yes, such rename will add a year of uncomfortable patch backporting..
diff --git a/include/block/block.h b/include/block/block.h index 550c5a7513..6788ccd25b 100644 --- a/include/block/block.h +++ b/include/block/block.h @@ -688,6 +688,9 @@ bool bdrv_can_set_aio_context(BlockDriverState *bs, AioContext *ctx, GSList **ignore, Error **errp); int bdrv_parent_try_set_aio_context(BdrvChild *c, AioContext *ctx, Error **errp); + +AioContext *bdrv_child_get_parent_aio_context(BdrvChild *c); + int bdrv_probe_blocksizes(BlockDriverState *bs, BlockSizes *bsz); int bdrv_probe_geometry(BlockDriverState *bs, HDGeometry *geo); diff --git a/include/block/block_int.h b/include/block/block_int.h index 9138aaf5ec..943fd855fe 100644 --- a/include/block/block_int.h +++ b/include/block/block_int.h @@ -772,6 +772,8 @@ struct BdrvChildClass { bool (*can_set_aio_ctx)(BdrvChild *child, AioContext *ctx, GSList **ignore, Error **errp); void (*set_aio_ctx)(BdrvChild *child, AioContext *ctx, GSList **ignore); + + AioContext *(*get_parent_aio_context)(BdrvChild *child); }; extern const BdrvChildClass child_of_bds; diff --git a/block.c b/block.c index 5d925c208d..95d3684d8d 100644 --- a/block.c +++ b/block.c @@ -1334,6 +1334,13 @@ static int bdrv_child_cb_update_filename(BdrvChild *c, BlockDriverState *base, return 0; } +static AioContext *bdrv_child_cb_get_parent_aio_context(BdrvChild *c) +{ + BlockDriverState *bs = c->opaque; + + return bdrv_get_aio_context(bs); +} + const BdrvChildClass child_of_bds = { .parent_is_bds = true, .get_parent_desc = bdrv_child_get_parent_desc, @@ -1347,8 +1354,14 @@ const BdrvChildClass child_of_bds = { .can_set_aio_ctx = bdrv_child_cb_can_set_aio_ctx, .set_aio_ctx = bdrv_child_cb_set_aio_ctx, .update_filename = bdrv_child_cb_update_filename, + .get_parent_aio_context = bdrv_child_cb_get_parent_aio_context, }; +AioContext *bdrv_child_get_parent_aio_context(BdrvChild *c) +{ + return c->klass->get_parent_aio_context(c); +} + static int bdrv_open_flags(BlockDriverState *bs, int flags) { int open_flags = flags; diff --git a/block/block-backend.c b/block/block-backend.c index ce78d30794..28efa0dff3 100644 --- a/block/block-backend.c +++ b/block/block-backend.c @@ -298,6 +298,13 @@ static void blk_root_detach(BdrvChild *child) } } +static AioContext *blk_root_get_parent_aio_context(BdrvChild *c) +{ + BlockBackend *blk = c->opaque; + + return blk_get_aio_context(blk); +} + static const BdrvChildClass child_root = { .inherit_options = blk_root_inherit_options, @@ -318,6 +325,8 @@ static const BdrvChildClass child_root = { .can_set_aio_ctx = blk_root_can_set_aio_ctx, .set_aio_ctx = blk_root_set_aio_ctx, + + .get_parent_aio_context = blk_root_get_parent_aio_context, }; /* diff --git a/blockjob.c b/blockjob.c index 9d0bed01c2..f671763c2c 100644 --- a/blockjob.c +++ b/blockjob.c @@ -163,6 +163,13 @@ static void child_job_set_aio_ctx(BdrvChild *c, AioContext *ctx, job->job.aio_context = ctx; } +static AioContext *child_job_get_parent_aio_context(BdrvChild *c) +{ + BlockJob *job = c->opaque; + + return job->job.aio_context; +} + static const BdrvChildClass child_job = { .get_parent_desc = child_job_get_parent_desc, .drained_begin = child_job_drained_begin, @@ -171,6 +178,7 @@ static const BdrvChildClass child_job = { .can_set_aio_ctx = child_job_can_set_aio_ctx, .set_aio_ctx = child_job_set_aio_ctx, .stay_at_node = true, + .get_parent_aio_context = child_job_get_parent_aio_context, }; void block_job_remove_all_bdrv(BlockJob *job)
Add new handler to get aio context and implement it in all child classes. Add corresponding public interface to be used soon. Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> --- include/block/block.h | 3 +++ include/block/block_int.h | 2 ++ block.c | 13 +++++++++++++ block/block-backend.c | 9 +++++++++ blockjob.c | 8 ++++++++ 5 files changed, 35 insertions(+)