From mboxrd@z Thu Jan 1 00:00:00 1970 From: "Jan Beulich" Subject: Re: [PATCH] blkback: Fix block I/O latency issue Date: Mon, 02 May 2011 09:13:19 +0100 Message-ID: <4DBE83BF020000780003F1DC@vpn.id2.novell.com> References: Mime-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: quoted-printable Return-path: In-Reply-To: Content-Disposition: inline List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xensource.com Errors-To: xen-devel-bounces@lists.xensource.com To: Pradeep Vincent Cc: "xen-devel@lists.xensource.com" List-Id: xen-devel@lists.xenproject.org >>> On 02.05.11 at 09:04, "Vincent, Pradeep" wrote: > In blkback driver, after I/O requests are submitted to Dom-0 block = I/O=20 > subsystem, blkback goes to 'sleep' effectively without letting blkfront = know=20 > about it (req_event isn't set appropriately). Hence blkfront doesn't = notify=20 > blkback when it submits a new I/O thus delaying the 'dispatch' of the = new I/O=20 > to Dom-0 block I/O subsystem. The new I/O is dispatched as soon as one = of the=20 > previous I/Os completes. >=20 > As a result of this issue, the block I/O latency performance is degraded = for=20 > some workloads on Xen guests using blkfront-blkback stack. >=20 > The following change addresses this issue: >=20 >=20 > Signed-off-by: Pradeep Vincent >=20 > diff --git a/drivers/xen/blkback/blkback.c b/drivers/xen/blkback/blkback.= c > --- a/drivers/xen/blkback/blkback.c > +++ b/drivers/xen/blkback/blkback.c > @@ -383,6 +383,12 @@ static int do_block_io_op(blkif_t *blkif) > cond_resched(); > } >=20 > + /* If blkback might go to sleep (i.e. more_to_do =3D=3D 0) then we = better > + let blkfront know about it (by setting req_event appropriately) so = that > + blkfront will bother to wake us up (via interrupt) when it submits a > + new I/O */ > + if (!more_to_do) > + RING_FINAL_CHECK_FOR_REQUESTS(&blk_rings->common, = more_to_do); To me this contradicts the comment preceding the use of RING_FINAL_CHECK_FOR_REQUESTS() in make_response() (there it's supposedly used to avoid unnecessary notification, here you say it's used to force notification). Albeit I agree that the change looks consistent with the comments in io/ring.h. Even if correct, you're not holding blkif->blk_ring_lock here, and hence I think you'll need to explain how this is not a problem. >>From a formal perspective, you also want to correct usage of tabs, and (assuming this is intended for the 2.6.18 tree) you'd also need to indicate so for Keir to pick this up and apply it to that tree (and it might then also be a good idea to submit an equivalent patch for the pv-ops trees). Jan > return more_to_do; > }