Re: [Qemu-devel] [PATCH 1/2] qcow2: add overlap check for bitmap directory

[Vladimir Sementsov-Ogievskiy]

02.02.2018 16:00, Max Reitz wrote:
> On 2018-02-02 13:07, Vladimir Sementsov-Ogievskiy wrote:
> > 29.01.2018 18:34, Max Reitz wrote:
> > > On 2017-11-30 17:47, Vladimir Sementsov-Ogievskiy wrote:
> > > > Signed-off-by: Vladimir Sementsov-Ogievskiy <address@hidden>
> > > > ---
> > > >    block/qcow2.h          |  7 +++++--
> > > >    block/qcow2-refcount.c | 12 ++++++++++++
> > > >    block/qcow2.c          |  6 ++++++
> > > >    3 files changed, 23 insertions(+), 2 deletions(-)
> > > >
> > > > diff --git a/block/qcow2.h b/block/qcow2.h
> > > > index 6f0ff15dd0..8f226a3609 100644
> > > > --- a/block/qcow2.h
> > > > +++ b/block/qcow2.h
> > > > @@ -98,6 +98,7 @@
> > > >    #define QCOW2_OPT_OVERLAP_SNAPSHOT_TABLE
> > > > "overlap-check.snapshot-table"
> > > >    #define QCOW2_OPT_OVERLAP_INACTIVE_L1 "overlap-check.inactive-l1"
> > > >    #define QCOW2_OPT_OVERLAP_INACTIVE_L2 "overlap-check.inactive-l2"
> > > > +#define QCOW2_OPT_OVERLAP_BITMAP_DIRECTORY
> > > > "overlap-check.bitmap-directory"
> > > >    #define QCOW2_OPT_CACHE_SIZE "cache-size"
> > > >    #define QCOW2_OPT_L2_CACHE_SIZE "l2-cache-size"
> > > >    #define QCOW2_OPT_REFCOUNT_CACHE_SIZE "refcount-cache-size"
> > > > @@ -406,8 +407,9 @@ typedef enum QCow2MetadataOverlap {
> > > >        QCOW2_OL_SNAPSHOT_TABLE_BITNR = 5,
> > > >        QCOW2_OL_INACTIVE_L1_BITNR    = 6,
> > > >        QCOW2_OL_INACTIVE_L2_BITNR    = 7,
> > > > +    QCOW2_OL_BITMAP_DIRECTORY_BITNR = 8,
> > > >    -    QCOW2_OL_MAX_BITNR            = 8,
> > > > +    QCOW2_OL_MAX_BITNR              = 9,
> > > >          QCOW2_OL_NONE           = 0,
> > > >        QCOW2_OL_MAIN_HEADER    = (1 << QCOW2_OL_MAIN_HEADER_BITNR),
> > > > @@ -420,12 +422,13 @@ typedef enum QCow2MetadataOverlap {
> > > >        /* NOTE: Checking overlaps with inactive L2 tables will result
> > > > in bdrv
> > > >         * reads. */
> > > >        QCOW2_OL_INACTIVE_L2    = (1 << QCOW2_OL_INACTIVE_L2_BITNR),
> > > > +    QCOW2_OL_BITMAP_DIRECTORY = (1 << QCOW2_OL_BITMAP_DIRECTORY_BITNR),
> > > >    } QCow2MetadataOverlap;
> > > >      /* Perform all overlap checks which can be done in constant time */
> > > >    #define QCOW2_OL_CONSTANT \
> > > >        (QCOW2_OL_MAIN_HEADER | QCOW2_OL_ACTIVE_L1 |
> > > > QCOW2_OL_REFCOUNT_TABLE | \
> > > > -     QCOW2_OL_SNAPSHOT_TABLE)
> > > > +     QCOW2_OL_SNAPSHOT_TABLE | QCOW2_OL_BITMAP_DIRECTORY)
> > > >      /* Perform all overlap checks which don't require disk access */
> > > >    #define QCOW2_OL_CACHED \
> > > > diff --git a/block/qcow2-refcount.c b/block/qcow2-refcount.c
> > > > index 3de1ab51ba..a7a2703f26 100644
> > > > --- a/block/qcow2-refcount.c
> > > > +++ b/block/qcow2-refcount.c
> > > > @@ -2585,6 +2585,18 @@ int
> > > > qcow2_check_metadata_overlap(BlockDriverState *bs, int ign, int64_t
> > > > offset,
> > > >            }
> > > >        }
> > > >    +    if ((chk & QCOW2_OL_BITMAP_DIRECTORY) &&
> > > > +        (s->autoclear_features & QCOW2_AUTOCLEAR_BITMAPS))
> > > > +    {
> > > > +        /* update_ext_header_and_dir_in_place firstly drop autoclear
> > > > flag,
> > > > +         * so it will not fail */
> > > That's really not an argument.  bitmap_list_store() has to pass
> > > QCOW2_OL_BITMAP_DIRECTORY to @ign anyway.  (Because there is no reason
> > > not to.)
> > in_place is a reason. When we store directory in_place, it definitely
> > overlaps with current directory.
> Well, then you just pass QCOW2_OL_BITMAP_DIRECTORY to @ign, which is
> what that argument is for? :-)

hmm. but actually, I should not, because of zeroed autoclear flag. So,
do you think, it is better to pass it, anyway?

> Max
>
> > But this is done with cleared autoclear flag (to make it safe), so we
> > will skip this check and will not
> > fail.


--
Best regards,
Vladimir