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@xxxxxxxxx>
> > ---
> > 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
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
