Re: [PATCH] Optimize wait_sb_inodes()

[OGAWA Hirofumi <hirofumi@mail.parknet.co.jp>]

Dave Chinner <david@fromorbit.com> writes:

>> On another view, wait_sb_inodes() would (arguably) be necessary for
>> legacy FSes. But, for example, if data=journal on ext*, wait_sb_inodes()
>> would be more than useless, because ext* can be done it by own
>> transaction list (and more efficient way).
>> 
>> Likewise, on tux3, the state is same with data=journal.
>> 
>> Also, even if data=ordered, ext* might be able to check in-flight I/O by
>> ordered data list (with some new additional check, I'm not sure).
>
> Why would you bother solving this problem differently in every
> single filesystem? It's solvable at the VFS by tracking inodes that
> are no longer dirty but still under writeback on the BDI. Then
> converting wait_sb_inodes() to walk all the dirty and writeback
> inodes would be sufficient for data integrity purposes, and it would
> be done under the bdi writeback lock, not the inode_sb_list_lock....
>
> Alternatively, splitting up the inode sb list and lock (say via the
> per-node list_lru structures in -mm and -next that are being added
> for exactly this purpose) would also significantly reduce lock
> contention on both the create/evict fast paths and the
> wait_sb_inodes() walk that is currently done....
>
> So I think that you should address the problem properly at the VFS
> level so everyone benefits, not push interfaces that allow
> filesystem specific hacks to work around VFS level deficiencies...

Optimizing wait_sb_inodes() might help lock contention, but it doesn't
help unnecessary wait/check. Since some FSes know about current
in-flight I/O already in those internal, so I think, those FSes can be
done it here, or are already doing in ->sync_fs().

For example, I guess ext4 implement (untested) would be something like
following. If ->sync_fs() does all, ext4 doesn't need to be bothered by
wait_sb_inodes().

static void ext4_wait_inodes(struct super_block *sb)
{
	/* ->sync_fs() guarantees to wait all */
	if (test_opt(inode->i_sb, DATA_FLAGS) == EXT4_MOUNT_JOURNAL_DATA)
        	return;

	/* FIXME: On data=ordered, we might be able to avoid this too. */
	wait_sb_inodes(sb);
}

Thanks.
-- 
OGAWA Hirofumi <hirofumi@mail.parknet.co.jp>