[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: Paolo Bonzini
Subject: Re: [PATCH v2 00/11] Refactor bdrv_try_set_aio_context using transactions
Date: Wed, 10 Aug 2022 12:22:40 +0200
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101 Thunderbird/91.12.0

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.


reply via email to

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