On Tue, Mar 10, 2020 at 08:20:09PM -0700, Eric Biggers wrote: > On Fri, Mar 06, 2020 at 09:00:43PM -0500, Theodore Ts'o wrote: > > In the case that an inode has dirty timestamp for longer than the > > lazytime expiration timeout (or if all such inodes are being flushed > > out due to a sync or syncfs system call), we need to inform the file > > system that the inode is dirty so that the inode's timestamps can be > > copied out to the on-disk data structures. That's because if the file > > system supports lazytime, it will have ignored the dirty_inode(inode, > > I_DIRTY_TIME) notification when the timestamp was modified in memory.q > > > > Previously, this was accomplished by calling mark_inode_dirty_sync(), > > but that has the unfortunate side effect of also putting the inode the > > writeback list, and that's not necessary in this case, since we will > > immediately call write_inode() afterwards. > > > > Eric Biggers noticed that this was causing problems for fscrypt after > > the key was removed[1]. > > > > [1] https://lore.kernel.org/r/20200306004555.GB225345@xxxxxxxxx > > > > Reported-by: Eric Biggers <ebiggers@xxxxxxxxxx> > > Signed-off-by: Theodore Ts'o <tytso@xxxxxxx> > > --- > > fs/fs-writeback.c | 5 +++-- > > 1 file changed, 3 insertions(+), 2 deletions(-) > > > > diff --git a/fs/fs-writeback.c b/fs/fs-writeback.c > > index 76ac9c7d32ec..32101349ba97 100644 > > --- a/fs/fs-writeback.c > > +++ b/fs/fs-writeback.c > > @@ -1504,8 +1504,9 @@ __writeback_single_inode(struct inode *inode, struct writeback_control *wbc) > > > > spin_unlock(&inode->i_lock); > > > > - if (dirty & I_DIRTY_TIME) > > - mark_inode_dirty_sync(inode); > > + /* This was a lazytime expiration; we need to tell the file system */ > > + if (dirty & I_DIRTY_TIME_EXPIRED && inode->i_sb->s_op->dirty_inode) > > + inode->i_sb->s_op->dirty_inode(inode, I_DIRTY_TIME_EXPIRED); > > /* Don't write the inode if only I_DIRTY_PAGES was set */ > > if (dirty & ~I_DIRTY_PAGES) { > > int err = write_inode(inode, wbc); > > -- > > Thanks Ted! This fixes the fscrypt test failure. > > However, are you sure this works correctly on all filesystems? I'm not sure > about XFS. XFS only implements ->dirty_inode(), not ->write_inode(), and in its > ->dirty_inode() it does: > > static void > xfs_fs_dirty_inode( > struct inode *inode, > int flag) > { > struct xfs_inode *ip = XFS_I(inode); > struct xfs_mount *mp = ip->i_mount; > struct xfs_trans *tp; > > if (!(inode->i_sb->s_flags & SB_LAZYTIME)) > return; > if (flag != I_DIRTY_SYNC || !(inode->i_state & I_DIRTY_TIME)) > return; > > if (xfs_trans_alloc(mp, &M_RES(mp)->tr_fsyncts, 0, 0, 0, &tp)) > return; > xfs_ilock(ip, XFS_ILOCK_EXCL); > xfs_trans_ijoin(tp, ip, XFS_ILOCK_EXCL); > xfs_trans_log_inode(tp, ip, XFS_ILOG_TIMESTAMP); > xfs_trans_commit(tp); > } > > > So flag=I_DIRTY_TIME_EXPIRED will be a no-op. > > Maybe you should be using I_DIRTY_SYNC instead? Or perhaps XFS should be > checking for either I_DIRTY_TIME_EXPIRED or I_DIRTY_SYNC? Right, XFS does not use the VFS inode writeback code at all - we track all metadata changes internally via journalling. The VFS uses I_DIRTY_SYNC to indicate and inode is metadata dirty and a writeback candidate. Hence if we need to mark an inode dirty for integrity purposes for _any reason_, then I_DIRTY_SYNC is the correct flag to be passing to ->dirty_inode. Cheers, Dave. -- Dave Chinner david@xxxxxxxxxxxxx
