[Qemu-devel] [PATCH 0/4] export internal snapshot by qemu-nbd

[Qemu-devel] [PATCH 0/4] export internal snapshot by qemu-nbd

[Wenchao Xia]

This series allow user to read internal snapshot's contents without qemu-img
convert. Another purpose is that, when qemu is online and have taken an
internal snapshot, let user invoke qemu-nbd to do any thing on it except write.

This brings two interesting issues:
1 is it safe to let qemu-nbd and qemu access that file at same time?
I think it is safe, since qemu-nbd is read only. The data will be correct from
qemu-nbd, if qemu does not delete that snapshot when qemu-nbd is running, and
data is flushed to storage after qemu take that snapshot so that qemu-nbd
would see the correct data.

2 should an nbd-server exporting internal snapshot be added in qemu?
I think no. Compared with driver-backup, the snapshot, or COW happens
in storage level, so it allows another program to read it itself. Actually
it should be OK to let another server other than qemu's host, do the
export I/O job, if data is flushed.

Next step:
As demonstrated before, an explict API should be added, which make sure
all I/O request is flushed and sent to underlining storage, and cache
is sync if it is writeback type. So at qemu level, we can make sure
no request is left behind, before qemu-nbd start. That API should
also benefit 3rd party block snapshot solution, such as LVM2.

More:
With this patch and previous qcow2 snapshot at block device level, I think
export/import/backup solution around qcow2, is nearly complete at qemu's
level. It can do similar things as backing chain but with better performance,
Some small optimization place are left:

1 compare two snapshot's data to get the diff with help of qcow2's L1/L2 table.
2 convertion between external snapshot and internal snapshot.

This series need following series applied first:
[PATCH V5 0/8] add internal snapshot support at block device level
http://lists.nongnu.org/archive/html/qemu-devel/2013-07/msg01831.html

Wenchao Xia (4):
  1 snapshot: distinguish id and name in load_tmp
  2 qemu-nbd: support internal snapshot export
  3 qemu-nbd: add doc for internal snapshot export
  4 qemu-iotests: add 057 internal snapshot export with qemu-nbd case

 block/qcow2-snapshot.c     |   16 +++++++-
 block/qcow2.h              |    5 ++-
 block/snapshot.c           |   37 ++++++++++++++++++-
 include/block/block_int.h  |    4 ++-
 include/block/snapshot.h   |    4 ++-
 qemu-img.c                 |    7 +++-
 qemu-nbd.c                 |   56 ++++++++++++++++++++++++++++-
 qemu-nbd.texi              |    3 ++
 tests/qemu-iotests/057     |   87 ++++++++++++++++++++++++++++++++++++++++++++
 tests/qemu-iotests/057.out |   26 +++++++++++++
 tests/qemu-iotests/group   |    1 +
 11 files changed, 237 insertions(+), 9 deletions(-)
 create mode 100755 tests/qemu-iotests/057
 create mode 100644 tests/qemu-iotests/057.out

[Qemu-devel] [PATCH 1/4] snapshot: distinguish id and name in load_tmp

[Wenchao Xia]

Since later this function will be used so improve it. The only caller of it
now is qemu-img, and it is not impacted by call the function twice to keep
old search logic.

Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com>
---
 block/qcow2-snapshot.c    |   16 ++++++++++++++--
 block/qcow2.h             |    5 ++++-
 block/snapshot.c          |   37 +++++++++++++++++++++++++++++++++++--
 include/block/block_int.h |    4 +++-
 include/block/snapshot.h  |    4 +++-
 qemu-img.c                |    7 ++++++-
 6 files changed, 65 insertions(+), 8 deletions(-)

diff --git a/block/qcow2-snapshot.c b/block/qcow2-snapshot.c
index 47db6ad..49a02a6 100644
--- a/block/qcow2-snapshot.c
+++ b/block/qcow2-snapshot.c
@@ -651,7 +651,10 @@ int qcow2_snapshot_list(BlockDriverState *bs, QEMUSnapshotInfo **psn_tab)
     return s->nb_snapshots;
 }
 
-int qcow2_snapshot_load_tmp(BlockDriverState *bs, const char *snapshot_name)
+int qcow2_snapshot_load_tmp(BlockDriverState *bs,
+                            const char *snapshot_id,
+                            const char *name,
+                            Error **errp)
 {
     int i, snapshot_index;
     BDRVQcowState *s = bs->opaque;
@@ -659,12 +662,17 @@ int qcow2_snapshot_load_tmp(BlockDriverState *bs, const char *snapshot_name)
     uint64_t *new_l1_table;
     int new_l1_bytes;
     int ret;
+    const char *device = bdrv_get_device_name(bs);
 
     assert(bs->read_only);
 
     /* Search the snapshot */
-    snapshot_index = find_snapshot_by_id_or_name(bs, snapshot_name);
+    snapshot_index = find_snapshot_by_id_and_name(bs, snapshot_id, name);
     if (snapshot_index < 0) {
+        error_setg(errp,
+                   "Can't find a snapshot with ID '%s' and name '%s' "
+                   "on device '%s'",
+                   STR_PRINT_CHAR(snapshot_id), STR_PRINT_CHAR(name), device);
         return -ENOENT;
     }
     sn = &s->snapshots[snapshot_index];
@@ -675,6 +683,10 @@ int qcow2_snapshot_load_tmp(BlockDriverState *bs, const char *snapshot_name)
 
     ret = bdrv_pread(bs->file, sn->l1_table_offset, new_l1_table, new_l1_bytes);
     if (ret < 0) {
+        error_setg(errp,
+                   "Failed to read l1 table for snapshot with ID '%s' and name "
+                   "'%s' on device '%s'",
+                   sn->id_str, sn->name, device);
         g_free(new_l1_table);
         return ret;
     }
diff --git a/block/qcow2.h b/block/qcow2.h
index 2e6c471..31b9c5c 100644
--- a/block/qcow2.h
+++ b/block/qcow2.h
@@ -416,7 +416,10 @@ int qcow2_snapshot_delete(BlockDriverState *bs,
                           const char *name,
                           Error **errp);
 int qcow2_snapshot_list(BlockDriverState *bs, QEMUSnapshotInfo **psn_tab);
-int qcow2_snapshot_load_tmp(BlockDriverState *bs, const char *snapshot_name);
+int qcow2_snapshot_load_tmp(BlockDriverState *bs,
+                            const char *snapshot_id,
+                            const char *name,
+                            Error **errp);
 
 void qcow2_free_snapshots(BlockDriverState *bs);
 int qcow2_read_snapshots(BlockDriverState *bs);
diff --git a/block/snapshot.c b/block/snapshot.c
index cf2f4ca..0811f3f 100644
--- a/block/snapshot.c
+++ b/block/snapshot.c
@@ -243,18 +243,51 @@ int bdrv_snapshot_list(BlockDriverState *bs,
     return -ENOTSUP;
 }
 
+/**
+ * Temporarily load an internal snapshot by @snapshot_id and @name.
+ * @bs: block device used in the operation
+ * @snapshot_id: unique snapshot ID, or NULL
+ * @name: snapshot name, or NULL
+ * @errp: location to store error
+ *
+ * If both @snapshot_id and @name are specified, load the first one with
+ * id @snapshot_id and name @name.
+ * If only @snapshot_id is specified, load the first one with id
+ * @snapshot_id.
+ * If only @name is specified, load the first one with name @name.
+ * if none is specified, return -ENINVAL.
+ *
+ * Returns: 0 on success, -errno on fail. If @bs is not inserted, return
+ * -ENOMEDIUM. If @bs is not readonly, return -EINVAL. If @bs did not support
+ * internal snapshot, return -ENOTSUP. If qemu can't find one matching @id and
+ * @name, return -ENOENT. If @bs do not support parameter @snapshot_id or
+ * @name, return -EINVAL. If @errp != NULL, it will always be filled on
+ * failure.
+ */
 int bdrv_snapshot_load_tmp(BlockDriverState *bs,
-        const char *snapshot_name)
+                           const char *snapshot_id,
+                           const char *name,
+                           Error **errp)
 {
     BlockDriver *drv = bs->drv;
+    const char *device = bdrv_get_device_name(bs);
     if (!drv) {
+        error_set(errp, QERR_DEVICE_HAS_NO_MEDIUM, device);
         return -ENOMEDIUM;
     }
+    if (!snapshot_id && !name) {
+        error_setg(errp, "snapshot_id and name are both NULL");
+        return -EINVAL;
+    }
     if (!bs->read_only) {
+        error_setg(errp, "Device '%s' is not readonly", device);
         return -EINVAL;
     }
     if (drv->bdrv_snapshot_load_tmp) {
-        return drv->bdrv_snapshot_load_tmp(bs, snapshot_name);
+        return drv->bdrv_snapshot_load_tmp(bs, snapshot_id, name, errp);
     }
+    error_set(errp, QERR_BLOCK_FORMAT_FEATURE_NOT_SUPPORTED,
+              drv->format_name, device,
+              "temporarily load internal snapshot");
     return -ENOTSUP;
 }
