Re: [RFC PATCH] kvm tools, qcow: Add support for growing refcount blocks

[Kevin Wolf <kwolf@redhat.com>]

Am 13.12.2011 04:05, schrieb lan,Tianyu:
> Thanks for your review.
> On 一, 2011-12-12 at 17:55 +0800, Kevin Wolf wrote:
>> Am 12.12.2011 03:03, schrieb Lan Tianyu:
>>> This patch enables allocating new refcount blocks and so then kvm tools
>>> could expand qcow2 image much larger.
>>>
>>> Signed-off-by: Lan Tianyu <tianyu.lan@intel.com>
>>> ---
>>>  tools/kvm/disk/qcow.c |  105 +++++++++++++++++++++++++++++++++++++++++-------
>>>  1 files changed, 89 insertions(+), 16 deletions(-)
>>>
>>> diff --git a/tools/kvm/disk/qcow.c b/tools/kvm/disk/qcow.c
>>> index e139fa5..929ba69 100644
>>> --- a/tools/kvm/disk/qcow.c
>>> +++ b/tools/kvm/disk/qcow.c
>>> @@ -12,6 +12,7 @@
>>>  #include <string.h>
>>>  #include <unistd.h>
>>>  #include <fcntl.h>
>>> +#include <errno.h>
>>>  #ifdef CONFIG_HAS_ZLIB
>>>  #include <zlib.h>
>>>  #endif
>>> @@ -20,6 +21,10 @@
>>>  #include <linux/kernel.h>
>>>  #include <linux/types.h>
>>>  
>>> +static int update_cluster_refcount(struct qcow *q, u64 clust_idx, u16 append);
>>> +static int qcow_write_refcount_table(struct qcow *q);
>>> +static u64 qcow_alloc_clusters(struct qcow *q, u64 size, int update_ref);
>>> +static void  qcow_free_clusters(struct qcow *q, u64 clust_start, u64 size);
>>>  
>>>  static inline int qcow_pwrite_sync(int fd,
>>>  	void *buf, size_t count, off_t offset)
>>> @@ -657,6 +662,56 @@ static struct qcow_refcount_block *refcount_block_search(struct qcow *q, u64 off
>>>  	return rfb;
>>>  }
>>>  
>>> +static struct qcow_refcount_block *qcow_grow_refcount_block(struct qcow *q,
>>> +	u64 clust_idx)
>>> +{
>>> +	struct qcow_header *header = q->header;
>>> +	struct qcow_refcount_table *rft = &q->refcount_table;
>>> +	struct qcow_refcount_block *rfb;
>>> +	u64 new_block_offset;
>>> +	u64 rft_idx;
>>> +
>>> +	rft_idx = clust_idx >> (header->cluster_bits -
>>> +		QCOW_REFCOUNT_BLOCK_SHIFT);
>>> +
>>> +	if (rft_idx >= rft->rf_size) {
>>> +		pr_warning("Don't support grow refcount block table");
>>> +		return NULL;
>>> +	}
>>> +
>>> +	new_block_offset = qcow_alloc_clusters(q, q->cluster_size, 0);
>>> +	if (new_block_offset < 0)
>>> +		return NULL;
>>> +
>>> +	rfb = new_refcount_block(q, new_block_offset);
>>> +	if (!rfb)
>>> +		return NULL;
>>> +
>>> +	memset(rfb->entries, 0x00, q->cluster_size);
>>> +	rfb->dirty = 1;
>>> +
>>> +	/* write refcount block */
>>> +	if (write_refcount_block(q, rfb) < 0)
>>> +		goto free_rfb;
>>> +
>>> +	if (cache_refcount_block(q, rfb) < 0)
>>> +		goto free_rfb;
>>> +
>>> +	rft->rf_table[rft_idx] = cpu_to_be64(new_block_offset);
>>> +	if (qcow_write_refcount_table(q) < 0)
>>> +		goto free_rfb;
>>> +
>>> +	if (update_cluster_refcount(q, new_block_offset >>
>>> +		    header->cluster_bits, 1) < 0)
>>> +		goto free_rfb;
>>
>> This order is unsafe, you could end up with a refcount block that is
>> already in use, but still has a refcount of 0.
> How about following?
> rft->rf_table[rft_idx] = cpu_to_be64(new_block_offset);
> 
> if (update_cluster_refcount(q, new_block_offset >>
> 	header->cluster_bits, 1) < 0) {
> 	rft->rf_table[rft_idx] =  0;
> 	goto free_rfb;
> }
> 
> if (qcow_write_refcount_table(q) < 0) {
> 	rft->rf_table[rft_idx] =  0;
> 	qcow_free_clusters(q, new_block_offset, q->cluster_size);
> 	goto free_rfb;
> }
> 
> update_cluster_refcount() will use the rft->rf_table. So updating the rft->rf_table
> must be before update_cluster_refcount().

Yes, something like this should work.

Kevin