Re: [PATCH] io stalls (was: -rc7 Re: Linux 2.4.21-rc6)

[Andrea Arcangeli]

On Tue, Jun 10, 2003 at 08:54:00PM -0400, Chris Mason wrote:
> On Tue, 2003-06-10 at 20:33, Andrea Arcangeli wrote:
> > On Mon, Jun 09, 2003 at 05:39:23PM -0400, Chris Mason wrote:
> > > +	if (!waitqueue_active(&q->wait_for_requests[rw]))
> > > +		clear_queue_full(q, rw);
> > 
> > you've an smp race above, the smp safe implementation is this:
> > 
> 
> clear_queue_full has a wmb() in my patch, and queue_full has a rmb(), I
> thought that covered these cases?  I'd rather remove those though, since
> the spot you point out is the only place done outside the
> io_request_lock.
> 
> > 	if (!waitqueue_active(&q->wait_for_requests[rw])) {
> > 		clear_queue_full(q, rw);
> > 		mb();
> > 		if (unlikely(waitqueue_active(&q->wait_for_requests[rw])))
> > 			wake_up(&q->wait_for_requests[rw]);
> > 	}
> > 
> I don't think we need the extra wake_up (this is in __get_request_wait,
> right?), since it gets done by get_request_wait_wakeup()

there's no get_request_wait_wakeup in blkdev_release_request. I put the
construct in both places though (i've the clear_queue_full explicit as
q->full = 0).

And I don't think any of your barriers is needed at all, I mean, we only
need to be careful to clear it right, we don't need to be careful to set
or read it right when it transits from 0 to 1. And the above seems
enough to me to get right the clearing.

> > I'm also unsure what the "waited" logic does, it doesn't seem necessary.
> 
> Once a process waits once, they are allowed to ignore the q->full flag. 
> This way existing waiters can make progress even when q->full is set. 
> Without the waited check, q->full will never get cleared because the
> last writer wouldn't proceed until the last writer was gone.  I had to
> make __get_request for the same reason.

__get_request makes perfect sense of course and it's needed, this is not
the issue, my point about the waited check is that the last writer has
to get the wakeup (and the wakeup has nothing to do with the waited
check since waited == 0), and after the wakeup it will get the request
and it won't re-run the loop, so I don't see why waited is needed.
Furthmore even if for whatever reason it doesn't get the request, it
will re-set full to 1 and it'll be still the first to get the wakeup,
and it will definitely get another wakeup if none request was available.

Andrea