Christoph Hellwig wrote:
>
> On Mon, Oct 21, 2002 at 07:56:20PM -0400, Luben Tuikov wrote:
> > Problem: on an SMP system, BANG#@!, the unthinkable happens.
> > Solution: never sleep when holding a lock.
> >
> > This patch applies to CVS code as of about 18:30 EDT
> > on Mon Oct 21 (today), and is self-explanatory.
> >
> > This patch fixes the problem of the mount going into D state
> > indefinitely when the RAID is syncing and mount is run
> > right after mkfs.xfs (from shell script, no sleep between,
> > low system load, SMP).
>
> Bug is real, fix is wrong.
I think so too!
BTW, shouldn't flags be unsigned long? (Rather than int?)
Also, why is there 2 identical if() checks for pb_resv_bh_cnt?
One inside the lock and one outside? Isn't this begging for
a race condition?
After the attidute I've seen here, I'd better put everything
in question form, else woe is me.
Now I'm not even sure in my own eyes... (I actually printed on paper
the code in question before typing all this... Thanks a lot Christoph!)
> The right fix for this issue is to
> move the wake_up outside the lock, like below. I'll commit
> this patch together with a few related changes tmorrow when
> I'm back in the office.
>
> --- /usr/tmp/TmpDir.8821-0/linux/fs/xfs/pagebuf/page_buf.c_1.66 Tue Oct 22
> 16:29:54 2002
> +++ linux/fs/xfs/pagebuf/page_buf.c Tue Oct 22 23:44:25 2002
> @@ -709,12 +709,8 @@
> bh->b_next = pb_resv_bh;
> pb_resv_bh = bh;
> pb_resv_bh_cnt++;
> -
> - if (waitqueue_active(&pb_resv_bh_wait)) {
> - wake_up(&pb_resv_bh_wait);
> - }
> -
> spin_unlock_irqrestore(&pb_resv_bh_lock, flags);
> + wake_up(&pb_resv_bh_wait);
> }
> }
>
--
Luben
|