Re: [PATCH 2/2] block: bdrv_reopen() with backing file in different AioContext

[Kevin Wolf <kwolf@redhat.com>]

Am 05.03.2020 um 16:54 hat Alberto Garcia geschrieben:
> On Thu 27 Feb 2020 07:18:04 PM CET, Kevin Wolf wrote:
> >      /*
> > -     * TODO: before removing the x- prefix from x-blockdev-reopen we
> > -     * should move the new backing file into the right AioContext
> > -     * instead of returning an error.
> > +     * Check AioContext compatibility so that the bdrv_set_backing_hd() call in
> > +     * bdrv_reopen_commit() won't fail.
> >       */
> > -    if (new_backing_bs) {
> > -        if (bdrv_get_aio_context(new_backing_bs) != bdrv_get_aio_context(bs)) {
> > -            error_setg(errp, "Cannot use a new backing file "
> > -                       "with a different AioContext");
> > -            return -EINVAL;
> > -        }
> > +    if (!bdrv_reopen_can_attach(bs->backing, bs, new_backing_bs, errp)) {
> > +        return -EINVAL;
> >      }
> 
> What happens here now if 'new_backing_bs' is NULL ?
> 
> It seems that you would be calling bdrv_can_set_aio_context(NULL, ...),
> and it looks like that would crash.

Not sure why I thought that this check isn't needed any more...

It actually works as long as everything runs in the main loop context
(because bdrv_get_aio_context(NULL) return the main context, so there is
nothing to do), which is why the test cases didn't fail. But as soon as
you move things to a different AioContext, they will fail.

Maybe even worse, the argument order for bdrv_reopen_can_attach() is
wrong.

Thanks for catching this, I'll send a v2.

Kevin