Re: [PATCH v3 2/3] fs: record I_DIRTY_TIME even if inode already has I_DIRTY_INODE

[Jan Kara <jack@suse.cz>]

On Fri 12-08-22 11:42:21, Eric Biggers wrote:
> On Fri, Aug 12, 2022 at 02:37:26PM +0200, Lukas Czerner wrote:
> > Currently the I_DIRTY_TIME will never get set if the inode already has
> > I_DIRTY_INODE with assumption that it supersedes I_DIRTY_TIME.  That's
> > true, however ext4 will only update the on-disk inode in
> > ->dirty_inode(), not on actual writeback. As a result if the inode
> > already has I_DIRTY_INODE state by the time we get to
> > __mark_inode_dirty() only with I_DIRTY_TIME, the time was already filled
> > into on-disk inode and will not get updated until the next I_DIRTY_INODE
> > update, which might never come if we crash or get a power failure.
> > 
> > The problem can be reproduced on ext4 by running xfstest generic/622
> > with -o iversion mount option.
> > 
> > Fix it by allowing I_DIRTY_TIME to be set even if the inode already has
> > I_DIRTY_INODE. Also make sure that the case is properly handled in
> > writeback_single_inode() as well. Additionally changes in
> > xfs_fs_dirty_inode() was made to accommodate for I_DIRTY_TIME in flag.
> > 
> > Thanks Jan Kara for suggestions on how to make this work properly.
> > 
> > Cc: Dave Chinner <david@fromorbit.com>
> > Cc: Christoph Hellwig <hch@infradead.org>
> > Signed-off-by: Lukas Czerner <lczerner@redhat.com>
> > Suggested-by: Jan Kara <jack@suse.cz>
> 
> Sorry for so many separate emails.  One more thought: isn't there a much more
> straightforward fix to this bug that wouldn't require changing the semantics of
> the inode flags: on __mark_inode_dirty(I_DIRTY_TIME), if the inode already has
> i_state & I_DIRTY_INODE, just call ->dirty_inode with i_state & I_DIRTY_INODE?
> That would fix the bug by making the filesystem update the on-disk inode.

This is a good question and I was also considering this when we first
discussed the problem with Lukas. It should fix the bug for ext4 but
eventually I've decided against this because XFS would still need something
else to fix the problem (see my previous email) and for ext4 it seemed as
unnecessary overhead (see below).

> Perhaps you aren't doing that in order to strictly maintain the semantics of
> 'lazytime', where timestamp updates are only persisted at certain times?  Is
> this useful even in the short window of time that an inode is dirty?

The result of this for ext4 will be that after the inode is dirtied for
some reason, we will be logging every timestamp update to the journal
(effectively disabling lazytime for the inode) for the 30s time window that
the inode stays dirty before writeback code decides to do writeback (which
just results in clearing the I_DIRTY_INODE flag for ext4). Not too bad I
guess but I'd prefer to avoid this overhead.

								Honza
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR