LinuxLists.cc - [PATCH] 2.5.28 small REQ

[permalink] [raw]

Subject: Re: [PATCH] 2.5.28 small REQ_SPECIAL abstraction

On Fri, Jul 26 2002, Marcin Dalecki wrote:
> The attached patch does the following:

Looks fine to me. One thing sticks out though:

> + /*
> + * tell I/O scheduler that this isn't a regular read/write (ie it
> + * must not attempt merges on this) and that it acts as a soft
> + * barrier
> + */
> + rq->flags &= REQ_QUEUED;

this can't be right. Either it's a bug for REQ_QUEUED to be set here, or
it needs to end the tag properly.

> + rq->flags |= REQ_SPECIAL | REQ_BARRIER;
> +
> + rq->special = data;
> +
> + spin_lock_irqsave(q->queue_lock, flags);
> + /* If command is tagged, release the tag */
> + if(blk_rq_tagged(rq))
> + blk_queue_end_tag(q, rq);

woops, you just possible leaked a tag. I really don't think you should
mix inserting a special and ending a tag into the same function, makes
no sense. If a driver wants that it should do:

if (blk_rq_tagged(rq))
blk_queue_end_tag(q, rq);

blk_insert_request(q, rq, bla bla);

Also, please use the right spacing, if(bla :-)

So kill any reference to tagging (and REQ_QUEUED)i in
blk_insert_request, and I'm ok with it.

--
Jens Axboe

2002-07-26 15:11:14

[permalink] [raw]

Subject: Re: [PATCH] 2.5.28 small REQ_SPECIAL abstraction

Jens Axboe wrote:
> On Fri, Jul 26 2002, Marcin Dalecki wrote:
>
>>The attached patch does the following:
>
>
> Looks fine to me. One thing sticks out though:

Hey it was *literal* cut and paste from SCSI code after all ;-)

>>+ rq->flags &= REQ_QUEUED;
>
>
> this can't be right. Either it's a bug for REQ_QUEUED to be set here, or
> it needs to end the tag properly.
>
>
>>+ rq->flags |= REQ_SPECIAL | REQ_BARRIER;
>>+
>>+ rq->special = data;
>>+
>>+ spin_lock_irqsave(q->queue_lock, flags);
>>+ /* If command is tagged, release the tag */
>>+ if(blk_rq_tagged(rq))
>>+ blk_queue_end_tag(q, rq);
>
>
> woops, you just possible leaked a tag. I really don't think you should
> mix inserting a special and ending a tag into the same function, makes
> no sense. If a driver wants that it should do:
>
> if (blk_rq_tagged(rq))
> blk_queue_end_tag(q, rq);

Yes.

> blk_insert_request(q, rq, bla bla);
>
> Also, please use the right spacing, if(bla :-)

Cut and paste damage from SCSI code.... no argument here.

> So kill any reference to tagging (and REQ_QUEUED)i in
> blk_insert_request, and I'm ok with it.

Ah, yes I'm pretty sure now. I looked up how blk_queue_end_tag()
works and it's indeed the case -> setting the flag
and undoing it immediately doesn't make sense anyway.
(Even the collateral damage to tag allocation aside...)
This was perhaps "defensive coding" by the SCSI people?

You are right the

rq->flags &= REQ_QUEUED;

and the

if (blk_rq_tagged(rq))
blk_queue_end_tag(q, rq);

should be just removed and things are fine.
They only survive becouse they don't provide a tag for the request in
first place.

Thanks for pointing it out.

2002-07-28 19:21:59

[permalink] [raw]

Subject: Re: [PATCH] 2.5.28 small REQ_SPECIAL abstraction

