[PATCH] 2.5.28 small REQ_SPECIAL abstraction

[PATCH] 2.5.28 small REQ_SPECIAL abstraction

[Marcin Dalecki]

[-- Attachment #1: Type: text/plain, Size: 850 bytes --]

The attached patch does the following:

1. Remove blkdev_release_request(Request); it was an unnecessary wrapper
    around blk_put_request(Request). Likely some leftover from pre-BIO
    time...

2. Abstract out the fine __scsi_insert_special() function out from
    the SCSI code.

    Now that I have finally managed to kill all those IDE 'specific'
    REQ_BLAH request types, we can do this final step, and it will be
    used soon at least by ATA code as well. The goal is that
    scsi_request_fn and do_ide_request should start to look similar
    like silblings.

    Its called blk_insert_request() now and even documented in code.

3. Change some stuff over from extern inline to static inline in
    blkdev.h. (trivia...)

This patch doesn't change *any* functionality, so its not exposing
SCSI to any danger :-).

Please apply.

Thanks.

[-- Attachment #2: blk-2.5.28.diff --]
[-- Type: text/plain, Size: 11645 bytes --]

diff -durNp -x '*.[ao]' -x '*~' -x '*.cmd' -x '*.orig' -x '*.rej' -x 'vmlinu*' -x bzImage -x bootsect -x conmakehash -x setup -x build -x asm -x config -x '.*' -x consolemap_deftbl.c -x defkeymap.c -x devlist.h -x classlist.h linux-2.5.28/drivers/block/DAC960.c linux/drivers/block/DAC960.c
--- linux-2.5.28/drivers/block/DAC960.c	2002-07-24 23:03:30.000000000 +0200
+++ linux/drivers/block/DAC960.c	2002-07-25 23:02:06.000000000 +0200
@@ -2884,7 +2884,7 @@ static boolean DAC960_ProcessRequest(DAC
   Command->BufferHeader = Request->bio;
   Command->RequestBuffer = Request->buffer;
   blkdev_dequeue_request(Request);
-  blkdev_release_request(Request);
+  blk_put_request(Request);
   DAC960_QueueReadWriteCommand(Command);
   return true;
 }
diff -durNp -x '*.[ao]' -x '*~' -x '*.cmd' -x '*.orig' -x '*.rej' -x 'vmlinu*' -x bzImage -x bootsect -x conmakehash -x setup -x build -x asm -x config -x '.*' -x consolemap_deftbl.c -x defkeymap.c -x devlist.h -x classlist.h linux-2.5.28/drivers/block/ll_rw_blk.c linux/drivers/block/ll_rw_blk.c
--- linux-2.5.28/drivers/block/ll_rw_blk.c	2002-07-24 23:03:20.000000000 +0200
+++ linux/drivers/block/ll_rw_blk.c	2002-07-25 23:02:06.000000000 +0200
@@ -1233,9 +1233,47 @@ struct request *__blk_get_request(reques
 	return rq;
 }
 
-void blk_put_request(struct request *rq)
+/**
+ * blk_insert_request - insert a special request in to a request queue
+ * @q:		request queue where request should be inserted
+ * @rq:		request to be inserted
+ * @at_head:	insert request at head or tail of queue
+ * @data:	private data
+ *
+ * Description:
+ *    Many block devices need to execute commands asynchronously, so they don't
+ *    block the whole kernel from preemption during request execution.  This is
+ *    accomplished normally by inserting aritficial requests tagged as
+ *    REQ_SPECIAL in to the corresponding request queue, and letting them be
+ *    scheduled for actual execution by the request queue.
+ *
+ *    We have the option of inserting the head or the tail of the queue.
+ *    Typically we use the tail for new ioctls and so forth.  We use the head
+ *    of the queue for things like a QUEUE_FULL message from a device, or a
+ *    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)
 {
-	blkdev_release_request(rq);
+	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);
 }
 
 /* RO fail safe mechanism */
@@ -1307,7 +1345,7 @@ static inline void add_request(request_q
 /*
  * Must be called with queue lock held and interrupts disabled
  */
-void blkdev_release_request(struct request *req)
+void blk_put_request(struct request *req)
 {
 	struct request_list *rl = req->rl;
 	request_queue_t *q = req->q;
@@ -1370,7 +1408,7 @@ static void attempt_merge(request_queue_
 
 		req->nr_sectors = req->hard_nr_sectors += next->hard_nr_sectors;
 
-		blkdev_release_request(next);
+		blk_put_request(next);
 	}
 }
 
@@ -1568,7 +1606,7 @@ get_rq:
 	add_request(q, req, insert_here);
 out:
 	if (freereq)
-		blkdev_release_request(freereq);
+		blk_put_request(freereq);
 	spin_unlock_irq(q->queue_lock);
 	return 0;
 
@@ -2003,7 +2041,7 @@ void end_that_request_last(struct reques
 	if (req->waiting)
 		complete(req->waiting);
 
-	blkdev_release_request(req);
+	blk_put_request(req);
 }
 
 #define MB(kb)	((kb) << 10)
@@ -2064,7 +2102,6 @@ EXPORT_SYMBOL(blk_cleanup_queue);
 EXPORT_SYMBOL(blk_queue_make_request);
 EXPORT_SYMBOL(blk_queue_bounce_limit);
 EXPORT_SYMBOL(generic_make_request);
-EXPORT_SYMBOL(blkdev_release_request);
 EXPORT_SYMBOL(generic_unplug_device);
 EXPORT_SYMBOL(blk_plug_device);
 EXPORT_SYMBOL(blk_remove_plug);
@@ -2088,6 +2125,7 @@ EXPORT_SYMBOL(blk_hw_contig_segment);
 EXPORT_SYMBOL(blk_get_request);
 EXPORT_SYMBOL(__blk_get_request);
 EXPORT_SYMBOL(blk_put_request);
+EXPORT_SYMBOL(blk_insert_request);
 
 EXPORT_SYMBOL(blk_queue_prep_rq);
 
diff -durNp -x '*.[ao]' -x '*~' -x '*.cmd' -x '*.orig' -x '*.rej' -x 'vmlinu*' -x bzImage -x bootsect -x conmakehash -x setup -x build -x asm -x config -x '.*' -x consolemap_deftbl.c -x defkeymap.c -x devlist.h -x classlist.h linux-2.5.28/drivers/scsi/scsi_lib.c linux/drivers/scsi/scsi_lib.c
--- linux-2.5.28/drivers/scsi/scsi_lib.c	2002-07-24 23:03:28.000000000 +0200
+++ linux/drivers/scsi/scsi_lib.c	2002-07-25 23:02:07.000000000 +0200
@@ -51,53 +51,6 @@
  */
 
 /*
- * Function:	__scsi_insert_special()
- *
- * Purpose:	worker for scsi_insert_special_*()
- *
- * Arguments:	q - request queue where request should be inserted
- *		rq - request to be inserted
- * 		data - private data
- *		at_head - insert request at head or tail of queue
- *
- * Lock status:	Assumed that queue lock is not held upon entry.
- *
- * Returns:	Nothing
- */
-static void __scsi_insert_special(request_queue_t *q, struct request *rq,
-				  void *data, int at_head)
-{
-	unsigned long flags;
-
-	ASSERT_LOCK(q->queue_lock, 0);
-
-	/*
-	 * 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;
-
-	/*
-	 * We have the option of inserting the head or the tail of the queue.
-	 * Typically we use the tail for new ioctls and so forth.  We use the
-	 * head of the queue for things like a QUEUE_FULL message from a
-	 * device, or a host that is unable to accept a particular command.
-	 */
-	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);
-}
-
-
-/*
  * Function:    scsi_insert_special_cmd()
  *
  * Purpose:     Insert pre-formed command into request queue.
@@ -121,7 +74,7 @@ int scsi_insert_special_cmd(Scsi_Cmnd * 
 {
 	request_queue_t *q = &SCpnt->device->request_queue;
 
-	__scsi_insert_special(q, SCpnt->request, SCpnt, at_head);
+	blk_insert_request(q, SCpnt->request, at_head, SCpnt);
 	return 0;
 }
 
@@ -149,7 +102,7 @@ int scsi_insert_special_req(Scsi_Request
 {
 	request_queue_t *q = &SRpnt->sr_device->request_queue;
 
-	__scsi_insert_special(q, SRpnt->sr_request, SRpnt, at_head);
+	blk_insert_request(q, SRpnt->sr_request, at_head, SRpnt);
 	return 0;
 }
 
diff -durNp -x '*.[ao]' -x '*~' -x '*.cmd' -x '*.orig' -x '*.rej' -x 'vmlinu*' -x bzImage -x bootsect -x conmakehash -x setup -x build -x asm -x config -x '.*' -x consolemap_deftbl.c -x defkeymap.c -x devlist.h -x classlist.h linux-2.5.28/include/linux/blkdev.h linux/include/linux/blkdev.h
--- linux-2.5.28/include/linux/blkdev.h	2002-07-24 23:03:22.000000000 +0200
+++ linux/include/linux/blkdev.h	2002-07-25 23:02:07.000000000 +0200
@@ -281,12 +281,13 @@ extern int wipe_partitions(kdev_t dev);
 extern void register_disk(struct gendisk *dev, kdev_t first, unsigned minors, struct block_device_operations *ops, long size);
 extern void generic_make_request(struct bio *bio);
 extern inline request_queue_t *bdev_get_queue(struct block_device *bdev);
-extern void blkdev_release_request(struct request *);
+extern void blk_put_request(struct request *);
 extern void blk_attempt_remerge(request_queue_t *, struct request *);
 extern void __blk_attempt_remerge(request_queue_t *, struct request *);
 extern struct request *blk_get_request(request_queue_t *, int, int);
 extern struct request *__blk_get_request(request_queue_t *, int);
 extern void blk_put_request(struct request *);
+extern void blk_insert_request(request_queue_t *, struct request *, int, void *);
 extern void blk_plug_device(request_queue_t *);
 extern int blk_remove_plug(request_queue_t *);
 extern void blk_recount_segments(request_queue_t *, struct bio *);
@@ -309,20 +310,21 @@ extern int blk_init_queue(request_queue_
 extern void blk_cleanup_queue(request_queue_t *);
 extern void blk_queue_make_request(request_queue_t *, make_request_fn *);
 extern void blk_queue_bounce_limit(request_queue_t *, u64);
-extern void blk_queue_max_sectors(request_queue_t *q, unsigned short);
-extern void blk_queue_max_phys_segments(request_queue_t *q, unsigned short);
-extern void blk_queue_max_hw_segments(request_queue_t *q, unsigned short);
-extern void blk_queue_max_segment_size(request_queue_t *q, unsigned int);
-extern void blk_queue_hardsect_size(request_queue_t *q, unsigned short);
-extern void blk_queue_segment_boundary(request_queue_t *q, unsigned long);
-extern void blk_queue_assign_lock(request_queue_t *q, spinlock_t *);
-extern void blk_queue_prep_rq(request_queue_t *q, prep_rq_fn *pfn);
+extern void blk_queue_max_sectors(request_queue_t *, unsigned short);
+extern void blk_queue_max_phys_segments(request_queue_t *, unsigned short);
+extern void blk_queue_max_hw_segments(request_queue_t *, unsigned short);
+extern void blk_queue_max_segment_size(request_queue_t *, unsigned int);
+extern void blk_queue_hardsect_size(request_queue_t *, unsigned short);
+extern void blk_queue_segment_boundary(request_queue_t *, unsigned long);
+extern void blk_queue_assign_lock(request_queue_t *, spinlock_t *);
+extern void blk_queue_prep_rq(request_queue_t *, prep_rq_fn *pfn);
 extern struct backing_dev_info *blk_get_backing_dev_info(struct block_device *bdev);
 
 extern int blk_rq_map_sg(request_queue_t *, struct request *, struct scatterlist *);
 extern void blk_dump_rq_flags(struct request *, char *);
 extern void generic_unplug_device(void *);
 
+
 /*
  * tag stuff
  */
@@ -348,15 +350,12 @@ extern int * blk_size[MAX_BLKDEV];	/* in
 
 extern void drive_stat_acct(struct request *, int, int);
 
-extern inline void blk_clear(int major)
+static inline void blk_clear(int major)
 {
 	blk_size[major] = NULL;
-#if 0
-	blk_size_in_bytes[major] = NULL;
-#endif
 }
 
-extern inline int queue_hardsect_size(request_queue_t *q)
+static inline int queue_hardsect_size(request_queue_t *q)
 {
 	int retval = 512;
 
@@ -366,7 +365,7 @@ extern inline int queue_hardsect_size(re
 	return retval;
 }
 
-extern inline int bdev_hardsect_size(struct block_device *bdev)
+static inline int bdev_hardsect_size(struct block_device *bdev)
 {
 	return queue_hardsect_size(bdev_get_queue(bdev));
 }
@@ -375,7 +374,7 @@ extern inline int bdev_hardsect_size(str
 #define blk_started_io(nsects)	do { } while (0)
 
 /* assumes size > 256 */
-extern inline unsigned int blksize_bits(unsigned int size)
+static inline unsigned int blksize_bits(unsigned int size)
 {
 	unsigned int bits = 8;
 	do {
diff -durNp -x '*.[ao]' -x '*~' -x '*.cmd' -x '*.orig' -x '*.rej' -x 'vmlinu*' -x bzImage -x bootsect -x conmakehash -x setup -x build -x asm -x config -x '.*' -x consolemap_deftbl.c -x defkeymap.c -x devlist.h -x classlist.h linux-2.5.28/include/linux/nbd.h linux/include/linux/nbd.h
--- linux-2.5.28/include/linux/nbd.h	2002-07-24 23:03:31.000000000 +0200
+++ linux/include/linux/nbd.h	2002-07-25 23:02:07.000000000 +0200
@@ -61,7 +61,7 @@ nbd_end_request(struct request *req)
 		bio->bi_next = NULL;
 		bio_endio(bio, uptodate);
 	}
-	blkdev_release_request(req);
+	blk_put_request(req);
 	spin_unlock_irqrestore(q->queue_lock, flags);
 }
 

Re: [PATCH] 2.5.28 small REQ_SPECIAL abstraction

[Jens Axboe]

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

Re: [PATCH] 2.5.28 small REQ_SPECIAL abstraction

[Marcin Dalecki]

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.

Re: [PATCH] 2.5.28 small REQ_SPECIAL abstraction

[Jens Axboe]

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

Re: [PATCH] 2.5.28 small REQ_SPECIAL abstraction

[James Bottomley]

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

axboe@suse.de 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.

Re: [PATCH] 2.5.28 small REQ_SPECIAL abstraction

[Linus Torvalds]

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

Re: [PATCH] 2.5.28 small REQ_SPECIAL abstraction

[Andries.Brouwer]

    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.]

