Re: [Qemu-devel] [PATCH 6/8] qcow2: add autoclear bit for dirty bitmaps

[Kevin Wolf]

Am 11.06.2015 um 01:42 hat John Snow geschrieben:
> 
> 
> On 06/08/2015 11:21 AM, Vladimir Sementsov-Ogievskiy wrote:
> > From: Vladimir Sementsov-Ogievskiy <address@hidden>
> > 
> > Signed-off-by: Vladimir Sementsov-Ogievskiy <address@hidden>
> > ---
> >  block/qcow2-dirty-bitmap.c |  5 +++++
> >  block/qcow2.c              | 13 +++++++++++--
> >  block/qcow2.h              |  9 +++++++++
> >  3 files changed, 25 insertions(+), 2 deletions(-)
> > 
> > diff --git a/block/qcow2-dirty-bitmap.c b/block/qcow2-dirty-bitmap.c
> > index db83112..686a121 100644
> > --- a/block/qcow2-dirty-bitmap.c
> > +++ b/block/qcow2-dirty-bitmap.c
> > @@ -188,6 +188,11 @@ static int qcow2_write_dirty_bitmaps(BlockDriverState 
> > *bs)
> >  
> >      s->dirty_bitmaps_offset = dirty_bitmaps_offset;
> >      s->dirty_bitmaps_size = dirty_bitmaps_size;
> > +    if (s->nb_dirty_bitmaps > 0) {
> > +        s->autoclear_features |= QCOW2_AUTOCLEAR_DIRTY_BITMAPS;
> > +    } else {
> > +        s->autoclear_features &= ~QCOW2_AUTOCLEAR_DIRTY_BITMAPS;
> > +    }
> >      ret = qcow2_update_header(bs);
> >      if (ret < 0) {
> >          fprintf(stderr, "Could not update qcow2 header\n");
> > diff --git a/block/qcow2.c b/block/qcow2.c
> > index 406e55d..f85a55a 100644
> > --- a/block/qcow2.c
> > +++ b/block/qcow2.c
> > @@ -182,6 +182,14 @@ static int qcow2_read_extensions(BlockDriverState *bs, 
> > uint64_t start_offset,
> >                  return ret;
> >              }
> >  
> > +            if (!(s->autoclear_features & QCOW2_AUTOCLEAR_DIRTY_BITMAPS) &&
> > +                s->nb_dirty_bitmaps > 0) {
> > +                ret = qcow2_delete_all_dirty_bitmaps(bs, errp);
> > +                if (ret < 0) {
> > +                    return ret;
> > +                }
> > +            }
> > +
> >  #ifdef DEBUG_EXT
> >              printf("Qcow2: Got dirty bitmaps extension:"
> >                     " offset=%" PRIu64 " nb_bitmaps=%" PRIu32 "\n",
> > @@ -928,8 +936,9 @@ static int qcow2_open(BlockDriverState *bs, QDict 
> > *options, int flags,
> >      }
> >  
> >      /* Clear unknown autoclear feature bits */
> > -    if (!bs->read_only && !(flags & BDRV_O_INCOMING) && 
> > s->autoclear_features) {
> > -        s->autoclear_features = 0;
> > +    if (!bs->read_only && !(flags & BDRV_O_INCOMING) &&
> > +        (s->autoclear_features & ~QCOW2_AUTOCLEAR_MASK)) {
> > +        s->autoclear_features |= QCOW2_AUTOCLEAR_MASK;
> 
> Like Stefan already mentioned, fixing this |= to &= will fix iotest 036,
> which is otherwise broken by this patch.
> 
> >          ret = qcow2_update_header(bs);
> >          if (ret < 0) {
> >              error_setg_errno(errp, -ret, "Could not update qcow2 header");
> > diff --git a/block/qcow2.h b/block/qcow2.h
> > index b5e576c..14bd6f9 100644
> > --- a/block/qcow2.h
> > +++ b/block/qcow2.h
> > @@ -215,6 +215,15 @@ enum {
> >      QCOW2_COMPAT_FEAT_MASK            = QCOW2_COMPAT_LAZY_REFCOUNTS,
> >  };
> >  
> > +/* Autoclear feature bits */
> > +enum {
> > +    QCOW2_AUTOCLEAR_DIRTY_BITMAPS_BITNR = 0,
> > +    QCOW2_AUTOCLEAR_DIRTY_BITMAPS       =
> > +        1 << QCOW2_AUTOCLEAR_DIRTY_BITMAPS_BITNR,
> > +
> > +    QCOW2_AUTOCLEAR_MASK                = QCOW2_AUTOCLEAR_DIRTY_BITMAPS,
> > +};
> > +
> 
> I find it a little awkward to have an enum with three different kinds of
> data in it, unless I am reading this incorrectly. (bit position, bit
> masks, and accumulated bit mask.)

This is only consistent with the enums for incompatible and compatible
feature flags. If we were to change that, we should change it
everywhere.

> Just enumerating the indices is probably sufficient:
> 
> enum {
>   QCOW2_AUTOCLEAR_BEGIN = 0,
>   QCOW2_AUTOCLEAR_DIRTY_BITMAPS = QCOW2_AUTOCLEAR_BEGIN,
>   ...,
>   QCOW2_AUTOCLEAR_END
> }

I don't mind the colour of the bikeshed, as long as all constants are
explicitly defined. Letting the compiler assign integers when these
integers are part of an external interface is too easy to break
accidentally.

Kevin