Re: [PATCH for-6.2 v3 01/12] job: Context changes in job_completed_txn_abort()

[Max Reitz]

On 06.08.21 21:16, Eric Blake wrote:
> On Fri, Aug 06, 2021 at 11:38:48AM +0200, Max Reitz wrote:
> > Finalizing the job may cause its AioContext to change.  This is noted by
> > job_exit(), which points at job_txn_apply() to take this fact into
> > account.
> >
> > However, job_completed() does not necessarily invoke job_txn_apply()
> > (through job_completed_txn_success()), but potentially also
> > job_completed_txn_abort().  The latter stores the context in a local
> > variable, and so always acquires the same context at its end that it has
> > released in the beginning -- which may be a different context from the
> > one that job_exit() releases at its end.  If it is different, qemu
> > aborts ("qemu_mutex_unlock_impl: Operation not permitted").
> Is this a bug fix that needs to make it into 6.1?

Well, I only encountered it as part of this series (which I really don’t 
think is 6.2 material at this point), and so I don’t know.

Can’t hurt, I suppose, but if we wanted this to be in 6.1, we’d better 
have a specific test for it, I think.

> > Drop the local @outer_ctx variable from job_completed_txn_abort(), and
> > instead re-acquire the actual job's context at the end of the function,
> > so job_exit() will release the same.
> >
> > Signed-off-by: Max Reitz <mreitz@redhat.com>
> > ---
> >   job.c | 23 ++++++++++++++++++-----
> >   1 file changed, 18 insertions(+), 5 deletions(-)
> The commit message makes sense, and does a good job at explaining the
> change.  I'm still a bit fuzzy on how jobs are supposed to play nice
> with contexts,

I can relate :)

> but since your patch matches the commit message, I'm
> happy to give:
>
> Reviewed-by: Eric Blake <eblake@redhat.com>

Thanks!