diff --git a/include/block/block_int.h b/include/block/block_int.h
index 4499b8b..fe44fda 100644
--- a/include/block/block_int.h
+++ b/include/block/block_int.h
@@ -173,7 +173,9 @@ struct BlockDriver {
     int (*bdrv_snapshot_list)(BlockDriverState *bs,
                               QEMUSnapshotInfo **psn_info);
     int (*bdrv_snapshot_load_tmp)(BlockDriverState *bs,
-                                  const char *snapshot_name);
+                                  const char *snapshot_id,
+                                  const char *name,
+                                  Error **errp);
     int (*bdrv_get_info)(BlockDriverState *bs, BlockDriverInfo *bdi);
 
     int (*bdrv_save_vmstate)(BlockDriverState *bs, QEMUIOVector *qiov,
diff --git a/include/block/snapshot.h b/include/block/snapshot.h
index e47e78d..9903d86 100644
--- a/include/block/snapshot.h
+++ b/include/block/snapshot.h
@@ -58,5 +58,7 @@ int bdrv_snapshot_delete(BlockDriverState *bs,
 int bdrv_snapshot_list(BlockDriverState *bs,
                        QEMUSnapshotInfo **psn_info);
 int bdrv_snapshot_load_tmp(BlockDriverState *bs,
-                           const char *snapshot_name);
+                           const char *snapshot_id,
+                           const char *name,
+                           Error **errp);
 #endif
diff --git a/qemu-img.c b/qemu-img.c
index f4e5818..4f504b0 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -1246,7 +1246,12 @@ static int img_convert(int argc, char **argv)
             ret = -1;
             goto out;
         }
-        if (bdrv_snapshot_load_tmp(bs[0], snapshot_name) < 0) {
+
+        ret = bdrv_snapshot_load_tmp(bs[0], snapshot_name, NULL, NULL);
+        if (ret == -ENOENT || ret == -EINVAL) {
+            ret = bdrv_snapshot_load_tmp(bs[0], NULL, snapshot_name, NULL);
+        }
+        if (ret < 0) {
             error_report("Failed to load snapshot");
             ret = -1;
             goto out;
-- 
1.7.1

[Qemu-devel] [PATCH 2/4] qemu-nbd: support internal snapshot export

[Wenchao Xia]

Now it is possible to directly export an internal snapshot, which
can be used to probe the snapshot's contents without qemu-img
convert.

Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com>
---
 qemu-nbd.c |   54 +++++++++++++++++++++++++++++++++++++++++++++++++++++-
 1 files changed, 53 insertions(+), 1 deletions(-)

diff --git a/qemu-nbd.c b/qemu-nbd.c
index 9c31d45..46be2b2 100644
--- a/qemu-nbd.c
+++ b/qemu-nbd.c
@@ -19,6 +19,7 @@
 #include "qemu-common.h"
 #include "block/block.h"
 #include "block/nbd.h"
+#include "block/snapshot.h"
 
 #include <stdarg.h>
 #include <stdio.h>
@@ -303,6 +304,23 @@ static void nbd_accept(void *opaque)
     }
 }
 
+#define SNAPSHOT_OPT_ID         "id"
+#define SNAPSHOT_OPT_NAME       "name"
+
+static QEMUOptionParameter snapshot_options[] = {
+    {
+        .name = SNAPSHOT_OPT_ID,
+        .type = OPT_STRING,
+        .help = "snapshot id"
+    },
+    {
+        .name = SNAPSHOT_OPT_NAME,
+        .type = OPT_STRING,
+        .help = "snapshot name"
+    },
+    { NULL }
+};
+
 int main(int argc, char **argv)
 {
     BlockDriverState *bs;
@@ -314,7 +332,11 @@ int main(int argc, char **argv)
     char *device = NULL;
     int port = NBD_DEFAULT_PORT;
     off_t fd_size;
-    const char *sopt = "hVb:o:p:rsnP:c:dvk:e:f:t";
+    QEMUOptionParameter *sn_param = NULL;
+    const QEMUOptionParameter *sn_param_id, *sn_param_name;
+    const char *sn_id = NULL, *sn_name = NULL;
+    Error *local_err = NULL;
+    const char *sopt = "hVb:o:p:rsnP:c:dvk:e:f:tl:";
     struct option lopt[] = {
         { "help", 0, NULL, 'h' },
         { "version", 0, NULL, 'V' },
@@ -327,6 +349,7 @@ int main(int argc, char **argv)
         { "connect", 1, NULL, 'c' },
         { "disconnect", 0, NULL, 'd' },
         { "snapshot", 0, NULL, 's' },
+        { "snapshot-load", 1, NULL, 'l' },
         { "nocache", 0, NULL, 'n' },
         { "cache", 1, NULL, QEMU_NBD_OPT_CACHE },
 #ifdef CONFIG_LINUX_AIO
@@ -426,6 +449,13 @@ int main(int argc, char **argv)
                 errx(EXIT_FAILURE, "Offset must be positive `%s'", optarg);
             }
             break;
+        case 'l':
+            sn_param = parse_option_parameters(optarg,
+                                               snapshot_options, sn_param);
+            if (!sn_param) {
+                errx(EXIT_FAILURE,
+                     "Invalid snapshot-load options '%s'", optarg);
+            }
         case 'r':
             nbdflags |= NBD_FLAG_READ_ONLY;
             flags &= ~BDRV_O_RDWR;
@@ -578,6 +608,24 @@ int main(int argc, char **argv)
         err(EXIT_FAILURE, "Failed to bdrv_open '%s'", argv[optind]);
     }
 
+    if (sn_param) {
+        sn_param_id = get_option_parameter(sn_param, SNAPSHOT_OPT_ID);
+        sn_param_name = get_option_parameter(sn_param, SNAPSHOT_OPT_NAME);
+        if (sn_param_id) {
+            sn_id = sn_param_id->value.s;
+        }
+        if (sn_param_name) {
+            sn_name = sn_param_name->value.s;
+        }
+        ret = bdrv_snapshot_load_tmp(bs, sn_id, sn_name, &local_err);
+        if (ret < 0) {
+            errno = -ret;
+            err(EXIT_FAILURE,
+                "Failed to load snapshot, reason:\n%s",
+                error_get_pretty(local_err));
+        }
+    }
+
     fd_size = bdrv_getlength(bs);
 
     if (partition != -1) {
@@ -638,6 +686,10 @@ int main(int argc, char **argv)
         unlink(sockpath);
     }
 
