[PATCH 0/2] swim3: use blk_end_request_all()

[PATCH 0/2] swim3: use blk_end_request_all()

[FUJITA Tomonori]

This is against for-2.6.31 branch in the block tree.

Tejun, don't we need to use blk_end_request_all() when we hit the
maximum retry count in swim3_interrupt? Looks like we can't handle a
request properly if we doesn't complete the request there.

There are some other places to use blk_end_request_all if we need to.

[PATCH] swim3: use blk_end_request_all when we hit the maximum retry count

[FUJITA Tomonori]

Looks like we need to use blk_end_request_all() when we hit the maximum
retry count.

Signed-off-by: FUJITA Tomonori <fujita.tomonori@lab.ntt.co.jp>
---
 drivers/block/swim3.c |    3 ++-
 1 files changed, 2 insertions(+), 1 deletions(-)

diff --git a/drivers/block/swim3.c b/drivers/block/swim3.c
index 80df93e..116d169 100644
--- a/drivers/block/swim3.c
+++ b/drivers/block/swim3.c
@@ -737,7 +737,8 @@ static irqreturn_t swim3_interrupt(int irq, void *dev_id)
 				printk("swim3: error %sing block %ld (err=%x)\n",
 				       rq_data_dir(fd_req) == WRITE? "writ": "read",
 				       (long)blk_rq_pos(fd_req), err);
-				swim3_end_request_cur(-EIO);
+				blk_end_request_all(fd_req, -EIO);
+				fd_req = NULL;
 				fs->state = idle;
 			}
 		} else {
-- 
1.6.0.6

[PATCH] swim3: use blk_end_request instead of blk_update_request

[FUJITA Tomonori]

swim3 is the only user of blk_update_request(). Let's use
blk_end_request instead of blk_update_request. swim3 doesn't need to
update a request manually. In addition, we can unexport
blk_update_request().

Signed-off-by: FUJITA Tomonori <fujita.tomonori@lab.ntt.co.jp>
---
 drivers/block/swim3.c |   23 +++++++++++++----------
 1 files changed, 13 insertions(+), 10 deletions(-)

diff --git a/drivers/block/swim3.c b/drivers/block/swim3.c
index 116d169..0dd619a 100644
--- a/drivers/block/swim3.c
+++ b/drivers/block/swim3.c
@@ -726,18 +726,21 @@ static irqreturn_t swim3_interrupt(int irq, void *dev_id)
 		resid = ld_le16(&cp->res_count);
 		if (intr & ERROR_INTR) {
 			n = fs->scount - 1 - resid / 512;
-			if (n > 0) {
-				blk_update_request(fd_req, 0, n << 9);
+			if (blk_end_request(fd_req, 0, n << 9)) {
 				fs->req_sector += n;
-			}
-			if (fs->retries < 5) {
-				++fs->retries;
-				act(fs);
+
+				if (fs->retries < 5) {
+					++fs->retries;
+					act(fs);
+				} else {
+					printk("swim3: error %sing block %ld (err=%x)\n",
+					       rq_data_dir(fd_req) == WRITE? "writ": "read",
+					       (long)blk_rq_pos(fd_req), err);
+					blk_end_request_all(fd_req, -EIO);
+					fd_req = NULL;
+					fs->state = idle;
+				}
 			} else {
-				printk("swim3: error %sing block %ld (err=%x)\n",
-				       rq_data_dir(fd_req) == WRITE? "writ": "read",
-				       (long)blk_rq_pos(fd_req), err);
-				blk_end_request_all(fd_req, -EIO);
 				fd_req = NULL;
 				fs->state = idle;
 			}
-- 
1.6.0.6

Re: [PATCH] swim3: use blk_end_request instead of blk_update_request

[Boaz Harrosh]

On 05/12/2009 02:29 PM, FUJITA Tomonori wrote:
> swim3 is the only user of blk_update_request(). Let's use
> blk_end_request instead of blk_update_request. swim3 doesn't need to
> update a request manually. 


> In addition, we can unexport
> blk_update_request().
> 

blk_update_request() was meant for request-based-multi-path, and not for swim3
please see:
    32fab448 block: add request update interface

CC: Kiyoshi Ueda<k-ueda@ct.jp.nec.com>

I think Tejun Heo was just using that in some recent block layer, transformation.
Tejun?

> Signed-off-by: FUJITA Tomonori <fujita.tomonori@lab.ntt.co.jp>
> ---
>  drivers/block/swim3.c |   23 +++++++++++++----------
>  1 files changed, 13 insertions(+), 10 deletions(-)
> 

Boaz

Re: [PATCH] swim3: use blk_end_request instead of blk_update_request

[FUJITA Tomonori]

On Tue, 12 May 2009 17:45:33 +0300
Boaz Harrosh <bharrosh@panasas.com> wrote:

> On 05/12/2009 02:29 PM, FUJITA Tomonori wrote:
> > swim3 is the only user of blk_update_request(). Let's use
> > blk_end_request instead of blk_update_request. swim3 doesn't need to
> > update a request manually. 
> 
> 
> > In addition, we can unexport
> > blk_update_request().
> > 
> 
> blk_update_request() was meant for request-based-multi-path, and not for swim3
> please see:
>     32fab448 block: add request update interface
> 
> CC: Kiyoshi Ueda<k-ueda@ct.jp.nec.com>

I see, thanks. Looks like we need to export it but I still think that
the bug fix patch for swim3 is valid and it's better for swim3 to use
blk_end_request().


> I think Tejun Heo was just using that in some recent block layer, transformation.
> Tejun?

At least, there is no users of it in Jens' for-2.6.31.

Re: [PATCH 0/2] swim3: use blk_end_request_all()

[Tejun Heo]

Hello,

FUJITA Tomonori wrote:
> This is against for-2.6.31 branch in the block tree.
> 
> Tejun, don't we need to use blk_end_request_all() when we hit the
> maximum retry count in swim3_interrupt? Looks like we can't handle a
> request properly if we doesn't complete the request there.

Ummm.... why so?  Can you elaborate the bug scenario a bit?  I was
trying to keep the original behavior whereever possible.

> There are some other places to use blk_end_request_all if we need to.

There are some other places where end_request_all can be used but I
hope there isn't any place where we should but aren't.  :-)

Thanks.

-- 
tejun

Re: [PATCH] swim3: use blk_end_request instead of blk_update_request

[Tejun Heo]

Hello,

Boaz Harrosh wrote:
> blk_update_request() was meant for request-based-multi-path, and not for swim3
> please see:
>     32fab448 block: add request update interface
> 
> CC: Kiyoshi Ueda<k-ueda@ct.jp.nec.com>
> 
> I think Tejun Heo was just using that in some recent block layer,
> transformation.
> Tejun?

Yeah, conversion was easy that way.  I think it's sane to have
blk_update_request() exported as long as request internal tinkering is
kept in block layer proper.

Thanks.

-- 
tejun

Re: [PATCH 0/2] swim3: use blk_end_request_all()

[FUJITA Tomonori]

On Wed, 13 May 2009 00:25:51 +0900
Tejun Heo <tj@kernel.org> wrote:

> Hello,
> 
> FUJITA Tomonori wrote:
> > This is against for-2.6.31 branch in the block tree.
> > 
> > Tejun, don't we need to use blk_end_request_all() when we hit the
> > maximum retry count in swim3_interrupt? Looks like we can't handle a
> > request properly if we doesn't complete the request there.
> 
> Ummm.... why so?  Can you elaborate the bug scenario a bit?  I was
> trying to keep the original behavior whereever possible.

If when a request hits the maximum retry count and
swim3_end_request_cur() doesn't complete the request there, where the
request will be freed?


> > There are some other places to use blk_end_request_all if we need to.
> 
> There are some other places where end_request_all can be used but I
> hope there isn't any place where we should but aren't.  :-)
> 
> Thanks.
> 
> -- 
> tejun
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH 0/2] swim3: use blk_end_request_all()

