[PATCH] block: integrate blk_end_bidi_request into blk_end_request

[PATCH] block: integrate blk_end_bidi_request into blk_end_request

[FUJITA Tomonori]

Hi Jens,

FC people have been working on FC pass thru feature using bsg bidi
support. Seems that the bidi API confuse them:

http://marc.info/?l=linux-scsi&m=122704347209856&w=2

=
From: FUJITA Tomonori <fujita.tomonori@lab.ntt.co.jp>
[PATCH] block: integrate blk_end_bidi_request with blk_end_request

This integrates blk_end_bidi_request into blk_end_request. IOW, it
changes blk_end_request to handle both bidi and non-bidi requests and
removes blk_end_bidi_request.

Currently, we have two functions to complete a request,
blk_end_bidi_request and blk_end_request. The former is for bidi
requests and the latter is for non-bidi. This seems to confuse
developers. Questions like "can blk_end_bidi_request be used with
non-bidi requests?", "what should be passed as the bidi_bytes argument
in blk_end_bidi_request" are often asked.

The callers don't care about whether a request is bidi or not. All
they want to do is to complete a request. I think that a single
function that can complete any request would be easy to understand.

We always must complete all bytes on both sides with bidi. So
blk_end_request easily can do it for the callers and handle both both
bidi and non-bidi requests.


Signed-off-by: FUJITA Tomonori <fujita.tomonori@lab.ntt.co.jp>
---
 block/blk-core.c        |   35 +++++++++++++----------------------
 drivers/scsi/scsi_lib.c |    3 +--
 include/linux/blkdev.h  |    2 --
 3 files changed, 14 insertions(+), 26 deletions(-)