+    if (sn_param) {
+        free_option_parameters(sn_param);
+    }
+
     if (device) {
         void *ret;
         pthread_join(client_thread, &ret);
-- 
1.7.1

[Qemu-devel] [PATCH 3/4] qemu-nbd: add doc for internal snapshot export

[Wenchao Xia]

Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com>
---
 qemu-nbd.c    |    2 ++
 qemu-nbd.texi |    3 +++
 2 files changed, 5 insertions(+), 0 deletions(-)

diff --git a/qemu-nbd.c b/qemu-nbd.c
index 46be2b2..8eeee33 100644
--- a/qemu-nbd.c
+++ b/qemu-nbd.c
@@ -80,6 +80,8 @@ static void usage(const char *name)
 "Block device options:\n"
 "  -r, --read-only      export read-only\n"
 "  -s, --snapshot       use snapshot file\n"
+"  -l, --snapshot-load  temporarily load an internal snapshot and export it as\n"
+"                       an read-only device, format is 'id=[ID],name=[NAME]'\n"
 "  -n, --nocache        disable host cache\n"
 "      --cache=MODE     set cache mode (none, writeback, ...)\n"
 #ifdef CONFIG_LINUX_AIO
diff --git a/qemu-nbd.texi b/qemu-nbd.texi
index 6055ec6..86e8e1b 100644
--- a/qemu-nbd.texi
+++ b/qemu-nbd.texi
@@ -28,6 +28,9 @@ Export QEMU disk image using NBD protocol.
   only expose partition @var{num}
 @item -s, --snapshot
   use snapshot file
+@item -l, --snapshot-load
+  temporarily load an internal snapshot and export it as
+  an read-only device, format is 'id=[ID],name=[NAME]'
 @item -n, --nocache
 @itemx --cache=@var{cache}
   set cache mode to be used with the file.  See the documentation of
-- 
1.7.1

[Qemu-devel] [PATCH 4/4] qemu-iotests: add 057 internal snapshot export with qemu-nbd case

[Wenchao Xia]

Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com>
---
 tests/qemu-iotests/057     |   87 ++++++++++++++++++++++++++++++++++++++++++++
 tests/qemu-iotests/057.out |   26 +++++++++++++
 tests/qemu-iotests/group   |    1 +
 3 files changed, 114 insertions(+), 0 deletions(-)
 create mode 100755 tests/qemu-iotests/057
 create mode 100644 tests/qemu-iotests/057.out

diff --git a/tests/qemu-iotests/057 b/tests/qemu-iotests/057
new file mode 100755
index 0000000..301ef1f
--- /dev/null
+++ b/tests/qemu-iotests/057
@@ -0,0 +1,87 @@
+#!/bin/bash
+#
+# Test export internal snapshot by qemu-nbd.
+#
+# Copyright (C) 2013 IBM, Inc.
+#
+# Based on 029.
+#
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 2 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program.  If not, see <http://www.gnu.org/licenses/>.
+#
+
+# creator
+owner=xiawenc@linux.vnet.ibm.com
+
+seq=`basename $0`
+echo "QA output created by $seq"
+
+here=`pwd`
+tmp=/tmp/$$
+status=1	# failure is the default!
+
+nbd_snapshot_port=10850
+nbd_snapshot_img="nbd:127.0.0.1:$nbd_snapshot_port"
+
+_export_nbd_snapshot()
+{
+    eval "$QEMU_NBD -v -t -b 127.0.0.1 -p $nbd_snapshot_port $TEST_IMG -l name=$1 &"
+    NBD_SNAPSHOT_PID=$!
+    sleep 1
+}
+
+_cleanup()
+{
+    if [ -n "$NBD_SNAPSHOT_PID" ]; then
+        kill $NBD_SNAPSHOT_PID
+    fi
+	_cleanup_test_img
+}
+trap "_cleanup; exit \$status" 0 1 2 3 15
+
+# get standard environment, filters and checks
+. ./common.rc
+. ./common.filter
+. ./common.pattern
+
+# Any format supporting intenal snapshots
+_supported_fmt qcow2
+_supported_proto generic
+_supported_os Linux
+
+echo
+echo "== preparing image =="
+_make_test_img 64M
+$QEMU_IO -c 'write -P 0xa 0x1000 0x1000' $TEST_IMG | _filter_qemu_io
+$QEMU_IO -c 'write -P 0xb 0x2000 0x1000' $TEST_IMG | _filter_qemu_io
+$QEMU_IMG snapshot -c foo $TEST_IMG
+$QEMU_IO -c 'write -P 0xc 0x1000 0x1000' $TEST_IMG | _filter_qemu_io
+$QEMU_IO -c 'write -P 0xd 0x2000 0x1000' $TEST_IMG | _filter_qemu_io
+_check_test_img
+
+echo
+echo "== verifying the image file with patterns =="
+$QEMU_IO -c 'read -P 0xc 0x1000 0x1000' $TEST_IMG | _filter_qemu_io
+$QEMU_IO -c 'read -P 0xd 0x2000 0x1000' $TEST_IMG | _filter_qemu_io
+
+_export_nbd_snapshot foo
+
+echo
+echo "== verifying the exported snapshot with patterns =="
+$QEMU_IO -c 'read -P 0xa 0x1000 0x1000' $nbd_snapshot_img | _filter_qemu_io
+$QEMU_IO -c 'read -P 0xb 0x2000 0x1000' $nbd_snapshot_img | _filter_qemu_io
+
+# success, all done
+echo "*** done"
+rm -f $seq.full
+status=0
diff --git a/tests/qemu-iotests/057.out b/tests/qemu-iotests/057.out
new file mode 100644
index 0000000..757811c
--- /dev/null
+++ b/tests/qemu-iotests/057.out
@@ -0,0 +1,26 @@
+QA output created by 057
+
+== preparing image ==
+Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=67108864
+wrote 4096/4096 bytes at offset 4096
+4 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+wrote 4096/4096 bytes at offset 8192
+4 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+wrote 4096/4096 bytes at offset 4096
+4 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+wrote 4096/4096 bytes at offset 8192
+4 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+No errors were found on the image.
+
+== verifying the image file with patterns ==
+read 4096/4096 bytes at offset 4096
+4 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+read 4096/4096 bytes at offset 8192
+4 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+
+== verifying the exported snapshot with patterns ==
+read 4096/4096 bytes at offset 4096
+4 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+read 4096/4096 bytes at offset 8192
+4 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+*** done
diff --git a/tests/qemu-iotests/group b/tests/qemu-iotests/group
index 5f5009b..1e30766 100644
--- a/tests/qemu-iotests/group
+++ b/tests/qemu-iotests/group
@@ -63,3 +63,4 @@
 054 rw auto
 055 rw auto
 056 rw auto
+057 rw auto
-- 
1.7.1

Re: [Qemu-devel] [PATCH 0/4] export internal snapshot by qemu-nbd

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 3691 bytes --]

On 07/17/2013 08:03 AM, Wenchao Xia wrote:
> This series allow user to read internal snapshot's contents without qemu-img
> convert. Another purpose is that, when qemu is online and have taken an
> internal snapshot, let user invoke qemu-nbd to do any thing on it except write.
> 
> This brings two interesting issues:
> 1 is it safe to let qemu-nbd and qemu access that file at same time?

Probably not, for the same reason we tell people to not use qemu-img
while qemu is active on a file.

> I think it is safe, since qemu-nbd is read only. The data will be correct from
> qemu-nbd, if qemu does not delete that snapshot when qemu-nbd is running, and
> data is flushed to storage after qemu take that snapshot so that qemu-nbd
> would see the correct data.

