Re: [Qemu-devel] [PATCH v5 03/23] block: Connect BlockBackend to BlockDriverState

[Markus Armbruster]

Kevin Wolf <address@hidden> writes:

> Am 02.10.2014 um 11:04 hat Markus Armbruster geschrieben:
>> Convenience function blk_new_with_bs() creates a BlockBackend with its
>> BlockDriverState.  Callers have to unref both.  The commit after next
>> will relieve them of the need to unref the BlockDriverState.
>> 
>> Complication: due to the silly way drive_del works, we need a way to
>> hide a BlockBackend, just like bdrv_make_anon().  To emphasize its
>> "special" status, give the function a suitably off-putting name:
>> blk_hide_on_behalf_of_do_drive_del().  Unfortunately, hiding turns the
>> BlockBackend's name into the empty string.  Can't avoid that without
>> breaking the blk->bs->device_name equals blk->name invariant.
>> 
>> The patch adds a memory leak: drive_del while a device model is
>> connected leaks the BlockBackend.  Avoiding the leak here is rather
>> hairy, but it'll become straightforward shortly, so I mark it FIXME in
>> the code now, and plug it when it's easy.
>
> Does this leak actually still exist now that you have a blk_unref() in
> drive_del() (which is called during autodel) rather than do_drive_del()?

Yes.  The following hunk adds it:

    @@ -1813,11 +1811,11 @@ int do_drive_del(Monitor *mon, const QDict *qdict, 
QObject **ret_data)
          * then we can just get rid of the block driver state right here.
          */
         if (bdrv_get_attached_dev(bs)) {
    -        bdrv_make_anon(bs);
    -
    +        blk_hide_on_behalf_of_do_drive_del(blk);
             /* Further I/O must not pause the guest */
             bdrv_set_on_error(bs, BLOCKDEV_ON_ERROR_REPORT,
                               BLOCKDEV_ON_ERROR_REPORT);
    +        /* FIXME bs->blk leaked when bs dies */
         } else {
             drive_del(dinfo);
         }

where blk_hide_on_behalf_of_do_drive_del() is from a few hunks further
up:

    +/*
    + * Hide @blk.
    + * @blk must not have been hidden already.
    + * Make attached BlockDriverState, if any, anonymous.
    + * Once hidden, @blk is invisible to all functions that don't receive
    + * it as argument.  For example, blk_by_name() won't return it.
    + * Strictly for use by do_drive_del().
    + * TODO get rid of it!
    + */
    +void blk_hide_on_behalf_of_do_drive_del(BlockBackend *blk)
    +{
    +    QTAILQ_REMOVE(&blk_backends, blk, link);
    +    blk->name[0] = 0;
    +    if (blk->bs) {
    +        bdrv_make_anon(blk->bs);
    +    }
    +}

The net effect is just

    +    QTAILQ_REMOVE(&blk_backends, blk, link);
    +    blk->name[0] = 0;

Admittedly not obvious: this messes with drive_del()!  Let's follow the
call chain.

    void blockdev_auto_del(BlockDriverState *bs)
    {
        DriveInfo *dinfo = drive_get_by_blockdev(bs);

        if (dinfo && dinfo->auto_del) {
            drive_del(dinfo);
        }
    }

drive_get_by_blockdev() still returns the DriveInfo, as we haven't
touched drives (we will in the next patch).  However:

    void drive_del(DriveInfo *dinfo)
    {
        BlockBackend *blk = blk_by_name(dinfo->id);

        bdrv_unref(dinfo->bdrv);
        blk_unref(blk);
    }

blk_by_name() returns null rather than the BB, because
blk_hide_on_behalf_of_do_drive_del() already removed the BB from
blk_backends.

PATCH 06 plugs the leak: blockdev_auto_del() passes the BB straight to
blk_unref().

But there's still a bug!  Your review of v3 convinced me that the leak
was already plugged in PATCH 04.  Maybe it was in v3 (I didn't check
again), but in v5, there's temporary breakage instead.  I'll try to
extend the test suite to cover it in v6.