On Fri, Jul 26 2002, Marcin Dalecki wrote:
> Jens Axboe wrote:
> >On Fri, Jul 26 2002, Marcin Dalecki wrote:
> >
> >>The attached patch does the following:
> >
> >
> >Looks fine to me. One thing sticks out though:
>
> Hey it was *literal* cut and paste from SCSI code after all ;-)
>
> >>+ rq->flags &= REQ_QUEUED;
> >
> >
> >this can't be right. Either it's a bug for REQ_QUEUED to be set here, or
> >it needs to end the tag properly.
> >
> >
> >>+ rq->flags |= REQ_SPECIAL | REQ_BARRIER;
> >>+
> >>+ rq->special = data;
> >>+
> >>+ spin_lock_irqsave(q->queue_lock, flags);
> >>+ /* If command is tagged, release the tag */
> >>+ if(blk_rq_tagged(rq))
> >>+ blk_queue_end_tag(q, rq);
> >
> >
> >woops, you just possible leaked a tag. I really don't think you should
> >mix inserting a special and ending a tag into the same function, makes
> >no sense. If a driver wants that it should do:
> >
> > if (blk_rq_tagged(rq))
> > blk_queue_end_tag(q, rq);
>
> Yes.
>
> > blk_insert_request(q, rq, bla bla);
> >
> >Also, please use the right spacing, if(bla :-)
>
> Cut and paste damage from SCSI code.... no argument here.
>
> >So kill any reference to tagging (and REQ_QUEUED)i in
> >blk_insert_request, and I'm ok with it.
>
> Ah, yes I'm pretty sure now. I looked up how blk_queue_end_tag()
> works and it's indeed the case -> setting the flag
> and undoing it immediately doesn't make sense anyway.
> (Even the collateral damage to tag allocation aside...)
> This was perhaps "defensive coding" by the SCSI people?
>
> You are right the
>
> rq->flags &= REQ_QUEUED;
>
> and the
>
> if (blk_rq_tagged(rq))
> blk_queue_end_tag(q, rq);
>
> should be just removed and things are fine.
> They only survive becouse they don't provide a tag for the request in
> first place.
>
> Thanks for pointing it out.

But the crap still got merged, sigh... Yet again an excellent point of
why stuff like this should go through the maintainer. Apparently Linus
blindly applies this stuff.

--
Jens Axboe

2002-07-28 20:10:43

by James Bottomley

[permalink] [raw]

Subject: Re: [PATCH] 2.5.28 small REQ_SPECIAL abstraction

> You are right the
> > rq->flags &= REQ_QUEUED;
> > and the
> > if (blk_rq_tagged(rq))
> blk_queue_end_tag(q, rq);
> > should be just removed and things are fine.
> They only survive becouse they don't provide a tag for the request in
> first place.
> > Thanks for pointing it out.

Please don't remove this.

insert_special isn't just used to start new requests, it's also used to queue
incoming requests that cannot be processed by the device (host adapter,
queue_full etc.).

In this latter case, the tag is already begun, so it needs to go back with
end_tag (we start a new tag when the device begins processing again).

I own up to introducing the &= REQ_QUEUED rubbish---I was just keeping the
original placement of the flag clearing code, but now we need to preserve
whether the request was queued or not for the blk_rq_tagged check. On
reflection it would have been better just to set the flags to REQ_SPECIAL |
REQ_BARRIER after the end tag code.

[email protected] said:
> But the crap still got merged, sigh... Yet again an excellent point of
> why stuff like this should go through the maintainer. Apparently Linus
> blindly applies this stuff.

Hmm, well I sent it to you and you are the Maintainer.

James

P.S. I just got back into the US from a long flight, I'll give this more
mature reflection tomorrow.

2002-07-28 23:28:10

[permalink] [raw]

Subject: Re: [PATCH] 2.5.28 small REQ_SPECIAL abstraction

On Sun, 28 Jul 2002, Jens Axboe wrote:
>
> But the crap still got merged, sigh... Yet again an excellent point of
> why stuff like this should go through the maintainer. Apparently Linus
> blindly applies this stuff.

Ehh, since there is no proactive maintainer for SCSI, I don't have much
choice, do I?

SCSI has been maintainerless for the last few years. Right now three
people work on it to some degree (Doug Ledford, James Bottomley and you),
but I don't get timely patches, and neither does apparently anybody else.

Case in point: I was debugging some USB storage issues with Matthew Dharm
yesterday, and he sent me patches to the SCSI subsystem that he claims
were supposedly considered valid on the scsi mailing list back in May.

Guess what? I've not seen the patches from any of the three people I
consider closest to being maintainers.

So your "should go through the maintainer" complaint is obviously a bunch
of bull. Feel free to step up to the plate, but before you do, don't throw
rocks in glass houses.

Linus

2002-07-28 23:56:45

by Andries E. Brouwer

[permalink] [raw]

Subject: Re: [PATCH] 2.5.28 small REQ_SPECIAL abstraction

On Sun, 28 Jul 2002, Jens Axboe wrote:

> But the crap still got merged, sigh... Yet again an excellent point of
> why stuff like this should go through the maintainer. Apparently Linus
> blindly applies this stuff.

Ehh, since there is no proactive maintainer for SCSI, I don't have much
choice, do I?

