Re: [Qemu-devel] [PATCH] block/quorum: add simple read pattern support

[Liu Yuan]

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 <address@hidden>
> > Cc: Kevin Wolf <address@hidden>
> > Cc: Stefan Hajnoczi <address@hidden>
> > Signed-off-by: Liu Yuan <address@hidden>
> > ---
> >  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