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
Cc: jens.axboe@oracle.com, 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:38:52 -0500 (EST)	[thread overview]
Message-ID: <20071204.163852.59650214.k-ueda@ct.jp.nec.com> (raw)
In-Reply-To: <47555C90.2080609@panasas.com>

Hi Boaz,

On Tue, 04 Dec 2007 15:56:32 +0200, Boaz Harrosh <bharrosh@panasas.com> wrote:
> > +int blk_end_request(struct request *rq, int uptodate, int nr_bytes)
> > +{
> > +	struct request_queue *q = rq->q;
> > +	unsigned long flags = 0UL;
> > +
> > +	if (blk_fs_request(rq) || blk_pc_request(rq)) {
> > +		if (__end_that_request_first(rq, uptodate, nr_bytes))
> > +			return 1;
> > +	}
> > +
> > +	add_disk_randomness(rq->rq_disk);
> > +
> > +	spin_lock_irqsave(q->queue_lock, flags);
> > +	complete_request(rq, uptodate);
> > +	spin_unlock_irqrestore(q->queue_lock, flags);
> > +
> > +	return 0;
> > +}
> > +EXPORT_SYMBOL_GPL(blk_end_request);
> > +
> > +/**
> > + * __blk_end_request - Helper function for drivers to complete the request.
> > + *
> > + * Description:
> > + *     Must be called with queue lock held unlike blk_end_request().
> > + **/
> > +int __blk_end_request(struct request *rq, int uptodate, int nr_bytes)
> > +{
> > +	if (blk_fs_request(rq) || blk_pc_request(rq)) {
> > +		if (__end_that_request_first(rq, uptodate, nr_bytes))
> > +			return 1;
> > +	}
> > +
> > +	add_disk_randomness(rq->rq_disk);
> > +
> > +	complete_request(rq, uptodate);
> > +
> > +	return 0;
> > +}
> > +EXPORT_SYMBOL_GPL(__blk_end_request);
> 
> I don't like it that you have two Identical but slightly different
> implementations  I wish you would do an internal-with-flags
> implementation and then API ones can call the internal one. Or maybe
> just hold the spin_lock just a bit longer and have one call the other.
> To prove my case see how hard it is to add new code like with
> the bidi patch, where you need to add exact same code in 3 places.
> (OK only 2 places actually, if _callback is gone)

As for the internal-with-flags implementation, I once proposed
something like below but it was rejected by Jens.
(http://marc.info/?l=linux-kernel&m=118880584720600&w=2)
----------------------------------------------------------------------
static int internal_function(rq, needlock)
{
	end_that_request_chunk(rq);

	if (needlock)
		spin_lock_irqsave();
	end_that_request_last(rq);
	if (needlock)
		spin_unlock_irqrestore();
}

int blk_end_request(rq)
{
	return internal_function(rq, 1);
}

int __blk_end_request(rq)
{
	return internal_function(rq, 0);
}
----------------------------------------------------------------------


As for the holding-queue-lock-longer implementation,
end_that_request_chunk() completes bios in the request and it can
reaches filesystem layer and may take time.
I guess many drivers like scsi are calling end_that_request_chunk()
without queue's lock because of the reason above.

I'll try to remove the duplication again by another patch-set
after blk_end_request interfaces are merged.
So I would like to leave the duplication for now.
Is it acceptable for you?

Thanks,
Kiyoshi Ueda

      parent reply	other threads:[~2007-12-04 21:41 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
2007-12-05  9:48     ` Jens Axboe
2007-12-04 21:38   ` Kiyoshi Ueda [this message]

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.163852.59650214.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).