SCSI has been maintainerless for the last few years. Right now three
people work on it to some degree (Doug Ledford, James Bottomley and you),
but I don't get timely patches, and neither does apparently anybody else.

Case in point: I was debugging some USB storage issues with Matthew Dharm
yesterday, and he sent me patches to the SCSI subsystem that he claims
were supposedly considered valid on the scsi mailing list back in May.

Guess what? I've not seen the patches from any of the three people I
consider closest to being maintainers.

So your "should go through the maintainer" complaint is obviously a bunch
of bull. Feel free to step up to the plate, but before you do, don't throw
rocks in glass houses.

Ha, Linus,

Yes, an interesting discussion with Matthew Dharm.
I have seen several messages discussing the topic today -
linux-scsi is not silent about it.

You killed the idea of maintainers yourself, proclaiming
that you did not work with maintainers but with lieutenants.

In the mathematical world, if someone wants to publish a paper,
it is sent to a handful of referees. These reply "reject",
or "accept", or "accept, but correct the following mistakes ...",
or procrastinate so much that the editor takes some random decision
herself.

Such a system would not be unreasonable in the Linux world.
A SCSI patch is sent to linux-scsi and also to the five people
active today in the area. They reply, preferably both to you
and on linux-scsi, and if within one or two days after a positive
reply no negative reply comes in, then apparently there are
no objections.

In the absence of a single active maintainer, peer review is a
good alternative.

Andries

[By the way, have you asked these people to be maintainer?
Many people are too modest to suggest themselves, but will
accept when asked.]

2002-07-29 00:28:38

[permalink] [raw]

Subject: Re: [PATCH] 2.5.28 small REQ_SPECIAL abstraction

On Mon, 29 Jul 2002 [email protected] wrote:
>
> In the absence of a single active maintainer, peer review is a
> good alternative.

I agree, but you still want to have somebody who actually steps up to the
plate after the peer review has taken place, and sends me the patch..

And yes, if it's not one of the regular lieutenants, that does mean that
me applying it will depend a bit more on just how much time I have. I
would obviously prefer that the SCSI maintainer wouldn't necessarily sync
directly with me, but with somebody I work with anyway - as long as it's
reasonably timely.

