Re: xfstests 073 regression

[Dave Chinner <david@fromorbit.com>]

On Sun, Jul 31, 2011 at 05:09:16PM +0800, Wu Fengguang wrote:
> On Sat, Jul 30, 2011 at 09:44:22PM +0800, Christoph Hellwig wrote:
> > On Fri, Jul 29, 2011 at 10:21:21PM +0800, Wu Fengguang wrote:
> > > I cannot reproduce the bug. However looking through the code, I find
> > > the only possible place that may keep wb_writeback() looping with
> > > wb->list_lock grabbed is the below requeue_io() call.
> > > 
> > > Would you try the patch?  Note that even if it fixed the soft lockup,
> > > it may not be suitable as the final fix.
> > 
> > This patch fixes the hang for me.
> 
> Great. It means grab_super_passive() always returns false for up to 22s,
> due to
> 
> a) list_empty(&sb->s_instances), which is very unlikely
> 
> b) failed to grab &sb->s_umount
> 
> So the chances are s_umount is mostly taken by others during the 22s.
> Maybe some task other than the flusher is actively doing writeback.

Writeback only holds a read lock on s_umount.

> These callers are not likely since they only do _small_ writes that
> hardly takes one second.
> 
>         bdi_forker_thread:
>                 writeback_inodes_wb(&bdi->wb, 1024);
> 
>         balance_dirty_pages:
>                 writeback_inodes_wb(&bdi->wb, write_chunk);

The "something else doing writeback" reason doesn't make sense to
me.

grab_super_passive() is doing a down_read_trylock(), so if the lock
is failing it must be held exclusively by something.  That only
happens if the filesystem is being mounted, unmounted, remounted,
frozen or thawed, right? 073 doesn't freeze/thaw filesystems, but it
does mount/remount/unmount them.

So is this a writeback vs remount,ro race?

> However the writeback_inodes_sb*() and sync_inodes_sb() functions will
> very likely take dozens of seconds to complete. They have the same
> pattern of
> 
>         down_read(&sb->s_umount);
>         bdi_queue_work(sb->s_bdi, &work);
>         wait_for_completion(&done);
>         up_read(&sb->s_umount);

As per above, those read locks will not hold off
grab_super_passive() which is also taking a read lock. There has to
be some other actor in this deadlock...

> Note that s_umount is grabbed as early as bdi_queue_work() time, when
> the flusher is actively working on some other works. And since the
> for_background/for_kupdate works will quit on seeing other pending
> works, the soft lockup should only happen when the flusher is
> executing some nr_pages=LARGE work when there comes a sync() which
> calls writeback_inodes_sb() for the wait=0 sync stage.
> 
> If we simply apply the change
> 
>                 if (!grab_super_passive(sb)) {
> -                       requeue_io(inode, wb);
> +                       redirty_tail(inode, wb);
>                         continue;
>                 }

I think the root cause of the deadlock needs to be explained before
we can determine the validity of the fix....

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com