From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:55543) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1XS1VC-000369-5C for qemu-devel@nongnu.org; Thu, 11 Sep 2014 06:21:27 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1XS1V6-0001ov-Kn for qemu-devel@nongnu.org; Thu, 11 Sep 2014 06:21:22 -0400 Received: from mx1.redhat.com ([209.132.183.28]:9073) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1XS1V6-0001oj-Be for qemu-devel@nongnu.org; Thu, 11 Sep 2014 06:21:16 -0400 From: Markus Armbruster References: <1410336832-22160-1-git-send-email-armbru@redhat.com> <1410336832-22160-3-git-send-email-armbru@redhat.com> <20140910124042.GC22376@irqsave.net> Date: Thu, 11 Sep 2014 12:21:11 +0200 In-Reply-To: <20140910124042.GC22376@irqsave.net> (=?utf-8?Q?=22Beno=C3=AE?= =?utf-8?Q?t?= Canet"'s message of "Wed, 10 Sep 2014 14:40:42 +0200") Message-ID: <87iokusbjc.fsf@blackfin.pond.sub.org> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable Subject: Re: [Qemu-devel] [PATCH 02/23] block: New BlockBackend List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: =?utf-8?Q?Beno=C3=AEt?= Canet Cc: kwolf@redhat.com, famz@redhat.com, qemu-devel@nongnu.org, stefanha@redhat.com Beno=C3=AEt Canet writes: > The Wednesday 10 Sep 2014 =C3=A0 10:13:31 (+0200), Markus Armbruster wrot= e : >> A block device consists of a frontend device model and a backend. >>=20 >> A block backend has a tree of block drivers doing the actual work. >> The tree is managed by the block layer. >>=20 >> We currently use a single abstraction BlockDriverState both for tree >> nodes and the backend as a whole. Drawbacks: >>=20 >> * 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. >>=20 >> * 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. >>=20 >> 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. >>=20 >> 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. >>=20 >> 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. >>=20 >> BlockBackend is reference-counted. Its reference count never exceeds >> one so far, but that's going to change. >>=20 >> Signed-off-by: Markus Armbruster >> --- >> 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 >>=20 >> 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 +=3D qed-check.o >> block-obj-$(CONFIG_VHDX) +=3D vhdx.o vhdx-endian.o vhdx-log.o >> block-obj-$(CONFIG_QUORUM) +=3D quorum.o >> block-obj-y +=3D parallels.o blkdebug.o blkverify.o >> -block-obj-y +=3D snapshot.o qapi.o >> +block-obj-y +=3D block-backend.o snapshot.o qapi.o >> block-obj-$(CONFIG_WIN32) +=3D raw-win32.o win32-aio.o >> block-obj-$(CONFIG_POSIX) +=3D raw-posix.o >> block-obj-$(CONFIG_LINUX_AIO) +=3D 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 , >> + * >> + * 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 =3D >> + 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 =3D 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 =3D g_strdup(name); >> + blk->refcnt =3D 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. >> */ >>=20=20 >> +#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) >> } >>=20=20 >> 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, QD= ict *bs_opts, >> int ro =3D 0; >> int bdrv_flags =3D 0; >> int on_read_error, on_write_error; >> + BlockBackend *blk; >> DriveInfo *dinfo; >> ThrottleConfig cfg; >> int snapshot =3D 0; >> @@ -456,6 +459,10 @@ static DriveInfo *blockdev_init(const char *file, Q= Dict *bs_opts, >> } >>=20=20 >> /* init */ >> + blk =3D 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) {=20=20=20=20=20=20=20=20=20=20=20=20=20=20=20= =20=20=20=20=20=20=20=20=20=20=20=20=20=20=20=20=20=20=20=20=20=20=20=20=20= =20=20=20=20=20=20=20=20=20=20=20=20=20=20 > if (has_driver_specific_opts) {=20=20=20=20=20=20=20=20=20=20=20= =20=20=20=20=20=20=20=20=20=20=20=20=20=20=20=20=20=20=20=20=20=20=20=20=20= =20=20=20=20=20 > file =3D NULL;=20=20=20=20=20=20=20=20=20=20=20=20=20=20=20= =20=20=20=20=20=20=20=20=20=20=20=20=20=20=20=20=20=20=20=20=20=20=20=20=20= =20=20=20=20=20=20=20=20=20=20=20=20=20=20=20=20 > } else {=20=20=20=20=20=20=20=20=20=20=20=20=20=20=20=20=20=20=20= =20=20=20=20=20=20=20=20=20=20=20=20=20=20=20=20=20=20=20=20=20=20=20=20=20= =20=20=20=20=20=20=20=20=20=20=20=20=20=20=20=20=20=20=20=20 > QDECREF(bs_opts);=20=20=20=20=20=20=20=20=20=20=20=20=20=20= =20=20=20=20=20=20=20=20=20=20=20=20=20=20=20=20=20=20=20=20=20=20=20=20=20= =20=20=20=20=20=20=20=20=20=20=20=20 > qemu_opts_del(opts);=20=20=20=20=20=20=20=20=20=20=20=20=20= =20=20=20=20=20=20=20=20=20=20=20=20=20=20=20=20=20=20=20=20=20=20=20=20=20= =20=20=20=20=20=20=20=20=20=20 > return dinfo;=20=20=20=20=20=20=20=20=20=20=20=20=20=20=20=20= =20=20=20=20=20=20=20=20=20=20=20=20=20=20=20=20=20=20=20=20=20=20=20=20=20= =20=20=20=20=20=20=20=20=20=20=20=20=20=20 > }=20=20=20=20=20=20=20=20=20=20=20=20=20=20=20=20=20=20=20=20=20= =20=20=20=20=20=20=20=20=20=20=20=20=20=20=20=20=20=20=20=20=20=20=20=20=20= =20=20=20=20=20=20=20=20=20=20=20=20=20=20=20=20=20=20=20=20=20=20=20=20=20 > }=20 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 =3D g_malloc0(sizeof(*dinfo)); >> dinfo->id =3D g_strdup(qemu_opts_id(opts)); >> dinfo->bdrv =3D 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" >>=20=20 >> /* ------------------------------------------------------------- */ >>=20=20 >> @@ -852,12 +853,18 @@ static int blk_connect(struct XenDevice *xendev) >> blkdev->dinfo =3D drive_get(IF_XEN, 0, index); >> if (!blkdev->dinfo) { >> Error *local_err =3D NULL; >> + BlockBackend *blk; >> BlockDriver *drv; >>=20=20 >> /* setup via xenbus -> create new block driver instance */ >> xen_be_printf(&blkdev->xendev, 2, "create new bdrv (xenbus setu= p)\n"); >> + blk =3D blk_new(blkdev->dev, NULL); >> + if (!blk) { >> + return -1; >> + } >> blkdev->bs =3D bdrv_new_named(blkdev->dev, NULL); >> if (!blkdev->bs) { >> + blk_unref(blk); >> return -1; >> } >>=20=20 >> @@ -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 =3D 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 =3D 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-backe= nd.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 , >> + * >> + * 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 >> @@ -575,6 +576,7 @@ static int img_check(int argc, char **argv) >> int c, ret; >> OutputFormat output_format =3D OFORMAT_HUMAN; >> const char *filename, *fmt, *output, *cache; >> + BlockBackend *blk; >> BlockDriverState *bs; >> int fix =3D 0; >> int flags =3D BDRV_O_FLAGS | BDRV_O_CHECK; >> @@ -649,6 +651,7 @@ static int img_check(int argc, char **argv) >> return 1; >> } >>=20=20 > >> + blk =3D 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 =3D bdrv_new_named(id, &error_abort); As you noted further down, the tools treat these failures as programming errors. That's appropriate. >> bs =3D 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); >>=20=20 >> 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 =3D false; >>=20=20 >> @@ -756,6 +761,7 @@ static int img_commit(int argc, char **argv) >> return 1; >> } >>=20=20 >> + blk =3D blk_new("image", &error_abort); >> bs =3D 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) >> } >>=20=20 >> 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 =3D NULL, *fmt2 =3D NULL, *cache, *filename1, *fil= ename2; >> + BlockBackend *blk1, *blk2; >> BlockDriverState *bs1, *bs2; >> int64_t total_sectors1, total_sectors2; >> uint8_t *buf1 =3D NULL, *buf2 =3D NULL; >> @@ -1011,6 +1019,7 @@ static int img_compare(int argc, char **argv) >> goto out3; >> } >>=20=20 >> + blk1 =3D blk_new("image 1", &error_abort); >> bs1 =3D bdrv_new_open("image 1", filename1, fmt1, flags, true, quie= t); >> if (!bs1) { >> error_report("Can't open file %s", filename1); >> @@ -1018,6 +1027,7 @@ static int img_compare(int argc, char **argv) >> goto out3; >> } >>=20=20 >> + blk2 =3D blk_new("image 2", &error_abort); >> bs2 =3D bdrv_new_open("image 2", filename2, fmt2, flags, true, quie= t); >> if (!bs2) { >> error_report("Can't open file %s", filename2); >> @@ -1184,10 +1194,12 @@ static int img_compare(int argc, char **argv) >>=20=20 >> 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 =3D 0, flags, src_flags; >> const char *fmt, *out_fmt, *cache, *src_cache, *out_baseimg, *out_f= ilename; >> BlockDriver *drv, *proto_drv; >> + BlockBackend **blk =3D NULL, *out_blk =3D NULL; >> BlockDriverState **bs =3D NULL, *out_bs =3D NULL; >> int64_t total_sectors, nb_sectors, sector_num, bs_offset; >> int64_t *bs_sectors =3D NULL; >> @@ -1354,6 +1367,7 @@ static int img_convert(int argc, char **argv) >>=20=20 >> qemu_progress_print(0, 100); >>=20=20 >> + blk =3D g_new0(BlockBackend *, bs_n); >> bs =3D g_new0(BlockDriverState *, bs_n); >> bs_sectors =3D g_new(int64_t, bs_n); >>=20=20 >> @@ -1361,6 +1375,7 @@ static int img_convert(int argc, char **argv) >> for (bs_i =3D 0; bs_i < bs_n; bs_i++) { >> char *id =3D bs_n > 1 ? g_strdup_printf("source %d", bs_i) >> : g_strdup("source"); >> + blk[bs_i] =3D blk_new(id, &error_abort); >> bs[bs_i] =3D bdrv_new_open(id, argv[optind + bs_i], fmt, src_fl= ags, >> true, quiet); >> g_free(id); >> @@ -1486,6 +1501,7 @@ static int img_convert(int argc, char **argv) >> goto out; >> } >>=20=20 >> + out_blk =3D blk_new("target", &error_abort); >> out_bs =3D bdrv_new_open("target", out_filename, out_fmt, flags, tr= ue, quiet); >> if (!out_bs) { >> ret =3D -1; >> @@ -1742,6 +1758,7 @@ out: >> if (out_bs) { >> bdrv_unref(out_bs); >> } >> + blk_unref(out_blk); >> if (bs) { >> for (bs_i =3D 0; bs_i < bs_n; bs_i++) { >> if (bs[bs_i]) { >> @@ -1750,6 +1767,12 @@ out: >> } >> g_free(bs); >> } >> + if (blk) { >> + for (bs_i =3D 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(cons= t char *filename, >> filenames =3D g_hash_table_new_full(g_str_hash, str_equal_func, NUL= L, NULL); >>=20=20 >> while (filename) { >> + BlockBackend *blk; >> BlockDriverState *bs; >> ImageInfo *info; >> ImageInfoList *elem; >> @@ -1869,6 +1893,7 @@ static ImageInfoList *collect_image_info_list(cons= t char *filename, >> } >> g_hash_table_insert(filenames, (gpointer)filename, NULL); >>=20=20 >> + blk =3D blk_new("image", &error_abort); >> bs =3D bdrv_new_open("image", filename, fmt, >> BDRV_O_FLAGS | BDRV_O_NO_BACKING, false, fal= se); >> if (!bs) { > > I think it misses an=20 >> + blk_unref(blk); > in if(!bs) branch. Yes. Kevin noted that, too. I'll fix it. [...]