Re: [Qemu-devel] [PATCH 04/10] qcow2: Return 0/-errno in qcow2_alloc_cluster_offset

2010-01-19 Thread Kevin Wolf
Am 19.01.2010 12:35, schrieb Christoph Hellwig:
 @@ -715,6 +721,7 @@ uint64_t qcow2_alloc_cluster_offset(BlockDriverState *bs,
  
  cluster_offset = ~QCOW_OFLAG_COPIED;
  m-nb_clusters = 0;
 +m-depends_on = NULL;
 
 What does this have to do with the rest?

It's needed to be able to distinguish between the case where the
clusters are already allocated (0/NULL) and the case where the request
depends on another one (0/non-NULL). This check previously used the
return value (cluster_offset for success, 0 for failure) and I didn't
want to overload m-cluster_offset with such a meaning. This is the
change in the caller:

   /* Need to wait for another request? If so, we are done for now. */
-  if (!acb-cluster_offset  acb-l2meta.depends_on != NULL) {
+  if (acb-l2meta.nb_clusters == 0  acb-l2meta.depends_on != NULL) {

The alternative would have been to keep using the return value and
hijack some errno value. This would possibly conflict with real
read/write errors though, so I decided to leave the return value alone.

Kevin




Re: [Qemu-devel] [PATCH 04/10] qcow2: Return 0/-errno in qcow2_alloc_cluster_offset

2010-01-19 Thread Christoph Hellwig
On Tue, Jan 19, 2010 at 12:57:35PM +0100, Kevin Wolf wrote:
 It's needed to be able to distinguish between the case where the
 clusters are already allocated (0/NULL) and the case where the request
 depends on another one (0/non-NULL). This check previously used the
 return value (cluster_offset for success, 0 for failure) and I didn't
 want to overload m-cluster_offset with such a meaning. This is the
 change in the caller:
 
/* Need to wait for another request? If so, we are done for now. */
 -  if (!acb-cluster_offset  acb-l2meta.depends_on != NULL) {
 +  if (acb-l2meta.nb_clusters == 0  acb-l2meta.depends_on != NULL) {
 
 The alternative would have been to keep using the return value and
 hijack some errno value. This would possibly conflict with real
 read/write errors though, so I decided to leave the return value alone.

Ok, makes sense.





Re: [Qemu-devel] [PATCH 04/10] qcow2: Return 0/-errno in qcow2_alloc_cluster_offset

2010-01-19 Thread Christoph Hellwig
 @@ -715,6 +721,7 @@ uint64_t qcow2_alloc_cluster_offset(BlockDriverState *bs,
  
  cluster_offset = ~QCOW_OFLAG_COPIED;
  m-nb_clusters = 0;
 +m-depends_on = NULL;

What does this have to do with the rest?

Otherwise looks good,


Reviewed-by: Christoph Hellwig h...@lst.de




[Qemu-devel] [PATCH 04/10] qcow2: Return 0/-errno in qcow2_alloc_cluster_offset

2010-01-18 Thread Kevin Wolf
Returning 0/-errno allows it to distingush different errors classes. The
cluster offset of newly allocated clusters is now returned in the QCowL2Meta
struct.

Signed-off-by: Kevin Wolf kw...@redhat.com
---
 block/qcow2-cluster.c |   28 ++--
 block/qcow2.c |   36 +++-
 block/qcow2.h |4 ++--
 3 files changed, 39 insertions(+), 29 deletions(-)

diff --git a/block/qcow2-cluster.c b/block/qcow2-cluster.c
index 424bedd..3e877a4 100644
--- a/block/qcow2-cluster.c
+++ b/block/qcow2-cluster.c
@@ -609,12 +609,12 @@ static int write_l2_entries(BDRVQcowState *s, uint64_t 
*l2_table,
 return 0;
 }
 
-int qcow2_alloc_cluster_link_l2(BlockDriverState *bs, uint64_t cluster_offset,
-QCowL2Meta *m)
+int qcow2_alloc_cluster_link_l2(BlockDriverState *bs, QCowL2Meta *m)
 {
 BDRVQcowState *s = bs-opaque;
 int i, j = 0, l2_index, ret;
 uint64_t *old_cluster, start_sect, l2_offset, *l2_table;
+uint64_t cluster_offset = m-cluster_offset;
 
 if (m-nb_clusters == 0)
 return 0;
@@ -675,16 +675,22 @@ err:
 /*
  * alloc_cluster_offset
  *
- * For a given offset of the disk image, return cluster offset in
- * qcow2 file.
- *
+ * For a given offset of the disk image, return cluster offset in qcow2 file.
  * If the offset is not found, allocate a new cluster.
  *
- * Return the cluster offset if successful,
- * Return 0, otherwise.
+ * If the cluster was already allocated, m-nb_clusters is set to 0,
+ * m-depends_on is set to NULL and the other fields in m are meaningless.
+ *
+ * If the cluster is newly allocated, m-nb_clusters is set to the number of
+ * contiguous clusters that have been allocated. This may be 0 if the request
+ * conflict with another write request in flight; in this case, m-depends_on
+ * is set and the remaining fields of m are meaningless.
  *
+ * If m-nb_clusters is non-zero, the other fields of m are valid and contain
+ * information about the first allocated cluster.
+ *
+ * Return 0 on success and -errno in error cases
  */
-
 uint64_t qcow2_alloc_cluster_offset(BlockDriverState *bs,
 uint64_t offset,
 int n_start, int n_end,
@@ -698,7 +704,7 @@ uint64_t qcow2_alloc_cluster_offset(BlockDriverState *bs,
 
 ret = get_cluster_table(bs, offset, l2_table, l2_offset, l2_index);
 if (ret  0) {
-return 0;
+return ret;
 }
 
 nb_clusters = size_to_clusters(s, n_end  9);
@@ -715,6 +721,7 @@ uint64_t qcow2_alloc_cluster_offset(BlockDriverState *bs,
 
 cluster_offset = ~QCOW_OFLAG_COPIED;
 m-nb_clusters = 0;
+m-depends_on = NULL;
 
 goto out;
 }
@@ -793,10 +800,11 @@ uint64_t qcow2_alloc_cluster_offset(BlockDriverState *bs,
 
 out:
 m-nb_available = MIN(nb_clusters  (s-cluster_bits - 9), n_end);
+m-cluster_offset = cluster_offset;
 
 *num = m-nb_available - n_start;
 
-return cluster_offset;
+return 0;
 }
 
 static int decompress_buffer(uint8_t *out_buf, int out_buf_size,
diff --git a/block/qcow2.c b/block/qcow2.c
index e06f4dd..773d381 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -561,7 +561,7 @@ static void qcow_aio_write_cb(void *opaque, int ret)
 acb-hd_aiocb = NULL;
 
 if (ret = 0) {
-ret = qcow2_alloc_cluster_link_l2(bs, acb-cluster_offset, 
acb-l2meta);
+ret = qcow2_alloc_cluster_link_l2(bs, acb-l2meta);
 }
 
 run_dependent_requests(acb-l2meta);
@@ -585,21 +585,23 @@ static void qcow_aio_write_cb(void *opaque, int ret)
 n_end  QCOW_MAX_CRYPT_CLUSTERS * s-cluster_sectors)
 n_end = QCOW_MAX_CRYPT_CLUSTERS * s-cluster_sectors;
 
-acb-cluster_offset = qcow2_alloc_cluster_offset(bs, acb-sector_num  9,
-  index_in_cluster,
-  n_end, acb-n, acb-l2meta);
+ret = qcow2_alloc_cluster_offset(bs, acb-sector_num  9,
+index_in_cluster, n_end, acb-n, acb-l2meta);
+if (ret  0) {
+goto done;
+}
+
+acb-cluster_offset = acb-l2meta.cluster_offset;
 
 /* Need to wait for another request? If so, we are done for now. */
-if (!acb-cluster_offset  acb-l2meta.depends_on != NULL) {
+if (acb-l2meta.nb_clusters == 0  acb-l2meta.depends_on != NULL) {
 QLIST_INSERT_HEAD(acb-l2meta.depends_on-dependent_requests,
 acb, next_depend);
 return;
 }
 
-if (!acb-cluster_offset || (acb-cluster_offset  511) != 0) {
-ret = -EIO;
-goto done;
-}
+assert((acb-cluster_offset  511) == 0);
+
 if (s-crypt_method) {
 if (!acb-cluster_data) {
 acb-cluster_data = qemu_mallocz(QCOW_MAX_CRYPT_CLUSTERS *
@@ -782,27 +784,27 @@ static int get_bits_from_size(size_t size)
 static int preallocate(BlockDriverState *bs)
 {
 BDRVQcowState *s = bs-opaque;
-uint64_t cluster_offset = 0;
 uint64_t nb_sectors;
 uint64_t offset;
 int num;