diff --git a/block/blk-core.c b/block/blk-core.c
index 10e8a64..634f918 100644
--- a/block/blk-core.c
+++ b/block/blk-core.c
@@ -1943,7 +1943,19 @@ static int blk_end_io(struct request *rq, int error, unsigned int nr_bytes,
  **/
 int blk_end_request(struct request *rq, int error, unsigned int nr_bytes)
 {
-	return blk_end_io(rq, error, nr_bytes, 0, NULL);
+ 	unsigned int bidi_bytes	= 0;
+
+	if (blk_bidi_rq(rq)) {
+		struct bio *bio;
+		/*
+		 * We can't use rq->next_rq->data_len here because the
+		 * callers might set it to a residual length.
+		 */
+		__rq_for_each_bio(bio, rq->next_rq)
+			bidi_bytes += bio->bi_size;
+	}
+
+	return blk_end_io(rq, error, nr_bytes, bidi_bytes, NULL);
 }
 EXPORT_SYMBOL_GPL(blk_end_request);
 
@@ -1974,27 +1986,6 @@ int __blk_end_request(struct request *rq, int error, unsigned int nr_bytes)
 EXPORT_SYMBOL_GPL(__blk_end_request);
 
 /**
- * blk_end_bidi_request - Helper function for drivers to complete bidi request.
- * @rq:         the bidi request being processed
- * @error:      %0 for success, < %0 for error
- * @nr_bytes:   number of bytes to complete @rq
- * @bidi_bytes: number of bytes to complete @rq->next_rq
- *
- * Description:
- *     Ends I/O on a number of bytes attached to @rq and @rq->next_rq.
- *
- * Return:
- *     %0 - we are done with this request
- *     %1 - still buffers pending for this request
- **/
-int blk_end_bidi_request(struct request *rq, int error, unsigned int nr_bytes,
-			 unsigned int bidi_bytes)
-{
-	return blk_end_io(rq, error, nr_bytes, bidi_bytes, NULL);
-}
-EXPORT_SYMBOL_GPL(blk_end_bidi_request);
-
-/**
  * blk_update_request - Special helper function for request stacking drivers
  * @rq:           the request being processed
  * @error:        %0 for success, < %0 for error
diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
index f5d3b96..e550e3b 100644
--- a/drivers/scsi/scsi_lib.c
+++ b/drivers/scsi/scsi_lib.c
@@ -842,13 +842,12 @@ static void scsi_end_bidi_request(struct scsi_cmnd *cmd)
 {
 	struct request *req = cmd->request;
 	unsigned int dlen = req->data_len;
-	unsigned int next_dlen = req->next_rq->data_len;
 
 	req->data_len = scsi_out(cmd)->resid;
 	req->next_rq->data_len = scsi_in(cmd)->resid;
 
 	/* The req and req->next_rq have not been completed */
-	BUG_ON(blk_end_bidi_request(req, 0, dlen, next_dlen));
+	BUG_ON(blk_end_request(req, 0, dlen));
 
 	scsi_release_buffers(cmd);
 
diff --git a/include/linux/blkdev.h b/include/linux/blkdev.h
index a135256..2a22755 100644
--- a/include/linux/blkdev.h
+++ b/include/linux/blkdev.h
@@ -799,8 +799,6 @@ extern int blk_end_request(struct request *rq, int error,
 				unsigned int nr_bytes);
 extern int __blk_end_request(struct request *rq, int error,
 				unsigned int nr_bytes);
-extern int blk_end_bidi_request(struct request *rq, int error,
-				unsigned int nr_bytes, unsigned int bidi_bytes);
 extern void end_request(struct request *, int);
 extern int blk_end_request_callback(struct request *rq, int error,
 				unsigned int nr_bytes,
-- 
1.5.5.GIT

Re: [PATCH] block: integrate blk_end_bidi_request into blk_end_request

[Boaz Harrosh]

FUJITA Tomonori wrote:
> Hi Jens,
> 
> FC people have been working on FC pass thru feature using bsg bidi
> support. Seems that the bidi API confuse them:
> 
> http://marc.info/?l=linux-scsi&m=122704347209856&w=2
> 
> =
> From: FUJITA Tomonori <fujita.tomonori@lab.ntt.co.jp>
> [PATCH] block: integrate blk_end_bidi_request with blk_end_request
> 
> This integrates blk_end_bidi_request into blk_end_request. IOW, it
> changes blk_end_request to handle both bidi and non-bidi requests and
> removes blk_end_bidi_request.
> 
> Currently, we have two functions to complete a request,
> blk_end_bidi_request and blk_end_request. The former is for bidi
> requests and the latter is for non-bidi. This seems to confuse
> developers. Questions like "can blk_end_bidi_request be used with
> non-bidi requests?", "what should be passed as the bidi_bytes argument
> in blk_end_bidi_request" are often asked.
> 
> The callers don't care about whether a request is bidi or not. All
> they want to do is to complete a request. I think that a single
> function that can complete any request would be easy to understand.
> 
> We always must complete all bytes on both sides with bidi. So
> blk_end_request easily can do it for the callers and handle both both
> bidi and non-bidi requests.
> 
> 
> Signed-off-by: FUJITA Tomonori <fujita.tomonori@lab.ntt.co.jp>

NACK by Boaz Harrosh.

> ---
>  block/blk-core.c        |   35 +++++++++++++----------------------
>  drivers/scsi/scsi_lib.c |    3 +--
>  include/linux/blkdev.h  |    2 --
>  3 files changed, 14 insertions(+), 26 deletions(-)
> 
> diff --git a/block/blk-core.c b/block/blk-core.c
> index 10e8a64..634f918 100644
> --- a/block/blk-core.c
> +++ b/block/blk-core.c
> @@ -1943,7 +1943,19 @@ static int blk_end_io(struct request *rq, int error, unsigned int nr_bytes,
>   **/
>  int blk_end_request(struct request *rq, int error, unsigned int nr_bytes)
>  {
> -	return blk_end_io(rq, error, nr_bytes, 0, NULL);
> + 	unsigned int bidi_bytes	= 0;
> +
> +	if (blk_bidi_rq(rq)) {
> +		struct bio *bio;
> +		/*
> +		 * We can't use rq->next_rq->data_len here because the
> +		 * callers might set it to a residual length.
> +		 */
> +		__rq_for_each_bio(bio, rq->next_rq)
> +			bidi_bytes += bio->bi_size;

No. I do not want a loop to calculate the size here. This is ugly.
I have this information, and I just lost it do to BAD API.

Since we do not want to change all users of blk_end_request. (We should,
but we are too lazy. I'm). Then please just let blk_end_bidi_request()
be. Confused users will be corrected.

Perhaps we can put a comment above blk_end_bidi_request() that it can
be used for both bidi or uni commands, so users will confuse less.

> +	}
> +
> +	return blk_end_io(rq, error, nr_bytes, bidi_bytes, NULL);
>  }
>  EXPORT_SYMBOL_GPL(blk_end_request);
>  
> @@ -1974,27 +1986,6 @@ int __blk_end_request(struct request *rq, int error, unsigned int nr_bytes)
>  EXPORT_SYMBOL_GPL(__blk_end_request);
>  
>  /**
> - * blk_end_bidi_request - Helper function for drivers to complete bidi request.
> - * @rq:         the bidi request being processed
> - * @error:      %0 for success, < %0 for error
> - * @nr_bytes:   number of bytes to complete @rq
> - * @bidi_bytes: number of bytes to complete @rq->next_rq
> - *
> - * Description:
> - *     Ends I/O on a number of bytes attached to @rq and @rq->next_rq.
> - *
> - * Return:
> - *     %0 - we are done with this request
> - *     %1 - still buffers pending for this request
> - **/
> -int blk_end_bidi_request(struct request *rq, int error, unsigned int nr_bytes,
> -			 unsigned int bidi_bytes)
> -{
> -	return blk_end_io(rq, error, nr_bytes, bidi_bytes, NULL);
> -}
> -EXPORT_SYMBOL_GPL(blk_end_bidi_request);
> -
> -/**
>   * blk_update_request - Special helper function for request stacking drivers
>   * @rq:           the request being processed
>   * @error:        %0 for success, < %0 for error
> diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
> index f5d3b96..e550e3b 100644
> --- a/drivers/scsi/scsi_lib.c
> +++ b/drivers/scsi/scsi_lib.c
> @@ -842,13 +842,12 @@ static void scsi_end_bidi_request(struct scsi_cmnd *cmd)
>  {
>  	struct request *req = cmd->request;
>  	unsigned int dlen = req->data_len;
> -	unsigned int next_dlen = req->next_rq->data_len;
>  
>  	req->data_len = scsi_out(cmd)->resid;
>  	req->next_rq->data_len = scsi_in(cmd)->resid;
>  
>  	/* The req and req->next_rq have not been completed */
> -	BUG_ON(blk_end_bidi_request(req, 0, dlen, next_dlen));
> +	BUG_ON(blk_end_request(req, 0, dlen));
>  
>  	scsi_release_buffers(cmd);
>  
> diff --git a/include/linux/blkdev.h b/include/linux/blkdev.h
> index a135256..2a22755 100644
> --- a/include/linux/blkdev.h
> +++ b/include/linux/blkdev.h
> @@ -799,8 +799,6 @@ extern int blk_end_request(struct request *rq, int error,
>  				unsigned int nr_bytes);
>  extern int __blk_end_request(struct request *rq, int error,
>  				unsigned int nr_bytes);
> -extern int blk_end_bidi_request(struct request *rq, int error,
> -				unsigned int nr_bytes, unsigned int bidi_bytes);
>  extern void end_request(struct request *, int);
>  extern int blk_end_request_callback(struct request *rq, int error,
>  				unsigned int nr_bytes,

Boaz

Re: [PATCH] block: integrate blk_end_bidi_request into blk_end_request

[FUJITA Tomonori]

On Sun, 30 Nov 2008 13:10:31 +0200
Boaz Harrosh <bharrosh@panasas.com> wrote:

> FUJITA Tomonori wrote:
> > Hi Jens,
> > 
> > FC people have been working on FC pass thru feature using bsg bidi
> > support. Seems that the bidi API confuse them:
> > 
> > http://marc.info/?l=linux-scsi&m=122704347209856&w=2
> > 
> > =
> > From: FUJITA Tomonori <fujita.tomonori@lab.ntt.co.jp>
> > [PATCH] block: integrate blk_end_bidi_request with blk_end_request
> > 
> > This integrates blk_end_bidi_request into blk_end_request. IOW, it
> > changes blk_end_request to handle both bidi and non-bidi requests and
> > removes blk_end_bidi_request.
> > 
> > Currently, we have two functions to complete a request,
> > blk_end_bidi_request and blk_end_request. The former is for bidi
> > requests and the latter is for non-bidi. This seems to confuse
> > developers. Questions like "can blk_end_bidi_request be used with
> > non-bidi requests?", "what should be passed as the bidi_bytes argument
> > in blk_end_bidi_request" are often asked.
> > 
> > The callers don't care about whether a request is bidi or not. All
> > they want to do is to complete a request. I think that a single
> > function that can complete any request would be easy to understand.
> > 
> > We always must complete all bytes on both sides with bidi. So
> > blk_end_request easily can do it for the callers and handle both both
> > bidi and non-bidi requests.
> > 
> > 
> > Signed-off-by: FUJITA Tomonori <fujita.tomonori@lab.ntt.co.jp>
> 
> NACK by Boaz Harrosh.
> 
> > ---
> >  block/blk-core.c        |   35 +++++++++++++----------------------
> >  drivers/scsi/scsi_lib.c |    3 +--
> >  include/linux/blkdev.h  |    2 --
> >  3 files changed, 14 insertions(+), 26 deletions(-)
> > 
> > diff --git a/block/blk-core.c b/block/blk-core.c
> > index 10e8a64..634f918 100644
> > --- a/block/blk-core.c
> > +++ b/block/blk-core.c
> > @@ -1943,7 +1943,19 @@ static int blk_end_io(struct request *rq, int error, unsigned int nr_bytes,
> >   **/
> >  int blk_end_request(struct request *rq, int error, unsigned int nr_bytes)
> >  {
> > -	return blk_end_io(rq, error, nr_bytes, 0, NULL);
> > + 	unsigned int bidi_bytes	= 0;
> > +
> > +	if (blk_bidi_rq(rq)) {
> > +		struct bio *bio;
> > +		/*
> > +		 * We can't use rq->next_rq->data_len here because the
> > +		 * callers might set it to a residual length.
> > +		 */
> > +		__rq_for_each_bio(bio, rq->next_rq)
> > +			bidi_bytes += bio->bi_size;
> 
> No. I do not want a loop to calculate the size here. This is ugly.
> I have this information, and I just lost it do to BAD API.

IMO, being ugly is better than confusing people.

Do you think that the current API is good, which make developers
always wrongly use it and feel that they have to ask how to use and
add comments about necessary tricks to use it?


> Since we do not want to change all users of blk_end_request. (We should,
> but we are too lazy. I'm). Then please just let blk_end_bidi_request()
> be. Confused users will be corrected.

No. blk_end_bidi_request interface and the name is confusing. So
forcing everyone to use blk_end_bidi_request doesn't help.

And it's better to name a function to complete a request
blk_end_request(). The callers doesn't care if a request is bidi or
not.


> Perhaps we can put a comment above blk_end_bidi_request() that it can
> be used for both bidi or uni commands, so users will confuse less.

I don't think that it's a good idea to make up the confusion.

Re: [PATCH] block: integrate blk_end_bidi_request into blk_end_request

[Boaz Harrosh]

FUJITA Tomonori wrote:
> On Sun, 30 Nov 2008 13:10:31 +0200
> Boaz Harrosh <bharrosh@panasas.com> wrote:
> 
>> FUJITA Tomonori wrote:
>>> Hi Jens,
>>>
>>> FC people have been working on FC pass thru feature using bsg bidi
>>> support. Seems that the bidi API confuse them:
>>>
>>> http://marc.info/?l=linux-scsi&m=122704347209856&w=2
>>>
>>> =
>>> From: FUJITA Tomonori <fujita.tomonori@lab.ntt.co.jp>
>>> [PATCH] block: integrate blk_end_bidi_request with blk_end_request
>>>
>>> This integrates blk_end_bidi_request into blk_end_request. IOW, it
>>> changes blk_end_request to handle both bidi and non-bidi requests and
>>> removes blk_end_bidi_request.
>>>
>>> Currently, we have two functions to complete a request,
>>> blk_end_bidi_request and blk_end_request. The former is for bidi
>>> requests and the latter is for non-bidi. This seems to confuse
>>> developers. Questions like "can blk_end_bidi_request be used with
>>> non-bidi requests?", "what should be passed as the bidi_bytes argument
>>> in blk_end_bidi_request" are often asked.
>>>
>>> The callers don't care about whether a request is bidi or not. All
>>> they want to do is to complete a request. I think that a single
>>> function that can complete any request would be easy to understand.
>>>
>>> We always must complete all bytes on both sides with bidi. So
>>> blk_end_request easily can do it for the callers and handle both both
>>> bidi and non-bidi requests.
>>>
>>>
>>> Signed-off-by: FUJITA Tomonori <fujita.tomonori@lab.ntt.co.jp>
>> NACK by Boaz Harrosh.
>>
>>> ---
>>>  block/blk-core.c        |   35 +++++++++++++----------------------
>>>  drivers/scsi/scsi_lib.c |    3 +--
>>>  include/linux/blkdev.h  |    2 --
>>>  3 files changed, 14 insertions(+), 26 deletions(-)
>>>
>>> diff --git a/block/blk-core.c b/block/blk-core.c
>>> index 10e8a64..634f918 100644
>>> --- a/block/blk-core.c
>>> +++ b/block/blk-core.c
>>> @@ -1943,7 +1943,19 @@ static int blk_end_io(struct request *rq, int error, unsigned int nr_bytes,
>>>   **/
>>>  int blk_end_request(struct request *rq, int error, unsigned int nr_bytes)
>>>  {
>>> -	return blk_end_io(rq, error, nr_bytes, 0, NULL);
>>> + 	unsigned int bidi_bytes	= 0;
>>> +
>>> +	if (blk_bidi_rq(rq)) {
>>> +		struct bio *bio;
>>> +		/*
>>> +		 * We can't use rq->next_rq->data_len here because the
>>> +		 * callers might set it to a residual length.
>>> +		 */
>>> +		__rq_for_each_bio(bio, rq->next_rq)
>>> +			bidi_bytes += bio->bi_size;
>> No. I do not want a loop to calculate the size here. This is ugly.
>> I have this information, and I just lost it do to BAD API.
> 
> IMO, being ugly is better than confusing people.

Come on man. Ugly was an understatement. Can't you see?
Make a loop for information we have and decided to drop on the floor.

> 
> Do you think that the current API is good, which make developers
> always wrongly use it and feel that they have to ask how to use and
> add comments about necessary tricks to use it?
> 

No I think (and said) that the API is crap. But not that bad that I want to
take the effort and change it.

> 
>> Since we do not want to change all users of blk_end_request. (We should,
>> but we are too lazy. I'm). Then please just let blk_end_bidi_request()
>> be. Confused users will be corrected.
> 
> No. blk_end_bidi_request interface and the name is confusing. So
> forcing everyone to use blk_end_bidi_request doesn't help.
> 

We should rename blk_end_bidi_request => blk_end_request() and
change all users to add an extra 0, and the original blk_end_request()
should be dropped.

But since the big majority is uni and could careless, then the two
bidi drivers can take the extra effort and a bang on the head when
they get it wrong, just for the sake of the majority.

> And it's better to name a function to complete a request
> blk_end_request(). The callers doesn't care if a request is bidi or
> not.
> 

Right and the API for that is with 2 length(s) and even better with
2 residuals. But do you want to change all users for that?

> 
>> Perhaps we can put a comment above blk_end_bidi_request() that it can
>> be used for both bidi or uni commands, so users will confuse less.
> 
> I don't think that it's a good idea to make up the confusion.

And a loop is? better a comment then a loop any day in my book.
I don't mind changing the name though. perhaps:
	blk_end_bidi_request() => blk_complete_request()
So new users get tempted to use that and eventually blk_end_request()
will be dropped?

Boaz

Re: [PATCH] block: integrate blk_end_bidi_request into blk_end_request

[Christoph Hellwig]

On Mon, Dec 01, 2008 at 11:42:48AM +0200, Boaz Harrosh wrote:
> No I think (and said) that the API is crap. But not that bad that I want to
> take the effort and change it.

Get your ass of the chair and simply do it ;-)  This thread is a candidate
for everyone to waste more time arguing than takes to do the simple
interface change.

Re: [PATCH] block: integrate blk_end_bidi_request into blk_end_request

[FUJITA Tomonori]

On Mon, 01 Dec 2008 11:42:48 +0200
Boaz Harrosh <bharrosh@panasas.com> wrote:

> >> No. I do not want a loop to calculate the size here. This is ugly.
> >> I have this information, and I just lost it do to BAD API.
> > 
> > IMO, being ugly is better than confusing people.
> 
> Come on man. Ugly was an understatement. Can't you see?
> Make a loop for information we have and decided to drop on the floor.

Honestly, I can't see. Whatever you call the patch, the current API is
worse than it for me.


> > Do you think that the current API is good, which make developers
> > always wrongly use it and feel that they have to ask how to use and
> > add comments about necessary tricks to use it?
> > 
> 
> No I think (and said) that the API is crap. But not that bad that I want to
> take the effort and change it.
> 
> > 
> >> Since we do not want to change all users of blk_end_request. (We should,
> >> but we are too lazy. I'm). Then please just let blk_end_bidi_request()
> >> be. Confused users will be corrected.
> > 
> > No. blk_end_bidi_request interface and the name is confusing. So
> > forcing everyone to use blk_end_bidi_request doesn't help.
> > 
> 
> We should rename blk_end_bidi_request => blk_end_request() and
> change all users to add an extra 0, and the original blk_end_request()
> should be dropped.

That might be better than the current situation but why do we need
bidi_bytes (number of bytes to complete for bidi) in blk_end_request?

Having bidi_bytes is theoretically correct but we always must complete
all the bytes with bidi. Do you say that someone might need it in the
future?

Re: [PATCH] block: integrate blk_end_bidi_request into blk_end_request

[Boaz Harrosh]

FUJITA Tomonori wrote:
> On Mon, 01 Dec 2008 11:42:48 +0200
> Boaz Harrosh <bharrosh@panasas.com> wrote:
> 
>>>> No. I do not want a loop to calculate the size here. This is ugly.
>>>> I have this information, and I just lost it do to BAD API.
>>> IMO, being ugly is better than confusing people.
>> Come on man. Ugly was an understatement. Can't you see?
>> Make a loop for information we have and decided to drop on the floor.
> 
> Honestly, I can't see. Whatever you call the patch, the current API is
> worse than it for me.
> 
> 
>>> Do you think that the current API is good, which make developers
>>> always wrongly use it and feel that they have to ask how to use and
>>> add comments about necessary tricks to use it?
>>>
>> No I think (and said) that the API is crap. But not that bad that I want to
>> take the effort and change it.
>>
>>>> Since we do not want to change all users of blk_end_request. (We should,
>>>> but we are too lazy. I'm). Then please just let blk_end_bidi_request()
>>>> be. Confused users will be corrected.
>>> No. blk_end_bidi_request interface and the name is confusing. So
>>> forcing everyone to use blk_end_bidi_request doesn't help.
>>>
>> We should rename blk_end_bidi_request => blk_end_request() and
>> change all users to add an extra 0, and the original blk_end_request()
>> should be dropped.
> 
> That might be better than the current situation but why do we need
> bidi_bytes (number of bytes to complete for bidi) in blk_end_request?
> 
> Having bidi_bytes is theoretically correct but we always must complete
> all the bytes with bidi. Do you say that someone might need it in the
> future?

I will try to explain. The situation was bad for a long time. Not at
all because of the amount of bytes to complete but because of the residual
count. Look at current code that wants to return residual. It needs to save
on the side what ever was there in req->data_len then set req->data_len
to residual and call blk_end_request with the saved count. Now the thing
became worse because we have two of these.

Yes sure I want to complete both sides and I don't mind calling
blk_end_io( req, error, ~0U, ~0U, ...) so lets make a compromise
in your patch, don't loop on the bio's just set bidi_bytes = ~0U;
that is:

 int blk_end_request(struct request *rq, int error, unsigned int nr_bytes)
 {
-	return blk_end_io(rq, error, nr_bytes, 0, NULL);
+ 	unsigned int bidi_bytes	= 0;
+
+	if (blk_bidi_rq(rq)) 
+		bidi_bytes = ~0U;
+
+	return blk_end_io(rq, error, nr_bytes, bidi_bytes, NULL);
 }
 EXPORT_SYMBOL_GPL(blk_end_request);


Boaz

Re: [PATCH] block: integrate blk_end_bidi_request into blk_end_request

[FUJITA Tomonori]

On Mon, 01 Dec 2008 12:19:09 +0200
Boaz Harrosh <bharrosh@panasas.com> wrote:

> FUJITA Tomonori wrote:
> > On Mon, 01 Dec 2008 11:42:48 +0200
> > Boaz Harrosh <bharrosh@panasas.com> wrote:
> > 
> >>>> No. I do not want a loop to calculate the size here. This is ugly.
> >>>> I have this information, and I just lost it do to BAD API.
> >>> IMO, being ugly is better than confusing people.
> >> Come on man. Ugly was an understatement. Can't you see?
> >> Make a loop for information we have and decided to drop on the floor.
> > 
> > Honestly, I can't see. Whatever you call the patch, the current API is
> > worse than it for me.
> > 
> > 
> >>> Do you think that the current API is good, which make developers
> >>> always wrongly use it and feel that they have to ask how to use and
> >>> add comments about necessary tricks to use it?
> >>>
> >> No I think (and said) that the API is crap. But not that bad that I want to
> >> take the effort and change it.
> >>
> >>>> Since we do not want to change all users of blk_end_request. (We should,
> >>>> but we are too lazy. I'm). Then please just let blk_end_bidi_request()
> >>>> be. Confused users will be corrected.
> >>> No. blk_end_bidi_request interface and the name is confusing. So
> >>> forcing everyone to use blk_end_bidi_request doesn't help.
> >>>
> >> We should rename blk_end_bidi_request => blk_end_request() and
> >> change all users to add an extra 0, and the original blk_end_request()
> >> should be dropped.
> > 
> > That might be better than the current situation but why do we need
> > bidi_bytes (number of bytes to complete for bidi) in blk_end_request?
> > 
> > Having bidi_bytes is theoretically correct but we always must complete
> > all the bytes with bidi. Do you say that someone might need it in the
> > future?
> 
> I will try to explain. The situation was bad for a long time. Not at
> all because of the amount of bytes to complete but because of the residual
> count. Look at current code that wants to return residual. It needs to save
> on the side what ever was there in req->data_len then set req->data_len
> to residual and call blk_end_request with the saved count. Now the thing
> became worse because we have two of these.
> 
> Yes sure I want to complete both sides and I don't mind calling
> blk_end_io( req, error, ~0U, ~0U, ...) so lets make a compromise
> in your patch, don't loop on the bio's just set bidi_bytes = ~0U;
> that is:
> 
>  int blk_end_request(struct request *rq, int error, unsigned int nr_bytes)
>  {
> -	return blk_end_io(rq, error, nr_bytes, 0, NULL);
> + 	unsigned int bidi_bytes	= 0;
> +
> +	if (blk_bidi_rq(rq)) 
> +		bidi_bytes = ~0U;
> +
> +	return blk_end_io(rq, error, nr_bytes, bidi_bytes, NULL);
>  }
>  EXPORT_SYMBOL_GPL(blk_end_request);

Well, I also thought about the exact same approach.

I guess that this taints part_stat_add() in
__end_that_request_first()? Well, bidi is not fs requests for now (or
forever?) so the patch should be fine now.

Re: [PATCH] block: integrate blk_end_bidi_request into blk_end_request

[Boaz Harrosh]

FUJITA Tomonori wrote:
> On Mon, 01 Dec 2008 12:19:09 +0200
> Boaz Harrosh <bharrosh@panasas.com> wrote:
> 
>> FUJITA Tomonori wrote:
>>> On Mon, 01 Dec 2008 11:42:48 +0200
>>> Boaz Harrosh <bharrosh@panasas.com> wrote:
>>>
>>>>>> No. I do not want a loop to calculate the size here. This is ugly.
>>>>>> I have this information, and I just lost it do to BAD API.
>>>>> IMO, being ugly is better than confusing people.
>>>> Come on man. Ugly was an understatement. Can't you see?
>>>> Make a loop for information we have and decided to drop on the floor.
>>> Honestly, I can't see. Whatever you call the patch, the current API is
>>> worse than it for me.
>>>
>>>
>>>>> Do you think that the current API is good, which make developers
>>>>> always wrongly use it and feel that they have to ask how to use and
>>>>> add comments about necessary tricks to use it?
>>>>>
>>>> No I think (and said) that the API is crap. But not that bad that I want to
>>>> take the effort and change it.
>>>>
>>>>>> Since we do not want to change all users of blk_end_request. (We should,
>>>>>> but we are too lazy. I'm). Then please just let blk_end_bidi_request()
>>>>>> be. Confused users will be corrected.
>>>>> No. blk_end_bidi_request interface and the name is confusing. So
>>>>> forcing everyone to use blk_end_bidi_request doesn't help.
>>>>>
>>>> We should rename blk_end_bidi_request => blk_end_request() and
>>>> change all users to add an extra 0, and the original blk_end_request()
>>>> should be dropped.
>>> That might be better than the current situation but why do we need
>>> bidi_bytes (number of bytes to complete for bidi) in blk_end_request?
>>>
>>> Having bidi_bytes is theoretically correct but we always must complete
>>> all the bytes with bidi. Do you say that someone might need it in the
>>> future?
>> I will try to explain. The situation was bad for a long time. Not at
>> all because of the amount of bytes to complete but because of the residual
>> count. Look at current code that wants to return residual. It needs to save
>> on the side what ever was there in req->data_len then set req->data_len
>> to residual and call blk_end_request with the saved count. Now the thing
>> became worse because we have two of these.
>>
>> Yes sure I want to complete both sides and I don't mind calling
>> blk_end_io( req, error, ~0U, ~0U, ...) so lets make a compromise
>> in your patch, don't loop on the bio's just set bidi_bytes = ~0U;
>> that is:
>>
>>  int blk_end_request(struct request *rq, int error, unsigned int nr_bytes)
>>  {
>> -	return blk_end_io(rq, error, nr_bytes, 0, NULL);
>> + 	unsigned int bidi_bytes	= 0;
>> +
>> +	if (blk_bidi_rq(rq)) 
>> +		bidi_bytes = ~0U;
>> +
>> +	return blk_end_io(rq, error, nr_bytes, bidi_bytes, NULL);
>>  }
>>  EXPORT_SYMBOL_GPL(blk_end_request);
> 
> Well, I also thought about the exact same approach.
> 
> I guess that this taints part_stat_add() in
> __end_that_request_first()? Well, bidi is not fs requests for now (or
> forever?) so the patch should be fine now.

Thanks TOMO. I appreciate this very much.

Boaz