Re: 2.5.20 RAID5 compile error

[Jens Axboe]

On Thu, Jun 06 2002, Neil Brown wrote:
> > > In ll_rw_blk.c we change blk_plug_device to avoid the check for
> > > the queue being empty as this may not be well define for
> > > umem/raid5.  Instead, blk_plug_device is not called when
> > > the queue is not empty (which is mostly wasn' anyway).
> > 
> > I left it that way on purpose, and yes I did see you changed that in the
> > previous patch as well. I think it's a bit of a mess to have both
> > blk_plug_device and blk_plug_queue. Without looking at it, I have no
> > idea which does what?! blk_queue_empty() will always be true for
> > make_request_fn type drivers, so no change is necessary there.
> 
> I'm not pushing the blk_plug_device vs blk_plug_queue distinction.
> 
> It is just that I think the current blk_plug_device is wrong...  let
> me try to show you why..
> 
> First, look where it is called, in __make_request (which I always
> thought should be spelt "elevator_make_request") - note that this is
> the ONLY place that it is called other than the calls that have just
> been introduced in md and umem - :
> 
> 
> 	if (blk_queue_empty(q) || bio_barrier(bio)) {
> 		blk_plug_device(q);
> 		goto get_rq;
> 	}
> 
> Which says "if the queue is empty or this is a barrier bio, then
> plug the queue and go an make a request, skipping the merging".
> 
> One then wonders why you would want to plug the queue for a barrier
> bio.  The queue-empty bit makes sense, but not the barrier bit...

No for the barrier bit we need not do the plugging, we just need to skip
the merge step and go directly to grabbing a new request. However if the
queue is indeed empty, then we do need to plug it. See?

> Ok, lets see exactly what blk_plug_device(q) does by (mentally)
> inlining it:
> 
> 	if (blk_queue_empty(q) || bio_barrier(bio)) {
> 
> 		if (!elv_queue_empty(q))
> 			goto return;
> 
> 			if (!blk_queue_plugged(q)) {
> 				spin_lock(&blk_plug_lock);
> 				list_add_tail(&q->plug_list, &blk_plug_list);
> 				spin_unlock(&blk_plug_lock);
> 			}
> 	return:
> 		goto get_rq;
> 	}
> 
> So we are actually plugging it if it isn't already plugged (makes
> sense) and elv_queue_empty, and blk_queue_empty ... or bio_barrier.
> I wander what those two differnt *_queue_empty functions are .....
> looks in blkdev.h.. Oh, they are the same.

Oh agreed, I'll get rid of one of them.

> So we can simplify this a bit:
> 
> 	If (the_queue_is_empty(q)) {
> 		plug_if_not_plugged(q);
> 		goto get_rq;
> 	}
> 	if (bio_barrier(bio)) {
> 	   /* we know the queue is not empty, so avoid that check and 
> 	      simply don't plug */
> 	   goto get_rq;
>         }
> 
> Now that makes sense.  The answer to the question "why would you want
> to plug the queue for a barrier bio?" is that you don't.

The answer is that you don't want to plug it anymore than what you do
for a regular request, but see what I wrote above.

> This is how I came to the change the I suggested.  The current code is
> confusing, and testing elv_queue_empty in blk_plug_device is really a
> layering violation.
> 
> You are correct from a operational sense when you say that "no change
> is necessary there" (providing the queue_head is initialised, which it
> is by blk_init_queue via elevator_init, but isn't by
> blk_queue_make_request) but I don't think you are correct from an
> abstractional purity perspective.

Alright you've convinced me about that part. Will you send me the
updated patch?

-- 
Jens Axboe