[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH 2/2] block: bdrv_reopen() with backing file in different AioC
From: |
Kevin Wolf |
Subject: |
Re: [PATCH 2/2] block: bdrv_reopen() with backing file in different AioContext |
Date: |
Fri, 6 Mar 2020 15:10:03 +0100 |
User-agent: |
Mutt/1.12.1 (2019-06-15) |
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