Re: [Qemu-devel] [PATCH v4 2/3] qcow2: Use QcowCache

[Kevin Wolf <kwolf@redhat.com>]

[ Re-adding qemu-devel to CC ]

Am 24.01.2011 15:34, schrieb Stefan Hajnoczi:
> On Thu, Jan 20, 2011 at 5:10 PM, Kevin Wolf <kwolf@redhat.com> wrote:
>> @@ -702,17 +622,30 @@ int qcow2_alloc_cluster_link_l2(BlockDriverState *bs, QCowL2Meta *m)
>>
>>     if (m->nb_available & (s->cluster_sectors - 1)) {
>>         uint64_t end = m->nb_available & ~(uint64_t)(s->cluster_sectors - 1);
>> +        cow = true;
>>         ret = copy_sectors(bs, start_sect + end, cluster_offset + (end << 9),
>>                 m->nb_available - end, s->cluster_sectors);
>>         if (ret < 0)
>>             goto err;
>>     }
>>
>> -    /* update L2 table */
>> +    /*
>> +     * Update L2 table.
>> +     *
>> +     * Before we update the L2 table to actually point to the new cluster, we
>> +     * need to be sure that the refcounts have been increased and COW was
>> +     * handled.
>> +     */
>> +    if (cow) {
>> +        bdrv_flush(bs->file);
> 
> Just bdrv_flush(bs->file) and not a refcounts cache flush?

Refcounts and data need not to be ordered against each other. They only
must both be on disk when we write the L2 table.

>> +    }
>> +
>> +    qcow2_cache_set_dependency(bs, s->l2_table_cache, s->refcount_block_cache);
>>     ret = get_cluster_table(bs, m->offset, &l2_table, &l2_offset, &l2_index);
>>     if (ret < 0) {
>>         goto err;
>>     }
>> +    qcow2_cache_entry_mark_dirty(s->l2_table_cache, l2_table);
>>
>>     for (i = 0; i < m->nb_clusters; i++) {
>>         /* if two concurrent writes happen to the same unallocated cluster
>> @@ -728,16 +661,9 @@ int qcow2_alloc_cluster_link_l2(BlockDriverState *bs, QCowL2Meta *m)
>>                     (i << s->cluster_bits)) | QCOW_OFLAG_COPIED);
>>      }
>>
>> -    /*
>> -     * Before we update the L2 table to actually point to the new cluster, we
>> -     * need to be sure that the refcounts have been increased and COW was
>> -     * handled.
>> -     */
>> -    bdrv_flush(bs->file);
>>
>> -    ret = write_l2_entries(bs, l2_table, l2_offset, l2_index, m->nb_clusters);
>> +    ret = qcow2_cache_put(bs, s->l2_table_cache, (void**) &l2_table);
>>     if (ret < 0) {
>> -        qcow2_l2_cache_reset(bs);
>>         goto err;
>>     }
>>
> 
> The function continues like this:
> 
> /*
>  * If this was a COW, we need to decrease the refcount of the old cluster.
>  * Also flush bs->file to get the right order for L2 and refcount update.
>  */
> if (j != 0) {
>     bdrv_flush(bs->file);
>     for (i = 0; i < j; i++) {
>         qcow2_free_any_clusters(bs,
>             be64_to_cpu(old_cluster[i]) & ~QCOW_OFLAG_COPIED, 1);
>     }
> }
> 
> Does bdrv_flush(bs->file) "get the right order for L2 and refcount
> update"?  We've just put an L2 table, should this be an L2 table
> flush?

I agree, this looks wrong. What we need is a dependency to ensure that
first L2 is flushed and then the refcount block.
qcow2_free_any_clusters() calls update_refcount() indirectly, which
takes care of setting this dependency.

So in the end I think it's just an unnecessary bdrv_flush. Makes sense?

Kevin