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;
>> }
>
>
>
blkback-bugfix-reqevent-assignment.patch
Description: blkback-bugfix-reqevent-assignment.patch
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxx
http://lists.xensource.com/xen-devel
|
Home