Re: [PATCH v2 02/11] block: use transactions as a replacement of ->{can_}set_aio_context()

[Emanuele Giuseppe Esposito]

> So this looks good under two conditions that I haven't checked yet: That
> bdrv_detach_aio_context() is actually safe when called with the "wrong"
> AioContext lock held, and that the .change_aio_context() callbacks are
> implemented correctly in a later patch.

So regarding bdrv_detach_aio_context(), I added a
aio_context_lock_acquire/release pair around that function in commit().
The callbacks should be implemented correctly, as you didn't point out
anything later on :)

> 
> To reiterate my initial point, reviewing this took me some effort to
> match the new functions with the existing ones they duplicate and then
> manual diffing of each, which is kind of error prone. I feel the better
> approach would have been adding a tran parameter (with empty commit and
> abort) to the existing functions in a first patch and then change stuff
> in a second patch (in the real code, not dead code to be enabled later),
> so that you would actually see the real changes instead of having to
> find them between lots of unchanged copied code.
> 

Yes, I see what you mean. I'll change my approach on the next series,
thanks for the suggestion!

Thank you,
Emanuele