You're making assumptions that qemu won't be touching any metadata in a
manner in which the read-only qemu-nbd could get confused; I'm not sure
we are ready to make that guarantee.  I think the export has to be from
the running qemu process itself, rather than from a second process.

> 
> 2 should an nbd-server exporting internal snapshot be added in qemu?
> I think no. Compared with driver-backup, the snapshot, or COW happens
> in storage level, so it allows another program to read it itself. Actually
> it should be OK to let another server other than qemu's host, do the
> export I/O job, if data is flushed.

Unfortunately, I disagree, and think the answer to this question is yes,
we need to do the export from within the single qemu process, if we want
to guarantee safety.

> 
> Next step:
> As demonstrated before, an explict API should be added, which make sure
> all I/O request is flushed and sent to underlining storage, and cache
> is sync if it is writeback type. So at qemu level, we can make sure
> no request is left behind, before qemu-nbd start. That API should
> also benefit 3rd party block snapshot solution, such as LVM2.
> 
> More:
> With this patch and previous qcow2 snapshot at block device level, I think
> export/import/backup solution around qcow2, is nearly complete at qemu's
> level. It can do similar things as backing chain but with better performance,
> Some small optimization place are left:
> 
> 1 compare two snapshot's data to get the diff with help of qcow2's L1/L2 table.
> 2 convertion between external snapshot and internal snapshot.
> 
> This series need following series applied first:
> [PATCH V5 0/8] add internal snapshot support at block device level
> http://lists.nongnu.org/archive/html/qemu-devel/2013-07/msg01831.html
> 
> Wenchao Xia (4):
>   1 snapshot: distinguish id and name in load_tmp
>   2 qemu-nbd: support internal snapshot export
>   3 qemu-nbd: add doc for internal snapshot export
>   4 qemu-iotests: add 057 internal snapshot export with qemu-nbd case
> 
>  block/qcow2-snapshot.c     |   16 +++++++-
>  block/qcow2.h              |    5 ++-
>  block/snapshot.c           |   37 ++++++++++++++++++-
>  include/block/block_int.h  |    4 ++-
>  include/block/snapshot.h   |    4 ++-
>  qemu-img.c                 |    7 +++-
>  qemu-nbd.c                 |   56 ++++++++++++++++++++++++++++-
>  qemu-nbd.texi              |    3 ++
>  tests/qemu-iotests/057     |   87 ++++++++++++++++++++++++++++++++++++++++++++
>  tests/qemu-iotests/057.out |   26 +++++++++++++
>  tests/qemu-iotests/group   |    1 +
>  11 files changed, 237 insertions(+), 9 deletions(-)
>  create mode 100755 tests/qemu-iotests/057
>  create mode 100644 tests/qemu-iotests/057.out
> 
> 
> 
> 
> 

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 621 bytes --]

Re: [Qemu-devel] [PATCH 0/4] export internal snapshot by qemu-nbd

[Kevin Wolf]

Am 17.07.2013 um 16:23 hat Eric Blake geschrieben:
> On 07/17/2013 08:03 AM, Wenchao Xia wrote:
> > This series allow user to read internal snapshot's contents without qemu-img
> > convert. Another purpose is that, when qemu is online and have taken an
> > internal snapshot, let user invoke qemu-nbd to do any thing on it except write.
> > 
> > This brings two interesting issues:
> > 1 is it safe to let qemu-nbd and qemu access that file at same time?
> 
> Probably not, for the same reason we tell people to not use qemu-img
> while qemu is active on a file.

No, it's not. There's the built-in NBD server, but making internal
snapshots usable with it would require some non-trivial changes in the
block layer and the qcow2 code.

> > I think it is safe, since qemu-nbd is read only. The data will be correct from
> > qemu-nbd, if qemu does not delete that snapshot when qemu-nbd is running, and
> > data is flushed to storage after qemu take that snapshot so that qemu-nbd
> > would see the correct data.
> 
> You're making assumptions that qemu won't be touching any metadata in a
> manner in which the read-only qemu-nbd could get confused; I'm not sure
> we are ready to make that guarantee.  I think the export has to be from
> the running qemu process itself, rather than from a second process.

I think a while ago I convinced myself that in practice it does work,
but it's not a guarantee we're making and I won't hesitate to break the
assumption if it's helpful for some feature.

> > 2 should an nbd-server exporting internal snapshot be added in qemu?
> > I think no. Compared with driver-backup, the snapshot, or COW happens
> > in storage level, so it allows another program to read it itself. Actually
> > it should be OK to let another server other than qemu's host, do the
> > export I/O job, if data is flushed.
> 
> Unfortunately, I disagree, and think the answer to this question is yes,
> we need to do the export from within the single qemu process, if we want
> to guarantee safety.

Agreed.

Kevin

Re: [Qemu-devel] [PATCH 0/4] export internal snapshot by qemu-nbd

[Wenchao Xia]

于 2013-7-17 23:21, Kevin Wolf 写道:
> Am 17.07.2013 um 16:23 hat Eric Blake geschrieben:
>> On 07/17/2013 08:03 AM, Wenchao Xia wrote:
>>> This series allow user to read internal snapshot's contents without qemu-img
>>> convert. Another purpose is that, when qemu is online and have taken an
>>> internal snapshot, let user invoke qemu-nbd to do any thing on it except write.
>>>
>>> This brings two interesting issues:
>>> 1 is it safe to let qemu-nbd and qemu access that file at same time?
>>
>> Probably not, for the same reason we tell people to not use qemu-img
>> while qemu is active on a file.
>
> No, it's not. There's the built-in NBD server, but making internal
> snapshots usable with it would require some non-trivial changes in the
> block layer and the qcow2 code.
>
   What changes should be done? I think currently, it is safe except
resizing/snapshot delete, those operation can be forbidded when nbd
is running.

>>> I think it is safe, since qemu-nbd is read only. The data will be correct from
>>> qemu-nbd, if qemu does not delete that snapshot when qemu-nbd is running, and
>>> data is flushed to storage after qemu take that snapshot so that qemu-nbd
>>> would see the correct data.
>>
>> You're making assumptions that qemu won't be touching any metadata in a
>> manner in which the read-only qemu-nbd could get confused; I'm not sure
>> we are ready to make that guarantee.  I think the export has to be from
>> the running qemu process itself, rather than from a second process.
>
> I think a while ago I convinced myself that in practice it does work,
> but it's not a guarantee we're making and I won't hesitate to break the
> assumption if it's helpful for some feature.
>
   I think it can be listed out about the features that may break the
assumption, if added later, then simply forbid nbd running at that case
by user.

   This related to qcow2's specification: Do we allow another
process read an snapshot of one image while it is used, in normal
condition? It would be nice to have it as a storage software.

   My thought is that, focus more on one format for qemu, that is qcow2,
enhance it if possible instead of new format, so add some guarantee for
it may be not bad.


>>> 2 should an nbd-server exporting internal snapshot be added in qemu?
>>> I think no. Compared with driver-backup, the snapshot, or COW happens
>>> in storage level, so it allows another program to read it itself. Actually
>>> it should be OK to let another server other than qemu's host, do the
>>> export I/O job, if data is flushed.
>>
>> Unfortunately, I disagree, and think the answer to this question is yes,
>> we need to do the export from within the single qemu process, if we want
>> to guarantee safety.
>
> Agreed.
>
> Kevin
>


-- 
Best Regards

Wenchao Xia

Re: [Qemu-devel] [PATCH 0/4] export internal snapshot by qemu-nbd

[Stefan Hajnoczi]

On Wed, Jul 17, 2013 at 10:03:51PM +0800, Wenchao Xia wrote:
> This series allow user to read internal snapshot's contents without qemu-img
> convert. Another purpose is that, when qemu is online and have taken an
> internal snapshot, let user invoke qemu-nbd to do any thing on it except write.

