Re: [PATCH 1/3] xen/blkback: Support 'feature-barrier' aka old-style BARRIER requests.

[Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>]

On Mon, Oct 17, 2011 at 01:27:05PM +0100, Jan Beulich wrote:
> >>> On 10.10.11 at 17:28, Konrad Rzeszutek Wilk <konrad.wilk@oracle.com> wrote:
> > We emulate the barrier requests by draining the outstanding bio's
> > and then sending the WRITE_FLUSH command. To drain the I/Os
> > we use the refcnt that is used during disconnect to wait for all
> > the I/Os before disconnecting from the frontend. We latch on its
> > value and if it reaches either the threshold for disconnect or when
> > there are no more outstanding I/Os, then we have drained all I/Os.
> > 
> > Suggested-by: Christopher Hellwig <hch@infradead.org>
> > Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
> > ---
> >  drivers/block/xen-blkback/blkback.c |   37 +++++++++++++++++++++++++++++++++-
> >  drivers/block/xen-blkback/common.h  |    5 ++++
> >  drivers/block/xen-blkback/xenbus.c  |   18 +++++++++++++++++
> >  3 files changed, 58 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/block/xen-blkback/blkback.c 
> > b/drivers/block/xen-blkback/blkback.c
> > index e0dab61..184b133 100644
> > --- a/drivers/block/xen-blkback/blkback.c
> > +++ b/drivers/block/xen-blkback/blkback.c
> > @@ -452,6 +452,23 @@ static void xen_blk_discard(struct xen_blkif *blkif, 
> > struct blkif_request *req)
> >  	make_response(blkif, req->id, req->operation, status);
> >  }
> >  
> > +static void xen_blk_drain_io(struct xen_blkif *blkif)
> > +{
> > +	atomic_set(&blkif->drain, 1);
> > +	do {
> > +		wait_for_completion_interruptible_timeout(
> > +				&blkif->drain_complete, HZ);
> > +
> > +		if (!atomic_read(&blkif->drain))
> > +			break;
> > +		/* The initial value is one, and one refcnt taken at the
> > +		 * start of the xen_blkif_schedule thread. */
> > +		if (atomic_read(&blkif->refcnt) <= 2)
> > +			break;
> 
> Shouldn't this test be done the very first thing in the loop? It looks
> racy the way it's placed now, and it would incur a 1 sec stall if this
> was the only request currently being processed (as no completion
> of ane earlier request could signal completion).

Sure does. An earlier version of this (not posted), had this check right
before going in the loop as all of the requests might have been already
processed.

I accidently dropped that logic as I moved this whole code into a function.