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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20111017165247.GB19724@phenom.dumpdata.com>
Date:	Mon, 17 Oct 2011 12:52:47 -0400
From:	Konrad Rzeszutek Wilk <konrad.wilk@...cle.com>
To:	Jan Beulich <JBeulich@...e.com>
Cc:	hch@...radead.org, xen-devel@...ts.xensource.com,
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH 1/3] xen/blkback: Support 'feature-barrier' aka old-style
 BARRIER requests.

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@...cle.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@...radead.org>
> > Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@...cle.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.

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/