(The _good_ news is that there haven't actually been all that many reasons
to maintain SCSI for a while - most of the maintenance has actually been
due to the generic block layer changes, which Jens has naturally been very
good about. The rest has _mostly_ been about updating specific drivers to
the PCI DMA interface (and various one-liners for the block layer
changes).

> [By the way, have you asked these people to be maintainer?
> Many people are too modest to suggest themselves, but will
> accept when asked.]

Jens is actually documented as being the SCSI maintainer, but that is
probably because he is the block device maintainer and he ended up
maintaining the more fundamental changes. I've seen James Bottomley more
as the "change SCSI internals" guy, and Doug mentioned that he will have
more time to work on 2.5.x not that long ago, so I do think all three
consider themselves at least partial maintainers already.

I certainly take patches from all three (see above on whether this is
optimal for me, though ;)

Linus

2002-07-29 00:49:35

by Dave Jones

[permalink] [raw]

Subject: Re: [PATCH] 2.5.28 small REQ_SPECIAL abstraction

On Sun, Jul 28, 2002 at 05:33:25PM -0700, Linus Torvalds wrote:

> Jens is actually documented as being the SCSI maintainer, but that is
> probably because he is the block device maintainer and he ended up
> maintaining the more fundamental changes. I've seen James Bottomley more
> as the "change SCSI internals" guy, and Doug mentioned that he will have
> more time to work on 2.5.x not that long ago, so I do think all three
> consider themselves at least partial maintainers already.

*nods* Both Jens and Doug were invaluable at times when I bought forward a
bunch of stuff from 2.4, (and later scooped up various other small
SCSI bits from assorted places). With a lack of much understanding
in this area, (and lack of motivation to dig too deeply -- I don't even
own any SCSI hard disks/controllers except for some ancient cpqarray thats
rarely powered on), pushing the various bits onto you with meaningful
comments attached became quite hard work, and at times, impossible
without further explanation from Jens or Doug.

Thankfully, Doug did an excellent job at weeding out the crap bits I had
merged, and pushing on the good tried-n-tested bits.

So they both get my vote. James I think is actually doing some of the
grunt work which means offloading the 'syncing/pushing to Linus' fun
onto one of the others is probably a good idea if they're not opposed to
doing it.

Dave

--
| Dave Jones. http://www.codemonkey.org.uk
| SuSE Labs

2002-07-29 05:34:11

[permalink] [raw]

Subject: Re: [PATCH] 2.5.28 small REQ_SPECIAL abstraction

On Sun, Jul 28 2002, James Bottomley wrote:
> > You are right the
> > > rq->flags &= REQ_QUEUED;
> > > and the
> > > if (blk_rq_tagged(rq))
> > blk_queue_end_tag(q, rq);
> > > should be just removed and things are fine.
> > They only survive becouse they don't provide a tag for the request in
> > first place.
> > > Thanks for pointing it out.
>
>
> Please don't remove this.
>
> insert_special isn't just used to start new requests, it's also used to queue
> incoming requests that cannot be processed by the device (host adapter,
> queue_full etc.).
>
> In this latter case, the tag is already begun, so it needs to go back with
> end_tag (we start a new tag when the device begins processing again).
>
> I own up to introducing the &= REQ_QUEUED rubbish---I was just keeping the
> original placement of the flag clearing code, but now we need to preserve
> whether the request was queued or not for the blk_rq_tagged check. On
> reflection it would have been better just to set the flags to REQ_SPECIAL |
> REQ_BARRIER after the end tag code.

I think you are missing the point. The stuff should not be in the
_generic_ blk_insert_request(). As I posted in my first reply to Martin,
SCSI needs to clear the tag before calling blk_insert_request() if it
needs to.

> [email protected] said:
> > But the crap still got merged, sigh... Yet again an excellent point of
> > why stuff like this should go through the maintainer. Apparently Linus
> > blindly applies this stuff.
>
> Hmm, well I sent it to you and you are the Maintainer.

I've never seen it?!

--
Jens Axboe

2002-07-29 05:36:08

[permalink] [raw]

Subject: Re: [PATCH] 2.5.28 small REQ_SPECIAL abstraction

On Sun, Jul 28 2002, Linus Torvalds wrote:
>
>
> On Sun, 28 Jul 2002, Jens Axboe wrote:
> >
> > But the crap still got merged, sigh... Yet again an excellent point of
> > why stuff like this should go through the maintainer. Apparently Linus
> > blindly applies this stuff.
>
> Ehh, since there is no proactive maintainer for SCSI, I don't have much
> choice, do I?
>
> SCSI has been maintainerless for the last few years. Right now three
> people work on it to some degree (Doug Ledford, James Bottomley and you),
> but I don't get timely patches, and neither does apparently anybody else.
>
> Case in point: I was debugging some USB storage issues with Matthew Dharm
> yesterday, and he sent me patches to the SCSI subsystem that he claims
> were supposedly considered valid on the scsi mailing list back in May.
>
> Guess what? I've not seen the patches from any of the three people I
> consider closest to being maintainers.

SCSI is always the first to get neglected it seems, and yes I'm guilty
of that as well. Maybe that can change in the future.

> So your "should go through the maintainer" complaint is obviously a bunch
> of bull. Feel free to step up to the plate, but before you do, don't throw
> rocks in glass houses.

I was referring to the block layer, not the SCSI layer. The broken
changes were applied to the block layer after all, I had not even
noticed that the SCSI one was broken.

--
Jens Axboe

2002-07-29 05:46:16

[permalink] [raw]

Subject: Re: [PATCH] 2.5.28 small REQ_SPECIAL abstraction

On Mon, 29 Jul 2002, Jens Axboe wrote:
>
> I was referring to the block layer, not the SCSI layer. The broken
> changes were applied to the block layer after all, I had not even
> noticed that the SCSI one was broken.

Heh.

Anyway, I don't think the situation is "wrong" per se. Martin took code
that was generic from SCSI, and moved it to the common place. In the
process, you noticed that the original code was broken. Downsides? I guess
it gets fixed now. Sounds reasonable to me, and we should just be happy
that these things get noticed eventually, even if the reason for noticing
it is the "wrong" one.

Linus

2002-07-29 05:51:49

[permalink] [raw]

Subject: Re: [PATCH] 2.5.28 small REQ_SPECIAL abstraction

On Mon, Jul 29 2002, Jens Axboe wrote:
> On Sun, Jul 28 2002, James Bottomley wrote:
> > > You are right the
> > > > rq->flags &= REQ_QUEUED;
> > > > and the
> > > > if (blk_rq_tagged(rq))
> > > blk_queue_end_tag(q, rq);
> > > > should be just removed and things are fine.
> > > They only survive becouse they don't provide a tag for the request in
> > > first place.
> > > > Thanks for pointing it out.
> >
> >
> > Please don't remove this.
> >
> > insert_special isn't just used to start new requests, it's also used to queue
> > incoming requests that cannot be processed by the device (host adapter,
> > queue_full etc.).
> >
> > In this latter case, the tag is already begun, so it needs to go back with
> > end_tag (we start a new tag when the device begins processing again).
> >
> > I own up to introducing the &= REQ_QUEUED rubbish---I was just keeping the
> > original placement of the flag clearing code, but now we need to preserve
> > whether the request was queued or not for the blk_rq_tagged check. On
> > reflection it would have been better just to set the flags to REQ_SPECIAL |
> > REQ_BARRIER after the end tag code.
>
> I think you are missing the point. The stuff should not be in the
> _generic_ blk_insert_request(). As I posted in my first reply to Martin,
> SCSI needs to clear the tag before calling blk_insert_request() if it
> needs to.

Here's the patch to fix it, btw. Linus, please apply.

# This is a BitKeeper generated patch for the following project:
# Project Name: Linux kernel tree
# This patch format is intended for GNU patch command version 2.5 or higher.
# This patch includes the following deltas:
# ChangeSet 1.509 -> 1.510
# drivers/block/ll_rw_blk.c 1.96 -> 1.97
# drivers/scsi/scsi_lib.c 1.29 -> 1.30
#
# The following is the BitKeeper ChangeSet Log
# --------------------------------------------
# 02/07/29 [email protected] 1.510
# undo REQ_QUEUED breakage in recent blk_insert_request() introduction
# --------------------------------------------
#
diff -Nru a/drivers/block/ll_rw_blk.c b/drivers/block/ll_rw_blk.c
--- a/drivers/block/ll_rw_blk.c Mon Jul 29 07:53:43 2002
+++ b/drivers/block/ll_rw_blk.c Mon Jul 29 07:53:43 2002
@@ -1253,7 +1253,7 @@
* host that is unable to accept a particular command.
*/
void blk_insert_request(request_queue_t *q, struct request *rq,
- int at_head, void *data)
+ int at_head, void *data)
{
unsigned long flags;

@@ -1262,15 +1262,11 @@
* must not attempt merges on this) and that it acts as a soft
* barrier
*/
- rq->flags &= REQ_QUEUED;
rq->flags |= REQ_SPECIAL | REQ_BARRIER;

