On Fri, Nov 05, 2021 at 01:28:10PM +0800, Zhongwei Cai wrote:

On 11/5/21 7:22 AM, Dave Chinner wrote:

quoted

No. Some filesystems don't track inode metadata dirty status using
the VFS inode; instead they track it more efficiently in internal
inode and/or journal based structures. Hence the only way to get
"inode needs journal flush for data stability" information to
generic IO code is to have a specific per-IO mapping flag for it.

Could we add IOMAP_REPORT_DIRTY flag in the flags field of
struct iomap_iter to indicate whether the IOMAP_F_DIRTY flag
needs to be set or not?

You can try. It might turn out OK, but you're also going to have to
modify all the iomap code that current needs IOMAP_F_DIRTY to first
set that flag, then change all the code that currently sets
IOMAP_F_DIRTY to look at IOMAP_REPORT_DIRTY. i.e you now have to
change iomap, ext4 and XFS to do this.

Currently the IOMAP_F_DIRTY flag is only checked in
iomap_swapfile_activate(), dax_iomap_fault() and iomap_dio_rw()
(To be more specific, only the write path in dax_iomap_fault() and
iomap_dio_rw()). So it would be unnecessary to set the IOMAP_F_DIRTY
flag in dax_iomap_rw() called in the previous tests.

I think you're trying to optimise the wrong thing - the API is not
the problem, the problem is that the journal->j_state_lock is
contended and the ext4 dirty inode check needs to take it. Fix the
dirty check not to need the journal state lock and the ext4 problem
goes away and there is no need to change the iomap infrastructure.

Other file systems that set the IOMAP_F_DIRTY flag efficiently
could ignore the IOMAP_REPORT_DIRTY flag.

No, that's just bad API design. If we are adding IOMAP_REPORT_DIRTY
then the iomap infrastructure should only use that control flag when
it needs to know if the inode is dirty. At this point, it really
becomes mandatory for all filesystems using iomap to support it
because the absence of IOMAP_F_DIRTY because a filesystem doesn't
support it is not the same as "filesystem didn't set it because the
inode is clean".

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com