[Top][All Lists]

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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

From: Vladimir Sementsov-Ogievskiy
Subject: Re: [PATCH v2 00/11] Refactor bdrv_try_set_aio_context using transactions
Date: Sat, 6 Aug 2022 18:32:48 +0300
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101 Thunderbird/91.8.1

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>

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

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?


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


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


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.


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

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

Best regards,

reply via email to

[Prev in Thread] Current Thread [Next in Thread]