Re: [PATCH v5 21/45] block: add bdrv_try_set_aio_context_tran transaction action

[Vladimir Sementsov-Ogievskiy]

On 6/13/22 10:46, Hanna Reitz wrote:
> On 30.03.22 23:28, Vladimir Sementsov-Ogievskiy wrote:
> > To be used in further commit.
> >
> > Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@openvz.org>
> > ---
> >   block.c | 48 ++++++++++++++++++++++++++++++++++++++++++++++++
> >   1 file changed, 48 insertions(+)
> >
> > diff --git a/block.c b/block.c
> > index be19964f89..1900cdf277 100644
> > --- a/block.c
> > +++ b/block.c
> > @@ -2907,6 +2907,54 @@ static void bdrv_child_free(BdrvChild *child)
> >       g_free(child);
> >   }
> > +typedef struct BdrvTrySetAioContextState {
> > +    BlockDriverState *bs;
> > +    AioContext *old_ctx;
> > +} BdrvTrySetAioContextState;
> > +
> > +static void bdrv_try_set_aio_context_abort(void *opaque)
> > +{
> > +    BdrvTrySetAioContextState *s = opaque;
> > +
> > +    if (bdrv_get_aio_context(s->bs) != s->old_ctx) {
> > +        bdrv_try_set_aio_context(s->bs, s->old_ctx, &error_abort);
>
> As far as I understand, users of this transaction will need to do a bit of 
> AioContext lock shuffling: To set the context, they need to hold old_ctx, but 
> not new_ctx; but in case of abort, they need to release old_ctx and acquire 
> new_ctx before the abort handlers are called.  (Due to the constraints on 
> bdrv_set_aio_context_ignore().)
>
> If that’s true, I think that should be documented somewhere.

Hmm.. Actually, I think that bdrv_try_set_aio_context_abort() should do this 
shuffling by it self. The only hope to correctly rollback a transaction, is 
operation in assumption that on .abort() we are exactly on the same state as on 
.prepare(), regardless of other actions. And this means that old_ctx is 
acquired and new_ctx is not.


--
Best regards,
Vladimir