linux-ext4 - Re: [PATCH 11/13] fs: add a lazytime

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <X/eFxSh3ac6EGdYI@gmail.com>
Date:   Thu, 7 Jan 2021 14:05:57 -0800
From:   Eric Biggers <ebiggers@...nel.org>
To:     Jan Kara <jack@...e.cz>
Cc:     linux-fsdevel@...r.kernel.org, linux-ext4@...r.kernel.org,
        linux-f2fs-devel@...ts.sourceforge.net, linux-xfs@...r.kernel.org,
        Theodore Ts'o <tytso@....edu>, Christoph Hellwig <hch@....de>
Subject: Re: [PATCH 11/13] fs: add a lazytime_expired method

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@...gle.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@...gle.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