Re: [PATCH] 2.5.28 small REQ_SPECIAL abstraction

[Linus Torvalds]

On Mon, 29 Jul 2002 Andries.Brouwer@cwi.nl 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

Re: [PATCH] 2.5.28 small REQ_SPECIAL abstraction

[Dave Jones]

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

Re: [PATCH] 2.5.28 small REQ_SPECIAL abstraction

[Jens Axboe]

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.

> axboe@suse.de 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

Re: [PATCH] 2.5.28 small REQ_SPECIAL abstraction

[Jens Axboe]

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

Re: [PATCH] 2.5.28 small REQ_SPECIAL abstraction

[Linus Torvalds]

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

Re: [PATCH] 2.5.28 small REQ_SPECIAL abstraction

[Jens Axboe]

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	axboe@burns.home.kernel.dk	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

Re: [PATCH] 2.5.28 small REQ_SPECIAL abstraction

[Linus Torvalds]

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.

Re: [PATCH] 2.5.28 small REQ_SPECIAL abstraction

[Jens Axboe]

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	axboe@burns.home.kernel.dk	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

Re: [PATCH] 2.5.28 small REQ_SPECIAL abstraction

[Linus Torvalds]

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

Re: [PATCH] 2.5.28 small REQ_SPECIAL abstraction

