linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Kiyoshi Ueda <k-ueda@ct.jp.nec.com>
To: bharrosh@panasas.com, jens.axboe@oracle.com
Cc: linux-kernel@vger.kernel.org, linux-scsi@vger.kernel.org,
	linux-ide@vger.kernel.org, dm-devel@redhat.com,
	j-nomura@ce.jp.nec.com, k-ueda@ct.jp.nec.com
Subject: Re: [PATCH 01/28] blk_end_request: add new request completion interface (take 3)
Date: Tue, 04 Dec 2007 16:37:49 -0500 (EST)	[thread overview]
Message-ID: <20071204.163749.39152828.k-ueda@ct.jp.nec.com> (raw)
In-Reply-To: <47555C90.2080609@panasas.com>

Hi Boaz and Jens,

On Tue, 04 Dec 2007 15:56:32 +0200, Boaz Harrosh <bharrosh@panasas.com> wrote:
> > +/**
> > + * blk_end_request - Helper function for drivers to complete the request.
> > + * @rq:       the request being processed
> > + * @uptodate: 1 for success, 0 for I/O error, < 0 for specific error
> > + * @nr_bytes: number of bytes to complete
> > + *
> > + * Description:
> > + *     Ends I/O on a number of bytes attached to @rq.
> > + *     If @rq has leftover, sets it up for the next range of segments.
> > + *
> > + * Return:
> > + *     0 - we are done with this request
> > + *     1 - still buffers pending for this request
> > + **/
> > +int blk_end_request(struct request *rq, int uptodate, int nr_bytes)
> 
> I always hated that uptodate boolean with possible negative error value.
> I guess it was done for backward compatibility of then users of 
> end_that_request_first(). But since you are introducing a new API then
> this is not the case. Just have regular status int where 0 means ALL_OK
> and negative value means error code. 
> Just my $0.02.

Thank you for the comment.
I think it's quite reasonable.
By doing that, we don't need end_io_error() anymore.


Jens,
What do you think?
If you agree with the interface change above, I would prefer to
separate the patch-set from blk_end_request patch-set like below:
    o blk_end_request: remove end_that_request_*
    o change interface of 'uptodate' in blk_end_request to 'error'
It makes the purpose of blk_end_request patch-set clear
(and also, each patch of device drivers could be smaller).
But, it doubles your merging work.  So if you would like to get
the changes at once, I'll merge them into blk_end_request patch-set.
 
As for the patch inclusion, do you push the driver changes to Linus
all at once?  Or should I ask each maintainer to take the patch?

Thanks,
Kiyoshi Ueda

  reply	other threads:[~2007-12-04 21:44 UTC|newest]

Thread overview: 5+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2007-11-30 23:24 [PATCH 01/28] blk_end_request: add new request completion interface (take 3) Kiyoshi Ueda
2007-12-04 13:56 ` Boaz Harrosh
2007-12-04 21:37   ` Kiyoshi Ueda [this message]
2007-12-05  9:48     ` Jens Axboe
2007-12-04 21:38   ` Kiyoshi Ueda

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20071204.163749.39152828.k-ueda@ct.jp.nec.com \
    --to=k-ueda@ct.jp.nec.com \
    --cc=bharrosh@panasas.com \
    --cc=dm-devel@redhat.com \
    --cc=j-nomura@ce.jp.nec.com \
    --cc=jens.axboe@oracle.com \
    --cc=linux-ide@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-scsi@vger.kernel.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).