Re: [PATCH v2 00/11] Refactor bdrv_try_set_aio_context using transactions

[Emanuele Giuseppe Esposito]

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.
>>>
>>
> 
>