rq->special = data;

spin_lock_irqsave(q->queue_lock, flags);
- /* If command is tagged, release the tag */
- if(blk_rq_tagged(rq))
- blk_queue_end_tag(q, rq);
_elv_add_request(q, rq, !at_head, 0);
q->request_fn(q);
spin_unlock_irqrestore(q->queue_lock, flags);
diff -Nru a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
--- a/drivers/scsi/scsi_lib.c Mon Jul 29 07:53:43 2002
+++ b/drivers/scsi/scsi_lib.c Mon Jul 29 07:53:43 2002
@@ -74,6 +74,9 @@
{
request_queue_t *q = &SCpnt->device->request_queue;

+ if (blk_rq_tagged(SCpnt->request))
+ blk_queue_end_tag(q, SCpnt->request);
+
blk_insert_request(q, SCpnt->request, at_head, SCpnt);
return 0;
}
@@ -101,6 +104,9 @@
int scsi_insert_special_req(Scsi_Request * SRpnt, int at_head)
{
request_queue_t *q = &SRpnt->sr_device->request_queue;
+
+ if (blk_rq_tagged(SRpnt->sr_request))
+ blk_queue_end_tag(q, SRpnt->sr_request);

blk_insert_request(q, SRpnt->sr_request, at_head, SRpnt);
return 0;

--
Jens Axboe

2002-07-29 06:19:49

[permalink] [raw]

Subject: Re: [PATCH] 2.5.28 small REQ_SPECIAL abstraction

On Mon, 29 Jul 2002, Jens Axboe wrote:
> >
> > I think you are missing the point. The stuff should not be in the
> > _generic_ blk_insert_request(). As I posted in my first reply to Martin,
> > SCSI needs to clear the tag before calling blk_insert_request() if it
> > needs to.
>
> Here's the patch to fix it, btw. Linus, please apply.

I can't apply this while I think it looks horribly broken.

Your patch makes scsi_lib call blk_queue_end_tag() without holding the
queue spinlock.

Which looks horribly broken, since it _will_ corrupt the queues.

In fact, it looks about a million times more broken than the code that
Martin submitted.

Please explain to me why I am wrong.

Linus

PS. I actually do tend to look as the patches I apply. Right now it looks
like you're wrong, and Martin is right.

2002-07-29 06:30:32

[permalink] [raw]

Subject: Re: [PATCH] 2.5.28 small REQ_SPECIAL abstraction

On Sun, Jul 28 2002, Linus Torvalds wrote:
>
>
> On Mon, 29 Jul 2002, Jens Axboe wrote:
> > >
> > > I think you are missing the point. The stuff should not be in the
> > > _generic_ blk_insert_request(). As I posted in my first reply to Martin,
> > > SCSI needs to clear the tag before calling blk_insert_request() if it
> > > needs to.
> >
> > Here's the patch to fix it, btw. Linus, please apply.
>
> I can't apply this while I think it looks horribly broken.
>
> Your patch makes scsi_lib call blk_queue_end_tag() without holding the
> queue spinlock.
>
> Which looks horribly broken, since it _will_ corrupt the queues.
>
> In fact, it looks about a million times more broken than the code that
> Martin submitted.
>
> Please explain to me why I am wrong.

Duh yes, the locking is broken of course :/

Ok I'd rather just make a __blk_insert_request as well, and have SCSI
grab the lock itself.

> Linus
>
> PS. I actually do tend to look as the patches I apply. Right now it looks
> like you're wrong, and Martin is right.

I think Martin's was wrong in concept, mine was wrong in implementation.

# This is a BitKeeper generated patch for the following project:
# Project Name: Linux kernel tree
# This patch format is intended for GNU patch command version 2.5 or higher.
# This patch includes the following deltas:
# ChangeSet 1.509 -> 1.510
# drivers/block/ll_rw_blk.c 1.96 -> 1.97
# drivers/scsi/scsi_lib.c 1.29 -> 1.30
#
# The following is the BitKeeper ChangeSet Log
# --------------------------------------------
# 02/07/29 [email protected] 1.510
# blk_insert_request + REQ_QUEUED fixed
# --------------------------------------------
#
diff -Nru a/drivers/block/ll_rw_blk.c b/drivers/block/ll_rw_blk.c
--- a/drivers/block/ll_rw_blk.c Mon Jul 29 08:33:16 2002
+++ b/drivers/block/ll_rw_blk.c Mon Jul 29 08:33:16 2002
@@ -1233,6 +1233,23 @@
return rq;
}

