Re: [Qemu-devel] [PATCH 2/3] qcow2: Implement bdrv_amend_options

[Max Reitz <mreitz@redhat.com>]

Hi,

Am 28.08.2013 13:06, schrieb Kevin Wolf:
> Am 28.08.2013 um 10:08 hat Max Reitz geschrieben:
>> Implement bdrv_amend_options for compat, size, backing_file, backing_fmt
>> and lazy_refcounts.
>>
>> Downgrading images from compat=1.1 to compat=0.10 is achieved through
>> handling all incompatible flags accordingly, clearing all compatible and
>> autoclear flags and expanding all zero clusters.
>>
>> Signed-off-by: Max Reitz <mreitz@redhat.com>
>> ---
>> +int qcow2_expand_zero_clusters(BlockDriverState *bs)
>> +{
>> +    BDRVQcowState *s = bs->opaque;
>> +    int ret;
>> +    int i;
>> +
>> +    for (i = 0; i < s->l1_size; i++) {
> This fails to expand zero clusters in non-active L2 tables. (Please add
> a test case for this scenario.)
Oh, yes, right.

>> +        uint64_t *l2_table;
>> +        int l2_index;
>> +        int j;
>> +        bool l2_dirty = false;
>> +
>> +        ret = get_cluster_table(bs, (uint64_t)i << (s->l2_bits +
>> +                s->cluster_bits), &l2_table, &l2_index);
>> +        if (ret < 0) {
>> +            return ret;
>> +        }
>> +
>> +        for (j = 0; j < s->l2_size; j++) {
>> +            uint64_t l2_entry = be64_to_cpu(l2_table[j]);
>> +            if (!(l2_entry & QCOW_OFLAG_COMPRESSED) &&
>> +                (l2_entry & QCOW_OFLAG_ZERO)) {
> qcow2_get_cluster_type()?
Sounds like an option to go for.

>> +                /* uncompressed zero cluster */
>> +                int64_t offset = qcow2_alloc_clusters(bs, s->cluster_size);
>> +                if (offset < 0) {
>> +                    ret = offset;
>> +                    goto fail;
>> +                }
> Does it handle zero clusters with an offset (i.e. preallocation)
> correctly? I believe we must either reuse that cluster or free it.
Okay.

>> +                ret = bdrv_write_zeroes(bs->file, offset >> BDRV_SECTOR_BITS,
>> +                                        s->cluster_size >> BDRV_SECTOR_BITS);
>> +                if (ret < 0) {
>> +                    qcow2_free_clusters(bs, offset, s->cluster_size,
>> +                            QCOW2_DISCARD_ALWAYS);
>> +                    goto fail;
>> +                }
>> +
>> +                l2_table[j] = cpu_to_be64(offset | QCOW_OFLAG_COPIED);
>> +                l2_dirty = true;
>> +            }
>> +        }
>> +
>> +        ret = 0;
>> +
>> +fail:
>> +        if (l2_dirty) {
>> +            qcow2_cache_entry_mark_dirty(s->l2_table_cache, l2_table);
> qcow2_cache_depends_on_flush(s->l2_table_cache), too. The L2 table must
> only be written when the zeroes are stable on disk.
Okay.

>> +    /* clear incompatible features */
>> +    if (s->incompatible_features & QCOW2_INCOMPAT_DIRTY) {
>> +        BdrvCheckResult result;
>> +        ret = qcow2_check(bs, &result, BDRV_FIX_ERRORS);
>> +        if (ret < 0) {
>> +            return ret;
>> +        }
> This is unnecessary: The image could be opened, so we know that it was
> clean when we started. We also know that we haven't crashed yet, so if we
> flush all in-memory data, we'll have a consistent on-disk state again.
>
> qcow2_mark_clean() already calls bdrv_flush(bs), so it does everything
> that is needed in this respect.
So that flag should always be already cleared at this point anyway?

>> +        } else if (!strcmp(options[i].name, "encryption")) {
>> +            if (options[i].value.n != !!s->crypt_method) {
>> +                fprintf(stderr, "Changing the encryption flag is not "
>> +                        "supported.\n");
>> +                return -ENOTSUP;
>> +            }
>> +        } else if (!strcmp(options[i].name, "cluster_size")) {
>> +            if (options[i].value.n && (options[i].value.n != s->cluster_size)) {
>> +                fprintf(stderr, "Changing the cluster size is not "
>> +                        "supported.\n");
>> +                return -ENOTSUP;
>> +            }
>> +        } else if (!strcmp(options[i].name, "lazy_refcounts")) {
>> +            /* TODO: detect whether this flag was indeed explicitly given */
>> +            lazy_refcounts = options[i].value.n;
> I can see two ways to achieve this:
>
> 1. Add a new field 'bool assigned' to QEMUOptionParameter, which would
>     be cleared before parsing an option string and set for each option in
>     set_option_parameter()
>
> 2. Get the QemuOpts conversion series in and add a function that tells
>     whether a given option was specified or not.
>
> The same TODO should actually apply to encryption and cluster_size as
> well, shouldn't it?
Kind of; however, a cluster_size of 0 seems invalid to me, therefore it 
is pretty easy to detect that option not being given.

The TODO rather also applies to encryption; however, since the worst it 
does there is generate an error message, it isn't as bad as here (were 
the code might actually change the image if the flag is not given).

>> +        } else {
>> +            fprintf(stderr, "Unknown option '%s'.\n", options[i].name);
> That's actually a programming error, perhaps a case for assert(false);
True.

>> +        }
>> +    }
>> +
>> +    if (new_version != old_version) {
>> +        if (new_version > old_version) {
>> +            /* Upgrade */
>> +            s->qcow_version = new_version;
>> +            ret = qcow2_update_header(bs);
>> +            if (ret < 0) {
>> +                s->qcow_version = old_version;
>> +                return ret;
>> +            }
>> +        } else {
>> +            qcow2_downgrade(bs, new_version);
> Error handling?
Oh, right, forgot it. Sorry.


Max