qemu-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Qemu-devel] [PATCH 02/23] block: New BlockBackend


From: Markus Armbruster
Subject: Re: [Qemu-devel] [PATCH 02/23] block: New BlockBackend
Date: Thu, 11 Sep 2014 12:21:11 +0200
User-agent: Gnus/5.13 (Gnus v5.13) Emacs/24.3 (gnu/linux)

Benoît Canet <address@hidden> writes:

> The Wednesday 10 Sep 2014 à 10:13:31 (+0200), Markus Armbruster wrote :
>> A block device consists of a frontend device model and a backend.
>> 
>> A block backend has a tree of block drivers doing the actual work.
>> The tree is managed by the block layer.
>> 
>> We currently use a single abstraction BlockDriverState both for tree
>> nodes and the backend as a whole.  Drawbacks:
>> 
>> * Its API includes both stuff that makes sense only at the block
>>   backend level (root of the tree) and stuff that's only for use
>>   within the block layer.  This makes the API bigger and more complex
>>   than necessary.  Moreover, it's not obvious which interfaces are
>>   meant for device models, and which really aren't.
>> 
>> * Since device models keep a reference to their backend, the backend
>>   object can't just be destroyed.  But for media change, we need to
>>   replace the tree.  Our solution is to make the BlockDriverState
>>   generic, with actual driver state in a separate object, pointed to
>>   by member opaque.  That lets us replace the tree by deinitializing
>>   and reinitializing its root.  This special need of the root makes
>>   the data structure awkward everywhere in the tree.
>> 
>> The general plan is to separate the APIs into "block backend", for use
>> by device models, monitor and whatever other code dealing with block
>> backends, and "block driver", for use by the block layer and whatever
>> other code (if any) dealing with trees and tree nodes.
>> 
>> Code dealing with block backends, device models in particular, should
>> become completely oblivious of BlockDriverState.  This should let us
>> clean up both APIs, and the tree data structures.
>> 
>> This commit is a first step.  It creates a minimal "block backend"
>> API: type BlockBackend and functions to create, destroy and find them.
>> BlockBackend objects are created and destroyed, but not yet used for
>> anything; that'll come shortly.
>> 
>> BlockBackend is reference-counted.  Its reference count never exceeds
>> one so far, but that's going to change.
>> 
>> Signed-off-by: Markus Armbruster <address@hidden>
>> ---
>>  block/Makefile.objs            |   2 +-
>>  block/block-backend.c          | 110 
>> +++++++++++++++++++++++++++++++++++++++++
>>  blockdev.c                     |  10 +++-
>>  hw/block/xen_disk.c            |  11 +++++
>>  include/qemu/typedefs.h        |   1 +
>>  include/sysemu/block-backend.h |  26 ++++++++++
>>  qemu-img.c                     |  46 +++++++++++++++++
>>  qemu-io.c                      |   8 +++
>>  qemu-nbd.c                     |   3 +-
>>  9 files changed, 214 insertions(+), 3 deletions(-)
>>  create mode 100644 block/block-backend.c
>>  create mode 100644 include/sysemu/block-backend.h
>> 
>> diff --git a/block/Makefile.objs b/block/Makefile.objs
>> index f45f939..a70140b 100644
>> --- a/block/Makefile.objs
>> +++ b/block/Makefile.objs
>> @@ -5,7 +5,7 @@ block-obj-y += qed-check.o
>>  block-obj-$(CONFIG_VHDX) += vhdx.o vhdx-endian.o vhdx-log.o
>>  block-obj-$(CONFIG_QUORUM) += quorum.o
>>  block-obj-y += parallels.o blkdebug.o blkverify.o
>> -block-obj-y += snapshot.o qapi.o
>> +block-obj-y += block-backend.o snapshot.o qapi.o
>>  block-obj-$(CONFIG_WIN32) += raw-win32.o win32-aio.o
>>  block-obj-$(CONFIG_POSIX) += raw-posix.o
>>  block-obj-$(CONFIG_LINUX_AIO) += linux-aio.o
>> diff --git a/block/block-backend.c b/block/block-backend.c
>> new file mode 100644
>> index 0000000..833f7d9
>> --- /dev/null
>> +++ b/block/block-backend.c
>> @@ -0,0 +1,110 @@
>> +/*
>> + * QEMU Block backends
>> + *
>> + * Copyright (C) 2014 Red Hat, Inc.
>> + *
>> + * Authors:
>> + *  Markus Armbruster <address@hidden>,
>> + *
>> + * This work is licensed under the terms of the GNU GPL, version 2 or
>> + * later.  See the COPYING file in the top-level directory.
>> + */
>> +
>> +#include "sysemu/block-backend.h"
>> +#include "block/block_int.h"
>> +
>> +struct BlockBackend {
>> +    char *name;
>> +    int refcnt;
>> +    QTAILQ_ENTRY(BlockBackend) link; /* for blk_backends */
>> +};
>> +
>> +static QTAILQ_HEAD(, BlockBackend) blk_backends =
>> +    QTAILQ_HEAD_INITIALIZER(blk_backends);
>> +
>> +/**
>> + * blk_new:
>> + * @name: name, must not be %NULL or empty
>> + * @errp: return location for an error to be set on failure, or %NULL
>> + *
>> + * Create a new BlockBackend, with a reference count of one.  Fail if
>> + * @name already exists.
>> + *
>> + * Returns: the BlockBackend on success, %NULL on failure
>> + */
>> +BlockBackend *blk_new(const char *name, Error **errp)
>> +{
>> +    BlockBackend *blk = g_new0(BlockBackend, 1);
>> +
>> +    assert(name && name[0]);
>> +    if (blk_by_name(name)) {
>> +        error_setg(errp, "Device with id '%s' already exists", name);
>> +        return NULL;
>> +    }
>> +    blk->name = g_strdup(name);
>> +    blk->refcnt = 1;
>> +    QTAILQ_INSERT_TAIL(&blk_backends, blk, link);
>> +    return blk;
>> +}
>> +
>> +static void blk_delete(BlockBackend *blk)
>> +{
>> +    assert(!blk->refcnt);
>> +    QTAILQ_REMOVE(&blk_backends, blk, link);
>> +    g_free(blk->name);
>> +    g_free(blk);
>> +}
>> +
>> +/**
>> + * blk_ref:
>> + *
>> + * Increment @blk's reference count.
>> + */
>> +void blk_ref(BlockBackend *blk)
>> +{
>> +    blk->refcnt++;
>> +}
>> +
>> +/**
>> + * blk_unref:
>> + *
>> + * Decrement @blk's reference count.  If this drops it to zero,
>> + * destroy @blk.
>> + */
>> +void blk_unref(BlockBackend *blk)
>> +{
>> +    if (blk) {
>> +        g_assert(blk->refcnt > 0);
>> +        if (!--blk->refcnt) {
>> +            blk_delete(blk);
>> +        }
>> +    }
>> +}
>> +
>> +const char *blk_name(BlockBackend *blk)
>> +{
>> +    return blk->name;
>> +}
>> +
>> +BlockBackend *blk_by_name(const char *name)
>> +{
>> +    BlockBackend *blk;
>> +
>> +    QTAILQ_FOREACH(blk, &blk_backends, link) {
>> +        if (!strcmp(name, blk->name)) {
>> +            return blk;
>> +        }
>> +    }
>> +    return NULL;
>> +}
>> +
>> +/**
>> + * blk_next:
>> + *
>> + * Returns: the first BlockBackend if @blk is null, else @blk's next
>> + * sibling, which is %NULL for the last BlockBackend
>> + */
>> +BlockBackend *blk_next(BlockBackend *blk)
>> +{
>> +    return blk ? QTAILQ_NEXT(blk, link) : QTAILQ_FIRST(&blk_backends);
>> +}
>> diff --git a/blockdev.c b/blockdev.c
>> index 9fbd888..86596bc 100644
>> --- a/blockdev.c
>> +++ b/blockdev.c
>> @@ -30,6 +30,7 @@
>>   * THE SOFTWARE.
>>   */
>>  
>> +#include "sysemu/block-backend.h"
>>  #include "sysemu/blockdev.h"
>>  #include "hw/block/block.h"
>>  #include "block/blockjob.h"
>> @@ -221,6 +222,7 @@ void drive_del(DriveInfo *dinfo)
>>      }
>>  
>>      bdrv_unref(dinfo->bdrv);
>> +    blk_unref(blk_by_name(dinfo->id));
>>      g_free(dinfo->id);
>>      QTAILQ_REMOVE(&drives, dinfo, next);
>>      g_free(dinfo->serial);
>> @@ -301,6 +303,7 @@ static DriveInfo *blockdev_init(const char *file, QDict 
>> *bs_opts,
>>      int ro = 0;
>>      int bdrv_flags = 0;
>>      int on_read_error, on_write_error;
>> +    BlockBackend *blk;
>>      DriveInfo *dinfo;
>>      ThrottleConfig cfg;
>>      int snapshot = 0;
>> @@ -456,6 +459,10 @@ static DriveInfo *blockdev_init(const char *file, QDict 
>> *bs_opts,
>>      }
>>  
>>      /* init */
>> +    blk = blk_new(qemu_opts_id(opts), errp);
>> +    if (!blk) {
>> +        goto early_err;
>> +    }
>
> Here you create a new block backend.
> And you don't attach it to anything in any way yet.

Yes.  Right before creating the root BDS.

> So down in the code the following test will leak it:
>     if (!file || !*file) {                                                    
>   
>         if (has_driver_specific_opts) {                                       
>   
>             file = NULL;                                                      
>   
>         } else {                                                              
>   
>             QDECREF(bs_opts);                                                 
>   
>             qemu_opts_del(opts);                                              
>   
>             return dinfo;                                                     
>   
>         }                                                                     
>   
>     } 

The root BDS isn't destroyed here, and therefore the BB isn't, either.

The BB will be destroyed right when the root BDS is.

> I am sure one of your next patchs fixes this but for this
> precise commit this do look like a leak.
>
>>      dinfo = g_malloc0(sizeof(*dinfo));
>>      dinfo->id = g_strdup(qemu_opts_id(opts));
>>      dinfo->bdrv = bdrv_new_named(dinfo->id, &error);
>> @@ -525,6 +532,7 @@ err:
>>  bdrv_new_err:
>>      g_free(dinfo->id);
>>      g_free(dinfo);
>> +    blk_unref(blk);
>>  early_err:
>>      qemu_opts_del(opts);
>>  err_no_opts:
>> @@ -1770,7 +1778,7 @@ int do_drive_del(Monitor *mon, const QDict *qdict, 
>> QObject **ret_data)
>>       */
>>      if (bdrv_get_attached_dev(bs)) {
>>          bdrv_make_anon(bs);
>> -
>> +        blk_unref(blk_by_name(id));
>>          /* Further I/O must not pause the guest */
>>          bdrv_set_on_error(bs, BLOCKDEV_ON_ERROR_REPORT,
>>                            BLOCKDEV_ON_ERROR_REPORT);
>> diff --git a/hw/block/xen_disk.c b/hw/block/xen_disk.c
>> index 8bac7ff..730a021 100644
>> --- a/hw/block/xen_disk.c
>> +++ b/hw/block/xen_disk.c
>> @@ -39,6 +39,7 @@
>>  #include "hw/xen/xen_backend.h"
>>  #include "xen_blkif.h"
>>  #include "sysemu/blockdev.h"
>> +#include "sysemu/block-backend.h"
>>  
>>  /* ------------------------------------------------------------- */
>>  
>> @@ -852,12 +853,18 @@ static int blk_connect(struct XenDevice *xendev)
>>      blkdev->dinfo = drive_get(IF_XEN, 0, index);
>>      if (!blkdev->dinfo) {
>>          Error *local_err = NULL;
>> +        BlockBackend *blk;
>>          BlockDriver *drv;
>>  
>>          /* setup via xenbus -> create new block driver instance */
>>          xen_be_printf(&blkdev->xendev, 2, "create new bdrv (xenbus 
>> setup)\n");
>> +        blk = blk_new(blkdev->dev, NULL);
>> +        if (!blk) {
>> +            return -1;
>> +        }
>>          blkdev->bs = bdrv_new_named(blkdev->dev, NULL);
>>          if (!blkdev->bs) {
>> +            blk_unref(blk);
>>              return -1;
>>          }
>>  
>> @@ -868,6 +875,7 @@ static int blk_connect(struct XenDevice *xendev)
>>                            error_get_pretty(local_err));
>>              error_free(local_err);
>>              bdrv_unref(blkdev->bs);
>> +            blk_unref(blk);
>>              blkdev->bs = NULL;
>>              return -1;
>>          }
>> @@ -983,6 +991,9 @@ static void blk_disconnect(struct XenDevice *xendev)
>>      if (blkdev->bs) {
>>          bdrv_detach_dev(blkdev->bs, blkdev);
>>          bdrv_unref(blkdev->bs);
>> +        if (!blkdev->dinfo) {
>> +            blk_unref(blk_by_name(blkdev->dev));
>> +        }
>>          blkdev->bs = NULL;
>>      }
>>      xen_be_unbind_evtchn(&blkdev->xendev);
>> diff --git a/include/qemu/typedefs.h b/include/qemu/typedefs.h
>> index 5f20b0e..198da2e 100644
>> --- a/include/qemu/typedefs.h
>> +++ b/include/qemu/typedefs.h
>> @@ -35,6 +35,7 @@ typedef struct MachineClass MachineClass;
>>  typedef struct NICInfo NICInfo;
>>  typedef struct HCIInfo HCIInfo;
>>  typedef struct AudioState AudioState;
>> +typedef struct BlockBackend BlockBackend;
>>  typedef struct BlockDriverState BlockDriverState;
>>  typedef struct DriveInfo DriveInfo;
>>  typedef struct DisplayState DisplayState;
>> diff --git a/include/sysemu/block-backend.h b/include/sysemu/block-backend.h
>> new file mode 100644
>> index 0000000..3f8371c
>> --- /dev/null
>> +++ b/include/sysemu/block-backend.h
>> @@ -0,0 +1,26 @@
>> +/*
>> + * QEMU Block backends
>> + *
>> + * Copyright (C) 2014 Red Hat, Inc.
>> + *
>> + * Authors:
>> + *  Markus Armbruster <address@hidden>,
>> + *
>> + * This work is licensed under the terms of the GNU GPL, version 2 or
>> + * later.  See the COPYING file in the top-level directory.
>> + */
>> +
>> +#ifndef BLOCK_BACKEND_H
>> +#define BLOCK_BACKEND_H
>> +
>> +#include "qemu/typedefs.h"
>> +#include "qapi/error.h"
>> +
>> +BlockBackend *blk_new(const char *name, Error **errp);
>> +void blk_ref(BlockBackend *blk);
>> +void blk_unref(BlockBackend *blk);
>> +const char *blk_name(BlockBackend *blk);
>> +BlockBackend *blk_by_name(const char *name);
>> +BlockBackend *blk_next(BlockBackend *blk);
>> +
>> +#endif
>> diff --git a/qemu-img.c b/qemu-img.c
>> index 4490a22..bad3f64 100644
>> --- a/qemu-img.c
>> +++ b/qemu-img.c
>> @@ -29,6 +29,7 @@
>>  #include "qemu/error-report.h"
>>  #include "qemu/osdep.h"
>>  #include "sysemu/sysemu.h"
>> +#include "sysemu/block-backend.h"
>>  #include "block/block_int.h"
>>  #include "block/qapi.h"
>>  #include <getopt.h>
>> @@ -575,6 +576,7 @@ static int img_check(int argc, char **argv)
>>      int c, ret;
>>      OutputFormat output_format = OFORMAT_HUMAN;
>>      const char *filename, *fmt, *output, *cache;
>> +    BlockBackend *blk;
>>      BlockDriverState *bs;
>>      int fix = 0;
>>      int flags = BDRV_O_FLAGS | BDRV_O_CHECK;
>> @@ -649,6 +651,7 @@ static int img_check(int argc, char **argv)
>>          return 1;
>>      }
>>  
>
>> +    blk = blk_new("image", &error_abort);
> Hmm we are so sure this will work that we don't do if (!block) ?