+void __blk_insert_request(request_queue_t *q, struct request *rq,
+ int at_head, void *data)
+{
+ /*
+ * tell I/O scheduler that this isn't a regular read/write (ie it
+ * must not attempt merges on this) and that it acts as a soft
+ * barrier
+ */
+ rq->flags &= REQ_QUEUED;
+ rq->flags |= REQ_SPECIAL | REQ_BARRIER;
+
+ rq->special = data;
+
+ _elv_add_request(q, rq, !at_head, 0);
+ q->request_fn(q);
+}
+
/**
* blk_insert_request - insert a special request in to a request queue
* @q: request queue where request should be inserted
@@ -1253,24 +1270,11 @@
* host that is unable to accept a particular command.
*/
void blk_insert_request(request_queue_t *q, struct request *rq,
- int at_head, void *data)
+ int at_head, void *data)
{
unsigned long flags;

- /*
- * tell I/O scheduler that this isn't a regular read/write (ie it
- * must not attempt merges on this) and that it acts as a soft
- * barrier
- */
- rq->flags &= REQ_QUEUED;
- rq->flags |= REQ_SPECIAL | REQ_BARRIER;
-
- rq->special = data;
-
spin_lock_irqsave(q->queue_lock, flags);
- /* If command is tagged, release the tag */
- if(blk_rq_tagged(rq))
- blk_queue_end_tag(q, rq);
_elv_add_request(q, rq, !at_head, 0);
q->request_fn(q);
spin_unlock_irqrestore(q->queue_lock, flags);
@@ -2125,6 +2129,7 @@
EXPORT_SYMBOL(blk_get_request);
EXPORT_SYMBOL(__blk_get_request);
EXPORT_SYMBOL(blk_put_request);
+EXPORT_SYMBOL(__blk_insert_request);
EXPORT_SYMBOL(blk_insert_request);

EXPORT_SYMBOL(blk_queue_prep_rq);
diff -Nru a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
--- a/drivers/scsi/scsi_lib.c Mon Jul 29 08:33:16 2002
+++ b/drivers/scsi/scsi_lib.c Mon Jul 29 08:33:16 2002
@@ -73,8 +73,15 @@
int scsi_insert_special_cmd(Scsi_Cmnd * SCpnt, int at_head)
{
request_queue_t *q = &SCpnt->device->request_queue;
+ unsigned long flags;

- blk_insert_request(q, SCpnt->request, at_head, SCpnt);
+ spin_lock_irqsave(q->queue_lock, flags);
+
+ if (blk_rq_tagged(SCpnt->request))
+ blk_queue_end_tag(q, SCpnt->request);
+
+ __blk_insert_request(q, SCpnt->request, at_head, SCpnt);
+ spin_unlock_irqrestore(q->queue_lock, flags);
return 0;
}

@@ -101,8 +108,15 @@
int scsi_insert_special_req(Scsi_Request * SRpnt, int at_head)
{
request_queue_t *q = &SRpnt->sr_device->request_queue;
+ unsigned long flags;
+
+ spin_lock_irqsave(q->queue_lock, flags);
+
+ if (blk_rq_tagged(SRpnt->sr_request))
+ blk_queue_end_tag(q, SRpnt->sr_request);

- blk_insert_request(q, SRpnt->sr_request, at_head, SRpnt);
+ __blk_insert_request(q, SRpnt->sr_request, at_head, SRpnt);
+ spin_unlock_irqrestore(q->queue_lock, flags);
return 0;
}

--
Jens Axboe

2002-07-29 06:54:22

[permalink] [raw]

Subject: Re: [PATCH] 2.5.28 small REQ_SPECIAL abstraction

On Mon, 29 Jul 2002, Jens Axboe wrote:
>
> I think Martin's was wrong in concept, mine was wrong in implementation.

I don't understand why you think the concept is wrong. Right now all users
clearly do want to free the tag on re-issue, and doing so clearly cleans
up the code and avoids duplication.

So I still don't see the advantage of your patch, even once you've fixed
the locking issue.

HOWEVER, if you really think that some future users might not want to have
the tag played with, how about making the "at_head" thing a flags field,
and letting people say so by having "INSERT_NOTAG" (and making the
existing bit be INSERT_ATHEAD).

So then the SCSI users would look like

blk_insert_request(q, SRpnt->sr_request,
at_head ? INSERT_ATHEAD : 0,
SRpnt)

while your future non-tag user might do

blk_insert_request(q, newreq,
INSERT_ATHEAD | INSERT_NOTAG,
channel);

_without_ having that unnecessary code duplication.

Linus

2002-07-29 10:26:24

[permalink] [raw]

Subject: Re: [PATCH] 2.5.28 small REQ_SPECIAL abstraction

Jens Axboe wrote:

>
> But the crap still got merged, sigh... Yet again an excellent point of
> why stuff like this should go through the maintainer. Apparently Linus
> blindly applies this stuff.

Jens. Please note that this doesn't make *anything* worser then before,
since I don't use this function right now.

2002-07-29 10:40:19

[permalink] [raw]

Subject: Re: [PATCH] 2.5.28 small REQ_SPECIAL abstraction

On Sun, Jul 28 2002, Linus Torvalds wrote:
>
>
> On Mon, 29 Jul 2002, Jens Axboe wrote:
> >
> > I think Martin's was wrong in concept, mine was wrong in implementation.
>
> I don't understand why you think the concept is wrong. Right now all users
> clearly do want to free the tag on re-issue, and doing so clearly cleans
> up the code and avoids duplication.
>
> So I still don't see the advantage of your patch, even once you've fixed
> the locking issue.

Ok... I had two issues with the patch. 1) it did