I agree with Eric and Kevin that we cannot access image files while QEMU
has them open.

A bit more detail about using the run-time NBD server to do this safely:

Internal snapshots are not first-class block layer objects today.  They
are not BlockDriverStates, instead you must access their data through
bdrv_snapshot_goto() or bdrv_snapshot_load_tmp().

The problem with these functions is that they use the BlockDriverState.
So the guest cannot write to the main image while an internal snapshot
is loaded.

This can be solved by introducing a function that creates a read-only
internal snapshot BlockDriverState which is partially independent of the
main image BlockDriverState that the guest is accessing.

The challenge is implementing this cleanly:

1. The lifecycle of the main image BlockDriverState and the read-only
   internal snapshot BlockDriverState is related.  What happens when the
   main image BDS is deleted but the snapshot BDS is still alive?  What
   happens when bdrv_snapshot_delete() is called while the BDS snapshot
   exists?

2. At what level should resources like the bs->file and metadata caches
   (L1/L2/refcount) be shared?  If they are shared it's easy to keep
   them consistent but makes lifecycle handling harder.

Stefan

Re: [Qemu-devel] [PATCH 0/4] export internal snapshot by qemu-nbd

[Fam Zheng]

On Thu, 07/18 10:28, Wenchao Xia wrote:
> 于 2013-7-17 23:21, Kevin Wolf 写道:
> >Am 17.07.2013 um 16:23 hat Eric Blake geschrieben:
> >>On 07/17/2013 08:03 AM, Wenchao Xia wrote:
> >>>This series allow user to read internal snapshot's contents without qemu-img
> >>>convert. Another purpose is that, when qemu is online and have taken an
> >>>internal snapshot, let user invoke qemu-nbd to do any thing on it except write.
> >>>
> >>>This brings two interesting issues:
> >>>1 is it safe to let qemu-nbd and qemu access that file at same time?
> >>
> >>Probably not, for the same reason we tell people to not use qemu-img
> >>while qemu is active on a file.
> >
> >No, it's not. There's the built-in NBD server, but making internal
> >snapshots usable with it would require some non-trivial changes in the
> >block layer and the qcow2 code.
> >
>   What changes should be done? I think currently, it is safe except
> resizing/snapshot delete, those operation can be forbidded when nbd
> is running.
> 

For external NBD to a export an in use image the answer should be "don't
do it", with the same reasons as above. For internal NBD server, I agree
with Stefan's reply: a read-only BlockDriverState that is dependent on
the guest's used one, and share in-memory data for consistency.

> >>>I think it is safe, since qemu-nbd is read only. The data will be correct from
> >>>qemu-nbd, if qemu does not delete that snapshot when qemu-nbd is running, and
> >>>data is flushed to storage after qemu take that snapshot so that qemu-nbd
> >>>would see the correct data.
> >>
> >>You're making assumptions that qemu won't be touching any metadata in a
> >>manner in which the read-only qemu-nbd could get confused; I'm not sure
> >>we are ready to make that guarantee.  I think the export has to be from
> >>the running qemu process itself, rather than from a second process.
> >
> >I think a while ago I convinced myself that in practice it does work,
> >but it's not a guarantee we're making and I won't hesitate to break the
> >assumption if it's helpful for some feature.
> >
>   I think it can be listed out about the features that may break the
> assumption, if added later, then simply forbid nbd running at that case
> by user.
> 
>   This related to qcow2's specification: Do we allow another
> process read an snapshot of one image while it is used, in normal
> condition? It would be nice to have it as a storage software.

But I don't think it's something that a format specification could
achieve, i.e. it is not the right place to specify "how to synchronize
multiple reader/writers to me", we can't do it by adding whatever bits
to format itself. The question is similar to: we can't guarantee the
consistency of data, no matter how advanced and sophisticated its
structure is, unless we use lock to harmonize multiple users, which is
totally orthogonal to the data itself.

Thanks.

-- 
Fam

Re: [Qemu-devel] [PATCH 0/4] export internal snapshot by qemu-nbd

[Wenchao Xia]

于 2013-7-17 22:23, Eric Blake 写道:
> On 07/17/2013 08:03 AM, Wenchao Xia wrote:
>> This series allow user to read internal snapshot's contents without qemu-img
>> convert. Another purpose is that, when qemu is online and have taken an
>> internal snapshot, let user invoke qemu-nbd to do any thing on it except write.
>>
>> This brings two interesting issues:
>> 1 is it safe to let qemu-nbd and qemu access that file at same time?
>
> Probably not, for the same reason we tell people to not use qemu-img
> while qemu is active on a file.
>
   For external case, or backing chain, qemu-nbd export while qemu is
active, do you think it is OK?

base->imageA

qemu-nbd export base
qemu use imageA.


>> I think it is safe, since qemu-nbd is read only. The data will be correct from
>> qemu-nbd, if qemu does not delete that snapshot when qemu-nbd is running, and
>> data is flushed to storage after qemu take that snapshot so that qemu-nbd
>> would see the correct data.
>
> You're making assumptions that qemu won't be touching any metadata in a
> manner in which the read-only qemu-nbd could get confused; I'm not sure
> we are ready to make that guarantee.  I think the export has to be from
> the running qemu process itself, rather than from a second process.
>
>>
>> 2 should an nbd-server exporting internal snapshot be added in qemu?
>> I think no. Compared with driver-backup, the snapshot, or COW happens
>> in storage level, so it allows another program to read it itself. Actually
>> it should be OK to let another server other than qemu's host, do the
>> export I/O job, if data is flushed.
>
> Unfortunately, I disagree, and think the answer to this question is yes,
> we need to do the export from within the single qemu process, if we want
> to guarantee safety.
>
>>
>> Next step:
>> As demonstrated before, an explict API should be added, which make sure
>> all I/O request is flushed and sent to underlining storage, and cache
>> is sync if it is writeback type. So at qemu level, we can make sure
>> no request is left behind, before qemu-nbd start. That API should
>> also benefit 3rd party block snapshot solution, such as LVM2.
>>
>> More:
>> With this patch and previous qcow2 snapshot at block device level, I think
>> export/import/backup solution around qcow2, is nearly complete at qemu's
>> level. It can do similar things as backing chain but with better performance,
>> Some small optimization place are left:
>>
>> 1 compare two snapshot's data to get the diff with help of qcow2's L1/L2 table.
>> 2 convertion between external snapshot and internal snapshot.
>>
>> This series need following series applied first:
>> [PATCH V5 0/8] add internal snapshot support at block device level
>> http://lists.nongnu.org/archive/html/qemu-devel/2013-07/msg01831.html
>>
>> Wenchao Xia (4):
>>    1 snapshot: distinguish id and name in load_tmp
>>    2 qemu-nbd: support internal snapshot export
>>    3 qemu-nbd: add doc for internal snapshot export
>>    4 qemu-iotests: add 057 internal snapshot export with qemu-nbd case
>>
>>   block/qcow2-snapshot.c     |   16 +++++++-
>>   block/qcow2.h              |    5 ++-
>>   block/snapshot.c           |   37 ++++++++++++++++++-
>>   include/block/block_int.h  |    4 ++-
>>   include/block/snapshot.h   |    4 ++-
>>   qemu-img.c                 |    7 +++-
>>   qemu-nbd.c                 |   56 ++++++++++++++++++++++++++++-
>>   qemu-nbd.texi              |    3 ++
>>   tests/qemu-iotests/057     |   87 ++++++++++++++++++++++++++++++++++++++++++++
>>   tests/qemu-iotests/057.out |   26 +++++++++++++
>>   tests/qemu-iotests/group   |    1 +
>>   11 files changed, 237 insertions(+), 9 deletions(-)
>>   create mode 100755 tests/qemu-iotests/057
>>   create mode 100644 tests/qemu-iotests/057.out
>>
>>
>>
>>
>>
>


