Re: PATCH: sleeping while holding a lock in _pagebuf_free_bh()::page_buf.c

[Christoph Hellwig <hch@xxxxxxxxxxxxx>]

On Tue, Oct 22, 2002 at 05:20:14PM -0700, Andrew Morton wrote:
> > -
> > -               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);
> >         }
> >  }
> > 
> 
> I'd be interested in a description of how this can help
> avoid a deadlock.

After thinking more about it it probably can't - I was thinking of the
case where the waiting threads holds a sleeping lock.

I've digged a bit deeper into the code and found the following issues:

                add_wait_queue(&pb_resv_bh_wait, &wait);
                do {
                        run_task_queue(&tq_disk);

        This might take some time, while we're still TASK_RUNNING,
        try_to_wake_up() will skip us simplly here, we've might
        lose a waekup.  In addition tq function could possibly
        sleep, although no user of tq_disk in the 2.4 tree does.

                set_task_state(tsk, TASK_UNINTERRUPTIBLE);
                spin_unlock_irqrestore(&pb_resv_bh_lock, flags);
                schedule();

        No we schedule away after the lost wakeup.  We could get
        starved pretty soon by new callers putting themselves in
        front of us in the wq.

                spin_lock_irqsave(&pb_resv_bh_lock, flags);

Luben, could you try to reproduce it with the patch below?
(the double condition check might be a bit overdone, but it's
a common ideom in the kernel and I rather play save)

--- linux/fs/xfs/pagebuf/page_buf.c     2002/10/23 04:21:07     1.71
+++ linux/fs/xfs/pagebuf/page_buf.c     2002/10/23 14:49:01
@@ -657,9 +657,11 @@ _pagebuf_get_prealloc_bh(void)
 
                add_wait_queue(&pb_resv_bh_wait, &wait);
                do {
-                       run_task_queue(&tq_disk);
                        set_task_state(tsk, TASK_UNINTERRUPTIBLE);
+                       if (pb_resv_bh_cnt >= 1)
+                               break;
                        spin_unlock_irqrestore(&pb_resv_bh_lock, flags);
+                       run_task_queue(&tq_disk);
                        schedule();
                        spin_lock_irqsave(&pb_resv_bh_lock, flags);
                } while (pb_resv_bh_cnt < 1);