rq->flags &= REQ_QUEUED;

which is just broken. 2) it combined the act of inserting back into the
block queue with clearing the tag associated with the request. #1 is
clearly a bug that should be fixed regardless of what we do. Right now,
yes, the only user of blk_insert_request (SCSI) needs the tag cleared. I
still don't think that's a reason to mingle the two different tasks into
one. Code duplication is not an argument, the two scsi_insert_* should
be folded into one. The only difference is SRpnt->sr_request vs
SCpnt->request after all.

> HOWEVER, if you really think that some future users might not want to have
> the tag played with, how about making the "at_head" thing a flags field,
> and letting people say so by having "INSERT_NOTAG" (and making the
> existing bit be INSERT_ATHEAD).
>
> So then the SCSI users would look like
>
> blk_insert_request(q, SRpnt->sr_request,
> at_head ? INSERT_ATHEAD : 0,
> SRpnt)
>
> while your future non-tag user might do
>
> blk_insert_request(q, newreq,
> INSERT_ATHEAD | INSERT_NOTAG,
> channel);
>
> _without_ having that unnecessary code duplication.

*shrug* I guess we could do that. I don't see any immediate use beyond
at_head/back and tag clearing.

I'll back down, it's not a matter of life and death after all. Here's
the minimal patch that corrects the flag thing, and also makes
blk_insert_request() conform to kernel style. Are we all happy?

