On Thu, Jan 07, 2021 at 03:02:28PM +0100, Jan Kara wrote: > On Mon 04-01-21 16:54:50, Eric Biggers wrote: > > From: Eric Biggers <ebiggers@xxxxxxxxxx> > > > > 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 <ebiggers@xxxxxxxxxx> > > 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@xxxxxxxxxxxxx https://lore.kernel.org/r/20200325092057.GA25483@xxxxxxxxxxxxx https://lore.kernel.org/r/20200325154759.GY29339@magnolia https://lore.kernel.org/r/20200312223913.GL10776@xxxxxxxxxxxxxxxxxxx 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
