block.c | 360 ++++++++++++++++------------- block/block-backend.c | 74 +++--- block/export/export.c | 2 +- blockdev.c | 22 +- blockjob.c | 50 ++-- docs/devel/multiple-iothreads.txt | 4 +- include/block/block-global-state.h | 18 +- include/block/block_int-common.h | 6 +- job.c | 2 +- tests/unit/test-bdrv-drain.c | 6 +- tests/unit/test-block-iothread.c | 10 +- 11 files changed, 310 insertions(+), 244 deletions(-)
The aim of this series is to reorganize bdrv_try_set_aio_context
and drop BDS ->set_aio_context and ->can_set_aio_ctx callbacks in
favour of a new one, ->change_aio_ctx.
More informations in patch 3 (which is also RFC, due to the doubts
I have with AioContext locks).
Patch 1 just add assertions in the code, 2 extends the transactions API to be able to add also transactions in the tail
of the list.
Patch 3 is the core of this series, and introduces the new callback.
It is marked as RFC and the reason is explained in the commit message.
Patches 4-5-6 implement ->change_aio_ctx in the various block, blockjob
and block-backend BDSes.
Patch 7 substitutes ->change_aio_ctx with the old callbacks, and
patch 8 takes care of deleting the old callbacks and unused code.
This series is based on "job: replace AioContext lock with job_mutex",
but just because it uses job_set_aio_context() introduced there.
Suggested-by: Paolo Bonzini <pbonzini@redhat.com>
Based-on: <20220706201533.289775-1-eesposit@redhat.com>
---
v2:
* remove transaction patch, and drain transactions
* re-add old AioContext lock/unlock, since it makes sense to have it
* typos in commit message
* various cleanups in block-backend callbacks
* use hash map instead of glist when marking visited nodes
* 2 more additional patches, getting rid of
bdrv_try_set_aio_context and bdrv_child_try_change_aio_context
Emanuele Giuseppe Esposito (11):
block.c: assert bs->aio_context is written under BQL and drains
block: use transactions as a replacement of ->{can_}set_aio_context()
bdrv_change_aio_context: use hash table instead of list of visited
nodes
bdrv_child_try_change_aio_context: add transaction parameter
blockjob: implement .change_aio_ctx in child_job
block: implement .change_aio_ctx in child_of_bds
block-backend: implement .change_aio_ctx in child_root
block: use the new _change_ API instead of _can_set_ and _set_
block: remove all unused ->can_set_aio_ctx and ->set_aio_ctx callbacks
block: rename bdrv_child_try_change_aio_context in
bdrv_try_change_aio_context
block: remove bdrv_try_set_aio_context and replace it with
bdrv_try_change_aio_context
block.c | 360 ++++++++++++++++-------------
block/block-backend.c | 74 +++---
block/export/export.c | 2 +-
blockdev.c | 22 +-
blockjob.c | 50 ++--
docs/devel/multiple-iothreads.txt | 4 +-
include/block/block-global-state.h | 18 +-
include/block/block_int-common.h | 6 +-
job.c | 2 +-
tests/unit/test-bdrv-drain.c | 6 +-
tests/unit/test-block-iothread.c | 10 +-
11 files changed, 310 insertions(+), 244 deletions(-)
--
2.31.1
On 7/25/22 15:21, Emanuele Giuseppe Esposito wrote: > The aim of this series is to reorganize bdrv_try_set_aio_context > and drop BDS ->set_aio_context and ->can_set_aio_ctx callbacks in > favour of a new one, ->change_aio_ctx. > > More informations in patch 3 (which is also RFC, due to the doubts > I have with AioContext locks). > > Patch 1 just add assertions in the code, 2 extends the transactions API to be able to add also transactions in the tail > of the list. > Patch 3 is the core of this series, and introduces the new callback. > It is marked as RFC and the reason is explained in the commit message. > Patches 4-5-6 implement ->change_aio_ctx in the various block, blockjob > and block-backend BDSes. > Patch 7 substitutes ->change_aio_ctx with the old callbacks, and > patch 8 takes care of deleting the old callbacks and unused code. > > This series is based on "job: replace AioContext lock with job_mutex", > but just because it uses job_set_aio_context() introduced there. > > Suggested-by: Paolo Bonzini<pbonzini@redhat.com> > Based-on:<20220706201533.289775-1-eesposit@redhat.com> What I dislike here is that you refactor aio-context-change to use transactions, but you use it "internally" for aio-context-change. aio-context-change doesn't become a native part of block-graph modifiction transaction system after the series. For example, bdrv_attach_child_common(..., tran) still calls bdrv_try_change_aio_context() function which doesn't take @tran argument. And we have to call bdrv_try_change_aio_context() again in bdrv_attach_child_common_abort() with opposite arguments by hand. We create in _try_ separate Transaction object which is unrelated to the original block-graph-change transaction. With good refactoring we should get rid of these _try_ functions, and have just bdrv_change_aio_context(..., tran) that can be natively used with external tran object, where other block-graph change actions participate. This way we'll not have to call reverse aio_context_change() in .abort, it will be done automatically. Moreover, your *aio_context_change* functions that do have tran parameter cannot be simply used in the block-graph change transaction, as you don't follow the common paradigm, that in .prepare we do all visible changes. That's why you have to still use _try_*() version that creates seaparate transaction object and completes it: after that the action is actually done and other graph-modifying actions can be done on top. So, IMHO, we shouldn't go this way, as that adds transaction actions that actually can't be used in common block-graph-modifying transaction but only context of bdrv_try_change_aio_context() internal transaction. I agree that aio-context change should finally be rewritten to take a native place in block-graph transactions, but that should be a complete solution, introducing native bdrv_change_aio_context(..., tran) transaction action that is directly used in the block-graph transaction, do visible effect in .prepare and don't create extra Transaction objects. -- Best regards, Vladimir
Am 27/07/2022 um 18:13 schrieb Vladimir Sementsov-Ogievskiy: > On 7/25/22 15:21, Emanuele Giuseppe Esposito wrote: >> The aim of this series is to reorganize bdrv_try_set_aio_context >> and drop BDS ->set_aio_context and ->can_set_aio_ctx callbacks in >> favour of a new one, ->change_aio_ctx. >> >> More informations in patch 3 (which is also RFC, due to the doubts >> I have with AioContext locks). >> >> Patch 1 just add assertions in the code, 2 extends the transactions >> API to be able to add also transactions in the tail >> of the list. >> Patch 3 is the core of this series, and introduces the new callback. >> It is marked as RFC and the reason is explained in the commit message. >> Patches 4-5-6 implement ->change_aio_ctx in the various block, blockjob >> and block-backend BDSes. >> Patch 7 substitutes ->change_aio_ctx with the old callbacks, and >> patch 8 takes care of deleting the old callbacks and unused code. >> >> This series is based on "job: replace AioContext lock with job_mutex", >> but just because it uses job_set_aio_context() introduced there. >> >> Suggested-by: Paolo Bonzini<pbonzini@redhat.com> >> Based-on:<20220706201533.289775-1-eesposit@redhat.com> > > So, I read your email before going on PTO and at that point I got what your concerns were, but now after re-reading it I don't understand anymore what you mean :) > What I dislike here is that you refactor aio-context-change to use > transactions, but you use it "internally" for aio-context-change. > aio-context-change doesn't become a native part of block-graph > modifiction transaction system after the series. > > For example, bdrv_attach_child_common(..., tran) still calls > bdrv_try_change_aio_context() function which doesn't take @tran > argument. And we have to call bdrv_try_change_aio_context() again in > bdrv_attach_child_common_abort() with opposite arguments by hand. We > create in _try_ separate Transaction object which is unrelated to the > original block-graph-change transaction. > This can be fixed: patch 4 "bdrv_child_try_change_aio_context: add transaction parameter" supports taking transaction as a parameter. bdrv_attach_child_common could simply call bdrv_try_change_aio_context_tran (ok we need to deal with locking, but it could work). I think the main concern here is that during the prepare phase this serie doesn't change any aiocontext, so until we don't commit the rest of the code cannot assume that the aiocontext has been changed. But isn't it what happens also for permissions? Permission functions like bdrv_drv_set_perm perform bdrv_check_perm() in .prepare(), and then bdrv_set_perm() in commit. Another important question is that if we actually want to put everything in a single transaction, because .prepare functions like the one proposed here perform drains, so the logic following prepare and preceding commit must take into account that everything is drained. Also prepare itself has to take into account that maybe other .prepare took locks or drained themselves... > With good refactoring we should get rid of these _try_ functions, and > have just bdrv_change_aio_context(..., tran) that can be natively used > with external tran object, where other block-graph change actions > participate. This way we'll not have to call reverse > aio_context_change() in .abort, it will be done automatically. > > Moreover, your *aio_context_change* functions that do have tran > parameter cannot be simply used in the block-graph change transaction, > as you don't follow the common paradigm, that in .prepare we do all > visible changes. That's why you have to still use _try_*() version that > creates seaparate transaction object and completes it: after that the > action is actually done and other graph-modifying actions can be done on > top. > > So, IMHO, we shouldn't go this way, as that adds transaction actions > that actually can't be used in common block-graph-modifying transaction > but only context of bdrv_try_change_aio_context() internal transaction. > > I agree that aio-context change should finally be rewritten to take a > native place in block-graph transactions, but that should be a complete > solution, introducing native bdrv_change_aio_context(..., tran) > transaction action that is directly used in the block-graph transaction, > do visible effect in .prepare and don't create extra Transaction objects. >
On 8/5/22 16:22, Emanuele Giuseppe Esposito wrote: > > > Am 27/07/2022 um 18:13 schrieb Vladimir Sementsov-Ogievskiy: >> On 7/25/22 15:21, Emanuele Giuseppe Esposito wrote: >>> The aim of this series is to reorganize bdrv_try_set_aio_context >>> and drop BDS ->set_aio_context and ->can_set_aio_ctx callbacks in >>> favour of a new one, ->change_aio_ctx. >>> >>> More informations in patch 3 (which is also RFC, due to the doubts >>> I have with AioContext locks). >>> >>> Patch 1 just add assertions in the code, 2 extends the transactions >>> API to be able to add also transactions in the tail >>> of the list. >>> Patch 3 is the core of this series, and introduces the new callback. >>> It is marked as RFC and the reason is explained in the commit message. >>> Patches 4-5-6 implement ->change_aio_ctx in the various block, blockjob >>> and block-backend BDSes. >>> Patch 7 substitutes ->change_aio_ctx with the old callbacks, and >>> patch 8 takes care of deleting the old callbacks and unused code. >>> >>> This series is based on "job: replace AioContext lock with job_mutex", >>> but just because it uses job_set_aio_context() introduced there. >>> >>> Suggested-by: Paolo Bonzini<pbonzini@redhat.com> >>> Based-on:<20220706201533.289775-1-eesposit@redhat.com> >> >> > > So, I read your email before going on PTO and at that point I got what > your concerns were, but now after re-reading it I don't understand > anymore what you mean :) > >> What I dislike here is that you refactor aio-context-change to use >> transactions, but you use it "internally" for aio-context-change. >> aio-context-change doesn't become a native part of block-graph >> modifiction transaction system after the series. >> >> For example, bdrv_attach_child_common(..., tran) still calls >> bdrv_try_change_aio_context() function which doesn't take @tran >> argument. And we have to call bdrv_try_change_aio_context() again in >> bdrv_attach_child_common_abort() with opposite arguments by hand. We >> create in _try_ separate Transaction object which is unrelated to the >> original block-graph-change transaction. >> > > This can be fixed: patch 4 "bdrv_child_try_change_aio_context: add > transaction parameter" supports taking transaction as a parameter. > bdrv_attach_child_common could simply call > bdrv_try_change_aio_context_tran (ok we need to deal with locking, but > it could work). > > I think the main concern here is that during the prepare phase this > serie doesn't change any aiocontext, so until we don't commit the rest > of the code cannot assume that the aiocontext has been changed. > > But isn't it what happens also for permissions? Permission functions > like bdrv_drv_set_perm perform bdrv_check_perm() in .prepare(), and then > bdrv_set_perm() in commit. Not exactly. Partly that's just old bad naming. Ideally, driver handlers should be refactored to have one .bdrv_set_perm(, ... tran, errp) handler. Or at least renamed to .prepare and .commit instead of misleading .check and .set. Secondly what is important, is that corresponding .set actions are not visible to other block-graph modifying actions (like taking locks on fd. other actions, like attach/detach children don't care of it)/ (Or, at least, they _shoud not be_ visible :) To be honest, I don't have real expertise, of how much these .set actions are visible or not by other block-graph modifying actions, but I believe that we are OK in common scenarios). What is really visible at generic block layer? Visible is change of .perm / .shared_perm fields of BdrvChild. And they are set in .prepare, look at bdrv_child_set_perm(). So, after calling bdrv_child_set_perm() other actions of .prepare stage will see _updated_ permissions. And if transaction fails, we rollback .perm and .shared_perm fields in bdrv_child_set_perm_abort() > > Another important question is that if we actually want to put everything > in a single transaction, because .prepare functions like the one > proposed here perform drains, so the logic following prepare and > preceding commit must take into account that everything is drained. Also > prepare itself has to take into account that maybe other .prepare took > locks or drained themselves... Yes, untie the knot of drained sections during aio context change is a challenge.. And that's (at least on of) the reason why aio-context change is still not a native part of block graph modifying transactions. Could there be some simple solution? Like 1. drain the whole subgraph of all nodes connected with nodes that we are going to touch 2. do block graph modifying transaction (including aio context change) knowing that everything we need is drained. (so we don't have to care about drain during aio context change) 3. undrain the subgraph In other words, is that really necessary to lock/unlock different contexts during the aio-context-change procedure? Or we can move to a lot larger and simpler drained section? > >> With good refactoring we should get rid of these _try_ functions, and >> have just bdrv_change_aio_context(..., tran) that can be natively used >> with external tran object, where other block-graph change actions >> participate. This way we'll not have to call reverse >> aio_context_change() in .abort, it will be done automatically. >> >> Moreover, your *aio_context_change* functions that do have tran >> parameter cannot be simply used in the block-graph change transaction, >> as you don't follow the common paradigm, that in .prepare we do all >> visible changes. That's why you have to still use _try_*() version that >> creates seaparate transaction object and completes it: after that the >> action is actually done and other graph-modifying actions can be done on >> top. >> >> So, IMHO, we shouldn't go this way, as that adds transaction actions >> that actually can't be used in common block-graph-modifying transaction >> but only context of bdrv_try_change_aio_context() internal transaction. >> >> I agree that aio-context change should finally be rewritten to take a >> native place in block-graph transactions, but that should be a complete >> solution, introducing native bdrv_change_aio_context(..., tran) >> transaction action that is directly used in the block-graph transaction, >> do visible effect in .prepare and don't create extra Transaction objects. >> > -- Best regards, Vladimir
Am 05/08/2022 um 16:35 schrieb Vladimir Sementsov-Ogievskiy: > On 8/5/22 16:22, Emanuele Giuseppe Esposito wrote: >> >> >> Am 27/07/2022 um 18:13 schrieb Vladimir Sementsov-Ogievskiy: >>> On 7/25/22 15:21, Emanuele Giuseppe Esposito wrote: >>>> The aim of this series is to reorganize bdrv_try_set_aio_context >>>> and drop BDS ->set_aio_context and ->can_set_aio_ctx callbacks in >>>> favour of a new one, ->change_aio_ctx. >>>> >>>> More informations in patch 3 (which is also RFC, due to the doubts >>>> I have with AioContext locks). >>>> >>>> Patch 1 just add assertions in the code, 2 extends the transactions >>>> API to be able to add also transactions in the tail >>>> of the list. >>>> Patch 3 is the core of this series, and introduces the new callback. >>>> It is marked as RFC and the reason is explained in the commit message. >>>> Patches 4-5-6 implement ->change_aio_ctx in the various block, blockjob >>>> and block-backend BDSes. >>>> Patch 7 substitutes ->change_aio_ctx with the old callbacks, and >>>> patch 8 takes care of deleting the old callbacks and unused code. >>>> >>>> This series is based on "job: replace AioContext lock with job_mutex", >>>> but just because it uses job_set_aio_context() introduced there. >>>> >>>> Suggested-by: Paolo Bonzini<pbonzini@redhat.com> >>>> Based-on:<20220706201533.289775-1-eesposit@redhat.com> >>> >>> >> >> So, I read your email before going on PTO and at that point I got what >> your concerns were, but now after re-reading it I don't understand >> anymore what you mean :) >> >>> What I dislike here is that you refactor aio-context-change to use >>> transactions, but you use it "internally" for aio-context-change. >>> aio-context-change doesn't become a native part of block-graph >>> modifiction transaction system after the series. >>> >>> For example, bdrv_attach_child_common(..., tran) still calls >>> bdrv_try_change_aio_context() function which doesn't take @tran >>> argument. And we have to call bdrv_try_change_aio_context() again in >>> bdrv_attach_child_common_abort() with opposite arguments by hand. We >>> create in _try_ separate Transaction object which is unrelated to the >>> original block-graph-change transaction. >>> >> >> This can be fixed: patch 4 "bdrv_child_try_change_aio_context: add >> transaction parameter" supports taking transaction as a parameter. >> bdrv_attach_child_common could simply call >> bdrv_try_change_aio_context_tran (ok we need to deal with locking, but >> it could work). >> >> I think the main concern here is that during the prepare phase this >> serie doesn't change any aiocontext, so until we don't commit the rest >> of the code cannot assume that the aiocontext has been changed. >> >> But isn't it what happens also for permissions? Permission functions >> like bdrv_drv_set_perm perform bdrv_check_perm() in .prepare(), and then >> bdrv_set_perm() in commit. > > Not exactly. > > Partly that's just old bad naming. Ideally, driver handlers should be > refactored to have one > .bdrv_set_perm(, ... tran, errp) handler. Or at least renamed to > .prepare and .commit instead of misleading .check and .set. > > Secondly what is important, is that corresponding .set actions are not > visible to other block-graph modifying actions (like taking locks on fd. > other actions, like attach/detach children don't care of it)/ (Or, at > least, they _shoud not be_ visible :) To be honest, I don't have real > expertise, of how much these .set actions are visible or not by other > block-graph modifying actions, but I believe that we are OK in common > scenarios). > > What is really visible at generic block layer? Visible is change of > .perm / .shared_perm fields of BdrvChild. And they are set in .prepare, > look at bdrv_child_set_perm(). > > So, after calling bdrv_child_set_perm() other actions of .prepare stage > will see _updated_ permissions. And if transaction fails, we rollback > .perm and .shared_perm fields in bdrv_child_set_perm_abort() > > >> >> Another important question is that if we actually want to put everything >> in a single transaction, because .prepare functions like the one >> proposed here perform drains, so the logic following prepare and >> preceding commit must take into account that everything is drained. Also >> prepare itself has to take into account that maybe other .prepare took >> locks or drained themselves... > > Yes, untie the knot of drained sections during aio context change is a > challenge.. And that's (at least on of) the reason why aio-context > change is still not a native part of block graph modifying transactions. > > Could there be some simple solution? > > Like > > 1. drain the whole subgraph of all nodes connected with nodes that we > are going to touch > > 2. do block graph modifying transaction (including aio context change) > knowing that everything we need is drained. (so we don't have to care > about drain during aio context change) > > 3. undrain the subgraph > > In other words, is that really necessary to lock/unlock different > contexts during the aio-context-change procedure? Or we can move to a > lot larger and simpler drained section? Unfortunately I think the aiocontext lock have to stay where they currently are. I documented it in this serie. drained begin needs the old aiocontext, and drained end the new one, since we moved the graph to a different aiocontext. Also, if I understand correctly you suggest: .prepare = check and then change aiocontext .abort = revert aiocontext change .commit = nothing drain_begin_all(); prepare(); drain_end_all(); if prepare is not OK: tran_abort(); // or simply return error so the caller calls abort But then: 1) .abort needs draining too 2) it is not so different from what it was before, isn't it? Emanuele > >> >>> With good refactoring we should get rid of these _try_ functions, and >>> have just bdrv_change_aio_context(..., tran) that can be natively used >>> with external tran object, where other block-graph change actions >>> participate. This way we'll not have to call reverse >>> aio_context_change() in .abort, it will be done automatically. >>> >>> Moreover, your *aio_context_change* functions that do have tran >>> parameter cannot be simply used in the block-graph change transaction, >>> as you don't follow the common paradigm, that in .prepare we do all >>> visible changes. That's why you have to still use _try_*() version that >>> creates seaparate transaction object and completes it: after that the >>> action is actually done and other graph-modifying actions can be done on >>> top. >>> >>> So, IMHO, we shouldn't go this way, as that adds transaction actions >>> that actually can't be used in common block-graph-modifying transaction >>> but only context of bdrv_try_change_aio_context() internal transaction. >>> >>> I agree that aio-context change should finally be rewritten to take a >>> native place in block-graph transactions, but that should be a complete >>> solution, introducing native bdrv_change_aio_context(..., tran) >>> transaction action that is directly used in the block-graph transaction, >>> do visible effect in .prepare and don't create extra Transaction >>> objects. >>> >> > >
On 8/5/22 17:57, Emanuele Giuseppe Esposito wrote: > > > Am 05/08/2022 um 16:35 schrieb Vladimir Sementsov-Ogievskiy: >> On 8/5/22 16:22, Emanuele Giuseppe Esposito wrote: >>> >>> >>> Am 27/07/2022 um 18:13 schrieb Vladimir Sementsov-Ogievskiy: >>>> On 7/25/22 15:21, Emanuele Giuseppe Esposito wrote: >>>>> The aim of this series is to reorganize bdrv_try_set_aio_context >>>>> and drop BDS ->set_aio_context and ->can_set_aio_ctx callbacks in >>>>> favour of a new one, ->change_aio_ctx. >>>>> >>>>> More informations in patch 3 (which is also RFC, due to the doubts >>>>> I have with AioContext locks). >>>>> >>>>> Patch 1 just add assertions in the code, 2 extends the transactions >>>>> API to be able to add also transactions in the tail >>>>> of the list. >>>>> Patch 3 is the core of this series, and introduces the new callback. >>>>> It is marked as RFC and the reason is explained in the commit message. >>>>> Patches 4-5-6 implement ->change_aio_ctx in the various block, blockjob >>>>> and block-backend BDSes. >>>>> Patch 7 substitutes ->change_aio_ctx with the old callbacks, and >>>>> patch 8 takes care of deleting the old callbacks and unused code. >>>>> >>>>> This series is based on "job: replace AioContext lock with job_mutex", >>>>> but just because it uses job_set_aio_context() introduced there. >>>>> >>>>> Suggested-by: Paolo Bonzini<pbonzini@redhat.com> >>>>> Based-on:<20220706201533.289775-1-eesposit@redhat.com> >>>> >>>> >>> >>> So, I read your email before going on PTO and at that point I got what >>> your concerns were, but now after re-reading it I don't understand >>> anymore what you mean :) >>> >>>> What I dislike here is that you refactor aio-context-change to use >>>> transactions, but you use it "internally" for aio-context-change. >>>> aio-context-change doesn't become a native part of block-graph >>>> modifiction transaction system after the series. >>>> >>>> For example, bdrv_attach_child_common(..., tran) still calls >>>> bdrv_try_change_aio_context() function which doesn't take @tran >>>> argument. And we have to call bdrv_try_change_aio_context() again in >>>> bdrv_attach_child_common_abort() with opposite arguments by hand. We >>>> create in _try_ separate Transaction object which is unrelated to the >>>> original block-graph-change transaction. >>>> >>> >>> This can be fixed: patch 4 "bdrv_child_try_change_aio_context: add >>> transaction parameter" supports taking transaction as a parameter. >>> bdrv_attach_child_common could simply call >>> bdrv_try_change_aio_context_tran (ok we need to deal with locking, but >>> it could work). >>> >>> I think the main concern here is that during the prepare phase this >>> serie doesn't change any aiocontext, so until we don't commit the rest >>> of the code cannot assume that the aiocontext has been changed. >>> >>> But isn't it what happens also for permissions? Permission functions >>> like bdrv_drv_set_perm perform bdrv_check_perm() in .prepare(), and then >>> bdrv_set_perm() in commit. >> >> Not exactly. >> >> Partly that's just old bad naming. Ideally, driver handlers should be >> refactored to have one >> .bdrv_set_perm(, ... tran, errp) handler. Or at least renamed to >> .prepare and .commit instead of misleading .check and .set. >> >> Secondly what is important, is that corresponding .set actions are not >> visible to other block-graph modifying actions (like taking locks on fd. >> other actions, like attach/detach children don't care of it)/ (Or, at >> least, they _shoud not be_ visible :) To be honest, I don't have real >> expertise, of how much these .set actions are visible or not by other >> block-graph modifying actions, but I believe that we are OK in common >> scenarios). >> >> What is really visible at generic block layer? Visible is change of >> .perm / .shared_perm fields of BdrvChild. And they are set in .prepare, >> look at bdrv_child_set_perm(). >> >> So, after calling bdrv_child_set_perm() other actions of .prepare stage >> will see _updated_ permissions. And if transaction fails, we rollback >> .perm and .shared_perm fields in bdrv_child_set_perm_abort() >> >> >>> >>> Another important question is that if we actually want to put everything >>> in a single transaction, because .prepare functions like the one >>> proposed here perform drains, so the logic following prepare and >>> preceding commit must take into account that everything is drained. Also >>> prepare itself has to take into account that maybe other .prepare took >>> locks or drained themselves... >> >> Yes, untie the knot of drained sections during aio context change is a >> challenge.. And that's (at least on of) the reason why aio-context >> change is still not a native part of block graph modifying transactions. >> >> Could there be some simple solution? >> >> Like >> >> 1. drain the whole subgraph of all nodes connected with nodes that we >> are going to touch >> >> 2. do block graph modifying transaction (including aio context change) >> knowing that everything we need is drained. (so we don't have to care >> about drain during aio context change) >> >> 3. undrain the subgraph >> >> In other words, is that really necessary to lock/unlock different >> contexts during the aio-context-change procedure? Or we can move to a >> lot larger and simpler drained section? > > Unfortunately I think the aiocontext lock have to stay where they > currently are. I documented it in this serie. > > drained begin needs the old aiocontext, and drained end the new one, > since we moved the graph to a different aiocontext. > > Also, if I understand correctly you suggest: > > .prepare = check and then change aiocontext > .abort = revert aiocontext change > .commit = nothing Yes. And that's is how it used actually now in transactions, for example: bdrv_attach_child_common (which is .prepare) calls bdrv_try_set_aio_context() (which is check and then change) bdrv_attach_child_common_abort (which is .abort) calls bdrv_try_set_aio_context() to revert .prepare > > drain_begin_all(); > prepare(); > drain_end_all(); > > if prepare is not OK: > tran_abort(); // or simply return error so the caller calls abort > > But then: > 1) .abort needs draining too > 2) it is not so different from what it was before, isn't it? > No, I mean drain_begin_all(); Do the whole transaction, i.e. prepare + commit or prepare + abort. All the actions, connected into transaction: changing aio context, permissions and block graph relations. drain_end_all(); > > >> >>> >>>> With good refactoring we should get rid of these _try_ functions, and >>>> have just bdrv_change_aio_context(..., tran) that can be natively used >>>> with external tran object, where other block-graph change actions >>>> participate. This way we'll not have to call reverse >>>> aio_context_change() in .abort, it will be done automatically. >>>> >>>> Moreover, your *aio_context_change* functions that do have tran >>>> parameter cannot be simply used in the block-graph change transaction, >>>> as you don't follow the common paradigm, that in .prepare we do all >>>> visible changes. That's why you have to still use _try_*() version that >>>> creates seaparate transaction object and completes it: after that the >>>> action is actually done and other graph-modifying actions can be done on >>>> top. >>>> >>>> So, IMHO, we shouldn't go this way, as that adds transaction actions >>>> that actually can't be used in common block-graph-modifying transaction >>>> but only context of bdrv_try_change_aio_context() internal transaction. >>>> >>>> I agree that aio-context change should finally be rewritten to take a >>>> native place in block-graph transactions, but that should be a complete >>>> solution, introducing native bdrv_change_aio_context(..., tran) >>>> transaction action that is directly used in the block-graph transaction, >>>> do visible effect in .prepare and don't create extra Transaction >>>> objects. >>>> >>> >> >> > -- Best regards, Vladimir
On 8/6/22 17:32, Vladimir Sementsov-Ogievskiy wrote: >> if I understand correctly you suggest: >> >> .prepare = check and then change aiocontext >> .abort = revert aiocontext change >> .commit = nothing > > Yes. And that's is how it used actually now in transactions, for example: > bdrv_attach_child_common (which is .prepare) calls > bdrv_try_set_aio_context() (which is check and then change) > bdrv_attach_child_common_abort (which is .abort) calls > bdrv_try_set_aio_context() to revert .prepare I see your point, but I think this can be solved just with a doc comment explaining why the transaction is "local". This is already clear in bdrv_child_try_change_aio_context (which doesn't take a Transaction*), it can be documented in bdrv_child_change_aio_context as well. After all, the point of this series is just to avoid code duplication in the two visits of the graph, and secondarily to unify the .can_set and .set callbacks into one. We could do it just as well without transaction with just a GList of BdrvChild* (keeping the callbcks separate); what the Transaction API gives is a little more abstraction, by not having to do linked list manipulation by hand. To be honest I also don't like very much the placement of the drained_begin/drained_end in bdrv_change_aio_context. But if the needs arises to call bdrv_change_aio_context within another transaction, I think they can be pulled relatively easily (as a subtree drain) in bdrv_child_try_change_aio_context or in its callers. For now, this series is already an improvement on several counts. Paolo
Am 05/08/2022 um 15:22 schrieb Emanuele Giuseppe Esposito:
>
>
> Am 27/07/2022 um 18:13 schrieb Vladimir Sementsov-Ogievskiy:
>> On 7/25/22 15:21, Emanuele Giuseppe Esposito wrote:
>>> The aim of this series is to reorganize bdrv_try_set_aio_context
>>> and drop BDS ->set_aio_context and ->can_set_aio_ctx callbacks in
>>> favour of a new one, ->change_aio_ctx.
>>>
>>> More informations in patch 3 (which is also RFC, due to the doubts
>>> I have with AioContext locks).
>>>
>>> Patch 1 just add assertions in the code, 2 extends the transactions
>>> API to be able to add also transactions in the tail
>>> of the list.
>>> Patch 3 is the core of this series, and introduces the new callback.
>>> It is marked as RFC and the reason is explained in the commit message.
>>> Patches 4-5-6 implement ->change_aio_ctx in the various block, blockjob
>>> and block-backend BDSes.
>>> Patch 7 substitutes ->change_aio_ctx with the old callbacks, and
>>> patch 8 takes care of deleting the old callbacks and unused code.
>>>
>>> This series is based on "job: replace AioContext lock with job_mutex",
>>> but just because it uses job_set_aio_context() introduced there.
>>>
>>> Suggested-by: Paolo Bonzini<pbonzini@redhat.com>
>>> Based-on:<20220706201533.289775-1-eesposit@redhat.com>
>>
>>
>
> So, I read your email before going on PTO and at that point I got what
> your concerns were, but now after re-reading it I don't understand
> anymore what you mean :)
>
>> What I dislike here is that you refactor aio-context-change to use
>> transactions, but you use it "internally" for aio-context-change.
>> aio-context-change doesn't become a native part of block-graph
>> modifiction transaction system after the series.
>>
>> For example, bdrv_attach_child_common(..., tran) still calls
>> bdrv_try_change_aio_context() function which doesn't take @tran
>> argument. And we have to call bdrv_try_change_aio_context() again in
>> bdrv_attach_child_common_abort() with opposite arguments by hand. We
>> create in _try_ separate Transaction object which is unrelated to the
>> original block-graph-change transaction.
>>
>
> This can be fixed: patch 4 "bdrv_child_try_change_aio_context: add
> transaction parameter" supports taking transaction as a parameter.
> bdrv_attach_child_common could simply call
> bdrv_try_change_aio_context_tran (ok we need to deal with locking, but
> it could work).
No actually I don't get how it can work in bdrv_attach_child_common.
We have the following:
parent_ctx = bdrv_child_get_parent_aio_context(new_child);
if (child_ctx != parent_ctx) {
int ret = bdrv_try_change_aio_context(child_bs, parent_ctx, NULL,
&local_err);
if (ret < 0 && child_class->change_aio_ctx) {
ret_child = child_class->change_aio_ctx(new_child, child_ctx,
visited, tran, NULL);
tran_finalize(tran, ret_child == true ? 0 : -1);
}
if (ret < 0) {
return ret;
}
}
bdrv_replace_child_noperm(&new_child, child_bs, true);
So bdrv_try_change_aio_context would be changed in
bdrv_try_change_aio_context_tran, but then how can we call
bdrv_replace_child_noperm if no aiocontext has been changed at all?
I don't think we can mix transactional operations with non-transactional.
Emanuele
>
> I think the main concern here is that during the prepare phase this
> serie doesn't change any aiocontext, so until we don't commit the rest
> of the code cannot assume that the aiocontext has been changed.
>
> But isn't it what happens also for permissions? Permission functions
> like bdrv_drv_set_perm perform bdrv_check_perm() in .prepare(), and then
> bdrv_set_perm() in commit.
>
> Another important question is that if we actually want to put everything
> in a single transaction, because .prepare functions like the one
> proposed here perform drains, so the logic following prepare and
> preceding commit must take into account that everything is drained. Also
> prepare itself has to take into account that maybe other .prepare took
> locks or drained themselves...
>
>> With good refactoring we should get rid of these _try_ functions, and
>> have just bdrv_change_aio_context(..., tran) that can be natively used
>> with external tran object, where other block-graph change actions
>> participate. This way we'll not have to call reverse
>> aio_context_change() in .abort, it will be done automatically.
>>
>> Moreover, your *aio_context_change* functions that do have tran
>> parameter cannot be simply used in the block-graph change transaction,
>> as you don't follow the common paradigm, that in .prepare we do all
>> visible changes. That's why you have to still use _try_*() version that
>> creates seaparate transaction object and completes it: after that the
>> action is actually done and other graph-modifying actions can be done on
>> top.
>>
>> So, IMHO, we shouldn't go this way, as that adds transaction actions
>> that actually can't be used in common block-graph-modifying transaction
>> but only context of bdrv_try_change_aio_context() internal transaction.
>>
>> I agree that aio-context change should finally be rewritten to take a
>> native place in block-graph transactions, but that should be a complete
>> solution, introducing native bdrv_change_aio_context(..., tran)
>> transaction action that is directly used in the block-graph transaction,
>> do visible effect in .prepare and don't create extra Transaction objects.
>>
On 8/5/22 16:36, Emanuele Giuseppe Esposito wrote:
>
>
> Am 05/08/2022 um 15:22 schrieb Emanuele Giuseppe Esposito:
>>
>>
>> Am 27/07/2022 um 18:13 schrieb Vladimir Sementsov-Ogievskiy:
>>> On 7/25/22 15:21, Emanuele Giuseppe Esposito wrote:
>>>> The aim of this series is to reorganize bdrv_try_set_aio_context
>>>> and drop BDS ->set_aio_context and ->can_set_aio_ctx callbacks in
>>>> favour of a new one, ->change_aio_ctx.
>>>>
>>>> More informations in patch 3 (which is also RFC, due to the doubts
>>>> I have with AioContext locks).
>>>>
>>>> Patch 1 just add assertions in the code, 2 extends the transactions
>>>> API to be able to add also transactions in the tail
>>>> of the list.
>>>> Patch 3 is the core of this series, and introduces the new callback.
>>>> It is marked as RFC and the reason is explained in the commit message.
>>>> Patches 4-5-6 implement ->change_aio_ctx in the various block, blockjob
>>>> and block-backend BDSes.
>>>> Patch 7 substitutes ->change_aio_ctx with the old callbacks, and
>>>> patch 8 takes care of deleting the old callbacks and unused code.
>>>>
>>>> This series is based on "job: replace AioContext lock with job_mutex",
>>>> but just because it uses job_set_aio_context() introduced there.
>>>>
>>>> Suggested-by: Paolo Bonzini<pbonzini@redhat.com>
>>>> Based-on:<20220706201533.289775-1-eesposit@redhat.com>
>>>
>>>
>>
>> So, I read your email before going on PTO and at that point I got what
>> your concerns were, but now after re-reading it I don't understand
>> anymore what you mean :)
>>
>>> What I dislike here is that you refactor aio-context-change to use
>>> transactions, but you use it "internally" for aio-context-change.
>>> aio-context-change doesn't become a native part of block-graph
>>> modifiction transaction system after the series.
>>>
>>> For example, bdrv_attach_child_common(..., tran) still calls
>>> bdrv_try_change_aio_context() function which doesn't take @tran
>>> argument. And we have to call bdrv_try_change_aio_context() again in
>>> bdrv_attach_child_common_abort() with opposite arguments by hand. We
>>> create in _try_ separate Transaction object which is unrelated to the
>>> original block-graph-change transaction.
>>>
>>
>> This can be fixed: patch 4 "bdrv_child_try_change_aio_context: add
>> transaction parameter" supports taking transaction as a parameter.
>> bdrv_attach_child_common could simply call
>> bdrv_try_change_aio_context_tran (ok we need to deal with locking, but
>> it could work).
>
> No actually I don't get how it can work in bdrv_attach_child_common.
> We have the following:
>
> parent_ctx = bdrv_child_get_parent_aio_context(new_child);
> if (child_ctx != parent_ctx) {
> int ret = bdrv_try_change_aio_context(child_bs, parent_ctx, NULL,
> &local_err);
>
> if (ret < 0 && child_class->change_aio_ctx) {
> ret_child = child_class->change_aio_ctx(new_child, child_ctx,
> visited, tran, NULL);
>
> tran_finalize(tran, ret_child == true ? 0 : -1);
> }
>
> if (ret < 0) {
> return ret;
> }
> }
>
> bdrv_replace_child_noperm(&new_child, child_bs, true);
>
> So bdrv_try_change_aio_context would be changed in
> bdrv_try_change_aio_context_tran, but then how can we call
> bdrv_replace_child_noperm if no aiocontext has been changed at all?
> I don't think we can mix transactional operations with non-transactional.
>
So here, bdrv_try_change_aio_context() is .prepare in the way I mean.
And than in .abort we call bdrv_try_change_aio_context() again but with reverse argument, and it's a kind of ".abort".
Probably, we can refactor that, making a function bdrv_change_aio_context(, .., tran), which does what bdrv_try_change_aio_context does, and registers .abort callback, that will simulate calling bdrv_try_change_aio_context() with opposite arguement. But we should carefully refactor all the function names and avoid having nested transaction.
>
>>
>> I think the main concern here is that during the prepare phase this
>> serie doesn't change any aiocontext, so until we don't commit the rest
>> of the code cannot assume that the aiocontext has been changed.
>>
>> But isn't it what happens also for permissions? Permission functions
>> like bdrv_drv_set_perm perform bdrv_check_perm() in .prepare(), and then
>> bdrv_set_perm() in commit.
>>
>> Another important question is that if we actually want to put everything
>> in a single transaction, because .prepare functions like the one
>> proposed here perform drains, so the logic following prepare and
>> preceding commit must take into account that everything is drained. Also
>> prepare itself has to take into account that maybe other .prepare took
>> locks or drained themselves...
>>
>>> With good refactoring we should get rid of these _try_ functions, and
>>> have just bdrv_change_aio_context(..., tran) that can be natively used
>>> with external tran object, where other block-graph change actions
>>> participate. This way we'll not have to call reverse
>>> aio_context_change() in .abort, it will be done automatically.
>>>
>>> Moreover, your *aio_context_change* functions that do have tran
>>> parameter cannot be simply used in the block-graph change transaction,
>>> as you don't follow the common paradigm, that in .prepare we do all
>>> visible changes. That's why you have to still use _try_*() version that
>>> creates seaparate transaction object and completes it: after that the
>>> action is actually done and other graph-modifying actions can be done on
>>> top.
>>>
>>> So, IMHO, we shouldn't go this way, as that adds transaction actions
>>> that actually can't be used in common block-graph-modifying transaction
>>> but only context of bdrv_try_change_aio_context() internal transaction.
>>>
>>> I agree that aio-context change should finally be rewritten to take a
>>> native place in block-graph transactions, but that should be a complete
>>> solution, introducing native bdrv_change_aio_context(..., tran)
>>> transaction action that is directly used in the block-graph transaction,
>>> do visible effect in .prepare and don't create extra Transaction objects.
>>>
>
--
Best regards,
Vladimir
On 7/25/22 15:21, Emanuele Giuseppe Esposito wrote: > The aim of this series is to reorganize bdrv_try_set_aio_context > and drop BDS ->set_aio_context and ->can_set_aio_ctx callbacks in > favour of a new one, ->change_aio_ctx. > > More informations in patch 3 (which is also RFC, due to the doubts patch 2 now. > I have with AioContext locks). > Can't apply to master (neither patchew can: https://patchew.org/QEMU/20220725122120.309236-1-eesposit@redhat.com/ ) Do you have a published branch somewhere to look at? -- Best regards, Vladimir
Am 26/07/2022 um 16:24 schrieb Vladimir Sementsov-Ogievskiy: > On 7/25/22 15:21, Emanuele Giuseppe Esposito wrote: >> The aim of this series is to reorganize bdrv_try_set_aio_context >> and drop BDS ->set_aio_context and ->can_set_aio_ctx callbacks in >> favour of a new one, ->change_aio_ctx. >> >> More informations in patch 3 (which is also RFC, due to the doubts > > patch 2 now. > >> I have with AioContext locks). >> > > Can't apply to master (neither patchew can: > https://patchew.org/QEMU/20220725122120.309236-1-eesposit@redhat.com/ ) > Do you have a published branch somewhere to look at? > You need to apply the latest job series first. Anyways, the branch is here: https://gitlab.com/eesposit/qemu/-/tree/dp_tryset_final Emanuele
© 2016 - 2024 Red Hat, Inc.