Re: [PATCH 11/13] fs: add a lazytime_expired method

From: Eric Biggers <ebiggers@kernel.org>
To: Jan Kara <jack@suse.cz>
Cc: linux-fsdevel@vger.kernel.org, linux-ext4@vger.kernel.org,
	linux-f2fs-devel@lists.sourceforge.net,
	linux-xfs@vger.kernel.org, Theodore Ts'o <tytso@mit.edu>,
	Christoph Hellwig <hch@lst.de>
Subject: Re: [PATCH 11/13] fs: add a lazytime_expired method
Date: Thu, 7 Jan 2021 14:05:57 -0800	[thread overview]
Message-ID: <X/eFxSh3ac6EGdYI@gmail.com> (raw)
In-Reply-To: <20210107140228.GF12990@quack2.suse.cz>

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@google.com>
> > 
> > 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@google.com>
> 
> 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
	https://lore.kernel.org/r/20200325092057.GA25483@infradead.org
	https://lore.kernel.org/r/20200325154759.GY29339@magnolia
	https://lore.kernel.org/r/20200312223913.GL10776@dread.disaster.area

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