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

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

[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;
+
+	return rfb;
+
+free_rfb:
+	free(rfb);
+	return NULL;
+}
+
 static struct qcow_refcount_block *qcow_read_refcount_block(struct qcow *q, u64 clust_idx)
 {
 	struct qcow_header *header = q->header;
@@ -667,14 +722,11 @@ static struct qcow_refcount_block *qcow_read_refcount_block(struct qcow *q, u64
 
 	rft_idx = clust_idx >> (header->cluster_bits - QCOW_REFCOUNT_BLOCK_SHIFT);
 	if (rft_idx >= rft->rf_size)
-		return NULL;
+		return (void *)-ENOSPC;
 
 	rfb_offset = be64_to_cpu(rft->rf_table[rft_idx]);
-
-	if (!rfb_offset) {
-		pr_warning("Don't support to grow refcount table");
-		return NULL;
-	}
+	if (!rfb_offset)
+		return (void *)-ENOSPC;
 
 	rfb = refcount_block_search(q, rfb_offset);
 	if (rfb)
@@ -708,7 +760,8 @@ static u16 qcow_get_refcount(struct qcow *q, u64 clust_idx)
 	if (!rfb) {
 		pr_warning("Error while reading refcount table");
 		return -1;
-	}
+	} else if ((long)rfb == -ENOSPC)
+		return 0;
 
 	rfb_idx = clust_idx & (((1ULL <<
 		(header->cluster_bits - QCOW_REFCOUNT_BLOCK_SHIFT)) - 1));
@@ -732,6 +785,12 @@ static int update_cluster_refcount(struct qcow *q, u64 clust_idx, u16 append)
 	if (!rfb) {
 		pr_warning("error while reading refcount table");
 		return -1;
+	} else if ((long)rfb == -ENOSPC) {
+		rfb = qcow_grow_refcount_block(q, clust_idx);
+		if (!rfb) {
+			pr_warning("error while growing refcount table");
+			return -1;
+		}
 	}
 
 	rfb_idx = clust_idx & (((1ULL <<
@@ -774,11 +833,11 @@ static void  qcow_free_clusters(struct qcow *q, u64 clust_start, u64 size)
  * can satisfy the size. free_clust_idx is initialized to zero and
  * Record last position.
  */
-static u64 qcow_alloc_clusters(struct qcow *q, u64 size)
+static u64 qcow_alloc_clusters(struct qcow *q, u64 size, int update_ref)
 {
 	struct qcow_header *header = q->header;
 	u16 clust_refcount;
-	u32 clust_idx, i;
+	u32 clust_idx = 0, i;
 	u64 clust_num;
 
 	clust_num = (size + (q->cluster_size - 1)) >> header->cluster_bits;
@@ -793,12 +852,15 @@ again:
 			goto again;
 	}
 
-	for (i = 0; i < clust_num; i++)
-		if (update_cluster_refcount(q,
-			q->free_clust_idx - clust_num + i, 1))
-			return -1;
+	clust_idx++;
+
+	if (update_ref)
+		for (i = 0; i < clust_num; i++)
+			if (update_cluster_refcount(q,
+				clust_idx - clust_num + i, 1))
+				return -1;
 
-	return (q->free_clust_idx - clust_num) << header->cluster_bits;
+	return (clust_idx - clust_num) << header->cluster_bits;
 }
 
 static int qcow_write_l1_table(struct qcow *q)
@@ -848,7 +910,9 @@ static int get_cluster_table(struct qcow *q, u64 offset,
 		if (!l2t)
 			goto error;
 	} else {
-		l2t_new_offset = qcow_alloc_clusters(q, l2t_size*sizeof(u64));
+		l2t_new_offset = qcow_alloc_clusters(q,
+			l2t_size*sizeof(u64), 1);
+
 		if (l2t_new_offset < 0)
 			goto error;
 
@@ -937,7 +1001,7 @@ static ssize_t qcow_write_cluster(struct qcow *q, u64 offset,
 
 	clust_start &= QCOW2_OFFSET_MASK;
 	if (!(clust_flags & QCOW2_OFLAG_COPIED)) {
-		clust_new_start	= qcow_alloc_clusters(q, q->cluster_size);
+		clust_new_start	= qcow_alloc_clusters(q, q->cluster_size, 1);
 		if (clust_new_start < 0) {
 			pr_warning("Cluster alloc error");
 			goto error;
@@ -1143,6 +1207,15 @@ static int qcow_read_refcount_table(struct qcow *q)
 	return pread_in_full(q->fd, rft->rf_table, sizeof(u64) * rft->rf_size, header->refcount_table_offset);
 }
 
+static int qcow_write_refcount_table(struct qcow *q)
+{
+	struct qcow_header *header = q->header;
+	struct qcow_refcount_table *rft = &q->refcount_table;
+
+	return qcow_pwrite_sync(q->fd, rft->rf_table,
+		rft->rf_size * sizeof(u64), header->refcount_table_offset);
+}
+
 static int qcow_read_l1_table(struct qcow *q)
 {
 	struct qcow_header *header = q->header;
-- 
1.7.6.rc2.8.g28eb

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

[Kevin Wolf]

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.

> +
> +	return rfb;
> +
> +free_rfb:
> +	free(rfb);
> +	return NULL;
> +}
> +
>  static struct qcow_refcount_block *qcow_read_refcount_block(struct qcow *q, u64 clust_idx)
>  {
>  	struct qcow_header *header = q->header;
> @@ -667,14 +722,11 @@ static struct qcow_refcount_block *qcow_read_refcount_block(struct qcow *q, u64
>  
>  	rft_idx = clust_idx >> (header->cluster_bits - QCOW_REFCOUNT_BLOCK_SHIFT);
>  	if (rft_idx >= rft->rf_size)
> -		return NULL;
> +		return (void *)-ENOSPC;

Is this allowed style in kvm-tool? :-/

Kevin

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

[Pekka Enberg]

On Mon, 12 Dec 2011, Kevin Wolf wrote:
>> @@ -667,14 +722,11 @@ static struct qcow_refcount_block *qcow_read_refcount_block(struct qcow *q, u64
>>
>>  	rft_idx = clust_idx >> (header->cluster_bits - QCOW_REFCOUNT_BLOCK_SHIFT);
>>  	if (rft_idx >= rft->rf_size)
>> -		return NULL;
>> +		return (void *)-ENOSPC;
>
> Is this allowed style in kvm-tool? :-/

It needs to use ERR_PTR() and related macros but otherwise I don't see a 
big problem with it.

 			Pekka

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

[Kevin Wolf]

Am 12.12.2011 11:58, schrieb Pekka Enberg:
> On Mon, 12 Dec 2011, Kevin Wolf wrote:
>>> @@ -667,14 +722,11 @@ static struct qcow_refcount_block *qcow_read_refcount_block(struct qcow *q, u64
>>>
>>>  	rft_idx = clust_idx >> (header->cluster_bits - QCOW_REFCOUNT_BLOCK_SHIFT);
>>>  	if (rft_idx >= rft->rf_size)
>>> -		return NULL;
>>> +		return (void *)-ENOSPC;
>>
>> Is this allowed style in kvm-tool? :-/
> 
> It needs to use ERR_PTR() and related macros but otherwise I don't see a 
> big problem with it.

Can you be sure that it never clashes with a valid allocation when you
use this in userspace?

But yes, at least using appropriate functions should be required. And
this means that you can't only check for -ENOSPC, but you need to check
for all possible error codes (IS_ERR_VALUE() I guess).

Kevin

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

[Kevin Wolf]

Am 12.12.2011 12:15, schrieb Kevin Wolf:
> Am 12.12.2011 11:58, schrieb Pekka Enberg:
>> On Mon, 12 Dec 2011, Kevin Wolf wrote:
>>>> @@ -667,14 +722,11 @@ static struct qcow_refcount_block *qcow_read_refcount_block(struct qcow *q, u64
>>>>
>>>>  	rft_idx = clust_idx >> (header->cluster_bits - QCOW_REFCOUNT_BLOCK_SHIFT);
>>>>  	if (rft_idx >= rft->rf_size)
>>>> -		return NULL;
>>>> +		return (void *)-ENOSPC;
>>>
>>> Is this allowed style in kvm-tool? :-/
>>
>> It needs to use ERR_PTR() and related macros but otherwise I don't see a 
>> big problem with it.
> 
> Can you be sure that it never clashes with a valid allocation when you
> use this in userspace?

Er, not sure what I was thinking, but it's the top 4k of the address
space, which should be kernel memory. So I think you actually can be sure.

Kevin

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

[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().
> > +
> > +	return rfb;
> > +
> > +free_rfb:
> > +	free(rfb);
> > +	return NULL;
> > +}
> > +
> >  static struct qcow_refcount_block *qcow_read_refcount_block(struct qcow *q, u64 clust_idx)
> >  {
> >  	struct qcow_header *header = q->header;
> > @@ -667,14 +722,11 @@ static struct qcow_refcount_block *qcow_read_refcount_block(struct qcow *q, u64
> >  
> >  	rft_idx = clust_idx >> (header->cluster_bits - QCOW_REFCOUNT_BLOCK_SHIFT);
> >  	if (rft_idx >= rft->rf_size)
> > -		return NULL;
> > +		return (void *)-ENOSPC;
> 
> Is this allowed style in kvm-tool? :-/
> 
> Kevin

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

[lan,Tianyu]

On 一, 2011-12-12 at 18:58 +0800, Pekka Enberg wrote:
> On Mon, 12 Dec 2011, Kevin Wolf wrote:
> >> @@ -667,14 +722,11 @@ static struct qcow_refcount_block *qcow_read_refcount_block(struct qcow *q, u64
> >>
> >>  	rft_idx = clust_idx >> (header->cluster_bits - QCOW_REFCOUNT_BLOCK_SHIFT);
> >>  	if (rft_idx >= rft->rf_size)
> >> -		return NULL;
> >> +		return (void *)-ENOSPC;
> >
> > Is this allowed style in kvm-tool? :-/
> 
> It needs to use ERR_PTR() and related macros but otherwise I don't see a 
> big problem with it.
> 
>  			Pekka

I have tried to use ERR_PTR(). But when I included linux/err.h, a
compile error  was following.

  CC       disk/qcow.o
In file included from disk/qcow.c:20:
../../include/linux/err.h:22: error: expected '=', ',', ';', 'asm' or
'__attribute__' before 'ERR_PTR'
../../include/linux/err.h:27: error: expected '=', ',', ';', 'asm' or
'__attribute__' before 'PTR_ERR'
../../include/linux/err.h:32: error: expected '=', ',', ';', 'asm' or
'__attribute__' before 'IS_ERR'
../../include/linux/err.h:37: error: expected '=', ',', ';', 'asm' or
'__attribute__' before 'IS_ERR_OR_NULL'
../../include/linux/err.h:49: error: expected '=', ',', ';', 'asm' or
'__attribute__' before 'ERR_CAST'
../../include/linux/err.h:55: error: expected '=', ',', ';', 'asm' or
'__attribute__' before 'PTR_RET'


in the linux/err.h

static inline void * __must_check ERR_PTR(long error)
{
	return (void *) error;
}

when I comment "__must_check", the error disappear.

Finally I did cast myself rather than use ERR_PTR().
Are there some substitutions of ERR_PTR in the user space or choices?
 

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

[lan,Tianyu]

On 一, 2011-12-12 at 19:15 +0800, Kevin Wolf wrote:
> Am 12.12.2011 11:58, schrieb Pekka Enberg:
> > On Mon, 12 Dec 2011, Kevin Wolf wrote:
> >>> @@ -667,14 +722,11 @@ static struct qcow_refcount_block *qcow_read_refcount_block(struct qcow *q, u64
> >>>
> >>>  	rft_idx = clust_idx >> (header->cluster_bits - QCOW_REFCOUNT_BLOCK_SHIFT);
> >>>  	if (rft_idx >= rft->rf_size)
> >>> -		return NULL;
> >>> +		return (void *)-ENOSPC;
> >>
> >> Is this allowed style in kvm-tool? :-/
> > 
> > It needs to use ERR_PTR() and related macros but otherwise I don't see a 
> > big problem with it.
> 
> Can you be sure that it never clashes with a valid allocation when you
> use this in userspace?
> 
> But yes, at least using appropriate functions should be required. And
> this means that you can't only check for -ENOSPC, but you need to check
> for all possible error codes (IS_ERR_VALUE() I guess).
The qcow_read_refcount_block() is invoiked in the two places
qcow_get_refcount() and update_cluster_refcount() and will only return
NULL or -ENOSPC.

In the qcow_get_refcount(), when qcow_read_refcount_block() returned
-ENOSPEC(no refcount block is available.), returning zero which means
the cluster is not in use and the refcount block can be grew in the
update_cluster_refcount().  

In the update_cluster_refcount(), when qcow_read_refcount_block()
returned -ENOSPEC, it is necessary to grow the refcount blocks.

So the ENOSPEC should be specially dealt with.  

Does this make sense? :)
> Kevin

Thanks
Tianyu Lan.

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

[Kevin Wolf]

Am 13.12.2011 04:41, schrieb lan,Tianyu:
> On 一, 2011-12-12 at 19:15 +0800, Kevin Wolf wrote:
>> Am 12.12.2011 11:58, schrieb Pekka Enberg:
>>> On Mon, 12 Dec 2011, Kevin Wolf wrote:
>>>>> @@ -667,14 +722,11 @@ static struct qcow_refcount_block *qcow_read_refcount_block(struct qcow *q, u64
>>>>>
>>>>>  	rft_idx = clust_idx >> (header->cluster_bits - QCOW_REFCOUNT_BLOCK_SHIFT);
>>>>>  	if (rft_idx >= rft->rf_size)
>>>>> -		return NULL;
>>>>> +		return (void *)-ENOSPC;
>>>>
>>>> Is this allowed style in kvm-tool? :-/
>>>
>>> It needs to use ERR_PTR() and related macros but otherwise I don't see a 
>>> big problem with it.
>>
>> Can you be sure that it never clashes with a valid allocation when you
>> use this in userspace?
>>
>> But yes, at least using appropriate functions should be required. And
>> this means that you can't only check for -ENOSPC, but you need to check
>> for all possible error codes (IS_ERR_VALUE() I guess).
> The qcow_read_refcount_block() is invoiked in the two places
> qcow_get_refcount() and update_cluster_refcount() and will only return
> NULL or -ENOSPC.
> 
> In the qcow_get_refcount(), when qcow_read_refcount_block() returned
> -ENOSPEC(no refcount block is available.), returning zero which means
> the cluster is not in use and the refcount block can be grew in the
> update_cluster_refcount().  
> 
> In the update_cluster_refcount(), when qcow_read_refcount_block()
> returned -ENOSPEC, it is necessary to grow the refcount blocks.
> 
> So the ENOSPEC should be specially dealt with.  
> 
> Does this make sense? :)

I'm not saying that your code won't work today, just that it's brittle.
If someone adds a different error return code somewhere and doesn't
check if all callers properly deal with error codes, it will break.

Kevin

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

[Kevin Wolf]

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

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

[Pekka Enberg]

On Tue, Dec 13, 2011 at 5:16 AM, lan,Tianyu <tianyu.lan@intel.com> wrote:
> I have tried to use ERR_PTR(). But when I included linux/err.h, a
> compile error  was following.
>
>  CC       disk/qcow.o
> In file included from disk/qcow.c:20:
> ../../include/linux/err.h:22: error: expected '=', ',', ';', 'asm' or
> '__attribute__' before 'ERR_PTR'
> ../../include/linux/err.h:27: error: expected '=', ',', ';', 'asm' or
> '__attribute__' before 'PTR_ERR'
> ../../include/linux/err.h:32: error: expected '=', ',', ';', 'asm' or
> '__attribute__' before 'IS_ERR'
> ../../include/linux/err.h:37: error: expected '=', ',', ';', 'asm' or
> '__attribute__' before 'IS_ERR_OR_NULL'
> ../../include/linux/err.h:49: error: expected '=', ',', ';', 'asm' or
> '__attribute__' before 'ERR_CAST'
> ../../include/linux/err.h:55: error: expected '=', ',', ';', 'asm' or
> '__attribute__' before 'PTR_RET'

We need to borrow tools/perf/util/include/linux/compiler.h in
tools/kvm/include/linux.

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

[Pekka Enberg]

On Tue, Dec 13, 2011 at 10:57 AM, Kevin Wolf <kwolf@redhat.com> wrote:
>> So the ENOSPEC should be specially dealt with.
>>
>> Does this make sense? :)
>
> I'm not saying that your code won't work today, just that it's brittle.
> If someone adds a different error return code somewhere and doesn't
> check if all callers properly deal with error codes, it will break.

Indeed.

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

[lan,Tianyu]

oh，yeah. Thanks for your reply. It's a good lesson for me. :)
I will update soon.
On 二, 2011-12-13 at 16:57 +0800, Kevin Wolf wrote:
> Am 13.12.2011 04:41, schrieb lan,Tianyu:
> > On 一, 2011-12-12 at 19:15 +0800, Kevin Wolf wrote:
> >> Am 12.12.2011 11:58, schrieb Pekka Enberg:
> >>> On Mon, 12 Dec 2011, Kevin Wolf wrote:
> >>>>> @@ -667,14 +722,11 @@ static struct qcow_refcount_block *qcow_read_refcount_block(struct qcow *q, u64
> >>>>>
> >>>>>  	rft_idx = clust_idx >> (header->cluster_bits - QCOW_REFCOUNT_BLOCK_SHIFT);
> >>>>>  	if (rft_idx >= rft->rf_size)
> >>>>> -		return NULL;
> >>>>> +		return (void *)-ENOSPC;
> >>>>
> >>>> Is this allowed style in kvm-tool? :-/
> >>>
> >>> It needs to use ERR_PTR() and related macros but otherwise I don't see a 
> >>> big problem with it.
> >>
> >> Can you be sure that it never clashes with a valid allocation when you
> >> use this in userspace?
> >>
> >> But yes, at least using appropriate functions should be required. And
> >> this means that you can't only check for -ENOSPC, but you need to check
> >> for all possible error codes (IS_ERR_VALUE() I guess).
> > The qcow_read_refcount_block() is invoiked in the two places
> > qcow_get_refcount() and update_cluster_refcount() and will only return
> > NULL or -ENOSPC.
> > 
> > In the qcow_get_refcount(), when qcow_read_refcount_block() returned
> > -ENOSPEC(no refcount block is available.), returning zero which means
> > the cluster is not in use and the refcount block can be grew in the
> > update_cluster_refcount().  
> > 
> > In the update_cluster_refcount(), when qcow_read_refcount_block()
> > returned -ENOSPEC, it is necessary to grow the refcount blocks.
> > 
> > So the ENOSPEC should be specially dealt with.  
> > 
> > Does this make sense? :)
> 
> I'm not saying that your code won't work today, just that it's brittle.
> If someone adds a different error return code somewhere and doesn't
> check if all callers properly deal with error codes, it will break.
> 
> Kevin

Thanks
Tianyu Lan