-- 
Best Regards

Wenchao Xia

Re: [Qemu-devel] [PATCH 0/4] export internal snapshot by qemu-nbd

[Kevin Wolf]

Am 19.07.2013 um 08:29 hat Wenchao Xia geschrieben:
> 于 2013-7-17 22:23, Eric Blake 写道:
> >On 07/17/2013 08:03 AM, Wenchao Xia wrote:
> >>This series allow user to read internal snapshot's contents without qemu-img
> >>convert. Another purpose is that, when qemu is online and have taken an
> >>internal snapshot, let user invoke qemu-nbd to do any thing on it except write.
> >>
> >>This brings two interesting issues:
> >>1 is it safe to let qemu-nbd and qemu access that file at same time?
> >
> >Probably not, for the same reason we tell people to not use qemu-img
> >while qemu is active on a file.
> >
>   For external case, or backing chain, qemu-nbd export while qemu is
> active, do you think it is OK?
> 
> base->imageA
> 
> qemu-nbd export base
> qemu use imageA.

It depends. If you use 'qemu-nbd -r' to export it, it's okay.  The
reason is that it's allowed to have either one process opening the file
read-write, or an arbitrary number of processes opening the file
read-only. In the case of qemu using imageA, base is only opened
read-only, so qemu-nbd can be another read-only user.

Kevin

Re: [Qemu-devel] [PATCH 0/4] export internal snapshot by qemu-nbd

[Wenchao Xia]

于 2013-7-18 13:43, Stefan Hajnoczi 写道:
> On Wed, Jul 17, 2013 at 10:03:51PM +0800, Wenchao Xia wrote:
>> This series allow user to read internal snapshot's contents without qemu-img
>> convert. Another purpose is that, when qemu is online and have taken an
>> internal snapshot, let user invoke qemu-nbd to do any thing on it except write.
>
> I agree with Eric and Kevin that we cannot access image files while QEMU
> has them open.
>
> A bit more detail about using the run-time NBD server to do this safely:
>
> Internal snapshots are not first-class block layer objects today.  They
> are not BlockDriverStates, instead you must access their data through
> bdrv_snapshot_goto() or bdrv_snapshot_load_tmp().
>
   Also cc to Fam Zheng an Kevin:
   I think BlockDriverStates modification, is based on a more basic
question: does qcow2's data structure on disk, allow multiple snapshot
reader, while one active writer, without lock? I think the modification
showed above, already said "yes" to this question. If yes, just
create a new BlockDriverStates, we should say yes in spec.
   I wonder how vmdk allow direct snapshot access by VixDiskLib 1.1,
without network expense on host, it should be the format said support
of it.




> The problem with these functions is that they use the BlockDriverState.
> So the guest cannot write to the main image while an internal snapshot
> is loaded.
>
> This can be solved by introducing a function that creates a read-only
> internal snapshot BlockDriverState which is partially independent of the
> main image BlockDriverState that the guest is accessing.
>
> The challenge is implementing this cleanly:
>
> 1. The lifecycle of the main image BlockDriverState and the read-only
>     internal snapshot BlockDriverState is related.  What happens when the
>     main image BDS is deleted but the snapshot BDS is still alive?  What
>     happens when bdrv_snapshot_delete() is called while the BDS snapshot
>     exists?
>
> 2. At what level should resources like the bs->file and metadata caches
>     (L1/L2/refcount) be shared?  If they are shared it's easy to keep
>     them consistent but makes lifecycle handling harder.
>
> Stefan
>


-- 
Best Regards

Wenchao Xia

Re: [Qemu-devel] [PATCH 0/4] export internal snapshot by qemu-nbd

[Kevin Wolf]

Am 19.07.2013 um 11:03 hat Wenchao Xia geschrieben:
> 于 2013-7-18 13:43, Stefan Hajnoczi 写道:
> >On Wed, Jul 17, 2013 at 10:03:51PM +0800, Wenchao Xia wrote:
> >>This series allow user to read internal snapshot's contents without qemu-img
> >>convert. Another purpose is that, when qemu is online and have taken an
> >>internal snapshot, let user invoke qemu-nbd to do any thing on it except write.
> >
> >I agree with Eric and Kevin that we cannot access image files while QEMU
> >has them open.
> >
> >A bit more detail about using the run-time NBD server to do this safely:
> >
> >Internal snapshots are not first-class block layer objects today.  They
> >are not BlockDriverStates, instead you must access their data through
> >bdrv_snapshot_goto() or bdrv_snapshot_load_tmp().
> >
>   Also cc to Fam Zheng an Kevin:
>   I think BlockDriverStates modification, is based on a more basic
> question: does qcow2's data structure on disk, allow multiple snapshot
> reader, while one active writer, without lock? I think the modification
> showed above, already said "yes" to this question.

What exactly do you mean by "without lock"?

