qemu-devel
[Top][All Lists]
Advanced

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

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


From: Markus Armbruster
Subject: Re: [Qemu-devel] [PATCH 03/23] block: Connect BlockBackend to BlockDriverState
Date: Thu, 11 Sep 2014 17:27:45 +0200
User-agent: Gnus/5.13 (Gnus v5.13) Emacs/24.3 (gnu/linux)

Kevin Wolf <address@hidden> writes:

> Am 10.09.2014 um 10:13 hat Markus Armbruster geschrieben:
>> The pointer from BlockBackend to BlockDriverState is a strong
>> reference, managed with bdrv_ref() / bdrv_unref(), the back-pointer is
>> a weak one.
>> 
>> 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.
>> 
>> Signed-off-by: Markus Armbruster <address@hidden>
>
> I'm wondering whether this is the right direction to take. In creating a
> block device, we have the following operations:
>
> 1. Create a BlockBackend
> 2. Create a BlockDriverState
> 3. Open the BlockDriverState
>
> bdrv_open() can do 2 and 3, which is probably what we want to have in
> the end - new/create and delete/close should be a single operation for
> BDSes.

Agreed.

> Combining 1 and 2 into a single function might make it harder to actually
> use bdrv_open() in this way.

Not really.

Before my series, callers all look like:

    bs = bdrv_new(name, errp);
    if (!bs) {
        [bail out]
    }
    [stuff...]
    ret = bdrv_open(&bs, [...]);
    if (ret < 0) {
        [bail out, unreffing blk and bdrv (only blk after PATCH 05]
    }

Fusing bdrv_new() and bdrv_open() into bdrv_open_new() would make it
look like:

    [stuff'...]
    bs = bdrv_open_new([...]);
    if (!bs) {
        [bail out]
    }
    [stuff''...]

Transforming [stuff...] may take some thought.

Now let's consider how my patch affects such a future fusion.  After my
patch, we have:

    blk = blk_new_with_bs(name, errp);
    if (!blk) {
        [bail out]
    }
    bs = blk_bs(blk);
    [stuff...]
    ret = bdrv_open(&bs, [...]);
    if (ret < 0) {
        [bail out, unreffing blk and bdrv (only blk after PATCH 05]
    }

Avoiding the convenience function would make it look like:

    blk = blk_new(name, errp);
    if (!blk) {
        [bail out]
    }
    bs = bdrv_new_named(name, errp);
    if (!bs) {
        [bail out, unreffing blk]
    }
    blk_attach_bs(blk, bs);
    [stuff...]
    ret = bdrv_open(&bs, [...]);
    if (ret < 0) {
        [bail out, unreffing blk and bdrv (only blk after PATCH 05]
    }

More verbose, and more complicated error handling.  Okay if it really
buys us something.  Let's fuse and see:

    [stuff'...]
    blk = blk_new(name, errp);
    if (!blk) {
        [bail out]
    }
    [stuff''...]
    ret = bdrv_open(&bs, [...]);
    if (ret < 0) {
        [bail out, unreffing blk]
    }
    [stuff'''...]

As above, transforming [stuff...] may take some thought.  Having to
inline the convenience function, however, is utterly trivial, and
doesn't make the fusing any harder.

>> diff --git a/blockdev.c b/blockdev.c
>> index 86596bc..0a0b95e 100644
>> --- a/blockdev.c
>> +++ b/blockdev.c
>> @@ -304,6 +304,7 @@ static DriveInfo *blockdev_init(const char *file, QDict 
>> *bs_opts,
>>      int bdrv_flags = 0;
>>      int on_read_error, on_write_error;
>>      BlockBackend *blk;
>> +    BlockDriverState *bs;
>>      DriveInfo *dinfo;
>>      ThrottleConfig cfg;
>>      int snapshot = 0;
>> @@ -459,30 +460,28 @@ static DriveInfo *blockdev_init(const char *file, 
>> QDict *bs_opts,
>>      }
>>  
>>      /* init */
>> -    blk = blk_new(qemu_opts_id(opts), errp);
>> +    blk = blk_new_with_bs(qemu_opts_id(opts), errp);
>>      if (!blk) {
>>          goto early_err;
>>      }
>> +    bs = blk_bs(blk);
>> +    bs->open_flags = snapshot ? BDRV_O_SNAPSHOT : 0;
>> +    bs->read_only = ro;
>> +    bs->detect_zeroes = detect_zeroes;
>> +
>> +    bdrv_set_on_error(bs, on_read_error, on_write_error);
>> +
>> +    /* disk I/O throttling */
>> +    if (throttle_enabled(&cfg)) {
>> +        bdrv_io_limits_enable(bs);
>> +        bdrv_set_io_limits(bs, &cfg);
>> +    }
>> +
>>      dinfo = g_malloc0(sizeof(*dinfo));
>>      dinfo->id = g_strdup(qemu_opts_id(opts));
>> -    dinfo->bdrv = bdrv_new_named(dinfo->id, &error);
>> -    if (error) {
>> -        error_propagate(errp, error);
>> -        goto bdrv_new_err;
>> -    }
>> -    dinfo->bdrv->open_flags = snapshot ? BDRV_O_SNAPSHOT : 0;
>> -    dinfo->bdrv->read_only = ro;
>> -    dinfo->bdrv->detect_zeroes = detect_zeroes;
>> +    dinfo->bdrv = bs;
>>      QTAILQ_INSERT_TAIL(&drives, dinfo, next);
>>  
>> -    bdrv_set_on_error(dinfo->bdrv, on_read_error, on_write_error);
>> -
>> -    /* disk I/O throttling */
>> -    if (throttle_enabled(&cfg)) {
>> -        bdrv_io_limits_enable(dinfo->bdrv);
>> -        bdrv_set_io_limits(dinfo->bdrv, &cfg);
>> -    }
>> -
>>      if (!file || !*file) {
>>          if (has_driver_specific_opts) {
>>              file = NULL;
>
> This looks correct, but it's really doing two things at once (on the one
> hand changing code to use bs instead of dinfo->bdrv and reordering, on
> the other hand introducing blk_new_with_bs()).
>
> Might be worth doing in separate patches.

I'll give it a try.

>> @@ -509,7 +508,8 @@ static DriveInfo *blockdev_init(const char *file, QDict 
>> *bs_opts,
>>      bdrv_flags |= ro ? 0 : BDRV_O_RDWR;
>>  
>>      QINCREF(bs_opts);
>> -    ret = bdrv_open(&dinfo->bdrv, file, NULL, bs_opts, bdrv_flags, drv, 
>> &error);
>> +    ret = bdrv_open(&bs, file, NULL, bs_opts, bdrv_flags, drv, &error);
>> +    assert(bs == blk_bs(blk));
>>  
>>      if (ret < 0) {
>>          error_setg(errp, "could not open disk image %s: %s",
>> @@ -518,8 +518,9 @@ static DriveInfo *blockdev_init(const char *file, QDict 
>> *bs_opts,
>>          goto err;
>>      }
>>  
>> -    if (bdrv_key_required(dinfo->bdrv))
>> +    if (bdrv_key_required(bs)) {
>>          autostart = 0;
>> +    }
>>  
>>      QDECREF(bs_opts);
>>      qemu_opts_del(opts);
>> @@ -529,7 +530,6 @@ static DriveInfo *blockdev_init(const char *file, QDict 
>> *bs_opts,
>>  err:
>>      bdrv_unref(dinfo->bdrv);
>>      QTAILQ_REMOVE(&drives, dinfo, next);
>> -bdrv_new_err:
>>      g_free(dinfo->id);
>>      g_free(dinfo);
>>      blk_unref(blk);
>> @@ -1746,15 +1746,17 @@ void qmp_block_set_io_throttle(const char *device, 
>> int64_t bps, int64_t bps_rd,
>>  int do_drive_del(Monitor *mon, const QDict *qdict, QObject **ret_data)
>>  {
>>      const char *id = qdict_get_str(qdict, "id");
>> +    BlockBackend *blk;
>>      BlockDriverState *bs;
>>      AioContext *aio_context;
>>      Error *local_err = NULL;
>>  
>> -    bs = bdrv_find(id);
>> -    if (!bs) {
>> +    blk = blk_by_name(id);
>> +    if (!blk) {
>>          error_report("Device '%s' not found", id);
>>          return -1;
>>      }
>> +    bs = blk_bs(blk);
>>  
>>      aio_context = bdrv_get_aio_context(bs);
>>      aio_context_acquire(aio_context);
>> @@ -1777,8 +1779,9 @@ 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)) {
>> +        blk_detach_bs(blk);
>> +        blk_unref(blk);
>
> We're not doing real refcounting yet, but if we did, and if the refcount
> didn't drop to zero here, why would detaching the bs still be correct?
>
> blk_delete() already takes care of detaching after this patch, so I
> suppose removing the call here would be right.

I've since reworked this part to address the temporary leak you pointed
out in your review of PATCH 02.  My current tree has this hunk here:

@@ -1791,8 +1787,7 @@ 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_unref(blk_by_name(id));
+        blk_make_anon(blk);
         /* Further I/O must not pause the guest */
         bdrv_set_on_error(bs, BLOCKDEV_ON_ERROR_REPORT,
                           BLOCKDEV_ON_ERROR_REPORT);

Suggest you take another careful look when you review v2.

>>          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);
>
> Kevin



reply via email to

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