Re: [Xen-devel] [PATCH] blkback: Fix block I/O latency issue

["Vincent, Pradeep" <pradeepv@xxxxxxxxxx>]

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 <pradeepv@xxxxxxxxxx>

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 = 0;
+ unsigned long     flags;
 
  rc = blk_rings->common.req_cons;
  rp = blk_rings->common.sring->req_prod;
@@ -383,6 +384,15 @@ static int do_block_io_op(blkif_t *blkif)
   cond_resched();
  }
 
+ /* If blkback might go to sleep (i.e. more_to_do == 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;
 }
 







On 5/2/11 1:13 AM, "Jan Beulich" <JBeulich@xxxxxxxxxx> wrote:

>>>> On 02.05.11 at 09:04, "Vincent, Pradeep" <pradeepv@xxxxxxxxxx> 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 
>> about it (req_event isn't set appropriately). Hence blkfront doesn't
>>notify 
>> blkback when it submits a new I/O thus delaying the 'dispatch' of the
>>new I/O 
>> to Dom-0 block I/O subsystem. The new I/O is dispatched as soon as one
>>of the 
>> previous I/Os completes.
>> 
>> As a result of this issue, the block I/O latency performance is
>>degraded for 
>> some workloads on Xen guests using blkfront-blkback stack.
>> 
>> The following change addresses this issue:
>> 
>> 
>> Signed-off-by: Pradeep Vincent <pradeepv@xxxxxxxxxx>
>> 
>> 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();
>>   }
>> 
>> + /* If blkback might go to sleep (i.e. more_to_do == 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;
>>  }
>
>
>

Attachment: blkback-bugfix-reqevent-assignment.patch
Description: blkback-bugfix-reqevent-assignment.patch

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxx
http://lists.xensource.com/xen-devel