diff mbox

[14/16] block/parallels: introduce ParallelsSnapshot data structure

Message ID 1418632081-20667-15-git-send-email-den@openvz.org
State New
Headers show

Commit Message

Denis V. Lunev Dec. 15, 2014, 8:27 a.m. UTC 
In order to support snapshots of parallels images we should maintain
snapshots list in BDRVParallelsState. Snapshots actually forms tree.
Though, in read-only case, we do need to traverse only from current
snapshot leaf to the snapshot tree root. Thus interesting for us
snapshots forms old good linked list.

This patch just introduces the structure for this and fills it with
a signle image as was done before.

True parsing be done in the next patch.

Signed-off-by: Denis V. Lunev <den@openvz.org>
CC: Jeff Cody <jcody@redhat.com>
CC: Kevin Wolf <kwolf@redhat.com>
CC: Stefan Hajnoczi <stefanha@redhat.com>
---
 block/parallels.c | 110 ++++++++++++++++++++++++++++++++++++------------------
 1 file changed, 73 insertions(+), 37 deletions(-)

Comments

Kevin Wolf Dec. 15, 2014, 12:45 p.m. UTC | #1 
Am 15.12.2014 um 09:27 hat Denis V. Lunev geschrieben:
> In order to support snapshots of parallels images we should maintain
> snapshots list in BDRVParallelsState. Snapshots actually forms tree.
> Though, in read-only case, we do need to traverse only from current
> snapshot leaf to the snapshot tree root. Thus interesting for us
> snapshots forms old good linked list.
> 
> This patch just introduces the structure for this and fills it with
> a signle image as was done before.

s/signle/single/

> True parsing be done in the next patch.
> 
> Signed-off-by: Denis V. Lunev <den@openvz.org>
> CC: Jeff Cody <jcody@redhat.com>
> CC: Kevin Wolf <kwolf@redhat.com>
> CC: Stefan Hajnoczi <stefanha@redhat.com>

If I understand correctly, this is what actually describes the backing
file relationship. We should probably use the normal infrastructure for
this.

The challenge here seems to be that the single descriptor XML file
describes the complete chain of backing files. This is different from
the image formats that we support until now.

I think we need some design discussion here first before we even look at
code. Did you consider making the snapshots regular backing files, and
if so, what were the reasons that let you prefer a purely internal
solution?

Kevin
Denis V. Lunev Dec. 15, 2014, 1:32 p.m. UTC | #2 
On 15/12/14 15:45, Kevin Wolf wrote:
> Am 15.12.2014 um 09:27 hat Denis V. Lunev geschrieben:
>> In order to support snapshots of parallels images we should maintain
>> snapshots list in BDRVParallelsState. Snapshots actually forms tree.
>> Though, in read-only case, we do need to traverse only from current
>> snapshot leaf to the snapshot tree root. Thus interesting for us
>> snapshots forms old good linked list.
>>
>> This patch just introduces the structure for this and fills it with
>> a signle image as was done before.
> s/signle/single/
>
>> True parsing be done in the next patch.
>>
>> Signed-off-by: Denis V. Lunev <den@openvz.org>
>> CC: Jeff Cody <jcody@redhat.com>
>> CC: Kevin Wolf <kwolf@redhat.com>
>> CC: Stefan Hajnoczi <stefanha@redhat.com>
> If I understand correctly, this is what actually describes the backing
> file relationship. We should probably use the normal infrastructure for
> this.
>
> The challenge here seems to be that the single descriptor XML file
> describes the complete chain of backing files. This is different from
> the image formats that we support until now.
>
> I think we need some design discussion here first before we even look at
> code. Did you consider making the snapshots regular backing files, and
> if so, what were the reasons that let you prefer a purely internal
> solution?
>
> Kevin

This implementation is borrowed from the current VMDK support.
The idea is exactly the same, see below. I have taken this as
a source of architecture approach as format is quite similar.