But yes, there's no fundamental problem in the format specification that
would make it impossible to write a driver that accesses multiple
snapshots at the same time from the same qemu process. (Even r/w in
theory, though we're treating snapshots as immutable traditionally.)

Kevin

Re: [Qemu-devel] [PATCH 0/4] export internal snapshot by qemu-nbd

[Wenchao Xia]

于 2013-7-19 17:19, Kevin Wolf 写道:
> Am 19.07.2013 um 11:03 hat Wenchao Xia geschrieben:
>> 于 2013-7-18 13:43, Stefan Hajnoczi 写道:
>>> On Wed, Jul 17, 2013 at 10:03:51PM +0800, Wenchao Xia wrote:
>>>> This series allow user to read internal snapshot's contents without qemu-img
>>>> convert. Another purpose is that, when qemu is online and have taken an
>>>> internal snapshot, let user invoke qemu-nbd to do any thing on it except write.
>>>
>>> I agree with Eric and Kevin that we cannot access image files while QEMU
>>> has them open.
>>>
>>> A bit more detail about using the run-time NBD server to do this safely:
>>>
>>> Internal snapshots are not first-class block layer objects today.  They
>>> are not BlockDriverStates, instead you must access their data through
>>> bdrv_snapshot_goto() or bdrv_snapshot_load_tmp().
>>>
>>    Also cc to Fam Zheng an Kevin:
>>    I think BlockDriverStates modification, is based on a more basic
>> question: does qcow2's data structure on disk, allow multiple snapshot
>> reader, while one active writer, without lock? I think the modification
>> showed above, already said "yes" to this question.
>
> What exactly do you mean by "without lock"?
>
   I mean read of old snapshot and write on qcow2 except snapshot
operation, can happen in same time, without a mutex in qemu.

> But yes, there's no fundamental problem in the format specification that
> would make it impossible to write a driver that accesses multiple
> snapshots at the same time from the same qemu process. (Even r/w in
> theory, though we're treating snapshots as immutable traditionally.)
>
   What about one writer process and one snapshot reader process? I think
it works if manager forbid snapshot operation in writer process. It
works with a rule: "only snapshot operation and * operation, would
change the data on disk, which is used by reading pre-existing
snapshots".

   I know in principle, one process owning one image, make things
clear, but this can leverage internal snapshot as easy to use as
external ones.

> Kevin
>


-- 
Best Regards

Wenchao Xia

Re: [Qemu-devel] [PATCH 0/4] export internal snapshot by qemu-nbd

[Fam Zheng]

On Fri, 07/19 17:03, Wenchao Xia wrote:
> 于 2013-7-18 13:43, Stefan Hajnoczi 写道:
> >On Wed, Jul 17, 2013 at 10:03:51PM +0800, Wenchao Xia wrote:
> >>This series allow user to read internal snapshot's contents without qemu-img
> >>convert. Another purpose is that, when qemu is online and have taken an
> >>internal snapshot, let user invoke qemu-nbd to do any thing on it except write.
> >
> >I agree with Eric and Kevin that we cannot access image files while QEMU
> >has them open.
> >
> >A bit more detail about using the run-time NBD server to do this safely:
> >
> >Internal snapshots are not first-class block layer objects today.  They
> >are not BlockDriverStates, instead you must access their data through
> >bdrv_snapshot_goto() or bdrv_snapshot_load_tmp().
> >
>   Also cc to Fam Zheng an Kevin:
>   I think BlockDriverStates modification, is based on a more basic
> question: does qcow2's data structure on disk, allow multiple snapshot
> reader, while one active writer, without lock? I think the modification
> showed above, already said "yes" to this question. If yes, just
> create a new BlockDriverStates, we should say yes in spec.
>   I wonder how vmdk allow direct snapshot access by VixDiskLib 1.1,
> without network expense on host, it should be the format said support
> of it.
> 
Is the VMDK snapshot internal? Which subformat of VMDK do you mean here?

Thanks.

-- 
Fam

Re: [Qemu-devel] [PATCH 0/4] export internal snapshot by qemu-nbd

[Wenchao Xia]

于 2013-7-22 10:10, Fam Zheng 写道:
> On Fri, 07/19 17:03, Wenchao Xia wrote:
>> 于 2013-7-18 13:43, Stefan Hajnoczi 写道:
>>> On Wed, Jul 17, 2013 at 10:03:51PM +0800, Wenchao Xia wrote:
>>>> This series allow user to read internal snapshot's contents without qemu-img
>>>> convert. Another purpose is that, when qemu is online and have taken an
>>>> internal snapshot, let user invoke qemu-nbd to do any thing on it except write.
>>>
>>> I agree with Eric and Kevin that we cannot access image files while QEMU
>>> has them open.
>>>
>>> A bit more detail about using the run-time NBD server to do this safely:
>>>
>>> Internal snapshots are not first-class block layer objects today.  They
>>> are not BlockDriverStates, instead you must access their data through
>>> bdrv_snapshot_goto() or bdrv_snapshot_load_tmp().
>>>
>>    Also cc to Fam Zheng an Kevin:
>>    I think BlockDriverStates modification, is based on a more basic
>> question: does qcow2's data structure on disk, allow multiple snapshot
>> reader, while one active writer, without lock? I think the modification
>> showed above, already said "yes" to this question. If yes, just
>> create a new BlockDriverStates, we should say yes in spec.
>>    I wonder how vmdk allow direct snapshot access by VixDiskLib 1.1,
>> without network expense on host, it should be the format said support
>> of it.
>>
> Is the VMDK snapshot internal? Which subformat of VMDK do you mean here?
>
> Thanks.
>
   Rechecked VMDK's doc, guess it is using backing chain. Still, It helps
if internal snapshot can do similar things.

-- 
Best Regards

Wenchao Xia

Re: [Qemu-devel] [PATCH 0/4] export internal snapshot by qemu-nbd

[Wenchao Xia]

Eric, I'd like to have a discuss about snapshot and vmbackup in
qemu/libvirt with management stack's perspective, what is your time
zone? I hope to ping you in IRC.

> On 07/17/2013 08:03 AM, Wenchao Xia wrote:
>> This series allow user to read internal snapshot's contents without qemu-img
>> convert. Another purpose is that, when qemu is online and have taken an
>> internal snapshot, let user invoke qemu-nbd to do any thing on it except write.
>>
>> This brings two interesting issues:
>> 1 is it safe to let qemu-nbd and qemu access that file at same time?
>
> Probably not, for the same reason we tell people to not use qemu-img
> while qemu is active on a file.
>
>> I think it is safe, since qemu-nbd is read only. The data will be correct from
>> qemu-nbd, if qemu does not delete that snapshot when qemu-nbd is running, and
>> data is flushed to storage after qemu take that snapshot so that qemu-nbd
>> would see the correct data.
>
> You're making assumptions that qemu won't be touching any metadata in a
> manner in which the read-only qemu-nbd could get confused; I'm not sure
> we are ready to make that guarantee.  I think the export has to be from
> the running qemu process itself, rather than from a second process.
>
>>
>> 2 should an nbd-server exporting internal snapshot be added in qemu?
>> I think no. Compared with driver-backup, the snapshot, or COW happens
>> in storage level, so it allows another program to read it itself. Actually
>> it should be OK to let another server other than qemu's host, do the
>> export I/O job, if data is flushed.
>
> Unfortunately, I disagree, and think the answer to this question is yes,
> we need to do the export from within the single qemu process, if we want
> to guarantee safety.
>
>>
>> Next step:
>> As demonstrated before, an explict API should be added, which make sure
>> all I/O request is flushed and sent to underlining storage, and cache
>> is sync if it is writeback type. So at qemu level, we can make sure
>> no request is left behind, before qemu-nbd start. That API should
>> also benefit 3rd party block snapshot solution, such as LVM2.
>>
>> More:
>> With this patch and previous qcow2 snapshot at block device level, I think
>> export/import/backup solution around qcow2, is nearly complete at qemu's
>> level. It can do similar things as backing chain but with better performance,
>> Some small optimization place are left:
>>
>> 1 compare two snapshot's data to get the diff with help of qcow2's L1/L2 table.
>> 2 convertion between external snapshot and internal snapshot.
>>
>> This series need following series applied first:
>> [PATCH V5 0/8] add internal snapshot support at block device level
>> http://lists.nongnu.org/archive/html/qemu-devel/2013-07/msg01831.html
>>
>> Wenchao Xia (4):
>>    1 snapshot: distinguish id and name in load_tmp
>>    2 qemu-nbd: support internal snapshot export
>>    3 qemu-nbd: add doc for internal snapshot export
>>    4 qemu-iotests: add 057 internal snapshot export with qemu-nbd case
>>
>>   block/qcow2-snapshot.c     |   16 +++++++-
>>   block/qcow2.h              |    5 ++-
>>   block/snapshot.c           |   37 ++++++++++++++++++-
>>   include/block/block_int.h  |    4 ++-
>>   include/block/snapshot.h   |    4 ++-
>>   qemu-img.c                 |    7 +++-
>>   qemu-nbd.c                 |   56 ++++++++++++++++++++++++++++-
>>   qemu-nbd.texi              |    3 ++
>>   tests/qemu-iotests/057     |   87 ++++++++++++++++++++++++++++++++++++++++++++
>>   tests/qemu-iotests/057.out |   26 +++++++++++++
>>   tests/qemu-iotests/group   |    1 +
>>   11 files changed, 237 insertions(+), 9 deletions(-)
>>   create mode 100755 tests/qemu-iotests/057
>>   create mode 100644 tests/qemu-iotests/057.out
>>
>>
>>
>>
>>
>


-- 
Best Regards

Wenchao Xia

Re: [Qemu-devel] [PATCH 0/4] export internal snapshot by qemu-nbd

[Wenchao Xia]

  Besides the argument, I think it helps to probe snapshot without
qemu-img convert, hope to get comments for the code.


-- 
Best Regards

Wenchao Xia

Re: [Qemu-devel] [PATCH 0/4] export internal snapshot by qemu-nbd

[Stefan Hajnoczi]

On Thu, Jul 25, 2013 at 10:30:49AM +0800, Wenchao Xia wrote:
>   Besides the argument, I think it helps to probe snapshot without
> qemu-img convert, hope to get comments for the code.

Hi Wenchao,
Which patch are you referring to?

Stefan

Re: [Qemu-devel] [PATCH 0/4] export internal snapshot by qemu-nbd

[Wenchao Xia]

于 2013-7-25 16:06, Stefan Hajnoczi 写道:
> On Thu, Jul 25, 2013 at 10:30:49AM +0800, Wenchao Xia wrote:
>>    Besides the argument, I think it helps to probe snapshot without
>> qemu-img convert, hope to get comments for the code.
>
> Hi Wenchao,
> Which patch are you referring to?
>
> Stefan
>
   This series, which enable qemu-nbd load a snapshot. It is quite
simple, look forward to your opinion, especially the parameter of
qemu-nbd part in patch 2.

-- 
Best Regards

Wenchao Xia

Re: [Qemu-devel] [PATCH 3/4] qemu-nbd: add doc for internal snapshot export

[Stefan Hajnoczi]

On Wed, Jul 17, 2013 at 10:03:54PM +0800, Wenchao Xia wrote:
> Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com>
> ---
>  qemu-nbd.c    |    2 ++
>  qemu-nbd.texi |    3 +++
>  2 files changed, 5 insertions(+), 0 deletions(-)
> 
> diff --git a/qemu-nbd.c b/qemu-nbd.c
> index 46be2b2..8eeee33 100644
> --- a/qemu-nbd.c
> +++ b/qemu-nbd.c
> @@ -80,6 +80,8 @@ static void usage(const char *name)
>  "Block device options:\n"
>  "  -r, --read-only      export read-only\n"
>  "  -s, --snapshot       use snapshot file\n"
> +"  -l, --snapshot-load  temporarily load an internal snapshot and export it as\n"
> +"                       an read-only device, format is 'id=[ID],name=[NAME]'\n"

Does it really export it read-only?  I think you'll get an error unless
you pass qemu-nbd --read-only.

Re: [Qemu-devel] [PATCH 0/4] export internal snapshot by qemu-nbd

[Stefan Hajnoczi]

On Fri, Jul 26, 2013 at 10:23:24AM +0800, Wenchao Xia wrote:
> 于 2013-7-25 16:06, Stefan Hajnoczi 写道:
> >On Thu, Jul 25, 2013 at 10:30:49AM +0800, Wenchao Xia wrote:
> >>   Besides the argument, I think it helps to probe snapshot without
> >>qemu-img convert, hope to get comments for the code.
> >
> >Hi Wenchao,
> >Which patch are you referring to?
> >
> >Stefan
> >
>   This series, which enable qemu-nbd load a snapshot. It is quite
> simple, look forward to your opinion, especially the parameter of
> qemu-nbd part in patch 2.

I understand now.  Adding bdrv_snapshot_load_tmp() support to qemu-nbd
is useful, it's a shame that the --snapshot/-s option is already taken
and likely to cause confusion.  The documentation needs to be very clear
that --snapshot and --snapshot-load refer to different snapshot
concepts.

Stefan

Re: [Qemu-devel] [PATCH 3/4] qemu-nbd: add doc for internal snapshot export

[Wenchao Xia]

于 2013-7-26 16:11, Stefan Hajnoczi 写道:
> On Wed, Jul 17, 2013 at 10:03:54PM +0800, Wenchao Xia wrote:
>> Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com>
>> ---
>>   qemu-nbd.c    |    2 ++
>>   qemu-nbd.texi |    3 +++
>>   2 files changed, 5 insertions(+), 0 deletions(-)
>>
>> diff --git a/qemu-nbd.c b/qemu-nbd.c
>> index 46be2b2..8eeee33 100644
>> --- a/qemu-nbd.c
>> +++ b/qemu-nbd.c
>> @@ -80,6 +80,8 @@ static void usage(const char *name)
>>   "Block device options:\n"
>>   "  -r, --read-only      export read-only\n"
>>   "  -s, --snapshot       use snapshot file\n"
>> +"  -l, --snapshot-load  temporarily load an internal snapshot and export it as\n"
>> +"                       an read-only device, format is 'id=[ID],name=[NAME]'\n"
> 
> Does it really export it read-only?  I think you'll get an error unless
> you pass qemu-nbd --read-only.
> 
  I think so. In patch 2:

+        case 'l':
+            sn_param = parse_option_parameters(optarg,
+                                               snapshot_options, sn_param);
+            if (!sn_param) {
+                errx(EXIT_FAILURE,
+                     "Invalid snapshot-load options '%s'", optarg);
+            }
         case 'r':
             nbdflags |= NBD_FLAG_READ_ONLY;
             flags &= ~BDRV_O_RDWR;

  when "l" is set, readonly will also be set.

-- 
Best Regards

Wenchao Xia

Re: [Qemu-devel] [PATCH 0/4] export internal snapshot by qemu-nbd

[Wenchao Xia]

于 2013-7-26 16:20, Stefan Hajnoczi 写道:
> On Fri, Jul 26, 2013 at 10:23:24AM +0800, Wenchao Xia wrote:
>> 于 2013-7-25 16:06, Stefan Hajnoczi 写道:
>>> On Thu, Jul 25, 2013 at 10:30:49AM +0800, Wenchao Xia wrote:
>>>>    Besides the argument, I think it helps to probe snapshot without
>>>> qemu-img convert, hope to get comments for the code.
>>>
>>> Hi Wenchao,
>>> Which patch are you referring to?
>>>
>>> Stefan
>>>
>>    This series, which enable qemu-nbd load a snapshot. It is quite
>> simple, look forward to your opinion, especially the parameter of
>> qemu-nbd part in patch 2.
> 
> I understand now.  Adding bdrv_snapshot_load_tmp() support to qemu-nbd
> is useful, it's a shame that the --snapshot/-s option is already taken
> and likely to cause confusion.  The documentation needs to be very clear
> that --snapshot and --snapshot-load refer to different snapshot
> concepts.
> 
  I'll add that section about the difference.

> Stefan
> 


-- 
Best Regards

Wenchao Xia

Re: [Qemu-devel] [PATCH 3/4] qemu-nbd: add doc for internal snapshot export

[Stefan Hajnoczi]

On Mon, Jul 29, 2013 at 09:57:32AM +0800, Wenchao Xia wrote:
> 于 2013-7-26 16:11, Stefan Hajnoczi 写道:
> > On Wed, Jul 17, 2013 at 10:03:54PM +0800, Wenchao Xia wrote:
> >> Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com>
> >> ---
> >>   qemu-nbd.c    |    2 ++
> >>   qemu-nbd.texi |    3 +++
> >>   2 files changed, 5 insertions(+), 0 deletions(-)
> >>
> >> diff --git a/qemu-nbd.c b/qemu-nbd.c
> >> index 46be2b2..8eeee33 100644
> >> --- a/qemu-nbd.c
> >> +++ b/qemu-nbd.c
> >> @@ -80,6 +80,8 @@ static void usage(const char *name)
> >>   "Block device options:\n"
> >>   "  -r, --read-only      export read-only\n"
> >>   "  -s, --snapshot       use snapshot file\n"
> >> +"  -l, --snapshot-load  temporarily load an internal snapshot and export it as\n"
> >> +"                       an read-only device, format is 'id=[ID],name=[NAME]'\n"
> > 
> > Does it really export it read-only?  I think you'll get an error unless
> > you pass qemu-nbd --read-only.
> > 
>   I think so. In patch 2:
> 
> +        case 'l':
> +            sn_param = parse_option_parameters(optarg,
> +                                               snapshot_options, sn_param);
> +            if (!sn_param) {
> +                errx(EXIT_FAILURE,
> +                     "Invalid snapshot-load options '%s'", optarg);
> +            }
>          case 'r':
>              nbdflags |= NBD_FLAG_READ_ONLY;
>              flags &= ~BDRV_O_RDWR;
> 
>   when "l" is set, readonly will also be set.

I missed that, please add a /* fall through */ comment to make readers
aware.

Stefan