[ 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 <kw...@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
