Re: [Qemu-block] [PATCH v1 07/15] iotests: fix 097 when run with qcow
On Mon, Jan 16, 2017 at 09:04:31PM +0100, Max Reitz wrote: > On 03.01.2017 19:27, Daniel P. Berrange wrote: > > The previous commit: > > > > commit a3e1505daec31ef56f0489f8c8fff1b8e4ca92bd > > Author: Eric Blake > > Date: Mon Dec 5 09:49:34 2016 -0600 > > > > qcow2: Don't strand clusters near 2G intervals during commit > > > > extended the 097 test case so that it did two passes, once > > with an internal snapshot, once without. > > > > qcow (v1) does not support internal snapshots, so this change > > broke test 097 when run against qcow. > > > > This splits 097 in two, creating a new 173 that tests the > > internal snapshot codepath, effectively putting 097 back > > to its content before the above commit. > > > > Signed-off-by: Daniel P. Berrange > > --- > > tests/qemu-iotests/097 | 10 +--- > > tests/qemu-iotests/097.out | 125 > > ++-- > > tests/qemu-iotests/173 | 126 > > + > > tests/qemu-iotests/173.out | 119 ++ > > tests/qemu-iotests/group | 1 + > > 5 files changed, 251 insertions(+), 130 deletions(-) > > create mode 100755 tests/qemu-iotests/173 > > create mode 100644 tests/qemu-iotests/173.out > > I don't think the effort is worth it, considering that probably > literally nobody is still using qcow -- or so I hope, at least. The reason I fixed this was because I wanted to be able to verify my refactoring to qcow didn't break anything else. It is much easier if I can just run "check -qcow" and not have to worry about which failures are just test bugs, vs genuine code bugs I've created. IOW, as long as qcow.c exists in the code base, IMHO, we should make sure the iotests continue to pass On that point, IMHO, it would be beneficial if we had some CI system that was setup to run the iotests on all new changes, across all the different disk formats we expect the iotests to work on. We get far too many regressions with iotests breaking and no one noticing right now :-( Regards, Daniel -- |: http://berrange.com -o-http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://entangle-photo.org -o-http://search.cpan.org/~danberr/ :|
Re: [Qemu-block] [PATCH RFC v2 2/6] replication: add shared-disk and shared-disk-id options
On Mon, Dec 05, 2016 at 04:35:00PM +0800, zhanghailiang wrote:
> @@ -85,6 +99,9 @@ static int replication_open(BlockDriverState *bs, QDict
> *options,
> QemuOpts *opts = NULL;
> const char *mode;
> const char *top_id;
> +const char *shared_disk_id;
> +BlockBackend *blk;
> +BlockDriverState *tmp_bs;
>
> ret = -EINVAL;
> opts = qemu_opts_create(&replication_runtime_opts, NULL, 0,
> &error_abort);
> @@ -119,6 +136,25 @@ static int replication_open(BlockDriverState *bs, QDict
> *options,
> "The option mode's value should be primary or secondary");
> goto fail;
> }
> +s->is_shared_disk = qemu_opt_get_bool(opts, REPLICATION_SHARED_DISK,
> + false);
> +if (s->is_shared_disk && (s->mode == REPLICATION_MODE_PRIMARY)) {
> +shared_disk_id = qemu_opt_get(opts, REPLICATION_SHARED_DISK_ID);
> +if (!shared_disk_id) {
> +error_setg(&local_err, "Missing shared disk blk");
> +goto fail;
> +}
> +s->shared_disk_id = g_strdup(shared_disk_id);
> +blk = blk_by_name(s->shared_disk_id);
> +if (!blk) {
> +g_free(s->shared_disk_id);
> +error_setg(&local_err, "There is no %s block",
> s->shared_disk_id);
> +goto fail;
Please move the g_free() to the fail label to prevent future code
changes from introducing a memory leak.
signature.asc
Description: PGP signature
Re: [Qemu-block] [Qemu-stable] [PATCH] block/iscsi: avoid data corruption with cache=writeback
On Mon, 01/16 16:17, Peter Lieven wrote:
> nb_cls_shrunk in iscsi_allocmap_update can become -1 if the
> request starts and ends within the same cluster. This results
> in passing -1 to bitmap_set and bitmap_clear and they don't
> handle negative values properly. In the end this leads to data
> corruption.
>
> Fixes: e1123a3b40a1a9a625a29c8ed4debb7e206ea690
> Cc: qemu-sta...@nongnu.org
> Signed-off-by: Peter Lieven
> ---
> block/iscsi.c | 8 ++--
> 1 file changed, 6 insertions(+), 2 deletions(-)
>
> diff --git a/block/iscsi.c b/block/iscsi.c
> index 6aeeb9e..1860f1b 100644
> --- a/block/iscsi.c
> +++ b/block/iscsi.c
> @@ -499,14 +499,18 @@ iscsi_allocmap_update(IscsiLun *iscsilun, int64_t
> sector_num,
> if (allocated) {
> bitmap_set(iscsilun->allocmap, cl_num_expanded, nb_cls_expanded);
> } else {
> -bitmap_clear(iscsilun->allocmap, cl_num_shrunk, nb_cls_shrunk);
> +if (nb_cls_shrunk > 0) {
> +bitmap_clear(iscsilun->allocmap, cl_num_shrunk, nb_cls_shrunk);
> +}
> }
>
> if (iscsilun->allocmap_valid == NULL) {
> return;
> }
> if (valid) {
> -bitmap_set(iscsilun->allocmap_valid, cl_num_shrunk, nb_cls_shrunk);
> +if (nb_cls_shrunk > 0) {
> +bitmap_set(iscsilun->allocmap_valid, cl_num_shrunk,
> nb_cls_shrunk);
> +}
> } else {
> bitmap_clear(iscsilun->allocmap_valid, cl_num_expanded,
> nb_cls_expanded);
> --
> 1.9.1
>
>
It's probably a good idea to add assertions parameter in bitmap_*.
Reviewed-by: Fam Zheng
Re: [Qemu-block] [Qemu-stable] [PATCH] block/iscsi: avoid data corruption with cache=writeback
Am 17.01.2017 um 12:28 schrieb Fam Zheng:
On Mon, 01/16 16:17, Peter Lieven wrote:
nb_cls_shrunk in iscsi_allocmap_update can become -1 if the
request starts and ends within the same cluster. This results
in passing -1 to bitmap_set and bitmap_clear and they don't
handle negative values properly. In the end this leads to data
corruption.
Fixes: e1123a3b40a1a9a625a29c8ed4debb7e206ea690
Cc: qemu-sta...@nongnu.org
Signed-off-by: Peter Lieven
---
block/iscsi.c | 8 ++--
1 file changed, 6 insertions(+), 2 deletions(-)
diff --git a/block/iscsi.c b/block/iscsi.c
index 6aeeb9e..1860f1b 100644
--- a/block/iscsi.c
+++ b/block/iscsi.c
@@ -499,14 +499,18 @@ iscsi_allocmap_update(IscsiLun *iscsilun, int64_t
sector_num,
if (allocated) {
bitmap_set(iscsilun->allocmap, cl_num_expanded, nb_cls_expanded);
} else {
-bitmap_clear(iscsilun->allocmap, cl_num_shrunk, nb_cls_shrunk);
+if (nb_cls_shrunk > 0) {
+bitmap_clear(iscsilun->allocmap, cl_num_shrunk, nb_cls_shrunk);
+}
}
if (iscsilun->allocmap_valid == NULL) {
return;
}
if (valid) {
-bitmap_set(iscsilun->allocmap_valid, cl_num_shrunk, nb_cls_shrunk);
+if (nb_cls_shrunk > 0) {
+bitmap_set(iscsilun->allocmap_valid, cl_num_shrunk, nb_cls_shrunk);
+}
} else {
bitmap_clear(iscsilun->allocmap_valid, cl_num_expanded,
nb_cls_expanded);
--
1.9.1
It's probably a good idea to add assertions parameter in bitmap_*.
yes, i will send a follow-up patch.
Peter
Re: [Qemu-block] [PATCH RFC v2 3/6] replication: Split out backup_do_checkpoint() from secondary_do_checkpoint()
On Mon, Dec 05, 2016 at 04:35:01PM +0800, zhanghailiang wrote: > The helper backup_do_checkpoint() will be used for primary related > codes. Here we split it out from secondary_do_checkpoint(). > > Besides, it is unnecessary to call backup_do_checkpoint() in > replication starting and normally stop replication path. > We only need call it while do real checkpointing. > > Signed-off-by: zhanghailiang > --- > block/replication.c | 36 +++- > 1 file changed, 19 insertions(+), 17 deletions(-) Reviewed-by: Stefan Hajnoczi signature.asc Description: PGP signature
Re: [Qemu-block] [PATCH RFC v2 4/6] replication: fix code logic with the new shared_disk option
On Mon, Dec 05, 2016 at 04:35:02PM +0800, zhanghailiang wrote: > Some code logic only be needed in non-shared disk, here > we adjust these codes to prepare for shared disk scenario. > > Signed-off-by: zhanghailiang > --- > block/replication.c | 47 --- > 1 file changed, 28 insertions(+), 19 deletions(-) Reviewed-by: Stefan Hajnoczi signature.asc Description: PGP signature
Re: [Qemu-block] [PATCH RFC v2 5/6] replication: Implement block replication for shared disk case
On Mon, Dec 05, 2016 at 04:35:03PM +0800, zhanghailiang wrote:
> @@ -663,8 +695,12 @@ static void replication_stop(ReplicationState *rs, bool
> failover, Error **errp)
>
> switch (s->mode) {
> case REPLICATION_MODE_PRIMARY:
> -s->replication_state = BLOCK_REPLICATION_DONE;
> -s->error = 0;
> +if (s->is_shared_disk && s->primary_disk->bs->job) {
> +block_job_cancel(s->primary_disk->bs->job);
Should this be block_job_cancel_sync()?
> +} else {
> +s->replication_state = BLOCK_REPLICATION_DONE;
> +s->error = 0;
> +}
> break;
> case REPLICATION_MODE_SECONDARY:
> /*
> --
> 1.8.3.1
>
>
signature.asc
Description: PGP signature
Re: [Qemu-block] [Qemu-devel] [PATCH 3/3] block: get max_transfer limit for char (scsi-generic) devices
On 01/17/2017 02:04 AM, Fam Zheng wrote:
On Mon, 01/16 22:12, Eric Farman wrote:
Commit 6f607174 introduced a routine to get the maximum number
of bytes for a single I/O transfer for block devices, however
scsi generic devices are character devices, not block. Add
a condition for this, with slightly different logic because
the value is already in bytes, and need not be converted from
blocks as happens for block devices.
Signed-off-by: Eric Farman
---
block/file-posix.c | 7 +++
1 file changed, 7 insertions(+)
diff --git a/block/file-posix.c b/block/file-posix.c
index 2115155..c0843c2 100644
--- a/block/file-posix.c
+++ b/block/file-posix.c
@@ -679,6 +679,13 @@ static void raw_refresh_limits(BlockDriverState *bs, Error
**errp)
if (ret > 0 && ret <= BDRV_REQUEST_MAX_SECTORS) {
bs->bl.max_transfer = pow2floor(ret << BDRV_SECTOR_BITS);
}
+} else if (S_ISCHR(st.st_mode)) {
+/* sg returns transfer length in bytes already */
+int ret = hdev_get_max_transfer_length(bs, s->fd);
+if (ret > 0 &&
+(ret >> BDRV_SECTOR_BITS) <= BDRV_REQUEST_MAX_SECTORS) {
+bs->bl.max_transfer = pow2floor(ret);
+}
Please keep the sectors/bytes quirk in hdev_get_max_transfer_length and always
return bytes from there.
That's easy enough. I'll allow a day or two before sending a v2, in
case there's other considerations for the rats nest I've wandered into.
Thanks!
- Eric
Re: [Qemu-block] [PATCH v6 0/2] allow blockdev-add for NFS
Am 31.10.2016 um 18:20 schrieb Kevin Wolf:
Am 31.10.2016 um 16:05 hat Ashijeet Acharya geschrieben:
Previously posted series patches:
v5: https://lists.gnu.org/archive/html/qemu-devel/2016-10/msg07580.html
v4: https://lists.gnu.org/archive/html/qemu-devel/2016-10/msg07449.html
v3: https://lists.gnu.org/archive/html/qemu-devel/2016-10/msg06903.html
v2: https://lists.gnu.org/archive/html/qemu-devel/2016-10/msg05844.html
v1: https://lists.gnu.org/archive/html/qemu-devel/2016-10/msg04487.html
This series adds blockdev-add support for NFS block driver.
Thanks, fixed as commented on patch 1 and applied.
Hi,
it seems this series breaks passing options via URI.
1) in nfs_parse_uri
parse_uint_full(qp->p[i].value, NULL, 0)
segfaults, as the routine wants to set *NULL = val.
Program received signal SIGSEGV, Segmentation fault.
parse_uint (s=0x55d0ead0 "131072", value=value@entry=0x0,
endptr=endptr@entry=0x7fffd870, base=base@entry=0) at util/cutils.c:475
475*value = val;
(gdb) bt full
#0 parse_uint (s=0x55d0ead0 "131072", value=value@entry=0x0,
endptr=endptr@entry=0x7fffd870, base=base@entry=0) at util/cutils.c:475
r = 0
endp = 0x55d0ead6 ""
val = 131072
#1 0x55655ff2 in parse_uint_full (s=,
value=value@entry=0x0, base=base@entry=0) at util/cutils.c:499
endp = 0x55d093f0 "\004"
r =
#2 0x55603425 in nfs_parse_uri (filename=,
options=0x55d093f0, errp=0x7fffd980) at block/nfs.c:116
uri = 0x55d0e920
qp = 0x55d0ea30
ret = -22
i = 0
__func__ = "nfs_parse_uri"
#3 0x5559c7bb in bdrv_fill_options (errp=0x7fffd968, flags=0x7fffd95c,
filename=, options=) at block.c:1278
drvname =
protocol =
local_err = 0x0
parse_filename = true
drv = 0x558e3140
2) all parameters that have a different names in options and qdict
e.g. readahead-size vs. readahead cannot be passed via URI.
$ qemu-img convert -p
nfs://172.21.200.61/templates/VC_debian8-20170116.qcow2,linux\?readahead=131072
iscsi://172.21.200.56:3260/iqn.2001-05.com.equallogic:0-8a0906-69d384e0a-aa3004e55e15878d-XXX/0
qemu-img: Could not open
'nfs://172.21.200.61/vcore-dev-cdrom/templates/VC_debian8-20170116.qcow2,linux?readahead=131072':
Block protocol 'nfs' doesn't support the option 'readahead-size'
Please let me know if the below fix would be correct:
lieven@lieven-pc:~/git/qemu$ git diff
diff --git a/block/nfs.c b/block/nfs.c
index a564340..0ff8268 100644
--- a/block/nfs.c
+++ b/block/nfs.c
@@ -108,30 +108,31 @@ static int nfs_parse_uri(const char *filename, QDict
*options, Error **errp)
qdict_put(options, "path", qstring_from_str(uri->path));
for (i = 0; i < qp->n; i++) {
+unsigned long long val;
if (!qp->p[i].value) {
error_setg(errp, "Value for NFS parameter expected: %s",
qp->p[i].name);
goto out;
}
-if (parse_uint_full(qp->p[i].value, NULL, 0)) {
+if (parse_uint_full(qp->p[i].value, &val, 0)) {
error_setg(errp, "Illegal value for NFS parameter: %s",
qp->p[i].name);
goto out;
}
if (!strcmp(qp->p[i].name, "uid")) {
-qdict_put(options, "user",
+qdict_put(options, "uid",
qstring_from_str(qp->p[i].value));
} else if (!strcmp(qp->p[i].name, "gid")) {
-qdict_put(options, "group",
+qdict_put(options, "gid",
qstring_from_str(qp->p[i].value));
} else if (!strcmp(qp->p[i].name, "tcp-syncnt")) {
-qdict_put(options, "tcp-syn-count",
+qdict_put(options, "tcp-syncnt",
qstring_from_str(qp->p[i].value));
} else if (!strcmp(qp->p[i].name, "readahead")) {
-qdict_put(options, "readahead-size",
+qdict_put(options, "readahead",
qstring_from_str(qp->p[i].value));
} else if (!strcmp(qp->p[i].name, "pagecache")) {
-qdict_put(options, "page-cache-size",
+qdict_put(options, "pagecache",
qstring_from_str(qp->p[i].value));
} else if (!strcmp(qp->p[i].name, "debug")) {
qdict_put(options, "debug",
Thanks,
Peter
Re: [Qemu-block] [Qemu-devel] [PATCH v2] pflash_cfi01: fix per device sector length in CFI table
Am 16.01.2017 um 11:26 schrieb Dr. David Alan Gilbert:
* Andrew Jones (drjo...@redhat.com) wrote:
On Thu, Jan 12, 2017 at 10:42:41AM +, Peter Maydell wrote:
On 12 January 2017 at 10:35, David Engraf wrote:
The CFI entry for sector length must be set to sector length per device.
This is important for boards using multiple devices like the ARM Vexpress
board (width = 4, device-width = 2).
Linux and u-boots calculate the size ratio by dividing both values:
size_ratio = info->portwidth / info->chipwidth;
After that the sector length will be multiplied by the size_ratio, thus the
CFI entry for sector length is doubled. When Linux or u-boot send a sector
erase, they expect to erase the doubled sector length, but QEMU only erases
the board specified sector length.
This patch fixes the sector length in the CFI table to match the length per
device, equal to blocks_per_device.
Thanks for the patch. I haven't checked against the pflash spec yet,
but this looks like it's probably the right thing.
The only two machines which use a setup with multiple devices (ie
which specify device_width to the pflash_cfi01) are vexpress and virt.
For all other machines this patch leaves the behaviour unchanged.
Q: do we need to have some kind of nasty hack so that pre-2.9 virt
still gets the old broken values in the CFI table, for version and
migration compatibility? Ccing Drew for an opinion...
I'm pretty sure we need the nasty hack, but I'm also Ccing David for
his opinion.
Hmm I don't understand enough about pflash to understand the change here;
but generally if you need to keep something unchanged for older
machine types, add a property and then set that property in
the compatibility macros.
See include/hw/compat.h, I think you'd add the entry to HW_COMPAT_2_8
and follow a simple example like old_msi_addr on intel-hda.c,
that should get picked up by aarch, x86, ppc etc versioned machine types.
This is a new version of the previous patch including the HW_COMPAT entry.
After further tests with u-boot and Linux, I found another problem with
the blocks per device and write block size. Blocks per device must not
be divided with the number of devices because the resulting device size
would not match the overall size. However write block size must be
modified depending on the number of devices because the entry is per
device and when u-boot writes into the flash it calculates the write
size by using the CFI entry (write size per device) multiplied by the
number of chips.
Here is a configuration example of the vexpress board:
VEXPRESS_FLASH_SIZE = 64M
VEXPRESS_FLASH_SECT_SIZE 256K
num-blocks = VEXPRESS_FLASH_SIZE / VEXPRESS_FLASH_SECT_SIZE = 256
sector-length = 256K
width = 4
device-width = 2
The code will fill the CFI entry with the following entries
num-blocks = 256
sector-length = 128K
writeblock_size = 2048
This results in two chips, each with 256 * 128K = 32M device size and a
write block size of 2048.
A sector erase will be send to both chips, thus 256K must be erased.
When u-boot sends a block write command, it will write 4096 bytes data
at once (2048 per device).
I did not modify the CFI entry for write block size. Instead I kept the
hard coded value of 2048 in the CFI table and fixed the internal entry
for writeblock_size.
Best regards
- David
Signed-off-by: David Engraf
diff --git a/hw/block/pflash_cfi01.c b/hw/block/pflash_cfi01.c
index 5f0ee9d..996daa3 100644
--- a/hw/block/pflash_cfi01.c
+++ b/hw/block/pflash_cfi01.c
@@ -99,6 +99,7 @@ struct pflash_t {
char *name;
void *storage;
VMChangeStateEntry *vmstate;
+bool old_multiple_chip_handling;
};
static int pflash_post_load(void *opaque, int version_id);
@@ -703,7 +704,7 @@ static void pflash_cfi01_realize(DeviceState *dev, Error **errp)
pflash_t *pfl = CFI_PFLASH01(dev);
uint64_t total_len;
int ret;
-uint64_t blocks_per_device, device_len;
+uint64_t blocks_per_device, sector_len_per_device, device_len;
int num_devices;
Error *local_err = NULL;
@@ -726,8 +727,14 @@ static void pflash_cfi01_realize(DeviceState *dev, Error **errp)
* in the cfi_table[].
*/
num_devices = pfl->device_width ? (pfl->bank_width / pfl->device_width) : 1;
-blocks_per_device = pfl->nb_blocs / num_devices;
-device_len = pfl->sector_len * blocks_per_device;
+if (pfl->old_multiple_chip_handling) {
+blocks_per_device = pfl->nb_blocs / num_devices;
+sector_len_per_device = pfl->sector_len;
+} else {
+blocks_per_device = pfl->nb_blocs;
+sector_len_per_device = pfl->sector_len / num_devices;
+}
+device_len = sector_len_per_device * blocks_per_device;
/* XXX: to be fixed */
#if 0
@@ -832,6 +839,8 @@ static void pflash_cfi01_realize(DeviceState *dev, Error **errp)
pfl->cfi_table[0x2A] = 0x0B;
}
pfl->writeblock_size = 1 << pfl->cfi_table[0x2A];
+if (!pfl->old_multiple_chip_handling && num_devices > 1)
Re: [Qemu-block] [PATCH RFC v2 5/6] replication: Implement block replication for shared disk case
Hi Stefan,
On 2017/1/17 21:19, Stefan Hajnoczi wrote:
On Mon, Dec 05, 2016 at 04:35:03PM +0800, zhanghailiang wrote:
@@ -663,8 +695,12 @@ static void replication_stop(ReplicationState *rs, bool
failover, Error **errp)
switch (s->mode) {
case REPLICATION_MODE_PRIMARY:
-s->replication_state = BLOCK_REPLICATION_DONE;
-s->error = 0;
+if (s->is_shared_disk && s->primary_disk->bs->job) {
+block_job_cancel(s->primary_disk->bs->job);
Should this be block_job_cancel_sync()?
No, here it is different from the secondary side which needs to wait
until backup job been canceled before resumes to run (Or there will be
an error, https://patchwork.kernel.org/patch/9128841/).
For primary VM, Just as you can see the design scenario in patch 1,
It accesses the shared disk directly, the backup job whose source side
is just the shared disk does not influence primary VM's running,
So IMHO, it is safe to call block_job_cancel here.
Thanks,
Hailiang
+} else {
+s->replication_state = BLOCK_REPLICATION_DONE;
+s->error = 0;
+}
break;
case REPLICATION_MODE_SECONDARY:
/*
--
1.8.3.1
Re: [Qemu-block] [PATCH RFC v2 4/6] replication: fix code logic with the new shared_disk option
On 2016/12/20 20:42, Changlong Xie wrote:
On 12/05/2016 04:35 PM, zhanghailiang wrote:
Some code logic only be needed in non-shared disk, here
we adjust these codes to prepare for shared disk scenario.
Signed-off-by: zhanghailiang
---
block/replication.c | 47 ---
1 file changed, 28 insertions(+), 19 deletions(-)
diff --git a/block/replication.c b/block/replication.c
index 35e9ab3..6574cc2 100644
--- a/block/replication.c
+++ b/block/replication.c
@@ -531,21 +531,28 @@ static void replication_start(ReplicationState *rs,
ReplicationMode mode,
aio_context_release(aio_context);
return;
}
-bdrv_op_block_all(top_bs, s->blocker);
-bdrv_op_unblock(top_bs, BLOCK_OP_TYPE_DATAPLANE, s->blocker);
-job = backup_job_create(NULL, s->secondary_disk->bs,
s->hidden_disk->bs,
-0, MIRROR_SYNC_MODE_NONE, NULL, false,
+/*
+ * Only in the case of non-shared disk,
+ * the backup job is in the secondary side
+ */
+if (!s->is_shared_disk) {
+bdrv_op_block_all(top_bs, s->blocker);
+bdrv_op_unblock(top_bs, BLOCK_OP_TYPE_DATAPLANE, s->blocker);
+job = backup_job_create(NULL, s->secondary_disk->bs,
+s->hidden_disk->bs, 0,
+MIRROR_SYNC_MODE_NONE, NULL, false,
BLOCKDEV_ON_ERROR_REPORT,
BLOCKDEV_ON_ERROR_REPORT, BLOCK_JOB_INTERNAL,
backup_job_completed, bs, NULL, &local_err);
Coding style here.
Will fix it in next version, thanks.
-if (local_err) {
-error_propagate(errp, local_err);
-backup_job_cleanup(bs);
-aio_context_release(aio_context);
-return;
+if (local_err) {
+error_propagate(errp, local_err);
+backup_job_cleanup(bs);
+aio_context_release(aio_context);
+return;
+}
+block_job_start(job);
}
-block_job_start(job);
secondary_do_checkpoint(s, errp);
break;
@@ -575,14 +582,16 @@ static void replication_do_checkpoint(ReplicationState
*rs, Error **errp)
case REPLICATION_MODE_PRIMARY:
break;
case REPLICATION_MODE_SECONDARY:
-if (!s->secondary_disk->bs->job) {
-error_setg(errp, "Backup job was cancelled unexpectedly");
-break;
-}
-backup_do_checkpoint(s->secondary_disk->bs->job, &local_err);
-if (local_err) {
-error_propagate(errp, local_err);
-break;
+if (!s->is_shared_disk) {
+if (!s->secondary_disk->bs->job) {
+error_setg(errp, "Backup job was cancelled unexpectedly");
+break;
+}
+backup_do_checkpoint(s->secondary_disk->bs->job, &local_err);
+if (local_err) {
+error_propagate(errp, local_err);
+break;
+}
}
secondary_do_checkpoint(s, errp);
break;
@@ -663,7 +672,7 @@ static void replication_stop(ReplicationState *rs, bool
failover, Error **errp)
* before the BDS is closed, because we will access hidden
* disk, secondary disk in backup_job_completed().
*/
-if (s->secondary_disk->bs->job) {
+if (!s->is_shared_disk && s->secondary_disk->bs->job) {
block_job_cancel_sync(s->secondary_disk->bs->job);
}
.
Re: [Qemu-block] [Qemu-devel] [PATCH RFC v2 2/6] replication: add shared-disk and shared-disk-id options
On 2016/12/6 0:22, Eric Blake wrote:
On 12/05/2016 02:35 AM, zhanghailiang wrote:
We use these two options to identify which disk is
shared
Signed-off-by: zhanghailiang
Signed-off-by: Wen Congyang
Signed-off-by: Zhang Chen
---
v2:
- add these two options for BlockdevOptionsReplication to support
qmp blockdev-add command.
- fix a memory leak found by Changlong
---
+++ b/qapi/block-core.json
@@ -2232,12 +2232,19 @@
# node who owns the replication node chain. Must not be given in
# primary mode.
#
+# @shared-disk-id: #optional The id of shared disk while in replication mode.
+#
+# @shared-disk: #optional To indicate whether or not a disk is shared by
+# primary VM and secondary VM.
Both of these need a '(since 2.9)' designation since they've missed 2.8,
and could probably also use documentation on the default value assumed
when the parameter is omitted.
OK, i will add it in next version, thanks.
+#
# Since: 2.8
##
{ 'struct': 'BlockdevOptionsReplication',
'base': 'BlockdevOptionsGenericFormat',
'data': { 'mode': 'ReplicationMode',
-'*top-id': 'str' } }
+'*top-id': 'str',
+'*shared-disk-id': 'str',
+'*shared-disk': 'bool' } }
##
# @NFSTransport
Re: [Qemu-block] [PATCH RFC v2 2/6] replication: add shared-disk and shared-disk-id options
On 2017/1/17 19:25, Stefan Hajnoczi wrote:
On Mon, Dec 05, 2016 at 04:35:00PM +0800, zhanghailiang wrote:
@@ -85,6 +99,9 @@ static int replication_open(BlockDriverState *bs, QDict
*options,
QemuOpts *opts = NULL;
const char *mode;
const char *top_id;
+const char *shared_disk_id;
+BlockBackend *blk;
+BlockDriverState *tmp_bs;
ret = -EINVAL;
opts = qemu_opts_create(&replication_runtime_opts, NULL, 0, &error_abort);
@@ -119,6 +136,25 @@ static int replication_open(BlockDriverState *bs, QDict
*options,
"The option mode's value should be primary or secondary");
goto fail;
}
+s->is_shared_disk = qemu_opt_get_bool(opts, REPLICATION_SHARED_DISK,
+ false);
+if (s->is_shared_disk && (s->mode == REPLICATION_MODE_PRIMARY)) {
+shared_disk_id = qemu_opt_get(opts, REPLICATION_SHARED_DISK_ID);
+if (!shared_disk_id) {
+error_setg(&local_err, "Missing shared disk blk");
+goto fail;
+}
+s->shared_disk_id = g_strdup(shared_disk_id);
+blk = blk_by_name(s->shared_disk_id);
+if (!blk) {
+g_free(s->shared_disk_id);
+error_setg(&local_err, "There is no %s block", s->shared_disk_id);
+goto fail;
Please move the g_free() to the fail label to prevent future code
changes from introducing a memory leak.
OK, I will fix it in next version, thanks very much.
