Re: [PATCH] writeback_inodes can race with unmount

[Chris Mason <mason@suse.com>]

On Tue, 2004-06-08 at 17:56, Andrew Morton wrote:
> Chris Mason <mason@suse.com> wrote:

> > > Why don't we have the same race in the sync() path as well? Moving the 
> > > locking to sync_sb_inodes() itself would fix it also.
> > 
> > In the sync() path we're already taking a read lock on s_umount sem. 
> > Moving the locking into sync_sb_inodes would be tricky, it is sometimes
> > called with the write lock on s_umount_sem held and sometimes with a
> > read lock.
> > 
> 
> Plus we'd be dead if we had to do the above.  If that read_trylock() fails
> the sync() will forget to sync stuff.
> 
> And, contra your description, we'll fail the trylock relatively frequently
> - when some other process is writing back this superblock.

The write lock is taken much less frequently.  Should be just on
mount/unmount no?

> 
> But as this codepath is never used for data-integrity writeout things
> should be OK.  I'll slip a comment in there and maybe a
> BUG_ON(wbc->sync_mode == WB_SYNC_ALL).
> 
> You didn't tell us where the window is?  It's a bit weird that some other
> process can come in and unmount the fs while there are non-zero-refcount
> inodes floating about on the superblock.

It's the iput in sync_sb_inodes().  The test workload is 8 parallel runs
of this loop (each run to a different /dev/xxx)

while(true) ; do
    mkfs /dev/xxx
    mount /dev/xxx /xxx
    dd if=/dev/zero of=/xxx/foo bs=1MB count=80
    rm /xxx/foo
    umount /dev/xxx
done

So, each FS only has a single dirty inode, and that single inode gets
deleted during the run.  With enough memory pressure, write calls
frequently go into throttling, increasing the chance for sync_sb_inodes
to be called for writeback at the same time as unmount.

Right before the iput in sync_sb_inodes, we've dropped both the sb_lock
and the inode_lock, nothing prevents the unmount from continuing.  The
super block won't be freed because we've got it pinned, but the unmount
can proceed.

So, it looks like we get this:

CPU 0				CPU 1
writeback_inodes		generic_shutdown_super
sync_sb_inodes			
iget(inode)
spin_unlock(&inode_lock)
				sop->put_super(sb)
iput(inode)
generic_delete_inode()