# This is a BitKeeper generated patch for the following project:
# Project Name: Linux kernel tree
# This patch format is intended for GNU patch command version 2.5 or higher.
# This patch includes the following deltas:
# ChangeSet 1.509 -> 1.510
# drivers/block/ll_rw_blk.c 1.96 -> 1.97
#
# The following is the BitKeeper ChangeSet Log
# --------------------------------------------
# 02/07/29 [email protected] 1.510
# fix REQ_QUEUED clearing in blk_insert_request()
# --------------------------------------------
#
diff -Nru a/drivers/block/ll_rw_blk.c b/drivers/block/ll_rw_blk.c
--- a/drivers/block/ll_rw_blk.c Mon Jul 29 12:42:43 2002
+++ b/drivers/block/ll_rw_blk.c Mon Jul 29 12:42:43 2002
@@ -1253,7 +1253,7 @@
* host that is unable to accept a particular command.
*/
void blk_insert_request(request_queue_t *q, struct request *rq,
- int at_head, void *data)
+ int at_head, void *data)
{
unsigned long flags;

@@ -1262,15 +1262,18 @@
* must not attempt merges on this) and that it acts as a soft
* barrier
*/
- rq->flags &= REQ_QUEUED;
rq->flags |= REQ_SPECIAL | REQ_BARRIER;

rq->special = data;

spin_lock_irqsave(q->queue_lock, flags);
- /* If command is tagged, release the tag */
- if(blk_rq_tagged(rq))
+
+ /*
+ * If command is tagged, release the tag
+ */
+ if (blk_rq_tagged(rq))
blk_queue_end_tag(q, rq);
+
_elv_add_request(q, rq, !at_head, 0);
q->request_fn(q);
spin_unlock_irqrestore(q->queue_lock, flags);

--
Jens Axboe

2002-07-29 10:41:24

[permalink] [raw]

Subject: Re: [PATCH] 2.5.28 small REQ_SPECIAL abstraction

On Mon, Jul 29 2002, Marcin Dalecki wrote:
> Jens Axboe wrote:
>
> >
> >But the crap still got merged, sigh... Yet again an excellent point of
> >why stuff like this should go through the maintainer. Apparently Linus
> >blindly applies this stuff.
>
> Jens. Please note that this doesn't make *anything* worser then before,
> since I don't use this function right now.

SCSI does, though :-)

It's ok now, the issue is resolved as far as I'm concerned.

--
Jens Axboe

2002-07-29 11:06:45

[permalink] [raw]

Subject: Re: [PATCH] 2.5.28 small REQ_SPECIAL abstraction

Jens Axboe wrote:
> On Mon, Jul 29 2002, Marcin Dalecki wrote:
>
>>Jens Axboe wrote:
>>
>>
>>>But the crap still got merged, sigh... Yet again an excellent point of
>>>why stuff like this should go through the maintainer. Apparently Linus
>>>blindly applies this stuff.
>>
>>Jens. Please note that this doesn't make *anything* worser then before,
>>since I don't use this function right now.
>
>
> SCSI does, though :-)
>
> It's ok now, the issue is resolved as far as I'm concerned.

Fine. So I will start to use it soon in IDE too...

2002-07-29 13:40:55

by James Bottomley

[permalink] [raw]

Subject: Re: [PATCH] 2.5.28 small REQ_SPECIAL abstraction

[email protected] said:
> Ok... I had two issues with the patch. 1) it did
> rq->flags &= REQ_QUEUED;

Yes, that was inherited from SCSI. Previously it just cleared flags and then
set REQ_BARRIER|REQ_SPECIAL. Now I needed to clear flags but preserve the
state of REQ_QUEUED, which is what that code is doing, otherwise the
blk_rq_tagged() would always fail lower down.

> I'll back down, it's not a matter of life and death after all. Here's
> the minimal patch that corrects the flag thing, and also makes
> blk_insert_request() conform to kernel style. Are we all happy?

I'm happy (as long as it works on my SCSI card).

James

2002-07-29 13:52:04