On Thu, Jan 07, 2021 at 03:02:28PM +0100, Jan Kara wrote:

On Mon 04-01-21 16:54:50, Eric Biggers wrote:

quoted

From: Eric Biggers <redacted>

Add a lazytime_expired method to 'struct super_operations'.  Filesystems
can implement this to be notified when an inode's lazytime timestamps
have expired and need to be written to disk.

This avoids any potential ambiguity with
->dirty_inode(inode, I_DIRTY_SYNC), which can also mean a generic
dirtying of the inode, not just a lazytime timestamp expiration.
In particular, this will be useful for XFS.

If not implemented, then ->dirty_inode(inode, I_DIRTY_SYNC) continues to
be called.

Note that there are three cases where we have to make sure to call
lazytime_expired():

- __writeback_single_inode(): inode is being written now
- vfs_fsync_range(): inode is going to be synced
- iput(): inode is going to be evicted

In the latter two cases, the inode still needs to be put on the
writeback list.  So, we can't just replace the calls to
mark_inode_dirty_sync() with lazytime_expired().  Instead, add a new
flag I_DIRTY_TIME_EXPIRED which can be passed to __mark_inode_dirty().
It's like I_DIRTY_SYNC, except it causes the filesystem to be notified
of a lazytime expiration rather than a generic I_DIRTY_SYNC.

Signed-off-by: Eric Biggers <redacted>

Hum, seeing this patch I kind of wonder: Why don't we dirty the inode after
expiring the lazytime timestamps with I_DIRTY_SYNC | I_DIRTY_TIME_EXPIRED
and propagate I_DIRTY_TIME_EXPIRED even to ->dirty_inode() where XFS can
catch it and act? Functionally it would be the same but we'd save a bunch
of generic code and ->lazytime_expired helper used just by a single
filesystem...

Yes, that would be equivalent to what this patch does.

Either way, note that if we also use your suggestion for patch #1, then that
already fixes the XFS bug, since i_state will start containing I_DIRTY_TIME when
->dirty_inode(I_DIRTY_SYNC) is called.  So xfs_fs_dirty_inode() will start
working as intended.

That makes introducing ->lazytime_expired (or equivalently I_DIRTY_TIME_EXPIRED)
kind of useless since it wouldn't actually fix anything.

So I'm tempted to just drop it.

The XFS developers might have a different opinion though, as they were the ones
who requested it originally:

	https://lore.kernel.org/r/20200312143445.GA19160@infradead.org (local)
	https://lore.kernel.org/r/20200325092057.GA25483@infradead.org (local)
	https://lore.kernel.org/r/20200325154759.GY29339@magnolia (local)
	https://lore.kernel.org/r/20200312223913.GL10776@dread.disaster.area (local)

Any thoughts from anyone about whether we should still introduce a separate
notification for lazytime expiration, vs. just using ->dirty_inode(I_DIRTY_SYNC)
with I_DIRTY_TIME in i_state?

- Eric