Re: [Qemu-devel] [PATCH v2] qcow2: Patch for shrinking qcow2 disk image

[Jun Li]

On 05/15/2014 08:36 AM, Max Reitz wrote:
> On 09.05.2014 17:20, Jun Li wrote:
> > As the realization of raw shrinking, so when do qcow2 shrinking, do not
>
> *when doing
>
> > check l1 entries. When resize to size1(size1 < "disk size"), the 
> > custemer
>
> *customer
Sorry ~ :-)
> > knows this will destory the data. So no need to check the l1 entries 
> > which
> > is used or not.
>
> I'd somehow like to disagree, but you're correct. raw-posix truncates 
> the file regardless of whether there is data or not as well. Maybe we 
> should later add support for reporting potential data loss and asking 
> the user what to do (I'm thinking of some "force" or "accept_loss" 
> boolean for bdrv_truncate()).
ok, I will try to realize it in the update version of patch.
> > This is v2 of the original patch. thx.
>
> This should not be part of the commit message, but rather follow the 
> "---" under your Signed-off-by.
sorry, thx, :-)
> > Signed-off-by: Jun Li <address@hidden>
> > ---
> >   block/qcow2-cluster.c  | 17 ++++++++++++-----
> >   block/qcow2-snapshot.c |  2 +-
> >   block/qcow2.c          | 10 ++--------
> >   block/qcow2.h          |  4 ++--
> >   4 files changed, 17 insertions(+), 16 deletions(-)
> >
> > diff --git a/block/qcow2-cluster.c b/block/qcow2-cluster.c
> > index 76d2bcf..8fbbf7f 100644
> > --- a/block/qcow2-cluster.c
> > +++ b/block/qcow2-cluster.c
> > @@ -29,8 +29,8 @@
> >   #include "block/qcow2.h"
> >   #include "trace.h"
> >   -int qcow2_grow_l1_table(BlockDriverState *bs, uint64_t min_size,
> > -                        bool exact_size)
> > +int qcow2_truncate_l1_table(BlockDriverState *bs, uint64_t min_size,
> > +                            bool exact_size)
> >   {
> >       BDRVQcowState *s = bs->opaque;
> >       int new_l1_size2, ret, i;
> > @@ -39,8 +39,9 @@ int qcow2_grow_l1_table(BlockDriverState *bs, 
> > uint64_t min_size,
> >       int64_t new_l1_table_offset, new_l1_size;
> >       uint8_t data[12];
> >   -    if (min_size <= s->l1_size)
> > +    if (min_size == s->l1_size) {
> >           return 0;
> > +    }
> >         /* Do a sanity check on min_size before trying to calculate 
> > new_l1_size
> >        * (this prevents overflows during the while loop for the 
> > calculation of
> > @@ -73,7 +74,13 @@ int qcow2_grow_l1_table(BlockDriverState *bs, 
> > uint64_t min_size,
> >         new_l1_size2 = sizeof(uint64_t) * new_l1_size;
> >       new_l1_table = g_malloc0(align_offset(new_l1_size2, 512));
> > -    memcpy(new_l1_table, s->l1_table, s->l1_size * sizeof(uint64_t));
> > +
> > +    /* shrinking or growing l1 table */
> > +    if (min_size < s->l1_size) {
> > +        memcpy(new_l1_table, s->l1_table, new_l1_size2);
> > +    } else {
> > +        memcpy(new_l1_table, s->l1_table, s->l1_size * 
> > sizeof(uint64_t));
> > +    }
> >         /* write new table (align to cluster) */
> >       BLKDBG_EVENT(bs->file, BLKDBG_L1_GROW_ALLOC_TABLE);
>
> So, as far as I understand the new logic of qcow2_truncate_l1_table(), 
> it will always grow the table unless exact_size == true, in which case 
> it may be shrunk as well. This should probably be reflected in the if 
> condition ("if (exact_size ? min_size == s->l1_size : min_size <= 
> s->l1_size)" or something like that). But on the other hand, I don't 
> like a function which suggests being usable for both shrinking and 
> growing, which then can be used for shrinking only in a special case 
> (exact_size == true). You should at least add a comment which states 
> that this function is generally intended for growing the L1 table with 
> min_size being the new minimal size, but may also be used for 
> shrinking if exact_size is true.
ok, checking...
> Apart from that, if you're shrinking the L1 table, you should in my 
> opinion free all clusters referenced from the truncated area. It is 
> true that it's the user's responsibility to make sure no data is lost, 
> but if you just shrink the L1 table without doing anything about the 
> lost data references, clusters will be leaked. This can easily be 
> fixed by qemu-img check, but there are two problems with that: First, 
> data should be leaked only if it cannot be avoided. It can easily be 
> repaired, but unless there are errors during some operation, that 
> should not be necessary. Second, qemu-img check actually does not work 
> for all image sizes that qemu itself supports. This is even more 
> reason to avoid leaks: Normally, it can easily be repaired, but 
> sometimes, it cannot.
ok, thx~ Agree to free all clusters referenced from the truncated area. 
Checking... Will realize this in v3 of patch.
> > @@ -558,7 +565,7 @@ static int get_cluster_table(BlockDriverState 
> > *bs, uint64_t offset,
> >         l1_index = offset >> (s->l2_bits + s->cluster_bits);
> >       if (l1_index >= s->l1_size) {
> > -        ret = qcow2_grow_l1_table(bs, l1_index + 1, false);
> > +        ret = qcow2_truncate_l1_table(bs, l1_index + 1, false);
> >           if (ret < 0) {
> >               return ret;
> >           }
> > diff --git a/block/qcow2-snapshot.c b/block/qcow2-snapshot.c
> > index 0aa9def..6ba460e 100644
> > --- a/block/qcow2-snapshot.c
> > +++ b/block/qcow2-snapshot.c
> > @@ -483,7 +483,7 @@ int qcow2_snapshot_goto(BlockDriverState *bs, 
> > const char *snapshot_id)
> >        * L1 table of the snapshot. If the snapshot L1 table is 
> > smaller, the
> >        * current one must be padded with zeros.
> >        */
> > -    ret = qcow2_grow_l1_table(bs, sn->l1_size, true);
> > +    ret = qcow2_truncate_l1_table(bs, sn->l1_size, true);
> >       if (ret < 0) {
> >           goto fail;
> >       }
> > diff --git a/block/qcow2.c b/block/qcow2.c
> > index a4b97e8..70f951c 100644
> > --- a/block/qcow2.c
> > +++ b/block/qcow2.c
> > @@ -1877,7 +1877,7 @@ static int qcow2_truncate(BlockDriverState *bs, 
> > int64_t offset)
> >       int64_t new_l1_size;
> >       int ret;
> >   -    if (offset & 511) {
> > +    if (offset <= 0 || offset & 511) {
> >           error_report("The new size must be a multiple of 512");
> >           return -EINVAL;
> >       }
> > @@ -1888,14 +1888,8 @@ static int qcow2_truncate(BlockDriverState 
> > *bs, int64_t offset)
> >           return -ENOTSUP;
> >       }
> >   -    /* shrinking is currently not supported */
> > -    if (offset < bs->total_sectors * 512) {
> > -        error_report("qcow2 doesn't support shrinking images yet");
> > -        return -ENOTSUP;
> > -    }
> > -
> >       new_l1_size = size_to_l1(s, offset);
> > -    ret = qcow2_grow_l1_table(bs, new_l1_size, true);
> > +    ret = qcow2_truncate_l1_table(bs, new_l1_size, true);
> >       if (ret < 0) {
> >           return ret;
> >       }
> > diff --git a/block/qcow2.h b/block/qcow2.h
> > index b49424b..fa36930 100644
> > --- a/block/qcow2.h
> > +++ b/block/qcow2.h
> > @@ -499,8 +499,8 @@ int 
> > qcow2_pre_write_overlap_check(BlockDriverState *bs, int ign, int64_t 
> > offset,
> >                                     int64_t size);
> >     /* qcow2-cluster.c functions */
> > -int qcow2_grow_l1_table(BlockDriverState *bs, uint64_t min_size,
> > -                        bool exact_size);
> > +int qcow2_truncate_l1_table(BlockDriverState *bs, uint64_t min_size,
> > +                            bool exact_size);
> >   int qcow2_write_l1_entry(BlockDriverState *bs, int l1_index);
> >   void qcow2_l2_cache_reset(BlockDriverState *bs);
> >   int qcow2_decompress_cluster(BlockDriverState *bs, uint64_t 
> > cluster_offset);
>
> The rest of the code looks surprisingly simple, but I looks correct to 
> me.
>
> I'd really like a test case for qemu-iotests, though. :-)
ok, I will try.

Best Regards,
Jun Li