Matches what bdrv_new_open() does:

    bs = bdrv_new_named(id, &error_abort);

As you noted further down, the tools treat these failures as programming
errors.  That's appropriate.

>>      bs = bdrv_new_open("image", filename, fmt, flags, true, quiet);
>>      if (!bs) {
>>          return 1;
>> @@ -710,6 +713,7 @@ static int img_check(int argc, char **argv)
>>  fail:
>>      qapi_free_ImageCheck(check);
>>      bdrv_unref(bs);
>> +    blk_unref(blk);
>>  
>>      return ret;
>>  }
>> @@ -718,6 +722,7 @@ static int img_commit(int argc, char **argv)
>>  {
>>      int c, ret, flags;
>>      const char *filename, *fmt, *cache;
>> +    BlockBackend *blk;
>>      BlockDriverState *bs;
>>      bool quiet = false;
>>  
>> @@ -756,6 +761,7 @@ static int img_commit(int argc, char **argv)
>>          return 1;
>>      }
>>  
>> +    blk = blk_new("image", &error_abort);
>>      bs = bdrv_new_open("image", filename, fmt, flags, true, quiet);
>>      if (!bs) {
>>          return 1;
>> @@ -780,6 +786,7 @@ static int img_commit(int argc, char **argv)
>>      }
>>  
>>      bdrv_unref(bs);
>> +    blk_unref(blk);
>>      if (ret) {
>>          return 1;
>>      }
>> @@ -942,6 +949,7 @@ static int check_empty_sectors(BlockDriverState *bs, 
>> int64_t sect_num,
>>  static int img_compare(int argc, char **argv)
>>  {
>>      const char *fmt1 = NULL, *fmt2 = NULL, *cache, *filename1, *filename2;
>> +    BlockBackend *blk1, *blk2;
>>      BlockDriverState *bs1, *bs2;
>>      int64_t total_sectors1, total_sectors2;
>>      uint8_t *buf1 = NULL, *buf2 = NULL;
>> @@ -1011,6 +1019,7 @@ static int img_compare(int argc, char **argv)
>>          goto out3;
>>      }
>>  
>> +    blk1 = blk_new("image 1", &error_abort);
>>      bs1 = bdrv_new_open("image 1", filename1, fmt1, flags, true, quiet);
>>      if (!bs1) {
>>          error_report("Can't open file %s", filename1);
>> @@ -1018,6 +1027,7 @@ static int img_compare(int argc, char **argv)
>>          goto out3;
>>      }
>>  
>> +    blk2 = blk_new("image 2", &error_abort);
>>      bs2 = bdrv_new_open("image 2", filename2, fmt2, flags, true, quiet);
>>      if (!bs2) {
>>          error_report("Can't open file %s", filename2);
>> @@ -1184,10 +1194,12 @@ static int img_compare(int argc, char **argv)
>>  
>>  out:
>>      bdrv_unref(bs2);
>> +    blk_unref(blk2);
>>      qemu_vfree(buf1);
>>      qemu_vfree(buf2);
>>  out2:
>>      bdrv_unref(bs1);
>> +    blk_unref(blk1);
>>  out3:
>>      qemu_progress_end();
>>      return ret;
>> @@ -1200,6 +1212,7 @@ static int img_convert(int argc, char **argv)
>>      int progress = 0, flags, src_flags;
>>      const char *fmt, *out_fmt, *cache, *src_cache, *out_baseimg, 
>> *out_filename;
>>      BlockDriver *drv, *proto_drv;
>> +    BlockBackend **blk = NULL, *out_blk = NULL;
>>      BlockDriverState **bs = NULL, *out_bs = NULL;
>>      int64_t total_sectors, nb_sectors, sector_num, bs_offset;
>>      int64_t *bs_sectors = NULL;
>> @@ -1354,6 +1367,7 @@ static int img_convert(int argc, char **argv)
>>  
>>      qemu_progress_print(0, 100);
>>  
>> +    blk = g_new0(BlockBackend *, bs_n);
>>      bs = g_new0(BlockDriverState *, bs_n);
>>      bs_sectors = g_new(int64_t, bs_n);
>>  
>> @@ -1361,6 +1375,7 @@ static int img_convert(int argc, char **argv)
>>      for (bs_i = 0; bs_i < bs_n; bs_i++) {
>>          char *id = bs_n > 1 ? g_strdup_printf("source %d", bs_i)
>>                              : g_strdup("source");
>> +        blk[bs_i] = blk_new(id, &error_abort);
>>          bs[bs_i] = bdrv_new_open(id, argv[optind + bs_i], fmt, src_flags,
>>                                   true, quiet);
>>          g_free(id);
>> @@ -1486,6 +1501,7 @@ static int img_convert(int argc, char **argv)
>>          goto out;
>>      }
>>  
>> +    out_blk = blk_new("target", &error_abort);
>>      out_bs = bdrv_new_open("target", out_filename, out_fmt, flags, true, 
>> quiet);
>>      if (!out_bs) {
>>          ret = -1;
>> @@ -1742,6 +1758,7 @@ out:
>>      if (out_bs) {
>>          bdrv_unref(out_bs);
>>      }
>> +    blk_unref(out_blk);
>>      if (bs) {
>>          for (bs_i = 0; bs_i < bs_n; bs_i++) {
>>              if (bs[bs_i]) {
>> @@ -1750,6 +1767,12 @@ out:
>>          }
>>          g_free(bs);
>>      }
>> +    if (blk) {
>> +        for (bs_i = 0; bs_i < bs_n; bs_i++) {
>> +            blk_unref(blk[bs_i]);
>> +        }
>> +        g_free(blk);
>> +    }
>>      g_free(bs_sectors);
>>  fail_getopt:
>>      g_free(options);
>> @@ -1858,6 +1881,7 @@ static ImageInfoList *collect_image_info_list(const 
>> char *filename,
>>      filenames = g_hash_table_new_full(g_str_hash, str_equal_func, NULL, 
>> NULL);
>>  
>>      while (filename) {
>> +        BlockBackend *blk;
>>          BlockDriverState *bs;
>>          ImageInfo *info;
>>          ImageInfoList *elem;
>> @@ -1869,6 +1893,7 @@ static ImageInfoList *collect_image_info_list(const 
>> char *filename,
>>          }
>>          g_hash_table_insert(filenames, (gpointer)filename, NULL);
>>  
>> +        blk = blk_new("image", &error_abort);
>>          bs = bdrv_new_open("image", filename, fmt,
>>                             BDRV_O_FLAGS | BDRV_O_NO_BACKING, false, false);
>>          if (!bs) {
>
> I think it misses an 
>> +            blk_unref(blk);
> in if(!bs) branch.

Yes.  Kevin noted that, too.  I'll fix it.

[...]



reply via email to

[Prev in Thread] Current Thread [Next in Thread]