xfs
[Top] [All Lists]

Re: [PATCH 3/3] Add timeout feature

To: Andrew Morton <akpm@xxxxxxxxxxxxxxxxxxxx>
Subject: Re: [PATCH 3/3] Add timeout feature
From: Oleg Nesterov <oleg@xxxxxxxxxx>
Date: Sun, 24 Aug 2008 21:03:57 +0400
Cc: Takashi Sato <t-sato@xxxxxxxxxxxxx>, linux-fsdevel@xxxxxxxxxxxxxxx, dm-devel@xxxxxxxxxx, viro@xxxxxxxxxxxxxxxxxx, linux-ext4@xxxxxxxxxxxxxxx, xfs@xxxxxxxxxxx, hch@xxxxxxxxxxxxx, axboe@xxxxxxxxx, mtk.manpages@xxxxxxxxxxxxxx, linux-kernel@xxxxxxxxxxxxxxx
In-reply-to: <20080821132006.9949101c.akpm@xxxxxxxxxxxxxxxxxxxx>
References: <20080818212856t-sato@xxxxxxxxxxxxxxx> <20080821132006.9949101c.akpm@xxxxxxxxxxxxxxxxxxxx>
Sender: xfs-bounce@xxxxxxxxxxx
User-agent: Mutt/1.5.11
On 08/21, Andrew Morton wrote:
>
> On Mon, 18 Aug 2008 21:28:56 +0900
> Takashi Sato <t-sato@xxxxxxxxxxxxx> wrote:
>
> > +void del_freeze_timeout(struct block_device *bdev)
> > +{
> > +   /*
> > +    * It's possible that the delayed work task (freeze_timeout()) calls
> > +    * del_freeze_timeout().  If the delayed work task calls
> > +    * cancel_delayed_work_sync((), the deadlock will occur.
> > +    * So we need this check (delayed_work_pending()).
> > +    */
> > +   if (delayed_work_pending(&bdev->bd_freeze_timeout))
> > +           cancel_delayed_work_sync(&bdev->bd_freeze_timeout);
> > +}

I don't understand this patch, but the code above looks strange to me...

Let's suppose del_freeze_timeout() is called by ioctl_thaw()->thaw_bdev().
Now,

        IF delayed_work_pending() == T

                we can deadlock if the timer expires before
                cancel_delayed_work_sync() cancels it?
                in that case we are going to wait for this work,
                but freeze_timeout()->thaw_bdev() will block
                on ->bd_freeze_sem, no?

        ELSE

                we don't really flush the work, it is possible
                the timer has already expired and the work
                is pending. It will run later.

Perhaps this all is correct, but in that case, why can't we just do

        void del_freeze_timeout(struct block_device *bdev)
        {
                cancel_delayed_work(&bdev->bd_freeze_timeout);
        }

?

> Perhaps cancel_delayed_work_sync() shouldn't hang up if called from the
> work handler?

This is trivial,

        --- kernel/workqueue.c
        +++ kernel/workqueue.c
        @@ -516,6 +516,9 @@ static void wait_on_cpu_work(struct cpu_
                struct wq_barrier barr;
                int running = 0;
         
        +       if (cwq->thread == current)
        +               return;
        +
                spin_lock_irq(&cwq->lock);
                if (unlikely(cwq->current_work == work)) {
                        insert_wq_barrier(cwq, &barr, cwq->worklist.next);

but do we really need this?

We have a similar hack in flush_cpu_workqueue(), and we are going
to kill it once we fix the callers.

I dunno.

Oleg.


<Prev in Thread] Current Thread [Next in Thread>