From: Stefan Hajnoczi <stefanha@gmail.com>
To: Changlong Xie <xiecl.fnst@cn.fujitsu.com>
Cc: qemu devel <qemu-devel@nongnu.org>,
Eric Blake <eblake@redhat.com>, Alberto Garcia <berto@igalia.com>,
Kevin Wolf <kwolf@redhat.com>, Max Reitz <mreitz@redhat.com>,
Stefan Hajnoczi <stefanha@redhat.com>,
qemu block <qemu-block@nongnu.org>,
Jiang Yunhong <yunhong.jiang@intel.com>,
Dong Eddie <eddie.dong@intel.com>,
Markus Armbruster <armbru@redhat.com>,
"Dr. David Alan Gilbert" <dgilbert@redhat.com>
Subject: Re: [Qemu-devel] [PATCH v18 7/8] Implement new driver for block replication
Date: Fri, 6 May 2016 16:46:41 +0100 [thread overview]
Message-ID: <20160506154641.GA23075@stefanha-x1.localdomain> (raw)
In-Reply-To: <1460707838-13510-8-git-send-email-xiecl.fnst@cn.fujitsu.com>
[-- Attachment #1: Type: text/plain, Size: 2811 bytes --]
On Fri, Apr 15, 2016 at 04:10:37PM +0800, Changlong Xie wrote:
> +static void replication_close(BlockDriverState *bs)
> +{
> + BDRVReplicationState *s = bs->opaque;
> +
> + if (s->mode == REPLICATION_MODE_SECONDARY) {
> + g_free(s->top_id);
> + }
> +
> + if (s->replication_state == BLOCK_REPLICATION_RUNNING) {
> + replication_stop(s->rs, false, NULL);
> + }
There is a possible use-after-free with s->top_id. If we free it above
then replication_stop() must not call backup_job_cleanup(). I think it
could call it from replication_stop().
It would be safer to call replication_stop() before freeing s->top_id.
> + top_bs = bdrv_lookup_bs(s->top_id, s->top_id, errp);
Please check that bs is a child of top_bs. If it is not a child then
strange things could happen, for example the AioContexts might not match
(meaning it's not thread-safe) so this should be forbidden.
> + if (!top_bs) {
> + aio_context_release(aio_context);
> + return;
> + }
Error return paths after reopen_backing_file(s, true, &local_err) should
undo the operation.
> + bdrv_op_block_all(top_bs, s->blocker);
> + bdrv_op_unblock(top_bs, BLOCK_OP_TYPE_DATAPLANE, s->blocker);
> +
> + /*
> + * Must protect backup target if backup job was stopped/cancelled
> + * unexpectedly
> + */
> + bdrv_ref(s->hidden_disk->bs);
> +
> + backup_start(s->secondary_disk->bs, s->hidden_disk->bs, 0,
> + MIRROR_SYNC_MODE_NONE, NULL, BLOCKDEV_ON_ERROR_REPORT,
> + BLOCKDEV_ON_ERROR_REPORT, backup_job_completed,
> + s, NULL, &local_err);
Did you run stress tests where the primary is writing to the disk while
the secondary reads from the same sectors?
I thought about this some more and I'm wondering about the following
scenario:
NBD writes to secondary_disk and the guest reads from the disk at the
same time. There is a coroutine mutex in qcow2.c that protects both
read and write requests, but only until they perform the data I/O. It
may be possible that the read request from the Secondary VM could be
started before the NBD write but the preadv() syscall isn't entered
because of CPU scheduling decisions. In the meantime the
secondary_disk->hidden_disk backup operation takes place. With some
unlucky timing it may be possible for the Secondary VM to read the new
contents from secondary_disk instead of the old contents that were
backed up into hidden_disk.
Extra serialization would be needed.
block/backup.c:wait_for_overlapping_requests() and
block/io.c:mark_request_serialising() are good starting points for
solving this.
> + cleanup_imgs();
Please use qtest_add_abrt_handler() so cleanup happens even when SIGABRT
is received.
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 473 bytes --]
next prev parent reply other threads:[~2016-05-06 15:47 UTC|newest]
Thread overview: 18+ messages / expand[flat|nested] mbox.gz Atom feed top
2016-04-15 8:10 [Qemu-devel] [PATCH v18 0/8] Block replication for continuous checkpoints Changlong Xie
2016-04-15 8:10 ` [Qemu-devel] [PATCH v18 1/8] unblock backup operations in backing file Changlong Xie
2016-04-15 8:10 ` [Qemu-devel] [PATCH v18 2/8] Backup: clear all bitmap when doing block checkpoint Changlong Xie
2016-04-15 8:10 ` [Qemu-devel] [PATCH v18 3/8] Link backup into block core Changlong Xie
2016-04-15 8:10 ` [Qemu-devel] [PATCH v18 4/8] docs: block replication's description Changlong Xie
2016-04-15 8:10 ` [Qemu-devel] [PATCH v18 5/8] auto complete active commit Changlong Xie
2016-04-15 8:10 ` [Qemu-devel] [PATCH v18 6/8] Introduce new APIs to do replication operation Changlong Xie
2016-04-15 8:10 ` [Qemu-devel] [PATCH v18 7/8] Implement new driver for block replication Changlong Xie
2016-05-06 9:13 ` Changlong Xie
2016-05-06 15:46 ` Stefan Hajnoczi [this message]
2016-05-10 9:49 ` Changlong Xie
2016-05-18 9:19 ` Changlong Xie
2016-05-18 9:23 ` [Qemu-devel] [RFC] backup: export interfaces for extra serialization Changlong Xie
2016-04-15 8:10 ` [Qemu-devel] [PATCH v18 8/8] support replication driver in blockdev-add Changlong Xie
2016-04-25 9:40 ` [Qemu-devel] [PATCH v18 0/8] Block replication for continuous checkpoints Changlong Xie
2016-05-05 7:18 ` Hailiang Zhang
2016-05-06 15:51 ` [Qemu-devel] [Qemu-block] " Stefan Hajnoczi
2016-05-12 4:08 ` Changlong Xie
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20160506154641.GA23075@stefanha-x1.localdomain \
--to=stefanha@gmail.com \
--cc=armbru@redhat.com \
--cc=berto@igalia.com \
--cc=dgilbert@redhat.com \
--cc=eblake@redhat.com \
--cc=eddie.dong@intel.com \
--cc=kwolf@redhat.com \
--cc=mreitz@redhat.com \
--cc=qemu-block@nongnu.org \
--cc=qemu-devel@nongnu.org \
--cc=stefanha@redhat.com \
--cc=xiecl.fnst@cn.fujitsu.com \
--cc=yunhong.jiang@intel.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).