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

[Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>]

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