[Marcin Dalecki]

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.

Re: [PATCH] 2.5.28 small REQ_SPECIAL abstraction

[Jens Axboe]

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	axboe@burns.home.kernel.dk	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

Re: [PATCH] 2.5.28 small REQ_SPECIAL abstraction

[Jens Axboe]

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

Re: [PATCH] 2.5.28 small REQ_SPECIAL abstraction

[Marcin Dalecki]

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

Re: [PATCH] 2.5.28 small REQ_SPECIAL abstraction

[James Bottomley]

axboe@suse.de 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

Re: [PATCH] 2.5.28 small REQ_SPECIAL abstraction

[Marcin Dalecki]

James Bottomley wrote:
> axboe@suse.de 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


BTW.> I just noticed quite a "bunch" of single line functions in the
SCIS code. Sort of like:

int scsi_warp_foo(xxx)
{
      foor(whis and that);
}.

EXPORT_SYMBOL(scsi_wrap_foo);

All of them just eat space on the stack during execution.
Would you mind moving them over to scsi.h and making them static inline?

We all know that SCSI has sometimes problems with the limited stack 
depth during kernel code execution time, esp on "Black Big Boxen"...
Well the above "tactics" doesn't hlep buch, but a bit is a bit is a bit 
and "a man/farmer doesn't foregive someone still alive"... :-).

