[block #for-next] block: clear req->errors on bio completion only for fs requests

[block #for-next] block: clear req->errors on bio completion only for fs requests

[Tejun Heo]

Impact: subtle behavior change

For fs requests, rq is only carrier of bios and rq error status as a
whole doesn't mean much.  This is the reason why rq->errors is being
cleared on each partial completion of a request as on each partial
completion the error status is transferred to the respective bios.

For pc requests, rq->errors is used to carry error status to the
issuer and thus __end_that_request_first() doesn't clear it on such
cases.

The condition was fine till now as only fs and pc requests have used
bio and thus the bio completion path.  However, future changes will
unify data accesses to bio and all non fs users care about rq error
status.  Clear rq->errors on bio completion only for fs requests.

In general, the implicit clearing is a bit too subtle especially as
the meaning of rq->errors is completely dependent on low level
drivers.  Unifying / cleaning up rq->errors usage and letting llds
manage it would be better.  TODO comment added.

Signed-off-by: Tejun Heo <tj@kernel.org>
Cc: Jens Axboe <axboe@kernel.dk>
---
Jens, can you please ack this patch?  With FUJITA's scsi_lib cleanup
and Borislav's patch to make ide not use preallocated data structures,
this is the only block bit which is necessary for ide cleanup
patchets.  If you're okay with it, I'd like to push this through
Bartlomiej's tree.

Thanks.

 block/blk-core.c |   10 +++++++---
 1 file changed, 7 insertions(+), 3 deletions(-)

Index: block/block/blk-core.c
===================================================================
--- block.orig/block/blk-core.c
+++ block/block/blk-core.c
@@ -1739,10 +1739,14 @@ static int __end_that_request_first(stru
 	trace_block_rq_complete(req->q, req);
 
 	/*
-	 * for a REQ_TYPE_BLOCK_PC request, we want to carry any eventual
-	 * sense key with us all the way through
+	 * For fs requests, rq is just carrier of independent bio's
+	 * and each partial completion should be handled separately.
+	 * Reset per-request error on each partial completion.
+	 *
+	 * TODO: tj: This is too subtle.  It would be better to let
+	 * low level drivers do what they see fit.
 	 */
-	if (!blk_pc_request(req))
+	if (blk_fs_request(req))
 		req->errors = 0;
 
 	if (error && (blk_fs_request(req) && !(req->cmd_flags & REQ_QUIET))) {

Re: [block #for-next] block: clear req->errors on bio completion only for fs requests

[Jens Axboe]

On Thu, Apr 16 2009, Tejun Heo wrote:
> Impact: subtle behavior change
> 
> For fs requests, rq is only carrier of bios and rq error status as a
> whole doesn't mean much.  This is the reason why rq->errors is being
> cleared on each partial completion of a request as on each partial
> completion the error status is transferred to the respective bios.
> 
> For pc requests, rq->errors is used to carry error status to the
> issuer and thus __end_that_request_first() doesn't clear it on such
> cases.
> 
> The condition was fine till now as only fs and pc requests have used
> bio and thus the bio completion path.  However, future changes will
> unify data accesses to bio and all non fs users care about rq error
> status.  Clear rq->errors on bio completion only for fs requests.
> 
> In general, the implicit clearing is a bit too subtle especially as
> the meaning of rq->errors is completely dependent on low level
> drivers.  Unifying / cleaning up rq->errors usage and letting llds
> manage it would be better.  TODO comment added.
> 
> Signed-off-by: Tejun Heo <tj@kernel.org>
> Cc: Jens Axboe <axboe@kernel.dk>
> ---
> Jens, can you please ack this patch?  With FUJITA's scsi_lib cleanup
> and Borislav's patch to make ide not use preallocated data structures,
> this is the only block bit which is necessary for ide cleanup
> patchets.  If you're okay with it, I'd like to push this through
> Bartlomiej's tree.

Sure, you can add my acked-by.

-- 
Jens Axboe

Re: [block #for-next] block: clear req->errors on bio completion only for fs requests

[Tejun Heo]

Jens Axboe wrote:
>> Jens, can you please ack this patch?  With FUJITA's scsi_lib cleanup
>> and Borislav's patch to make ide not use preallocated data structures,
>> this is the only block bit which is necessary for ide cleanup
>> patchets.  If you're okay with it, I'd like to push this through
>> Bartlomiej's tree.
> 
> Sure, you can add my acked-by.

Thanks. :-)

-- 
tejun