Re: [Qemu-devel] [PATCH 05/16] block: Convert bs->file to BdrvChild

[Alberto Garcia]

On Thu 17 Sep 2015 03:48:09 PM CEST, Kevin Wolf <address@hidden> wrote:

> @@ -1929,6 +1925,11 @@ void bdrv_close(BlockDriverState *bs)
>              bdrv_unref(backing_hd);
>          }
>  
> +        if (bs->file != NULL) {
> +            bdrv_unref(bs->file->bs);
> +            bs->file = NULL;
> +        }
> +
>          QLIST_FOREACH_SAFE(child, &bs->children, next, next) {
>              /* TODO Remove bdrv_unref() from drivers' close function and use
>               * bdrv_unref_child() here */
> @@ -1953,11 +1954,6 @@ void bdrv_close(BlockDriverState *bs)
>          bs->options = NULL;
>          QDECREF(bs->full_open_options);
>          bs->full_open_options = NULL;
> -
> -        if (bs->file != NULL) {
> -            bdrv_unref(bs->file);
> -            bs->file = NULL;
> -        }
>      }

You are moving bdrv_unref(bs->file) up in the function and this seems to
be causing a problem, by turning this:

        bs->drv->bdrv_close(bs);
        bs->drv = NULL;
        bdrv_unref(bs->file);

into this:

        bs->drv->bdrv_close(bs);
        bdrv_unref(bs->file);
        bs->drv = NULL;

In the latter case, closing bs->file calls aio_poll(). This can trigger
new requests on bs, which at that point has a valid pointer to a
BlockDriver that has already been closed. If throttling is enabled on bs
those requests might be queued for later. At the point in which those
requests are scheduled bs->drv is already NULL, crashing QEMU. I can
reproduce this easily with x-data-plane=on.

I sent a separate patch that moves the bdrv_io_limits_disable() call to
the beginning of bdrv_close(). That solves the crash, but I guess that
this patch should also be changed.

Berto