Anyway, I am very open to a discussion and solid architecture
approach here would be very good.

Regards,
     Den

static int vmdk_probe(const uint8_t *buf, int buf_size, const char 
*filename)
{
     magic = be32_to_cpu(*(uint32_t *)buf);
     if (magic == VMDK3_MAGIC ||
         magic == VMDK4_MAGIC) {
         return 100;
     } else {
         /* test descriptor parsing */
     }
}


static int vmdk_open(BlockDriverState *bs, QDict *options, int flags,
                      Error **errp)
{
     char *buf;
     int ret;
     BDRVVmdkState *s = bs->opaque;
     uint32_t magic;

     buf = vmdk_read_desc(bs->file, 0, errp);
     if (!buf) {
         return -EINVAL;
     }

     magic = ldl_be_p(buf);
     switch (magic) {
         case VMDK3_MAGIC:
         case VMDK4_MAGIC:
             ret = vmdk_open_sparse(bs, bs->file, flags, buf, errp);
             s->desc_offset = 0x200;
             break;
         default:
             ret = vmdk_open_desc_file(bs, flags, buf, errp);
             break;
     }
     if (ret) {
         goto fail;
     }
     ...


static int vmdk_open_desc_file(BlockDriverState *bs, int flags, char *buf,
                                Error **errp)
{
    ....
    ret = vmdk_parse_extents(buf, bs, bs->file->filename, errp);
    ....
}

static int vmdk_parse_extents(const char *desc, BlockDriverState *bs,
                               const char *desc_file_path, Error **errp)
{
     ....
     while (*p) {
         ....

         path_combine(extent_path, sizeof(extent_path),
                 desc_file_path, fname);
         extent_file = NULL;
         ret = bdrv_open(&extent_file, extent_path, NULL, NULL,
                         bs->open_flags | BDRV_O_PROTOCOL, NULL, errp);
         ...
         /* save to extents array */
}
diff mbox

Patch

diff --git a/block/parallels.c b/block/parallels.c
index 0c0e669..d1b52f3 100644
--- a/block/parallels.c
+++ b/block/parallels.c
@@ -56,15 +56,22 @@  struct parallels_header {
     char padding[12];
 } QEMU_PACKED;
 
-typedef struct BDRVParallelsState {
-    CoMutex lock;
+typedef struct ParallelsSnapshot {
+    BlockDriverState *file;
 
     uint32_t *catalog_bitmap;
     unsigned int catalog_size;
 
     unsigned int tracks;
-
     unsigned int off_multiplier;
+} ParallelsSnapshot;
+
+typedef struct BDRVParallelsState {
+    CoMutex lock;
+
+    ParallelsSnapshot *snaps;
+    unsigned int snaps_count;
+
     unsigned int padding;
 } BDRVParallelsState;
 
@@ -101,49 +108,37 @@  static int parallels_probe(const uint8_t *buf, int buf_size, const char *filenam
     return parallels_probe_xml(buf, buf_size);
 }
 
-static int parallels_open_image(BlockDriverState *bs, Error **errp)
+static int parallels_open_image(ParallelsSnapshot *s, int64_t *total_sectors,
+                                Error **errp)
 {
-    BDRVParallelsState *s = bs->opaque;
     int i;
     struct parallels_header ph;
     int ret;
-    int64_t total_sectors;
-
-    bs->read_only = 1; // no write support yet
 
-    ret = bdrv_pread(bs->file, 0, &ph, sizeof(ph));
+    ret = bdrv_pread(s->file, 0, &ph, sizeof(ph));
     if (ret < 0) {
-        goto fail;
+        return ret;
     }
 
-    total_sectors = le64_to_cpu(ph.nb_sectors);
+    *total_sectors = le64_to_cpu(ph.nb_sectors);
 
     if (le32_to_cpu(ph.version) != HEADER_VERSION) {
         goto fail_format;
     }
     if (!memcmp(ph.magic, HEADER_MAGIC, 16)) {
         s->off_multiplier = 1;
-        total_sectors = 0xffffffff & total_sectors;
+        *total_sectors = 0xffffffff & *total_sectors;
     } else if (!memcmp(ph.magic, HEADER_MAGIC2, 16)) {
         s->off_multiplier = le32_to_cpu(ph.tracks);
     } else {
         goto fail_format;
     }
 
-    if (total_sectors == 0) {
+    if (*total_sectors == 0) {
         error_setg(errp, "Invalid image: zero total sectors");
         ret = -EINVAL;
         goto fail;
     }
-    if (bs->total_sectors == 0) {
-        /* no descriptor file, standalone image opened */
-        bs->total_sectors = total_sectors;
-    }
-    if (bs->total_sectors != total_sectors) {
-        error_setg(errp, "Invalid image: wrong total sectors");
-        ret = -EINVAL;
-        goto fail;
-    }
 
     s->tracks = le32_to_cpu(ph.tracks);
     if (s->tracks == 0) {
@@ -169,7 +164,7 @@  static int parallels_open_image(BlockDriverState *bs, Error **errp)
         goto fail;
     }
 
-    ret = bdrv_pread(bs->file, 64, s->catalog_bitmap, s->catalog_size * 4);
+    ret = bdrv_pread(s->file, 64, s->catalog_bitmap, s->catalog_size * 4);
     if (ret < 0) {
         goto fail;
     }
@@ -177,7 +172,6 @@  static int parallels_open_image(BlockDriverState *bs, Error **errp)
     for (i = 0; i < s->catalog_size; i++)
         le32_to_cpus(&s->catalog_bitmap[i]);
 
-    qemu_co_mutex_init(&s->lock);
     return 0;
 
 fail_format:
@@ -331,19 +325,25 @@  static int parallels_open_xml(BlockDriverState *bs, int flags, Error **errp)
         goto done;
     }
 
+    s->snaps_count = size;
+    s->snaps = g_malloc0(sizeof(ParallelsSnapshot) * s->snaps_count);
+
     path_combine(image_path, sizeof(image_path), bs->file->filename, data);
-    /* the caller (top layer bdrv_open) will close file for us if bs->file
-       is changed. */
-    bs->file = NULL;
 
-    ret = bdrv_open(&bs->file, image_path, NULL, NULL, flags | BDRV_O_PROTOCOL,
-                    NULL, &local_err);
+    ret = bdrv_open(&s->snaps[0].file, image_path, NULL, NULL,
+                    flags | BDRV_O_PROTOCOL, NULL, &local_err);
     if (ret < 0) {
         error_setg_errno(errp, -ret, "Could not open '%s': %s",
                          image_path, error_get_pretty(local_err));
         error_free(local_err);
     } else {
-        ret = parallels_open_image(bs, errp);
+        int64_t total_sectors;
+        ret = parallels_open_image(s->snaps, &total_sectors, errp);
+        if (bs->total_sectors != total_sectors) {
+            error_setg(errp, "Invalid image: wrong total sectors");
+            ret = -EINVAL;
+            goto done;
+        }
     }
 
 done:
@@ -367,17 +367,25 @@  static int parallels_open(BlockDriverState *bs, QDict *options, int flags,
     uint8_t buf[MAX(1024, HEADER_SIZE)];
     int size;
     const struct parallels_header *ph;
+    BDRVParallelsState *s = bs->opaque;
 
     size = bdrv_pread(bs->file, 0, buf, sizeof(buf));
     if (size < 0) {
         return size;
     }
 
+    qemu_co_mutex_init(&s->lock);
+    bs->read_only = 1;
+
     ph = (const struct parallels_header *)buf;
     if ((!memcmp(ph->magic, HEADER_MAGIC, 16) ||
          !memcmp(ph->magic, HEADER_MAGIC2, 16)) &&
         le32_to_cpu(ph->version) == HEADER_VERSION) {
-        return parallels_open_image(bs, errp);
+        s->snaps_count = 1;
+        s->snaps = g_malloc0(sizeof(ParallelsSnapshot));
+        s->snaps->file = bs->file;
+        bdrv_ref(bs->file);
+        return parallels_open_image(s->snaps, &bs->total_sectors, errp);
     } else if (parallels_probe_xml(buf, (unsigned)size)) {
 #if CONFIG_LIBXML2
         return parallels_open_xml(bs, flags, errp);
@@ -389,10 +397,14 @@  static int parallels_open(BlockDriverState *bs, QDict *options, int flags,
 }
 
 
-static int64_t seek_to_sector(BDRVParallelsState *s, int64_t sector_num)
+static int64_t seek_to_sector(ParallelsSnapshot *s, int64_t sector_num)
 {
     uint32_t index, offset;
 
+    if (s == NULL) {
+        return -1;
+    }
+
     index = sector_num / s->tracks;
     offset = sector_num % s->tracks;
 
@@ -405,10 +417,27 @@  static int64_t seek_to_sector(BDRVParallelsState *s, int64_t sector_num)
 static int cluster_remainder(BDRVParallelsState *s, int64_t sector_num,
         int nb_sectors)
 {
-    int ret = s->tracks - sector_num % s->tracks;
+    int ret = s->snaps->tracks - sector_num % s->snaps->tracks;
     return MIN(nb_sectors, ret);
 }
 
+static ParallelsSnapshot *find_snap(BDRVParallelsState *s, int64_t sector_num)
+{
+    int snap_idx;
+    uint32_t index = sector_num / s->snaps->tracks;
+
+    for (snap_idx = 0; snap_idx < s->snaps_count; snap_idx++) {
+        ParallelsSnapshot *snap = s->snaps + snap_idx;
+
+        if (index >= snap->catalog_size || snap->catalog_bitmap[index] == 0) {
+            continue;
+        }
+        return snap;
+    }
+
+    return NULL;
+}
+
 static int64_t coroutine_fn parallels_co_get_block_status(BlockDriverState *bs,
         int64_t sector_num, int nb_sectors, int *pnum)
 {
@@ -416,7 +445,7 @@  static int64_t coroutine_fn parallels_co_get_block_status(BlockDriverState *bs,
     int64_t offset;
 
     qemu_co_mutex_lock(&s->lock);
-    offset = seek_to_sector(s, sector_num);
+    offset = seek_to_sector(find_snap(s, sector_num), sector_num);
     qemu_co_mutex_unlock(&s->lock);
 
     *pnum = cluster_remainder(s, sector_num, nb_sectors);
@@ -437,10 +466,11 @@  static int parallels_read(BlockDriverState *bs, int64_t sector_num,
     sector_num += s->padding;
 
     while (nb_sectors > 0) {
-        int64_t position = seek_to_sector(s, sector_num);
+        ParallelsSnapshot *snap = find_snap(s, sector_num);
+        int64_t position = seek_to_sector(snap, sector_num);
         int n = cluster_remainder(s, sector_num, nb_sectors);
         if (position >= 0) {
-            int ret = bdrv_read(bs->file, position, buf, n);
+            int ret = bdrv_read(snap->file, position, buf, n);
             if (ret < 0) {
                 return ret;
             }
@@ -472,8 +502,14 @@  static coroutine_fn int parallels_co_read(BlockDriverState *bs, int64_t sector_n
 
 static void parallels_close(BlockDriverState *bs)
 {
+    int i;
     BDRVParallelsState *s = bs->opaque;
-    g_free(s->catalog_bitmap);
+
+    for (i = 0; i < s->snaps_count; i++) {
+        ParallelsSnapshot *snap = s->snaps + i;
+        g_free(snap->catalog_bitmap);
+        bdrv_unref(snap->file);
+    }
 }
 
 static BlockDriver bdrv_parallels = {