Re: [PATCH] 2.5.28 small REQ_SPECIAL abstraction

[Rob Landley]

On Sunday 28 July 2002 07:59 pm, Andries.Brouwer@cwi.nl wrote:

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

Linus didn't "kill the idea of maintainers", he just went to a four tier 
hierarchy for scalability reasons.

Here's my understanding of the current developent infrastructure.  Anybody 
who wants to disagree with this feel free, otherwise there should probably be 
some kind of FAQ entry or update to Documentation/SubmittingChanges.

Developers submit to maintainers.  Maintainers submit to lieutenants.  
Lieutenants submit to Linus.  Each level can take stuff from anyone below 
them if they want to, but only the layer IMMEDIATELY below them is owed any 
sort of explanation as to why a patch was NOT accepted.

I.E. If anybody but a lieutenant submits a patch to Linus, and he drops it, 
there's not much likelihood of an explanation why.  If a lieutenant signs off 
on a patch and forwards it to Linus, if it gets rejected he'll probably say 
why it was rejected.  (It could be his mailbox runneth over, but there 
shouldn't be the "I submitted this a dozen times and never heard anything" 
problem when Lieutenants submit to Linus.  In an ideal world, anyway. :)

This is, basically, what's special about lieutenants.  They are the ONLY 
people to whom Linus owes any kind of explanation when a patch gets rejected. 
 (The explanation may not be much more than "I don't like this, I'm not going 
to apply it, so there", but at least they aren't left hanging endlessly.  At 
the very least it's closure.)

Similarly, lieutenants should reply to the maintainers who report to them 
when they reject patches the maintainer has signed off on.  And maintainers 
should reply to individual developers who submit patches to them (by email; 
they may not see it if it's just posted on the list).  These replies are not 
only common courtesy, they help the system work.

So if a random developer wants to get a patch to Linus, and Linus doesn't 
just snatch it out of the ether, the way to proceed is find the appropriate 
maintainer, and get their opinion of the patch.  The developer needs to work 
with the maintainer until the patch is accepted by that maintainer: they need 
to fix anything the maintainer finds wrong with it, and if the maintainer 
says the whole thing is a bad idea, trying to "go over their heads" to a 
lieutenant is unlikely to work.  (They have a wonderful excuse to ignore you, 
which is the least effort solution on their part.  No you can't pester them 
into submission, you'll just wind up in an email auto-delete file.)

The maintainer may want changes, they may want peer review on a specific 
mailing list (sometimes linux-kernel, sometimes not), they may want benchmark 
results, they may want you to download and run a specific stress-testing 
suite...  Or they may be happy with it as is, or they may just say "no" to 
the whole idea and you have to go try another approach entirely.

Then, once a developer has gotten the maintainer to sign off on a patch 
("looks good to me"), the developer and/or the maintainer can submit it to 
the appropriate lieutenant, who is in charge of a larger part of the kernel 
and will most likely find a fresh batch of things wrong with the patch, so 
the process is repeated at the new level.  (Knowing which lieutenant to 
forward developers with patches to is part of a maintainer's job.)  When 
going for a lieutenant's blessing, it's still the developer's job to fix the 
patches.  The maintainer was just passing judgement, now the lieutenant is.  
Neither is under any obligation to do your work for you.  They find problems, 
but it's up to the developer to fix them.  They may be enthused enough by 
your idea to take it and run with it, but there's no guarantee of this, and 
they usually have a to-do list as long as your arm (and that's in a very 
small font) well before your patch enters the picture.

Again, the lieutenant can veto the entire idea of your patch (the 
maintainer's approval does not mean their lieutenant will approve), or raise 
any number of objections.  The lieutenant's approval is needed before 
proceeding, so the developer needs to fix anything the lieutenant finds wrong 
with the patch, regardless of what the maintainer thought.

Then once the lieutenant is appeased and signs off of it, the patch goes to 
Linus.  The lieutenant should probably be the one to forward it (cc:ing the 
developer) or else Linus probably won't even notice it (his in-box runneth 
over).

Trivial, obvious bugfixes can sometimes bypass this using the trivial patch 
manager (something like trivial@rustcorp.com, check the list archives.  Rusty 
Russel runs it).  Think of him as a "Lieutenant Jr. Grade", he accepts 
directly from developers, but only if it's a really obviously correct bug fix.

Bitkeeper helps this a bit: you can see when a patch goes in, so you get 
faster positive feedback.  And once it's in somebody's bitkeeper tree it may 
trickle upward without too much more active push from the developers (until 
something is found to be wrong with it).  But for negative feedback (that it 
was seen and rejected, and why) a developer still has to go through channels.

The advantage of all this is twofold:

1) Developers aren't left hanging endlessly with nobody responding to their 
patch submissions.  At each point, you know who to bug next to get an answer. 
 (Maybe not the answer you want, but an answer.)  This saves developers a lot 
of time and aggravation.

2) By the time a patch get to Linus, it's already been reviewed by two 
competent developers (somebody he directly trusts and somebody else they 
trust), the obvious cleanups have been done, and if they thought it needed 
wider peer review they'll have already suggested it before giving their 
approval.  (Some of them even run their own trees for testing purposes.)  
This saves Linus a lot of time and aggravation.  (He will still reject a lot 
of these patches.  Or require changes to be made, just like the first two 
levels.)

The reason some people think that maintainership is now meaningless is that 
Linus doesn't respond directly to maintainers.  Because Linus appoints 
lieutenants, and the lieutenants appoint maintainers under them, Linus may 
never even have heard of some of the maintainers, let alone trust their 
judgement.  But maintainers are needed to connect random unknown developers 
with lieutanants, so the lieutenants don't get overwhelmed.  As long as the 
lieutenants listen to their maintainers, and Linus listens to his 
lieutenants, the system works fine.

Was that a rambling and incoherent enough explanation for you?

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

The referres vet it, and then the editor vets it.  That's a three level 
hierarchy.  This is a four level one.

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

Nope.  A lack of negative does not equal a positive.  That way lies madness.  
(Lots of people getting very mad, in fact...)

Rob