[Qemu-devel] [PATCH] vmdk: Only read cid from image file when opening

[Qemu-devel] [PATCH] vmdk: Only read cid from image file when opening

[Fam Zheng]

Previously cid of parent is parsed from image file for every IO request.
We already have L1/L2 cache and don't have assumption that parent image
can be updated behind us, so remove this to get more efficiency.

The parent CID is checked them when opening backing file.

Signed-off-by: Fam Zheng <famz@redhat.com>
---
 block/vmdk.c | 53 ++++++++++++++++++++++-------------------------------
 1 file changed, 22 insertions(+), 31 deletions(-)

diff --git a/block/vmdk.c b/block/vmdk.c
index 90340eb..9611ff3 100644
--- a/block/vmdk.c
+++ b/block/vmdk.c
@@ -112,6 +112,7 @@ typedef struct BDRVVmdkState {
     CoMutex lock;
     uint64_t desc_offset;
     bool cid_updated;
+    uint32_t cid;
     uint32_t parent_cid;
     int num_extents;
     /* Extent array with num_extents entries, ascend ordered by address */
@@ -197,8 +198,6 @@ static int vmdk_probe(const uint8_t *buf, int buf_size, const char *filename)
     }
 }
 
-#define CHECK_CID 1
-
 #define SECTOR_SIZE 512
 #define DESC_SIZE (20 * SECTOR_SIZE)    /* 20 sectors of 512 bytes each */
 #define BUF_SIZE 4096
@@ -233,7 +232,7 @@ static void vmdk_free_last_extent(BlockDriverState *bs)
     s->extents = g_realloc(s->extents, s->num_extents * sizeof(VmdkExtent));
 }
 
-static uint32_t vmdk_read_cid(BlockDriverState *bs, int parent)
+static uint32_t vmdk_read_cid(BlockDriverState *bs, bool parent)
 {
     char desc[DESC_SIZE];
     uint32_t cid = 0xffffffff;
@@ -299,25 +298,6 @@ static int vmdk_write_cid(BlockDriverState *bs, uint32_t cid)
     return 0;
 }
 
-static int vmdk_is_cid_valid(BlockDriverState *bs)
-{
-#ifdef CHECK_CID
-    BDRVVmdkState *s = bs->opaque;
-    BlockDriverState *p_bs = bs->backing_hd;
-    uint32_t cur_pcid;
-
-    if (p_bs) {
-        cur_pcid = vmdk_read_cid(p_bs, 0);
-        if (s->parent_cid != cur_pcid) {
-            /* CID not valid */
-            return 0;
-        }
-    }
-#endif
-    /* CID valid */
-    return 1;
-}
-
 /* Queue extents, if any, for reopen() */
 static int vmdk_reopen_prepare(BDRVReopenState *state,
                                BlockReopenQueue *queue, Error **errp)
@@ -351,11 +331,12 @@ exit:
     return ret;
 }
 
-static int vmdk_parent_open(BlockDriverState *bs)
+static int vmdk_parent_open(BlockDriverState *bs, Error **errp)
 {
     char *p_name;
     char desc[DESC_SIZE + 1];
     BDRVVmdkState *s = bs->opaque;
+    BDRVVmdkState *backing;
     int ret;
 
     desc[DESC_SIZE] = '\0';
@@ -378,6 +359,22 @@ static int vmdk_parent_open(BlockDriverState *bs)
         }
 
         pstrcpy(bs->backing_file, end_name - p_name + 1, p_name);
+
+        ret = bdrv_open_backing_file(bs, NULL, errp);
+
+        if (ret < 0) {
+            error_setg_errno(errp, -ret, "Could not open backing file '%s'",
+                             bs->backing_file);
+            return ret;
+        }
+        assert(bs->backing_hd);
+        if (!strcmp(bs->backing_hd->drv->format_name, "vmdk")) {
+            backing = bs->backing_hd->opaque;
+            if (backing->cid != s->parent_cid) {
+                error_setg(errp, "Parent CID invalid");
+                return -EINVAL;
+            }
+        }
     }
 
     return 0;
@@ -836,10 +833,11 @@ static int vmdk_open(BlockDriverState *bs, QDict *options, int flags,
         }
     }
     /* try to open parent images, if exist */
-    ret = vmdk_parent_open(bs);
+    ret = vmdk_parent_open(bs, errp);
     if (ret) {
         goto fail;
     }
+    s->cid = vmdk_read_cid(bs, 0);
     s->parent_cid = vmdk_read_cid(bs, 1);
     qemu_co_mutex_init(&s->lock);
 
@@ -870,10 +868,6 @@ static int get_whole_cluster(BlockDriverState *bs,
     if (bs->backing_hd) {
         whole_grain =
             qemu_blockalign(bs, extent->cluster_sectors << BDRV_SECTOR_BITS);
-        if (!vmdk_is_cid_valid(bs)) {
-            ret = VMDK_ERROR;
-            goto exit;
-        }
 
         /* floor offset to cluster */
         offset -= offset % (extent->cluster_sectors * 512);
@@ -1238,9 +1232,6 @@ static int vmdk_read(BlockDriverState *bs, int64_t sector_num,
         if (ret != VMDK_OK) {
             /* if not allocated, try to read from parent image, if exist */
             if (bs->backing_hd && ret != VMDK_ZEROED) {
-                if (!vmdk_is_cid_valid(bs)) {
-                    return -EINVAL;
-                }
                 ret = bdrv_read(bs->backing_hd, sector_num, buf, n);
                 if (ret < 0) {
                     return ret;
-- 
1.8.3.1

Re: [Qemu-devel] [PATCH] vmdk: Only read cid from image file when opening

[Stefan Hajnoczi]

On Fri, Oct 11, 2013 at 03:55:39PM +0800, Fam Zheng wrote:
> @@ -378,6 +359,22 @@ static int vmdk_parent_open(BlockDriverState *bs)
>          }
>  
>          pstrcpy(bs->backing_file, end_name - p_name + 1, p_name);
> +
> +        ret = bdrv_open_backing_file(bs, NULL, errp);

This breaks 'backing.' options since we ignore them here.  I suggest
letting the block layer open the backing file as normal instead of
duplicating the code here.

Simply add a bool s->cid_checked field to remember whether the CID
needs to be checked.  Then it will only be done once.