From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:43688) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1X5oAW-0001H0-Sh for qemu-devel@nongnu.org; Fri, 11 Jul 2014 23:40:17 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1X5oAR-0003jL-Vb for qemu-devel@nongnu.org; Fri, 11 Jul 2014 23:40:12 -0400 Received: from mail-pa0-x229.google.com ([2607:f8b0:400e:c03::229]:58322) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1X5oAR-0003ib-OC for qemu-devel@nongnu.org; Fri, 11 Jul 2014 23:40:07 -0400 Received: by mail-pa0-f41.google.com with SMTP id fb1so2483983pad.28 for ; Fri, 11 Jul 2014 20:40:06 -0700 (PDT) Date: Sat, 12 Jul 2014 11:40:02 +0800 From: Liu Yuan Message-ID: <20140712034002.GA23778@ubuntu-trusty> References: <1405047682-26102-1-git-send-email-namei.unix@gmail.com> <20140711105653.GB4493@irqsave.net> MIME-Version: 1.0 Content-Type: text/plain; charset=iso-8859-1 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: <20140711105653.GB4493@irqsave.net> Subject: Re: [Qemu-devel] [PATCH] block/quorum: add simple read pattern support List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: =?iso-8859-1?Q?Beno=EEt?= Canet Cc: Kevin Wolf , qemu-devel@nongnu.org, Stefan Hajnoczi On Fri, Jul 11, 2014 at 12:56:53PM +0200, Benoît Canet wrote: > The Friday 11 Jul 2014 à 11:01:22 (+0800), Liu Yuan wrote : > > This patch adds single read pattern to quorum driver and quorum vote is default > > pattern. > > > > For now we do a quorum vote on all the reads, it is designed for unreliable > > underlying storage such as non-redundant NFS to make sure data integrity at the > > cost of the read performance. > > > > For some use cases as following: > > > > VM > > -------------- > > | | > > v v > > A B > > > > Both A and B has hardware raid storage to justify the data integrity on its own. > > So it would help performance if we do a single read instead of on all the nodes. > > Further, if we run VM on either of the storage node, we can make a local read > > request for better performance. > > > > This patch generalize the above 2 nodes case in the N nodes. That is, > > > > vm -> write to all the N nodes, read just one of them. If single read fails, we > > try to read next node in FIFO order specified by the startup command. > > > > The 2 nodes case is very similar to DRBD[1] though lack of auto-sync > > functionality in the single device/node failure for now. But compared with DRBD > > we still have some advantages over it: > > > > - Suppose we have 20 VMs running on one(assume A) of 2 nodes' DRBD backed > > storage. And if A crashes, we need to restart all the VMs on node B. But for > > practice case, we can't because B might not have enough resources to setup 20 VMs > > at once. So if we run our 20 VMs with quorum driver, and scatter the replicated > > images over the data center, we can very likely restart 20 VMs without any > > resource problem. > > > > After all, I think we can build a more powerful replicated image functionality > > on quorum and block jobs(block mirror) to meet various High Availibility needs. > > > > E.g, Enable single read pattern on 2 children, > > > > -drive driver=quorum,children.0.file.filename=0.qcow2,\ > > children.1.file.filename=1.qcow2,read-pattern=single,vote-threshold=1 > > > > [1] http://en.wikipedia.org/wiki/Distributed_Replicated_Block_Device > > > > Cc: Benoit Canet > > Cc: Kevin Wolf > > Cc: Stefan Hajnoczi > > Signed-off-by: Liu Yuan > > --- > > block/quorum.c | 174 +++++++++++++++++++++++++++++++++++++++++---------------- > > 1 file changed, 125 insertions(+), 49 deletions(-) > > > > diff --git a/block/quorum.c b/block/quorum.c > > index d5ee9c0..2f18755 100644 > > --- a/block/quorum.c > > +++ b/block/quorum.c > > @@ -24,6 +24,7 @@ > > #define QUORUM_OPT_VOTE_THRESHOLD "vote-threshold" > > #define QUORUM_OPT_BLKVERIFY "blkverify" > > #define QUORUM_OPT_REWRITE "rewrite-corrupted" > > +#define QUORUM_OPT_READ_PATTERN "read-pattern" > > > > /* This union holds a vote hash value */ > > typedef union QuorumVoteValue { > > @@ -74,6 +75,16 @@ typedef struct BDRVQuorumState { > > bool rewrite_corrupted;/* true if the driver must rewrite-on-read corrupted > > * block if Quorum is reached. > > */ > > + > > +#define READ_PATTERN_QUORUM 0 /* default */ > > +#define READ_PATTERN_SINGLE 1 > > Why not making these choices an enum ? It is just a matter of taste. For now we only have 2 constants and the bonus to have #define here is we can put them near the 'read_pattern' for easier code read. > Would READ_PATTERN_SINGLE be more accuratelly described by READ_PATTERN_ROUND_ROBIN ? How about FIFO? Since 'quorum' is a single word... > More generaly would s/single/round-robin/ be more descriptive. ditto. > > > + int read_pattern; /* single: read a single child and try first one > > + * first. If error, try next child in an > > + * FIFO order specifed by command line. > > + * Return error if no child read succeeds. > > + * quorum: read all the children and do a quorum > > + * vote on reads. > > + */ > > } BDRVQuorumState; > > > > typedef struct QuorumAIOCB QuorumAIOCB; > > @@ -117,6 +128,7 @@ struct QuorumAIOCB { > > > > bool is_read; > > int vote_ret; > > + int child_iter; /* which child to read in single pattern */ > > }; > > > > static bool quorum_vote(QuorumAIOCB *acb); > > @@ -256,6 +268,21 @@ static void quorum_rewrite_aio_cb(void *opaque, int ret) > > quorum_aio_finalize(acb); > > } > > > > +static BlockDriverAIOCB *read_single_child(QuorumAIOCB *acb); > > + > > +static void quorum_copy_qiov(QEMUIOVector *dest, QEMUIOVector *source) > > +{ > > + int i; > > + assert(dest->niov == source->niov); > > + assert(dest->size == source->size); > > + for (i = 0; i < source->niov; i++) { > > + assert(dest->iov[i].iov_len == source->iov[i].iov_len); > > + memcpy(dest->iov[i].iov_base, > > + source->iov[i].iov_base, > > + source->iov[i].iov_len); > > + } > > +} > > > + > > static void quorum_aio_cb(void *opaque, int ret) > > { > > QuorumChildRequest *sacb = opaque; > > @@ -263,6 +290,21 @@ static void quorum_aio_cb(void *opaque, int ret) > > BDRVQuorumState *s = acb->common.bs->opaque; > > bool rewrite = false; > > > > + if (acb->is_read && s->read_pattern == READ_PATTERN_SINGLE) { > > + /* We try to read next child in FIFO order if we fail to read */ > > + if (ret < 0 && ++acb->child_iter < s->num_children) { > > + read_single_child(acb); > > Here the previous failed read is never finalized/freed. quorum_aio_finalize() will free all the reads. But yes, I need to modify quorum_aio_finalize() to free exact reads we have done (all the failed read + succeeded read) > > + return; > > + } > > + > > + if (ret == 0) { > > + quorum_copy_qiov(acb->qiov, &acb->qcrs[acb->child_iter].qiov); > > + } > > + acb->vote_ret = ret; > > + quorum_aio_finalize(acb); > > + return; > > + } > > + > > sacb->ret = ret; > > acb->count++; > > if (ret == 0) { > > @@ -276,7 +318,6 @@ static void quorum_aio_cb(void *opaque, int ret) > > return; > > } > > > > - /* Do the vote on read */ > > Why removing this comment ? Hmmm, probably at then time I thought the name of quorum_vote() already desribed it. But yes, I shouldn't touch the lines that I don't modify. Thanks Yuan