[Tejun Heo]

FUJITA Tomonori wrote:
> On Wed, 13 May 2009 00:25:51 +0900
> Tejun Heo <tj@kernel.org> wrote:
> 
>> Hello,
>>
>> FUJITA Tomonori wrote:
>>> This is against for-2.6.31 branch in the block tree.
>>>
>>> Tejun, don't we need to use blk_end_request_all() when we hit the
>>> maximum retry count in swim3_interrupt? Looks like we can't handle a
>>> request properly if we doesn't complete the request there.
>> Ummm.... why so?  Can you elaborate the bug scenario a bit?  I was
>> trying to keep the original behavior whereever possible.
> 
> If when a request hits the maximum retry count and
> swim3_end_request_cur() doesn't complete the request there, where the
> request will be freed?

It won't be freed.  The current segment will be failed and the next
segment of the request will be tried on the next iteration, which was
the original behavior.  ie. it always fails requests
segment-by-segment and hitting max retry count doesn't fail the whole
request.

Thanks.

-- 
tejun

Re: [PATCH 0/2] swim3: use blk_end_request_all()

[FUJITA Tomonori]

On Wed, 13 May 2009 00:36:41 +0900
Tejun Heo <tj@kernel.org> wrote:

> FUJITA Tomonori wrote:
> > On Wed, 13 May 2009 00:25:51 +0900
> > Tejun Heo <tj@kernel.org> wrote:
> > 
> >> Hello,
> >>
> >> FUJITA Tomonori wrote:
> >>> This is against for-2.6.31 branch in the block tree.
> >>>
> >>> Tejun, don't we need to use blk_end_request_all() when we hit the
> >>> maximum retry count in swim3_interrupt? Looks like we can't handle a
> >>> request properly if we doesn't complete the request there.
> >> Ummm.... why so?  Can you elaborate the bug scenario a bit?  I was
> >> trying to keep the original behavior whereever possible.
> > 
> > If when a request hits the maximum retry count and
> > swim3_end_request_cur() doesn't complete the request there, where the
> > request will be freed?
> 
> It won't be freed.  The current segment will be failed and the next
> segment of the request will be tried on the next iteration, which was
> the original behavior.  ie. it always fails requests
> segment-by-segment and hitting max retry count doesn't fail the whole
> request.

