writeback_inodes can race with unmount

There's a small window where the filesystem can be unmounted during writeback_inodes. The end result is the iput done by sync_sb_inodes could be done after the FS put_super and and the super has been removed from all lists.

The fix is to hold the s_umount sem during sync_sb_inodes to make sure the FS doesn't get unmounted.

Index: linux.t/fs/fs-writeback.c
===================================================================

fs/fs-writeback.c +3 -1

@@ -360,9 +360,18 @@ restart:
 	sb = sb_entry(super_blocks.prev);
 	for (; sb != sb_entry(&super_blocks); sb = sb_entry(sb->s_list.prev)) {
 		if (!list_empty(&sb->s_dirty) || !list_empty(&sb->s_io)) {
+			/* we're making our own get_super here */
 			sb->s_count++;
 			spin_unlock(&sb_lock);
-			sync_sb_inodes(sb, wbc);
+			/* if we can't get the readlock, there's no sense in
+			 * waiting around, most of the time the FS is going

Quoted 6 lines · Chris Mason

There's a small window where the filesystem can be unmounted during writeback_inodes. The end result is the iput done by sync_sb_inodes could be done after the FS put_super and and the super has been removed from all lists.

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.

Quoted 9 lines · Mika Penttilä

Quoted 6 lines · Chris Mason

There's a small window where the filesystem can be unmounted during writeback_inodes. The end result is the iput done by sync_sb_inodes could be done after the FS put_super and and the super has been removed from all lists.

Quoted 2 lines

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.

Quoted 8 lines · Chris Mason

Quoted 2 lines

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.

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.

Quoted 11 lines · Andrew Morton

Quoted 8 lines · Chris Mason

Quoted 2 lines

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.

Quoted 5 lines

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?

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()

Something like this?