From mboxrd@z Thu Jan 1 00:00:00 1970 From: "Vincent, Pradeep" Subject: Re: [PATCH] blkback: Fix block I/O latency issue Date: Mon, 2 May 2011 18:10:22 -0700 Message-ID: References: <4DBE83BF020000780003F1DC@vpn.id2.novell.com> Mime-Version: 1.0 Content-Type: multipart/mixed; boundary="_002_C9E484DF1301Dpradeepvamazoncom_" Return-path: In-Reply-To: <4DBE83BF020000780003F1DC@vpn.id2.novell.com> Content-Language: en-US List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xensource.com Errors-To: xen-devel-bounces@lists.xensource.com To: Jan Beulich , "xen-devel@lists.xensource.com" , Jeremy Fitzhardinge List-Id: xen-devel@lists.xenproject.org --_002_C9E484DF1301Dpradeepvamazoncom_ Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable Thanks Jan. Re: avoid unnecessary notification If this was a deliberate design choice then the duration of the delay is at the mercy of the pending I/O latencies & I/O patterns and the delay is simply too long in some cases. E.g. A write I/O stuck behind a read I/O could see more than double the latency on a Xen guest compared to a baremetal host. Avoiding notifications this way results in significant latency degradation perceived by many applications. If this is about allowing I/O scheduler to coalesce more I/Os, then I bet I/O scheduler's 'wait and coalesce' logic is a great substitute for the delays introduced by blkback. I totally agree IRQ coalescing or delay is useful for both blkback and netback but we need a logic that doesn't impact I/O latencies significantly. Also, I don't think netback has this type of notification avoidance logic (at least in 2.6.18 code base). Re: Other points Good call. Changed the patch to include tabs. I wasn't very sure about blk_ring_lock usage and I should have clarified it before sending out the patch. Assuming blk_ring_lock was meant to protect shared ring manipulations within blkback, is there a reason 'blk_rings->common.req_cons' manipulation in do_block_io_op is not protected ? The reasons for the differences between locking logic in do_block_io_op and make_response weren't terribly obvious although the failure mode for the race condition may very well be benign. Anyway, I am attaching a patch with appropriate changes. Jeremey, Can you apply this patch to pvops Dom-0 (http://git.kernel.org/pub/scm/linux/kernel/git/jeremy/xen.git). Should I submit another patch for 2.6.18 Dom-0 ? Signed-off-by: Pradeep Vincent 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 @@ -315,6 +315,7 @@ static int do_block_io_op(blkif_t *blkif) pending_req_t *pending_req; RING_IDX rc, rp; int more_to_do =3D 0; + unsigned long flags; =20 rc =3D blk_rings->common.req_cons; rp =3D blk_rings->common.sring->req_prod; @@ -383,6 +384,15 @@ 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){ + spin_lock_irqsave(&blkif->blk_ring_lock, flags); + RING_FINAL_CHECK_FOR_REQUESTS(&blk_rings->common, more_to_do); + spin_unlock_irqrestore(&blkif->blk_ring_lock, flags); + } return more_to_do; } =20 On 5/2/11 1:13 AM, "Jan Beulich" wrote: >>>> 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 >> 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 bet= ter >> + 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; >> } > > > --_002_C9E484DF1301Dpradeepvamazoncom_ Content-Type: application/octet-stream; name="blkback-bugfix-reqevent-assignment.patch" Content-Description: blkback-bugfix-reqevent-assignment.patch Content-Disposition: attachment; filename="blkback-bugfix-reqevent-assignment.patch"; size=993; creation-date="Tue, 03 May 2011 01:10:23 GMT"; modification-date="Tue, 03 May 2011 01:10:23 GMT" Content-Transfer-Encoding: base64 U2lnbmVkLW9mZi1ieTogUHJhZGVlcCBWaW5jZW50IDxwcmFkZWVwdkBhbWF6b24uY29tPgoKZGlm ZiAtLWdpdCBhL2RyaXZlcnMveGVuL2Jsa2JhY2svYmxrYmFjay5jIGIvZHJpdmVycy94ZW4vYmxr YmFjay9ibGtiYWNrLmMKLS0tIGEvZHJpdmVycy94ZW4vYmxrYmFjay9ibGtiYWNrLmMKKysrIGIv ZHJpdmVycy94ZW4vYmxrYmFjay9ibGtiYWNrLmMKQEAgLTMxNSw2ICszMTUsNyBAQCBzdGF0aWMg aW50IGRvX2Jsb2NrX2lvX29wKGJsa2lmX3QgKmJsa2lmKQogCXBlbmRpbmdfcmVxX3QgKnBlbmRp bmdfcmVxOwogCVJJTkdfSURYIHJjLCBycDsKIAlpbnQgbW9yZV90b19kbyA9IDA7CisJdW5zaWdu ZWQgbG9uZyAgICAgZmxhZ3M7CiAKIAlyYyA9IGJsa19yaW5ncy0+Y29tbW9uLnJlcV9jb25zOwog CXJwID0gYmxrX3JpbmdzLT5jb21tb24uc3JpbmctPnJlcV9wcm9kOwpAQCAtMzgzLDYgKzM4NCwx NSBAQCBzdGF0aWMgaW50IGRvX2Jsb2NrX2lvX29wKGJsa2lmX3QgKmJsa2lmKQogCQljb25kX3Jl c2NoZWQoKTsKIAl9CiAKKwkvKiBJZiBibGtiYWNrIG1pZ2h0IGdvIHRvIHNsZWVwIChpLmUuIG1v cmVfdG9fZG8gPT0gMCkgdGhlbiB3ZSBiZXR0ZXIKKwkgICBsZXQgYmxrZnJvbnQga25vdyBhYm91 dCBpdCAoYnkgc2V0dGluZyByZXFfZXZlbnQgYXBwcm9wcmlhdGVseSkgc28gdGhhdAorCSAgIGJs a2Zyb250IHdpbGwgYm90aGVyIHRvIHdha2UgdXMgdXAgKHZpYSBpbnRlcnJ1cHQpIHdoZW4gaXQg c3VibWl0cyBhIAorCSAgIG5ldyBJL08gKi8KKwlpZiAoIW1vcmVfdG9fZG8peworCQlzcGluX2xv Y2tfaXJxc2F2ZSgmYmxraWYtPmJsa19yaW5nX2xvY2ssIGZsYWdzKTsKKwkJUklOR19GSU5BTF9D SEVDS19GT1JfUkVRVUVTVFMoJmJsa19yaW5ncy0+Y29tbW9uLCBtb3JlX3RvX2RvKTsKKwkJc3Bp bl91bmxvY2tfaXJxcmVzdG9yZSgmYmxraWYtPmJsa19yaW5nX2xvY2ssIGZsYWdzKTsKKwl9CiAJ cmV0dXJuIG1vcmVfdG9fZG87CiB9CiAK --_002_C9E484DF1301Dpradeepvamazoncom_ Content-Type: text/plain; charset="us-ascii" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit Content-Disposition: inline _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel --_002_C9E484DF1301Dpradeepvamazoncom_--