Re: [Qemu-devel] [PATCH v3 04/23] block: Connect BlockBackend and DriveInfo

[Markus Armbruster]

Kevin Wolf <address@hidden> writes:

> Am 16.09.2014 um 20:12 hat Markus Armbruster geschrieben:
>> Make the BlockBackend own the DriveInfo.  Change blockdev_init() to
>> return the BlockBackend instead of the DriveInfo.
>> 
>> Signed-off-by: Markus Armbruster <address@hidden>
>> ---
>>  block.c                   |  2 --
>>  block/block-backend.c     | 38 ++++++++++++++++++++++++
>>  blockdev.c                | 73 
>> ++++++++++++++++++++++++-----------------------
>>  include/sysemu/blockdev.h |  4 +++
>>  4 files changed, 79 insertions(+), 38 deletions(-)
>> 
>> diff --git a/block.c b/block.c
>> index 7ccf443..5f7dc45 100644
>> --- a/block.c
>> +++ b/block.c
>> @@ -29,7 +29,6 @@
>>  #include "qemu/module.h"
>>  #include "qapi/qmp/qjson.h"
>>  #include "sysemu/sysemu.h"
>> -#include "sysemu/blockdev.h"    /* FIXME layering violation */
>>  #include "qemu/notify.h"
>>  #include "block/coroutine.h"
>>  #include "block/qapi.h"
>> @@ -2124,7 +2123,6 @@ static void bdrv_delete(BlockDriverState *bs)
>>      /* remove from list, if necessary */
>>      bdrv_make_anon(bs);
>>  
>> -    drive_info_del(drive_get_by_blockdev(bs));
>>      g_free(bs);
>>  }
>>  
>> diff --git a/block/block-backend.c b/block/block-backend.c
>> index a12215a..9ee57c7 100644
>> --- a/block/block-backend.c
>> +++ b/block/block-backend.c
>> @@ -12,11 +12,13 @@
>>  
>>  #include "sysemu/block-backend.h"
>>  #include "block/block_int.h"
>> +#include "sysemu/blockdev.h"
>>  
>>  struct BlockBackend {
>>      char *name;
>>      int refcnt;
>>      BlockDriverState *bs;
>> +    DriveInfo *legacy_dinfo;
>>      QTAILQ_ENTRY(BlockBackend) link; /* for blk_backends */
>>  };
>>  
>> @@ -87,6 +89,7 @@ static void blk_delete(BlockBackend *blk)
>>          QTAILQ_REMOVE(&blk_backends, blk, link);
>>      }
>>      g_free(blk->name);
>> +    drive_info_del(blk->legacy_dinfo);
>>      g_free(blk);
>>  }
>>  
>> @@ -167,6 +170,41 @@ BlockDriverState *blk_bs(BlockBackend *blk)
>>  }
>>  
>>  /*
>> + * Return @blk's DriveInfo if any, else null.
>> + */
>> +DriveInfo *blk_legacy_dinfo(BlockBackend *blk)
>> +{
>> +    return blk->legacy_dinfo;
>> +}
>> +
>> +/*
>> + * Set @blk's DriveInfo to @dinfo, and return it.
>> + * @blk must not have a DriveInfo set already.
>> + * No other BlockBackend may have the same DriveInfo set.
>> + */
>> +DriveInfo *blk_set_legacy_dinfo(BlockBackend *blk, DriveInfo *dinfo)
>> +{
>> +    assert(!blk->legacy_dinfo);
>> +    return blk->legacy_dinfo = dinfo;
>
> Ugh, I don't like assignments in a return statement much more than
> setters that return something.

It's an assignment.  In C, assignments return a value.

> Fortunately, nobody uses the return value, so this can become a void
> function.
>
>> +}
>> +/*
>> + * Return the BlockBackend with DriveInfo @dinfo.
>> + * It must exist.
>> + */
>> +BlockBackend *blk_by_legacy_dinfo(DriveInfo *dinfo)
>> +{
>> +    BlockBackend *blk;
>> +
>> +    QTAILQ_FOREACH(blk, &blk_backends, link) {
>> +        if (blk->legacy_dinfo == dinfo) {
>> +            return blk;
>> +        }
>> +    }
>> +    assert(0);
>
> I'm surprised that the compiler doesn't complain here. Seems it
> understands that the condition is always false and the libc header sets
> the noreturn attribute for the internal function handling a failure.
> With NDEBUG set, this wouldn't work any more.
>
> I think abort() is better.

Okay.

>> +}
>> +
>> +/*
>>   * Hide @blk.
>>   * @blk must not have been hidden already.
>>   * Make attached BlockDriverState, if any, anonymous.
>> diff --git a/blockdev.c b/blockdev.c
>> index 5da6028..722d083 100644
>> --- a/blockdev.c
>> +++ b/blockdev.c
>> @@ -47,8 +47,6 @@
>>  #include "trace.h"
>>  #include "sysemu/arch_init.h"
>>  
>> -static QTAILQ_HEAD(drivelist, DriveInfo) drives = 
>> QTAILQ_HEAD_INITIALIZER(drives);
>> -
>>  static const char *const if_name[IF_COUNT] = {
>>      [IF_NONE] = "none",
>>      [IF_IDE] = "ide",
>> @@ -89,7 +87,8 @@ static const int if_max_devs[IF_COUNT] = {
>>   */
>>  void blockdev_mark_auto_del(BlockDriverState *bs)
>>  {
>> -    DriveInfo *dinfo = drive_get_by_blockdev(bs);
>> +    BlockBackend *blk = bs->blk;
>> +    DriveInfo *dinfo = blk_legacy_dinfo(blk);
>>  
>>      if (dinfo && !dinfo->enable_auto_del) {
>>          return;
>> @@ -105,7 +104,8 @@ void blockdev_mark_auto_del(BlockDriverState *bs)
>>  
>>  void blockdev_auto_del(BlockDriverState *bs)
>>  {
>> -    DriveInfo *dinfo = drive_get_by_blockdev(bs);
>> +    BlockBackend *blk = bs->blk;
>> +    DriveInfo *dinfo = blk_legacy_dinfo(blk);
>>  
>>      if (dinfo && dinfo->auto_del) {
>>          drive_del(dinfo);
>> @@ -153,15 +153,15 @@ QemuOpts *drive_add(BlockInterfaceType type, int 
>> index, const char *file,
>>  
>>  DriveInfo *drive_get(BlockInterfaceType type, int bus, int unit)
>>  {
>> +    BlockBackend *blk;
>>      DriveInfo *dinfo;
>>  
>> -    /* seek interface, bus and unit */
>> -
>> -    QTAILQ_FOREACH(dinfo, &drives, next) {
>> -        if (dinfo->type == type &&
>> -        dinfo->bus == bus &&
>> -        dinfo->unit == unit)
>> +    for (blk = blk_next(NULL); blk; blk = blk_next(blk)) {
>> +        dinfo = blk_legacy_dinfo(blk);
>> +        if (dinfo && dinfo->type == type
>> +            && dinfo->bus == bus && dinfo->unit == unit) {
>>              return dinfo;
>> +        }
>>      }
>>  
>>      return NULL;
>> @@ -177,13 +177,15 @@ DriveInfo *drive_get_by_index(BlockInterfaceType type, 
>> int index)
>>  int drive_get_max_bus(BlockInterfaceType type)
>>  {
>>      int max_bus;
>> +    BlockBackend *blk;
>>      DriveInfo *dinfo;
>>  
>>      max_bus = -1;
>> -    QTAILQ_FOREACH(dinfo, &drives, next) {
>> -        if(dinfo->type == type &&
>> -           dinfo->bus > max_bus)
>> +    for (blk = blk_next(NULL); blk; blk = blk_next(blk)) {
>> +        dinfo = blk_legacy_dinfo(blk);
>> +        if (dinfo && dinfo->type == type && dinfo->bus > max_bus) {
>>              max_bus = dinfo->bus;
>> +        }
>>      }
>>      return max_bus;
>>  }
>> @@ -200,11 +202,11 @@ DriveInfo *drive_get_next(BlockInterfaceType type)
>>  
>>  DriveInfo *drive_get_by_blockdev(BlockDriverState *bs)
>>  {
>> -    DriveInfo *dinfo;
>> +    BlockBackend *blk;
>>  
>> -    QTAILQ_FOREACH(dinfo, &drives, next) {
>> -        if (dinfo->bdrv == bs) {
>> -            return dinfo;
>> +    for (blk = blk_next(NULL); blk; blk = blk_next(blk)) {
>> +        if (blk_bs(blk) == bs) {
>> +            return blk_legacy_dinfo(blk);
>>          }
>>      }
>>      return NULL;
>
> Why don't you use bs->blk here?

Beats me; I guess I let my fingers pick it.  I'll switch to bs->blk.

> Also, why replace calls to drive_get_by_blockdev() above when the
> function stays? Wouldn't it make more sense to remove all callers in a
> single patch instead of doing part of it in an unrelated patch?

The general idea is to convert users of drive_get_by_blockdev() to
blk_legacy_dinfo() as soon as they use BlockBackend.

PATCH 04 (this one) converts do_drive_del(), because it's already using
BlockBackend.

It also converts blockdev_mark_auto_del() and blockdev_auto_del() to
BlockBackend.  I admit that's not yet necessary at this point of the
series.  I did it because I don't like having both old
drive_get_by_blockdev() and new blk_legacy_dinfo() used in the same
file.  Even when the file is still providing drive_get_by_blockdev() to
other users, which get converted to BB only in PATCH 14.

PATCH 14 "hw: Convert from BlockDriverState to BlockBackend, mostly"
converts all the other callers.

>> @@ -218,6 +220,7 @@ static void bdrv_format_print(void *opaque, const char 
>> *name)
>>  void drive_del(DriveInfo *dinfo)
>>  {
>>      bdrv_unref(dinfo->bdrv);
>> +    blk_unref(blk_by_legacy_dinfo(dinfo));
>>  }
>
> Hmm, let's see. drive_del() has four callers:
>
> * do_drive_del(): A blk_unref() is removed there, so the code remains
>   equivalent. Good.
>
> * blockdev_auto_del(): Here, the blk_unref() is new. If I'm reading the
>   code right, unplugging a device doesn't only automagically destroy the
>   DriveInfo now, but also the BlockBackend. Is this really magic that we
>   want BlockBackend to inherit? (I don't think so.)

Let me explain this stuff, not least because it helps me think.

"Unplug of device model destroys any block backends it's using" is a
misfeature we want to confine to backends created "the old way".  Where
"the old way" really means whatever we want to remain misfeature-
compatible.  Definitely not blockdev-add.  My working hypothesis for
"old way" is "anything that goes through drive_new()".

Both old and new way create a tree of BDS.  Before this series, they
both create a DriveInfo.  Afterwards, they both create a BlockBackend,
but only -drive & friends creates a DriveInfo.

blockdev_auto_del() is supposed to *nothing* for a "new way" argument.
Its condition guarding drive_del() must ensure that.

On device unplug, blockdev_auto_del() runs for all its backends.

For an "old way" backend, it destroys.  This patch makes it destroy the
BlockBackend, too.  Which means PATCH 02 leaked it.  Meh.  I can try to
avoid the temporary leak in v4.

For a "new way" backend, it does nothing.  No change with this patch.

> * drive_hot_add(): This is an error paths that frees a previously leaked
>   BlockBackend created by an indirect drive_new() call. Silent bug fix?

Similar temporary leak.  Can try to avoid in v4.

> * pci_piix3_xen_ide_unplug(): Xen magic I don't fully understand. The
>   guest can tell us to remove the IDE drives (because it sees the same
>   image through the PV driver, I think). Whether or not we want to
>   remove the BlockBackend, I'm not completely sure.

The general idea is to delete a BB exactly when the corresponding root
BDS gets deleted.  PATCH 02 doesn't get that completely right, but this
patch should heal it.  Getting it completely right in PATCH 02 is
impractical because HMP command drive_del is so nasty.  Getting it less
wrong would be nice, and I'm going to try.

Thanks!