Ah, I see. Thanks, then the current code is fine.

Well, it might be better to update some of the description of
blk_update_request, e.g., "the special helper function is only for
request stacking drivers".

Re: [PATCH 0/2] swim3: use blk_end_request_all()

[Tejun Heo]

Hello,

FUJITA Tomonori wrote:
>> It won't be freed.  The current segment will be failed and the next
>> segment of the request will be tried on the next iteration, which was
>> the original behavior.  ie. it always fails requests
>> segment-by-segment and hitting max retry count doesn't fail the whole
>> request.
> 
> Ah, I see. Thanks, then the current code is fine.
> 
> Well, it might be better to update some of the description of
> blk_update_request, e.g., "the special helper function is only for
> request stacking drivers".

Maybe but it encapsulates request details pretty well, so I wouldn't
really mind if it's used for other purposes (e.g. delaying actual
completion to ease internal bookkeeping).

Thanks.

-- 
tejun

Re: [PATCH] swim3: use blk_end_request instead of blk_update_request

[Kiyoshi Ueda]

Hi,

Thank you for pointing this, Boaz!

On 2009/05/13 0:27 +0900, Tejun Heo wrote:
> Hello,
> 
> Boaz Harrosh wrote:
>> blk_update_request() was meant for request-based-multi-path, and not for swim3
>> please see:
>>     32fab448 block: add request update interface
>>
>> CC: Kiyoshi Ueda<k-ueda@ct.jp.nec.com>
>>
>> I think Tejun Heo was just using that in some recent block layer,
>> transformation.
>> Tejun?
> 
> Yeah, conversion was easy that way.  I think it's sane to have
> blk_update_request() exported as long as request internal tinkering is
> kept in block layer proper.

blk_update_request() is needed for request-based dm to keep the request
completion ordering in bottom-up, although request-based dm is not
in upstream yet.

Jens, please keep blk_update_request() exported.

Thanks,
Kiyoshi Ueda

Re: [PATCH] swim3: use blk_end_request instead of blk_update_request

[Jens Axboe]

On Wed, May 13 2009, Kiyoshi Ueda wrote:
> Hi,
> 
> Thank you for pointing this, Boaz!
> 
> On 2009/05/13 0:27 +0900, Tejun Heo wrote:
> > Hello,
> > 
> > Boaz Harrosh wrote:
> >> blk_update_request() was meant for request-based-multi-path, and not for swim3
> >> please see:
> >>     32fab448 block: add request update interface
> >>
> >> CC: Kiyoshi Ueda<k-ueda@ct.jp.nec.com>
> >>
> >> I think Tejun Heo was just using that in some recent block layer,
> >> transformation.
> >> Tejun?
> > 
> > Yeah, conversion was easy that way.  I think it's sane to have
> > blk_update_request() exported as long as request internal tinkering is
> > kept in block layer proper.
> 
> blk_update_request() is needed for request-based dm to keep the request
> completion ordering in bottom-up, although request-based dm is not
> in upstream yet.
> 
> Jens, please keep blk_update_request() exported.

I did, I applied the swim3 patches yesterday as well.

-- 
Jens Axboe

Re: [PATCH] swim3: use blk_end_request instead of blk_update_request

[Tejun Heo]

Jens Axboe wrote:
>>> Yeah, conversion was easy that way.  I think it's sane to have
>>> blk_update_request() exported as long as request internal tinkering is
>>> kept in block layer proper.
>> blk_update_request() is needed for request-based dm to keep the request
>> completion ordering in bottom-up, although request-based dm is not
>> in upstream yet.
>>
>> Jens, please keep blk_update_request() exported.
> 
> I did, I applied the swim3 patches yesterday as well.

I don't think the patch is correct.  If it calls
blk_end_request_all(), it should also clear the current request which
the patch doesn't.  Also, given that the driver doesn't support
partially failing the request, I think it's correct to fail
segment-by-segment to avoid merged request failure affects unrelated
bios.

Thanks.

-- 
tejun

Re: [PATCH] swim3: use blk_end_request instead of blk_update_request

[Jens Axboe]

On Wed, May 13 2009, Tejun Heo wrote:
> Jens Axboe wrote:
> >>> Yeah, conversion was easy that way.  I think it's sane to have
> >>> blk_update_request() exported as long as request internal tinkering is
> >>> kept in block layer proper.
> >> blk_update_request() is needed for request-based dm to keep the request
> >> completion ordering in bottom-up, although request-based dm is not
> >> in upstream yet.
> >>
> >> Jens, please keep blk_update_request() exported.
> > 
> > I did, I applied the swim3 patches yesterday as well.
> 
> I don't think the patch is correct.  If it calls
> blk_end_request_all(), it should also clear the current request which
> the patch doesn't.  Also, given that the driver doesn't support
> partially failing the request, I think it's correct to fail
> segment-by-segment to avoid merged request failure affects unrelated
> bios.
> 
> Thanks.

OK, I'll back them out for now.

-- 
Jens Axboe