Re: [Qemu-devel] [2.4 PATCH v3 04/19] qmp: Add block-dirty-bitmap-add and block-dirty-bitmap-remove

[Max Reitz]

On 2015-03-16 at 16:53, John Snow wrote:
> On 03/16/2015 04:44 PM, Max Reitz wrote:
> > On 2015-03-13 at 14:30, John Snow wrote:
> > > The new command pair is added to manage a user created dirty bitmap. 
> > > The
> > > dirty bitmap's name is mandatory and must be unique for the same 
> > > device,
> > > but different devices can have bitmaps with the same names.
> > >
> > > The granularity is an optional field. If it is not specified, we will
> > > choose a default granularity based on the cluster size if available,
> > > clamped to between 4K and 64K to mirror how the 'mirror' code was
> > > already choosing granularity. If we do not have cluster size info
> > > available, we choose 64K. This code has been factored out into a helper
> > > shared with block/mirror.
> > >
> > > This patch also introduces the 'block_dirty_bitmap_lookup' helper,
> > > which takes a device name and a dirty bitmap name and validates the
> > > lookup, returning NULL and setting errp if there is a problem with
> > > either field. This helper will be re-used in future patches in this
> > > series.
> > >
> > > The types added to block-core.json will be re-used in future patches
> > > in this series, see:
> > > 'qapi: Add transaction support to block-dirty-bitmap-{add, enable,
> > > disable}'
> > >
> > > Signed-off-by: John Snow <address@hidden>
> > > ---
> > >   block.c               |  20 ++++++++++
> > >   block/mirror.c        |  10 +----
> > >   blockdev.c            | 102
> > > ++++++++++++++++++++++++++++++++++++++++++++++++++
> > >   include/block/block.h |   1 +
> > >   qapi/block-core.json  |  55 +++++++++++++++++++++++++++
> > >   qmp-commands.hx       |  56 +++++++++++++++++++++++++++
> > >   6 files changed, 235 insertions(+), 9 deletions(-)
> >
> > [snip]
> >
> > > diff --git a/blockdev.c b/blockdev.c
> > > index b9c1c0c..b8455b9 100644
> > > --- a/blockdev.c
> > > +++ b/blockdev.c
> > > @@ -1161,6 +1161,53 @@ out_aio_context:
> > >       return NULL;
> > >   }
> > > +/**
> > > + * Return a dirty bitmap (if present), after validating
> > > + * the node reference and bitmap names. Returns NULL on error,
> > > + * including when the BDS and/or bitmap is not found.
> > > + */
> > > +static BdrvDirtyBitmap *block_dirty_bitmap_lookup(const char *node,
> > > +                                                  const char *name,
> > > + BlockDriverState
> > > **pbs,
> > > +                                                  AioContext **paio,
> > > +                                                  Error **errp)
> > > +{
> > > +    BlockDriverState *bs;
> > > +    BdrvDirtyBitmap *bitmap;
> > > +    AioContext *aio_context;
> > > +
> > > +    if (!node) {
> > > +        error_setg(errp, "Node cannot be NULL");
> > > +        return NULL;
> > > +    }
> > > +    if (!name) {
> > > +        error_setg(errp, "Bitmap name cannot be NULL");
> > > +        return NULL;
> > > +    }
> > > +    bs = bdrv_lookup_bs(node, node, NULL);
> > > +    if (!bs) {
> > > +        error_setg(errp, "Node '%s' not found", node);
> > > +        return NULL;
> > > +    }
> > > +
> > > +    /* If caller provided a BDS*, provide the result of that lookup,
> > > too. */
> > > +    if (pbs) {
> > > +        assert(paio);
> > > +        aio_context = bdrv_get_aio_context(bs);
> > > +        aio_context_acquire(aio_context);
> > > +        *pbs = bs;
> > > +        *paio = aio_context;
> >
> > General question (because I'm too lazy to look up, or find out where to
> > look it up in the first place): Do you maybe want to acquire the AIO
> > context always before bdrv_find_dirty_bitmap(), even if paio == pbs ==
> > NULL?
>
> Somewhat a leftover from an earlier revision when not every caller 
> actually cared to receive the BDS for a bitmap lookup -- There is the 
> assumption that maybe certain callers don't care, already know the 
> BDS, etc.
>
> In these cases maybe the lock isn't important because they already 
> have a lock from acquiring the BDS.

Recursively acquiring AIO contexts is just fine. :-)

Max

> Impossible to say, anyway, since nobody uses the function as such 
> right now, so it might be just as good to remove the optional-ness of 
> these parameters "for now."
>
> > > +    }
> > > +
> > > +    bitmap = bdrv_find_dirty_bitmap(bs, name);
> > > +    if (!bitmap) {
> > > +        error_setg(errp, "Dirty bitmap '%s' not found", name);
> >
> > I'd propose a aio_context_release(aio_context); *paio = NULL; *pbs =
> > NULL; here. Makes error handling easier.
> >
> > > +        return NULL;
> > > +    }
> > > +
> > > +    return bitmap;
> > > +}
> > > +
> > >   /* New and old BlockDriverState structs for atomic group 
> > > operations */
> > >   typedef struct BlkTransactionState BlkTransactionState;
> > > @@ -1941,6 +1988,61 @@ void qmp_block_set_io_throttle(const char
> > > *device, int64_t bps, int64_t bps_rd,
> > >       aio_context_release(aio_context);
> > >   }
> > > +void qmp_block_dirty_bitmap_add(const char *node, const char *name,
> > > +                                bool has_granularity, uint32_t
> > > granularity,
> > > +                                Error **errp)
> > > +{
> > > +    AioContext *aio_context;
> > > +    BlockDriverState *bs;
> > > +
> > > +    if (!name || name[0] == '\0') {
> > > +        error_setg(errp, "Bitmap name cannot be empty");
> > > +        return;
> > > +    }
> > > +
> > > +    bs = bdrv_lookup_bs(node, node, errp);
> > > +    if (!bs) {
> > > +        return;
> > > +    }
> > > +
> > > +    aio_context = bdrv_get_aio_context(bs);
> > > +    aio_context_acquire(aio_context);
> > > +
> > > +    if (has_granularity) {
> > > +        if (granularity < 512 || !is_power_of_2(granularity)) {
> > > +            error_setg(errp, "Granularity must be power of 2 "
> > > +                             "and at least 512");
> > > +            goto out;
> > > +        }
> > > +    } else {
> > > +        /* Default to cluster size, if available: */
> > > +        granularity = bdrv_get_default_bitmap_granularity(bs);
> > > +    }
> > > +
> > > +    bdrv_create_dirty_bitmap(bs, granularity, name, errp);
> > > +
> > > + out:
> > > +    aio_context_release(aio_context);
> > > +}
> > > +
> > > +void qmp_block_dirty_bitmap_remove(const char *node, const char *name,
> > > +                                   Error **errp)
> > > +{
> > > +    AioContext *aio_context;
> > > +    BlockDriverState *bs;
> > > +    BdrvDirtyBitmap *bitmap;
> > > +
> > > +    bitmap = block_dirty_bitmap_lookup(node, name, &bs, &aio_context,
> > > errp);
> > > +    if (!bitmap || !bs) {
> >
> > Like here, where aio_context is not released if !bitmap && bs.
> >
> > Max