Commit cbfecb92 authored by Lukas Czerner's avatar Lukas Czerner Committed by Theodore Ts'o

fs: record I_DIRTY_TIME even if inode already has I_DIRTY_INODE

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>
Cc: stable@kernel.org
Signed-off-by: default avatarLukas Czerner <lczerner@redhat.com>
Suggested-by: default avatarJan Kara <jack@suse.cz>
Reviewed-by: default avatarJan Kara <jack@suse.cz>
Link: https://lore.kernel.org/r/20220825100657.44217-1-lczerner@redhat.comSigned-off-by: default avatarTheodore Ts'o <tytso@mit.edu>
parent 50f094a5
...@@ -274,6 +274,9 @@ or bottom half). ...@@ -274,6 +274,9 @@ or bottom half).
This is specifically for the inode itself being marked dirty, This is specifically for the inode itself being marked dirty,
not its data. If the update needs to be persisted by fdatasync(), not its data. If the update needs to be persisted by fdatasync(),
then I_DIRTY_DATASYNC will be set in the flags argument. then I_DIRTY_DATASYNC will be set in the flags argument.
I_DIRTY_TIME will be set in the flags in case lazytime is enabled
and struct inode has times updated since the last ->dirty_inode
call.
``write_inode`` ``write_inode``
this method is called when the VFS needs to write an inode to this method is called when the VFS needs to write an inode to
......
...@@ -1718,9 +1718,14 @@ static int writeback_single_inode(struct inode *inode, ...@@ -1718,9 +1718,14 @@ static int writeback_single_inode(struct inode *inode,
*/ */
if (!(inode->i_state & I_DIRTY_ALL)) if (!(inode->i_state & I_DIRTY_ALL))
inode_cgwb_move_to_attached(inode, wb); inode_cgwb_move_to_attached(inode, wb);
else if (!(inode->i_state & I_SYNC_QUEUED) && else if (!(inode->i_state & I_SYNC_QUEUED)) {
(inode->i_state & I_DIRTY)) if ((inode->i_state & I_DIRTY))
redirty_tail_locked(inode, wb); redirty_tail_locked(inode, wb);
else if (inode->i_state & I_DIRTY_TIME) {
inode->dirtied_when = jiffies;
inode_io_list_move_locked(inode, wb, &wb->b_dirty_time);
}
}
spin_unlock(&wb->list_lock); spin_unlock(&wb->list_lock);
inode_sync_complete(inode); inode_sync_complete(inode);
...@@ -2369,6 +2374,20 @@ void __mark_inode_dirty(struct inode *inode, int flags) ...@@ -2369,6 +2374,20 @@ void __mark_inode_dirty(struct inode *inode, int flags)
trace_writeback_mark_inode_dirty(inode, flags); trace_writeback_mark_inode_dirty(inode, flags);
if (flags & I_DIRTY_INODE) { if (flags & I_DIRTY_INODE) {
/*
* Inode timestamp update will piggback on this dirtying.
* We tell ->dirty_inode callback that timestamps need to
* be updated by setting I_DIRTY_TIME in flags.
*/
if (inode->i_state & I_DIRTY_TIME) {
spin_lock(&inode->i_lock);
if (inode->i_state & I_DIRTY_TIME) {
inode->i_state &= ~I_DIRTY_TIME;
flags |= I_DIRTY_TIME;
}
spin_unlock(&inode->i_lock);
}
/* /*
* Notify the filesystem about the inode being dirtied, so that * Notify the filesystem about the inode being dirtied, so that
* (if needed) it can update on-disk fields and journal the * (if needed) it can update on-disk fields and journal the
...@@ -2378,7 +2397,8 @@ void __mark_inode_dirty(struct inode *inode, int flags) ...@@ -2378,7 +2397,8 @@ void __mark_inode_dirty(struct inode *inode, int flags)
*/ */
trace_writeback_dirty_inode_start(inode, flags); trace_writeback_dirty_inode_start(inode, flags);
if (sb->s_op->dirty_inode) if (sb->s_op->dirty_inode)
sb->s_op->dirty_inode(inode, flags & I_DIRTY_INODE); sb->s_op->dirty_inode(inode,
flags & (I_DIRTY_INODE | I_DIRTY_TIME));
trace_writeback_dirty_inode(inode, flags); trace_writeback_dirty_inode(inode, flags);
/* I_DIRTY_INODE supersedes I_DIRTY_TIME. */ /* I_DIRTY_INODE supersedes I_DIRTY_TIME. */
...@@ -2399,21 +2419,15 @@ void __mark_inode_dirty(struct inode *inode, int flags) ...@@ -2399,21 +2419,15 @@ void __mark_inode_dirty(struct inode *inode, int flags)
*/ */
smp_mb(); smp_mb();
if (((inode->i_state & flags) == flags) || if ((inode->i_state & flags) == flags)
(dirtytime && (inode->i_state & I_DIRTY_INODE)))
return; return;
spin_lock(&inode->i_lock); spin_lock(&inode->i_lock);
if (dirtytime && (inode->i_state & I_DIRTY_INODE))
goto out_unlock_inode;
if ((inode->i_state & flags) != flags) { if ((inode->i_state & flags) != flags) {
const int was_dirty = inode->i_state & I_DIRTY; const int was_dirty = inode->i_state & I_DIRTY;
inode_attach_wb(inode, NULL); inode_attach_wb(inode, NULL);
/* I_DIRTY_INODE supersedes I_DIRTY_TIME. */
if (flags & I_DIRTY_INODE)
inode->i_state &= ~I_DIRTY_TIME;
inode->i_state |= flags; inode->i_state |= flags;
/* /*
...@@ -2486,7 +2500,6 @@ void __mark_inode_dirty(struct inode *inode, int flags) ...@@ -2486,7 +2500,6 @@ void __mark_inode_dirty(struct inode *inode, int flags)
out_unlock: out_unlock:
if (wb) if (wb)
spin_unlock(&wb->list_lock); spin_unlock(&wb->list_lock);
out_unlock_inode:
spin_unlock(&inode->i_lock); spin_unlock(&inode->i_lock);
} }
EXPORT_SYMBOL(__mark_inode_dirty); EXPORT_SYMBOL(__mark_inode_dirty);
......
...@@ -653,7 +653,7 @@ xfs_fs_destroy_inode( ...@@ -653,7 +653,7 @@ xfs_fs_destroy_inode(
static void static void
xfs_fs_dirty_inode( xfs_fs_dirty_inode(
struct inode *inode, struct inode *inode,
int flag) int flags)
{ {
struct xfs_inode *ip = XFS_I(inode); struct xfs_inode *ip = XFS_I(inode);
struct xfs_mount *mp = ip->i_mount; struct xfs_mount *mp = ip->i_mount;
...@@ -661,7 +661,13 @@ xfs_fs_dirty_inode( ...@@ -661,7 +661,13 @@ xfs_fs_dirty_inode(
if (!(inode->i_sb->s_flags & SB_LAZYTIME)) if (!(inode->i_sb->s_flags & SB_LAZYTIME))
return; return;
if (flag != I_DIRTY_SYNC || !(inode->i_state & I_DIRTY_TIME))
/*
* Only do the timestamp update if the inode is dirty (I_DIRTY_SYNC)
* and has dirty timestamp (I_DIRTY_TIME). I_DIRTY_TIME can be passed
* in flags possibly together with I_DIRTY_SYNC.
*/
if ((flags & ~I_DIRTY_TIME) != I_DIRTY_SYNC || !(flags & I_DIRTY_TIME))
return; return;
if (xfs_trans_alloc(mp, &M_RES(mp)->tr_fsyncts, 0, 0, 0, &tp)) if (xfs_trans_alloc(mp, &M_RES(mp)->tr_fsyncts, 0, 0, 0, &tp))
......
...@@ -2371,13 +2371,14 @@ static inline void kiocb_clone(struct kiocb *kiocb, struct kiocb *kiocb_src, ...@@ -2371,13 +2371,14 @@ static inline void kiocb_clone(struct kiocb *kiocb, struct kiocb *kiocb_src,
* don't have to write inode on fdatasync() when only * don't have to write inode on fdatasync() when only
* e.g. the timestamps have changed. * e.g. the timestamps have changed.
* I_DIRTY_PAGES Inode has dirty pages. Inode itself may be clean. * I_DIRTY_PAGES Inode has dirty pages. Inode itself may be clean.
* I_DIRTY_TIME The inode itself only has dirty timestamps, and the * I_DIRTY_TIME The inode itself has dirty timestamps, and the
* lazytime mount option is enabled. We keep track of this * lazytime mount option is enabled. We keep track of this
* separately from I_DIRTY_SYNC in order to implement * separately from I_DIRTY_SYNC in order to implement
* lazytime. This gets cleared if I_DIRTY_INODE * lazytime. This gets cleared if I_DIRTY_INODE
* (I_DIRTY_SYNC and/or I_DIRTY_DATASYNC) gets set. I.e. * (I_DIRTY_SYNC and/or I_DIRTY_DATASYNC) gets set. But
* either I_DIRTY_TIME *or* I_DIRTY_INODE can be set in * I_DIRTY_TIME can still be set if I_DIRTY_SYNC is already
* i_state, but not both. I_DIRTY_PAGES may still be set. * in place because writeback might already be in progress
* and we don't want to lose the time update
* I_NEW Serves as both a mutex and completion notification. * I_NEW Serves as both a mutex and completion notification.
* New inodes set I_NEW. If two processes both create * New inodes set I_NEW. If two processes both create
* the same inode, one of them will release its inode and * the same inode, one of them will release its inode and
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment