lazytime fix and cleanups

---
 fs/fs-writeback.c | 24 +++++++++++++-----------
 1 file changed, 13 insertions(+), 11 deletions(-)

fs/fs-writeback.c +13 ~~-11~~

index acfb55834af23..c41cb887eb7d3 100644
@@ -1474,21 +1474,25 @@ __writeback_single_inode(struct inode *inode, struct writeback_control *wbc)
 	}
 
 	/*
-	 * Some filesystems may redirty the inode during the writeback
-	 * due to delalloc, clear dirty metadata flags right before
-	 * write_inode()
+	 * If the inode has dirty timestamps and we need to write them, call
+	 * mark_inode_dirty_sync() to notify the filesystem about it and to
+	 * change I_DIRTY_TIME into I_DIRTY_SYNC.
 	 */
-	spin_lock(&inode->i_lock);
-
-	dirty = inode->i_state & I_DIRTY;
 	if ((inode->i_state & I_DIRTY_TIME) &&
-	    ((dirty & I_DIRTY_INODE) ||
-	     wbc->sync_mode == WB_SYNC_ALL || wbc->for_sync ||
+	    (wbc->sync_mode == WB_SYNC_ALL || wbc->for_sync ||
 	     time_after(jiffies, inode->dirtied_time_when +
 			dirtytime_expire_interval * HZ))) {
-		dirty |= I_DIRTY_TIME;
 		trace_writeback_lazytime(inode);
+		mark_inode_dirty_sync(inode);
 	}
+
+	/*
+	 * Some filesystems may redirty the inode during the writeback
+	 * due to delalloc, clear dirty metadata flags right before
+	 * write_inode()
+	 */
+	spin_lock(&inode->i_lock);
+	dirty = inode->i_state & I_DIRTY;
 	inode->i_state &= ~dirty;
 
 	/*

@@ -1509,8 +1513,6 @@ __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);
 	/* Don't write the inode if only I_DIRTY_PAGES was set */
 	if (dirty & ~I_DIRTY_PAGES) {
 		int err = write_inode(inode, wbc);

Signature · 8 lines

2.30.0



_______________________________________________
Linux-f2fs-devel mailing list
[email protected]
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel

Christoph Hellwig

Reviewed-by on v2 1/12

2021-01-11 10:48 UTC #2 lore raw

Re: [PATCH v2 01/12] fs: fix lazytime expiration handling in __writeback_single_inode()

Looks good,

_______________________________________________
Linux-f2fs-devel mailing list
[email protected]
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel

Jan Kara

maintainer Reviewed-by on v2 1/12

2021-01-11 14:46 UTC #3 lore raw

Re: [PATCH v2 01/12] fs: fix lazytime expiration handling in __writeback_single_inode()

Quoted 45 lines · Eric Biggers

From: Eric Biggers <[email protected]>
When lazytime is enabled and an inode is being written due to its in-memory updated timestamps having expired, either due to a sync() or syncfs() system call or due to dirtytime_expire_interval having elapsed, the VFS needs to inform the filesystem so that the filesystem can copy the inode's timestamps out to the on-disk data structures.
This is done by __writeback_single_inode() calling mark_inode_dirty_sync(), which then calls ->dirty_inode(I_DIRTY_SYNC).
However, this occurs after __writeback_single_inode() has already cleared the dirty flags from ->i_state. This causes two bugs:
- mark_inode_dirty_sync() redirties the inode, causing it to remain dirty. This wastefully causes the inode to be written twice. But more importantly, it breaks cases where sync_filesystem() is expected to clean dirty inodes. This includes the FS_IOC_REMOVE_ENCRYPTION_KEY ioctl (as reported at
https://lore.kernel.org/r/[email protected]), as well as possibly filesystem freezing (freeze_super()).
- Since ->i_state doesn't contain I_DIRTY_TIME when ->dirty_inode() is called from __writeback_single_inode() for lazytime expiration, xfs_fs_dirty_inode() ignores the notification. (XFS only cares about lazytime expirations, and it assumes that I_DIRTY_TIME will contain i_state during those.) Therefore, lazy timestamps aren't persisted by sync(), syncfs(), or dirtytime_expire_interval on XFS.
Fix this by moving the call to mark_inode_dirty_sync() to earlier in __writeback_single_inode(), before the dirty flags are cleared from i_state. This makes filesystems be properly notified of the timestamp expiration, and it avoids incorrectly redirtying the inode.
This fixes xfstest generic/580 (which tests FS_IOC_REMOVE_ENCRYPTION_KEY) when run on ext4 or f2fs with lazytime enabled. It also fixes the new lazytime xfstest I've proposed, which reproduces the above-mentioned XFS bug
(https://lore.kernel.org/r/[email protected]).
Alternatively, we could call ->dirty_inode(I_DIRTY_SYNC) directly. But due to the introduction of I_SYNC_QUEUED, mark_inode_dirty_sync() is the right thing to do because mark_inode_dirty_sync() now knows not to move the inode to a writeback list if it is currently queued for sync.

Quoted 5 lines

Fixes 0ae45f63d4ef ("vfs: add support for a lazytime mount option")
Cc [email protected]
Depends-on: 5afced3bf281 ("writeback: Avoid skipping inode writeback")
Suggested-by Jan Kara <[email protected]>
Signed-off-by Eric Biggers <[email protected]>

Thanks for writing this fix! It looks good to me. You can add:

Honza

Quoted 55 lines

---
 fs/fs-writeback.c | 24 +++++++++++++-----------
 1 file changed, 13 insertions(+), 11 deletions(-)

fs/fs-writeback.c +13 ~~-11~~

index acfb55834af23..c41cb887eb7d3 100644
@@ -1474,21 +1474,25 @@ __writeback_single_inode(struct inode *inode, struct writeback_control *wbc)
 	}
 
 	/*
-	 * Some filesystems may redirty the inode during the writeback
-	 * due to delalloc, clear dirty metadata flags right before
-	 * write_inode()
+	 * If the inode has dirty timestamps and we need to write them, call
+	 * mark_inode_dirty_sync() to notify the filesystem about it and to
+	 * change I_DIRTY_TIME into I_DIRTY_SYNC.
 	 */
-	spin_lock(&inode->i_lock);
-
-	dirty = inode->i_state & I_DIRTY;
 	if ((inode->i_state & I_DIRTY_TIME) &&
-	    ((dirty & I_DIRTY_INODE) ||
-	     wbc->sync_mode == WB_SYNC_ALL || wbc->for_sync ||
+	    (wbc->sync_mode == WB_SYNC_ALL || wbc->for_sync ||
 	     time_after(jiffies, inode->dirtied_time_when +
 			dirtytime_expire_interval * HZ))) {
-		dirty |= I_DIRTY_TIME;
 		trace_writeback_lazytime(inode);
+		mark_inode_dirty_sync(inode);
 	}
+
+	/*
+	 * Some filesystems may redirty the inode during the writeback
+	 * due to delalloc, clear dirty metadata flags right before
+	 * write_inode()
+	 */
+	spin_lock(&inode->i_lock);
+	dirty = inode->i_state & I_DIRTY;
 	inode->i_state &= ~dirty;
 
 	/*

@@ -1509,8 +1513,6 @@ __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);
 	/* Don't write the inode if only I_DIRTY_PAGES was set */
 	if (dirty & ~I_DIRTY_PAGES) {
 		int err = write_inode(inode, wbc);

Signature · 1 line

2.30.0

Signature · 8 lines

Jan Kara <[email protected]>
SUSE Labs, CR


_______________________________________________
Linux-f2fs-devel mailing list
[email protected]
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel

Eric Biggers

author

2021-01-09 08:00 UTC #4 lore raw

PATCH v2 2/12 fs: correctly document the inode dirty flags

From: Eric Biggers <[email protected]>

The documentation for I_DIRTY_SYNC and I_DIRTY_DATASYNC is a bit misleading, and I_DIRTY_TIME isn't documented at all. Fix this.

---
 include/linux/fs.h | 18 +++++++++++++-----
 1 file changed, 13 insertions(+), 5 deletions(-)

include/linux/fs.h +13 -5

index fd47deea7c176..45a0303b2aeb6 100644
@@ -2084,8 +2084,8 @@ static inline void kiocb_clone(struct kiocb *kiocb, struct kiocb *kiocb_src,
 /*
  * Inode state bits.  Protected by inode->i_lock
  *
- * Three bits determine the dirty state of the inode, I_DIRTY_SYNC,
- * I_DIRTY_DATASYNC and I_DIRTY_PAGES.
+ * Four bits determine the dirty state of the inode: I_DIRTY_SYNC,
+ * I_DIRTY_DATASYNC, I_DIRTY_PAGES, and I_DIRTY_TIME.
  *
  * Four bits define the lifetime of an inode.  Initially, inodes are I_NEW,
  * until that flag is cleared.  I_WILL_FREE, I_FREEING and I_CLEAR are set at

@@ -2094,12 +2094,20 @@ static inline void kiocb_clone(struct kiocb *kiocb, struct kiocb *kiocb_src,
  * Two bits are used for locking and completion notification, I_NEW and I_SYNC.
  *
  * I_DIRTY_SYNC		Inode is dirty, but doesn't have to be written on
- *			fdatasync().  i_atime is the usual cause.
- * I_DIRTY_DATASYNC	Data-related inode changes pending. We keep track of
+ *			fdatasync() (unless I_DIRTY_DATASYNC is also set).
+ *			Timestamp updates are the usual cause.
+ * I_DIRTY_DATASYNC	Data-related inode changes pending.  We keep track of
  *			these changes separately from I_DIRTY_SYNC so that we
  *			don't have to write inode on fdatasync() when only
- *			mtime has changed in it.
+ *			e.g. the timestamps have changed.
  * I_DIRTY_PAGES	Inode has dirty pages.  Inode itself may be clean.
+ * I_DIRTY_TIME		The inode itself only has dirty timestamps, and the
+ *			lazytime mount option is enabled.  We keep track of this
+ *			separately from I_DIRTY_SYNC in order to implement
+ *			lazytime.  This gets cleared if I_DIRTY_INODE
+ *			(I_DIRTY_SYNC and/or I_DIRTY_DATASYNC) gets set.  I.e.
+ *			either I_DIRTY_TIME *or* I_DIRTY_INODE can be set in
+ *			i_state, but not both.  I_DIRTY_PAGES may still be set.
  * I_NEW		Serves as both a mutex and completion notification.
  *			New inodes set I_NEW.  If two processes both create
  *			the same inode, one of them will release its inode and

Signature · 8 lines

2.30.0



_______________________________________________
Linux-f2fs-devel mailing list
[email protected]
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel

Jan Kara

maintainer Reviewed-by on v2 2/12

2021-01-11 14:49 UTC #5 lore raw

Re: [PATCH v2 02/12] fs: correctly document the inode dirty flags

Quoted 7 lines · Eric Biggers

From: Eric Biggers <[email protected]>
The documentation for I_DIRTY_SYNC and I_DIRTY_DATASYNC is a bit misleading, and I_DIRTY_TIME isn't documented at all. Fix this.
Reviewed-by Christoph Hellwig <[email protected]>
Signed-off-by Eric Biggers <[email protected]>

Looks good to me. You can add:

Honza

Quoted 46 lines

---
 include/linux/fs.h | 18 +++++++++++++-----
 1 file changed, 13 insertions(+), 5 deletions(-)

include/linux/fs.h +13 -5

index fd47deea7c176..45a0303b2aeb6 100644
@@ -2084,8 +2084,8 @@ static inline void kiocb_clone(struct kiocb *kiocb, struct kiocb *kiocb_src,
 /*
  * Inode state bits.  Protected by inode->i_lock
  *
- * Three bits determine the dirty state of the inode, I_DIRTY_SYNC,
- * I_DIRTY_DATASYNC and I_DIRTY_PAGES.
+ * Four bits determine the dirty state of the inode: I_DIRTY_SYNC,
+ * I_DIRTY_DATASYNC, I_DIRTY_PAGES, and I_DIRTY_TIME.
  *
  * Four bits define the lifetime of an inode.  Initially, inodes are I_NEW,
  * until that flag is cleared.  I_WILL_FREE, I_FREEING and I_CLEAR are set at

@@ -2094,12 +2094,20 @@ static inline void kiocb_clone(struct kiocb *kiocb, struct kiocb *kiocb_src,
  * Two bits are used for locking and completion notification, I_NEW and I_SYNC.
  *
  * I_DIRTY_SYNC		Inode is dirty, but doesn't have to be written on
- *			fdatasync().  i_atime is the usual cause.
- * I_DIRTY_DATASYNC	Data-related inode changes pending. We keep track of
+ *			fdatasync() (unless I_DIRTY_DATASYNC is also set).
+ *			Timestamp updates are the usual cause.
+ * I_DIRTY_DATASYNC	Data-related inode changes pending.  We keep track of
  *			these changes separately from I_DIRTY_SYNC so that we
  *			don't have to write inode on fdatasync() when only
- *			mtime has changed in it.
+ *			e.g. the timestamps have changed.
  * I_DIRTY_PAGES	Inode has dirty pages.  Inode itself may be clean.
+ * I_DIRTY_TIME		The inode itself only has dirty timestamps, and the
+ *			lazytime mount option is enabled.  We keep track of this
+ *			separately from I_DIRTY_SYNC in order to implement
+ *			lazytime.  This gets cleared if I_DIRTY_INODE
+ *			(I_DIRTY_SYNC and/or I_DIRTY_DATASYNC) gets set.  I.e.
+ *			either I_DIRTY_TIME *or* I_DIRTY_INODE can be set in
+ *			i_state, but not both.  I_DIRTY_PAGES may still be set.
  * I_NEW		Serves as both a mutex and completion notification.
  *			New inodes set I_NEW.  If two processes both create
  *			the same inode, one of them will release its inode and

Signature · 1 line

2.30.0

Signature · 8 lines

Jan Kara <[email protected]>
SUSE Labs, CR


_______________________________________________
Linux-f2fs-devel mailing list
[email protected]
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel

Eric Biggers

author

2021-01-09 08:00 UTC #6 lore raw

PATCH v2 3/12 fs: only specify I_DIRTY_TIME when needed in generic_update_time()

From: Eric Biggers <[email protected]>

generic_update_time() always passes I_DIRTY_TIME to __mark_inode_dirty(), which doesn't really make sense because (a) generic_update_time() might be asked to do only an i_version update, not also a timestamps update; and (b) I_DIRTY_TIME is only supposed to be set in i_state if the filesystem has lazytime enabled, so using it unconditionally in generic_update_time() is inconsistent.

As a result there is a weird edge case where if only an i_version update was requested (not also a timestamps update) but it is no longer needed (i.e. inode_maybe_inc_iversion() returns false), then I_DIRTY_TIME will be set in i_state even if the filesystem isn't mounted with lazytime.

Fix this by only passing I_DIRTY_TIME to __mark_inode_dirty() if the timestamps were updated and the filesystem has lazytime enabled.

---
 fs/inode.c | 38 ++++++++++++++++++++------------------
 1 file changed, 20 insertions(+), 18 deletions(-)

fs/inode.c +20 ~~-18~~

index 6442d97d9a4ab..d0fa43d8e9d5c 100644
@@ -1743,24 +1743,26 @@ static int relatime_need_update(struct vfsmount *mnt, struct inode *inode,
 
 int generic_update_time(struct inode *inode, struct timespec64 *time, int flags)
 {
-	int iflags = I_DIRTY_TIME;
-	bool dirty = false;
-
-	if (flags & S_ATIME)
-		inode->i_atime = *time;
-	if (flags & S_VERSION)
-		dirty = inode_maybe_inc_iversion(inode, false);
-	if (flags & S_CTIME)
-		inode->i_ctime = *time;
-	if (flags & S_MTIME)
-		inode->i_mtime = *time;
-	if ((flags & (S_ATIME | S_CTIME | S_MTIME)) &&
-	    !(inode->i_sb->s_flags & SB_LAZYTIME))
-		dirty = true;
-
-	if (dirty)
-		iflags |= I_DIRTY_SYNC;
-	__mark_inode_dirty(inode, iflags);
+	int dirty_flags = 0;
+
+	if (flags & (S_ATIME | S_CTIME | S_MTIME)) {
+		if (flags & S_ATIME)
+			inode->i_atime = *time;
+		if (flags & S_CTIME)
+			inode->i_ctime = *time;
+		if (flags & S_MTIME)
+			inode->i_mtime = *time;
+
+		if (inode->i_sb->s_flags & SB_LAZYTIME)
+			dirty_flags |= I_DIRTY_TIME;
+		else
+			dirty_flags |= I_DIRTY_SYNC;
+	}
+
+	if ((flags & S_VERSION) && inode_maybe_inc_iversion(inode, false))
+		dirty_flags |= I_DIRTY_SYNC;
+
+	__mark_inode_dirty(inode, dirty_flags);
 	return 0;
 }
 EXPORT_SYMBOL(generic_update_time);

Signature · 8 lines

2.30.0



_______________________________________________
Linux-f2fs-devel mailing list
[email protected]
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel

Jan Kara

maintainer Reviewed-by on v2 3/12

2021-01-11 14:50 UTC #7 lore raw

Re: [PATCH v2 03/12] fs: only specify I_DIRTY_TIME when needed in generic_update_time()

Quoted 14 lines · Eric Biggers

From: Eric Biggers <[email protected]>
generic_update_time() always passes I_DIRTY_TIME to __mark_inode_dirty(), which doesn't really make sense because (a) generic_update_time() might be asked to do only an i_version update, not also a timestamps update; and (b) I_DIRTY_TIME is only supposed to be set in i_state if the filesystem has lazytime enabled, so using it unconditionally in generic_update_time() is inconsistent.
As a result there is a weird edge case where if only an i_version update was requested (not also a timestamps update) but it is no longer needed (i.e. inode_maybe_inc_iversion() returns false), then I_DIRTY_TIME will be set in i_state even if the filesystem isn't mounted with lazytime.

Quoted 5 lines

Fix this by only passing I_DIRTY_TIME to __mark_inode_dirty() if the timestamps were updated and the filesystem has lazytime enabled.
Reviewed-by Christoph Hellwig <[email protected]>
Signed-off-by Eric Biggers <[email protected]>

Looks good to me. You can add:

Honza

Quoted 56 lines

---
 fs/inode.c | 38 ++++++++++++++++++++------------------
 1 file changed, 20 insertions(+), 18 deletions(-)

fs/inode.c +20 ~~-18~~

index 6442d97d9a4ab..d0fa43d8e9d5c 100644
@@ -1743,24 +1743,26 @@ static int relatime_need_update(struct vfsmount *mnt, struct inode *inode,
 
 int generic_update_time(struct inode *inode, struct timespec64 *time, int flags)
 {
-	int iflags = I_DIRTY_TIME;
-	bool dirty = false;
-
-	if (flags & S_ATIME)
-		inode->i_atime = *time;
-	if (flags & S_VERSION)
-		dirty = inode_maybe_inc_iversion(inode, false);
-	if (flags & S_CTIME)
-		inode->i_ctime = *time;
-	if (flags & S_MTIME)
-		inode->i_mtime = *time;
-	if ((flags & (S_ATIME | S_CTIME | S_MTIME)) &&
-	    !(inode->i_sb->s_flags & SB_LAZYTIME))
-		dirty = true;
-
-	if (dirty)
-		iflags |= I_DIRTY_SYNC;
-	__mark_inode_dirty(inode, iflags);
+	int dirty_flags = 0;
+
+	if (flags & (S_ATIME | S_CTIME | S_MTIME)) {
+		if (flags & S_ATIME)
+			inode->i_atime = *time;
+		if (flags & S_CTIME)
+			inode->i_ctime = *time;
+		if (flags & S_MTIME)
+			inode->i_mtime = *time;
+
+		if (inode->i_sb->s_flags & SB_LAZYTIME)
+			dirty_flags |= I_DIRTY_TIME;
+		else
+			dirty_flags |= I_DIRTY_SYNC;
+	}
+
+	if ((flags & S_VERSION) && inode_maybe_inc_iversion(inode, false))
+		dirty_flags |= I_DIRTY_SYNC;
+
+	__mark_inode_dirty(inode, dirty_flags);
 	return 0;
 }
 EXPORT_SYMBOL(generic_update_time);

Signature · 1 line

2.30.0

Signature · 8 lines

Jan Kara <[email protected]>
SUSE Labs, CR


_______________________________________________
Linux-f2fs-devel mailing list
[email protected]
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel

Eric Biggers

author

2021-01-09 08:00 UTC #8 lore raw

PATCH v2 4/12 fat: only specify I_DIRTY_TIME when needed in fat_update_time()

From: Eric Biggers <[email protected]>

As was done for generic_update_time(), only pass I_DIRTY_TIME to __mark_inode_dirty() when the inode's timestamps were actually updated and lazytime is enabled. This avoids a weird edge case where I_DIRTY_TIME could be set in i_state when lazytime isn't enabled.

---
 fs/fat/misc.c | 23 ++++++++++++-----------
 1 file changed, 12 insertions(+), 11 deletions(-)

fs/fat/misc.c +12 ~~-11~~

index f1b2a1fc2a6a4..18a50a46b57f8 100644
@@ -329,22 +329,23 @@ EXPORT_SYMBOL_GPL(fat_truncate_time);
 
 int fat_update_time(struct inode *inode, struct timespec64 *now, int flags)
 {
-	int iflags = I_DIRTY_TIME;
-	bool dirty = false;
+	int dirty_flags = 0;
 
 	if (inode->i_ino == MSDOS_ROOT_INO)
 		return 0;
 
-	fat_truncate_time(inode, now, flags);
-	if (flags & S_VERSION)
-		dirty = inode_maybe_inc_iversion(inode, false);
-	if ((flags & (S_ATIME | S_CTIME | S_MTIME)) &&
-	    !(inode->i_sb->s_flags & SB_LAZYTIME))
-		dirty = true;
+	if (flags & (S_ATIME | S_CTIME | S_MTIME)) {
+		fat_truncate_time(inode, now, flags);
+		if (inode->i_sb->s_flags & SB_LAZYTIME)
+			dirty_flags |= I_DIRTY_TIME;
+		else
+			dirty_flags |= I_DIRTY_SYNC;
+	}
+
+	if ((flags & S_VERSION) && inode_maybe_inc_iversion(inode, false))
+		dirty_flags |= I_DIRTY_SYNC;
 
-	if (dirty)
-		iflags |= I_DIRTY_SYNC;
-	__mark_inode_dirty(inode, iflags);
+	__mark_inode_dirty(inode, dirty_flags);
 	return 0;
 }
 EXPORT_SYMBOL_GPL(fat_update_time);

Signature · 8 lines

2.30.0



_______________________________________________
Linux-f2fs-devel mailing list
[email protected]
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel

Christoph Hellwig

2021-01-11 10:52 UTC #9 lore raw

Re: [PATCH v2 04/12] fat: only specify I_DIRTY_TIME when needed in fat_update_time()

fs/fat/misc.c EXPORT_SYMBOL_GPL(fat_truncate_time); · line 346

Quoted 2 lines · Eric Biggers

fs/fat/misc.c +2 -0

+	if ((flags & S_VERSION) && inode_maybe_inc_iversion(inode, false))
+		dirty_flags |= I_DIRTY_SYNC;

fat does not support i_version updates, so this bit can be skipped.

_______________________________________________
Linux-f2fs-devel mailing list
[email protected]
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel

Eric Biggers

author

2021-01-11 19:50 UTC #10 lore raw

Re: [PATCH v2 04/12] fat: only specify I_DIRTY_TIME when needed in fat_update_time()

Quoted 5 lines · Christoph Hellwig

fs/fat/misc.c EXPORT_SYMBOL_GPL(fat_truncate_time); · line 346
Quoted 2 lines · Eric Biggers
fs/fat/misc.c +2 -0
+	if ((flags & S_VERSION) && inode_maybe_inc_iversion(inode, false))
+		dirty_flags |= I_DIRTY_SYNC;
fat does not support i_version updates, so this bit can be skipped.

Is that really the case? Any filesystem (including fat) can be mounted with "iversion", which causes SB_I_VERSION to be set.

A lot of filesystems (including fat) don't store i_version to disk, but it looks like it will still get updated in-memory. Could anything be relying on that?

- Eric

_______________________________________________
Linux-f2fs-devel mailing list
[email protected]
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel

Dave Chinner

2021-01-12 05:22 UTC #11 lore raw

Re: [PATCH v2 04/12] fat: only specify I_DIRTY_TIME when needed in fat_update_time()

Quoted 9 lines · Eric Biggers

Quoted 5 lines · Christoph Hellwig
fs/fat/misc.c EXPORT_SYMBOL_GPL(fat_truncate_time); · line 346
Quoted 2 lines · Eric Biggers
fs/fat/misc.c +2 -0
+	if ((flags & S_VERSION) && inode_maybe_inc_iversion(inode, false))
+		dirty_flags |= I_DIRTY_SYNC;
fat does not support i_version updates, so this bit can be skipped.
Is that really the case? Any filesystem (including fat) can be mounted with "iversion", which causes SB_I_VERSION to be set.

That's a bug. Filesystems taht don't support persistent i_version on disk need to clear SB_I_VERSION in their mount and remount paths because the VFS iversion mount option was a complete screwup from the start.

Quoted 2 lines

A lot of filesystems (including fat) don't store i_version to disk, but it looks like it will still get updated in-memory. Could anything be relying on that?

If they do, then they are broken by definition. i_version as reported to observers is defined as monotonically increasing with every change to the inode. i.e. it never goes backwards. Which, of course, it will do if you crash or even just unmount/mount a filesystem that doesn't persist it.

Cheers,

Dave.

Signature · 2 lines

Dave Chinner
[email protected]

Christoph Hellwig

Reviewed-by

2021-01-12 13:23 UTC #12 lore raw

Re: [PATCH v2 04/12] fat: only specify I_DIRTY_TIME when needed in fat_update_time()

Quoted 7 lines · Eric Biggers

Quoted 5 lines · Christoph Hellwig
fs/fat/misc.c EXPORT_SYMBOL_GPL(fat_truncate_time); · line 346
Quoted 2 lines · Eric Biggers
fs/fat/misc.c +2 -0
+	if ((flags & S_VERSION) && inode_maybe_inc_iversion(inode, false))
+		dirty_flags |= I_DIRTY_SYNC;
fat does not support i_version updates, so this bit can be skipped.

Quoted 5 lines

Is that really the case? Any filesystem (including fat) can be mounted with "iversion", which causes SB_I_VERSION to be set.
A lot of filesystems (including fat) don't store i_version to disk, but it looks like it will still get updated in-memory. Could anything be relying on that?

As Dave pointed out i_version can't really work for fat. But I guess that is indeed out of scope for this series, so let's go ahead with this version for now:

_______________________________________________
Linux-f2fs-devel mailing list
[email protected]
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel

Jan Kara

maintainer Reviewed-by on v2 4/12

2021-01-11 14:52 UTC #13 lore raw

Re: [PATCH v2 04/12] fat: only specify I_DIRTY_TIME when needed in fat_update_time()

Quoted 8 lines · Eric Biggers

From: Eric Biggers <[email protected]>
As was done for generic_update_time(), only pass I_DIRTY_TIME to __mark_inode_dirty() when the inode's timestamps were actually updated and lazytime is enabled. This avoids a weird edge case where I_DIRTY_TIME could be set in i_state when lazytime isn't enabled.
Signed-off-by Eric Biggers <[email protected]>

Looks good to me. You can add:

Honza

Quoted 46 lines

---
 fs/fat/misc.c | 23 ++++++++++++-----------
 1 file changed, 12 insertions(+), 11 deletions(-)

fs/fat/misc.c +12 ~~-11~~

index f1b2a1fc2a6a4..18a50a46b57f8 100644
@@ -329,22 +329,23 @@ EXPORT_SYMBOL_GPL(fat_truncate_time);
 
 int fat_update_time(struct inode *inode, struct timespec64 *now, int flags)
 {
-	int iflags = I_DIRTY_TIME;
-	bool dirty = false;
+	int dirty_flags = 0;
 
 	if (inode->i_ino == MSDOS_ROOT_INO)
 		return 0;
 
-	fat_truncate_time(inode, now, flags);
-	if (flags & S_VERSION)
-		dirty = inode_maybe_inc_iversion(inode, false);
-	if ((flags & (S_ATIME | S_CTIME | S_MTIME)) &&
-	    !(inode->i_sb->s_flags & SB_LAZYTIME))
-		dirty = true;
+	if (flags & (S_ATIME | S_CTIME | S_MTIME)) {
+		fat_truncate_time(inode, now, flags);
+		if (inode->i_sb->s_flags & SB_LAZYTIME)
+			dirty_flags |= I_DIRTY_TIME;
+		else
+			dirty_flags |= I_DIRTY_SYNC;
+	}
+
+	if ((flags & S_VERSION) && inode_maybe_inc_iversion(inode, false))
+		dirty_flags |= I_DIRTY_SYNC;
 
-	if (dirty)
-		iflags |= I_DIRTY_SYNC;
-	__mark_inode_dirty(inode, iflags);
+	__mark_inode_dirty(inode, dirty_flags);
 	return 0;
 }
 EXPORT_SYMBOL_GPL(fat_update_time);

Signature · 1 line

2.30.0

Signature · 8 lines

Jan Kara <[email protected]>
SUSE Labs, CR


_______________________________________________
Linux-f2fs-devel mailing list
[email protected]
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel

Eric Biggers

author

2021-01-09 08:00 UTC #14 lore raw

PATCH v2 5/12 fs: don't call ->dirty_inode for lazytime timestamp updates

From: Eric Biggers <[email protected]>

There is no need to call ->dirty_inode for lazytime timestamp updates (i.e. for __mark_inode_dirty(I_DIRTY_TIME)), since by the definition of lazytime, filesystems must ignore these updates. Filesystems only need to care about the updated timestamps when they expire.

Therefore, only call ->dirty_inode when I_DIRTY_INODE is set.

Based on a patch from Christoph Hellwig:
https://lore.kernel.org/r/[email protected]

---
 fs/ext4/inode.c   | 12 +-----------
 fs/f2fs/super.c   |  3 ---
 fs/fs-writeback.c |  6 +++---
 fs/gfs2/super.c   |  2 --
 4 files changed, 4 insertions(+), 19 deletions(-)

fs/ext4/inode.c +1 ~~-11~~

index 27946882d4ce4..4cc6c7834312f 100644
@@ -5933,26 +5933,16 @@ int __ext4_mark_inode_dirty(handle_t *handle, struct inode *inode,
  * If the inode is marked synchronous, we don't honour that here - doing
  * so would cause a commit on atime updates, which we don't bother doing.
  * We handle synchronous inodes at the highest possible level.
- *
- * If only the I_DIRTY_TIME flag is set, we can skip everything.  If
- * I_DIRTY_TIME and I_DIRTY_SYNC is set, the only inode fields we need
- * to copy into the on-disk inode structure are the timestamp files.
  */
 void ext4_dirty_inode(struct inode *inode, int flags)
 {
 	handle_t *handle;
 
-	if (flags == I_DIRTY_TIME)
-		return;
 	handle = ext4_journal_start(inode, EXT4_HT_INODE, 2);
 	if (IS_ERR(handle))
-		goto out;
-
+		return;
 	ext4_mark_inode_dirty(handle, inode);
-
 	ext4_journal_stop(handle);
-out:
-	return;
 }
 
 int ext4_change_inode_journal_flag(struct inode *inode, int val)

fs/f2fs/super.c +0 -3

index b4a07fe62d1a5..cc98dc49f4a26 100644
@@ -1196,9 +1196,6 @@ static void f2fs_dirty_inode(struct inode *inode, int flags)
 			inode->i_ino == F2FS_META_INO(sbi))
 		return;
 
-	if (flags == I_DIRTY_TIME)
-		return;
-
 	if (is_inode_flag_set(inode, FI_AUTO_RECOVER))
 		clear_inode_flag(inode, FI_AUTO_RECOVER);

fs/fs-writeback.c +3 -3

index c41cb887eb7d3..b7616bbd55336 100644
@@ -2255,16 +2255,16 @@ void __mark_inode_dirty(struct inode *inode, int flags)
 	 * Don't do this for I_DIRTY_PAGES - that doesn't actually
 	 * dirty the inode itself
 	 */
-	if (flags & (I_DIRTY_INODE | I_DIRTY_TIME)) {
+	if (flags & I_DIRTY_INODE) {
 		trace_writeback_dirty_inode_start(inode, flags);
 
 		if (sb->s_op->dirty_inode)
 			sb->s_op->dirty_inode(inode, flags);
 
 		trace_writeback_dirty_inode(inode, flags);
-	}
-	if (flags & I_DIRTY_INODE)
+
 		flags &= ~I_DIRTY_TIME;
+	}
 	dirtytime = flags & I_DIRTY_TIME;
 
 	/*

fs/gfs2/super.c +0 -2

index 2f56acc41c049..042b94288ff11 100644
@@ -562,8 +562,6 @@ static void gfs2_dirty_inode(struct inode *inode, int flags)
 	int need_endtrans = 0;
 	int ret;
 
-	if (!(flags & I_DIRTY_INODE))
-		return;
 	if (unlikely(gfs2_withdrawn(sdp)))
 		return;
 	if (!gfs2_glock_is_locked_by_me(ip->i_gl)) {

Signature · 8 lines

2.30.0



_______________________________________________
Linux-f2fs-devel mailing list
[email protected]
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel

Jan Kara

maintainer Reviewed-by on v2 5/12

2021-01-11 14:55 UTC #15 lore raw

Re: [PATCH v2 05/12] fs: don't call ->dirty_inode for lazytime timestamp updates

Quoted 7 lines · Eric Biggers

From: Eric Biggers <[email protected]>
There is no need to call ->dirty_inode for lazytime timestamp updates (i.e. for __mark_inode_dirty(I_DIRTY_TIME)), since by the definition of lazytime, filesystems must ignore these updates. Filesystems only need to care about the updated timestamps when they expire.

Quoted 7 lines

Therefore, only call ->dirty_inode when I_DIRTY_INODE is set.
Based on a patch from Christoph Hellwig:
https://lore.kernel.org/r/[email protected]
Reviewed-by Christoph Hellwig <[email protected]>
Signed-off-by Eric Biggers <[email protected]>

Looks good to me. You can add:

Honza

Quoted 93 lines

---
 fs/ext4/inode.c   | 12 +-----------
 fs/f2fs/super.c   |  3 ---
 fs/fs-writeback.c |  6 +++---
 fs/gfs2/super.c   |  2 --
 4 files changed, 4 insertions(+), 19 deletions(-)

fs/ext4/inode.c +1 ~~-11~~

index 27946882d4ce4..4cc6c7834312f 100644
@@ -5933,26 +5933,16 @@ int __ext4_mark_inode_dirty(handle_t *handle, struct inode *inode,
  * If the inode is marked synchronous, we don't honour that here - doing
  * so would cause a commit on atime updates, which we don't bother doing.
  * We handle synchronous inodes at the highest possible level.
- *
- * If only the I_DIRTY_TIME flag is set, we can skip everything.  If
- * I_DIRTY_TIME and I_DIRTY_SYNC is set, the only inode fields we need
- * to copy into the on-disk inode structure are the timestamp files.
  */
 void ext4_dirty_inode(struct inode *inode, int flags)
 {
 	handle_t *handle;
 
-	if (flags == I_DIRTY_TIME)
-		return;
 	handle = ext4_journal_start(inode, EXT4_HT_INODE, 2);
 	if (IS_ERR(handle))
-		goto out;
-
+		return;
 	ext4_mark_inode_dirty(handle, inode);
-
 	ext4_journal_stop(handle);
-out:
-	return;
 }
 
 int ext4_change_inode_journal_flag(struct inode *inode, int val)

fs/f2fs/super.c +0 -3

index b4a07fe62d1a5..cc98dc49f4a26 100644
@@ -1196,9 +1196,6 @@ static void f2fs_dirty_inode(struct inode *inode, int flags)
 			inode->i_ino == F2FS_META_INO(sbi))
 		return;
 
-	if (flags == I_DIRTY_TIME)
-		return;
-
 	if (is_inode_flag_set(inode, FI_AUTO_RECOVER))
 		clear_inode_flag(inode, FI_AUTO_RECOVER);

fs/fs-writeback.c +3 -3

index c41cb887eb7d3..b7616bbd55336 100644
@@ -2255,16 +2255,16 @@ void __mark_inode_dirty(struct inode *inode, int flags)
 	 * Don't do this for I_DIRTY_PAGES - that doesn't actually
 	 * dirty the inode itself
 	 */
-	if (flags & (I_DIRTY_INODE | I_DIRTY_TIME)) {
+	if (flags & I_DIRTY_INODE) {
 		trace_writeback_dirty_inode_start(inode, flags);
 
 		if (sb->s_op->dirty_inode)
 			sb->s_op->dirty_inode(inode, flags);
 
 		trace_writeback_dirty_inode(inode, flags);
-	}
-	if (flags & I_DIRTY_INODE)
+
 		flags &= ~I_DIRTY_TIME;
+	}
 	dirtytime = flags & I_DIRTY_TIME;
 
 	/*

fs/gfs2/super.c +0 -2

index 2f56acc41c049..042b94288ff11 100644
@@ -562,8 +562,6 @@ static void gfs2_dirty_inode(struct inode *inode, int flags)
 	int need_endtrans = 0;
 	int ret;
 
-	if (!(flags & I_DIRTY_INODE))
-		return;
 	if (unlikely(gfs2_withdrawn(sdp)))
 		return;
 	if (!gfs2_glock_is_locked_by_me(ip->i_gl)) {

Signature · 1 line

2.30.0

Signature · 8 lines

Jan Kara <[email protected]>
SUSE Labs, CR


_______________________________________________
Linux-f2fs-devel mailing list
[email protected]
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel

Eric Biggers

author

2021-01-09 08:00 UTC #16 lore raw

PATCH v2 6/12 fs: pass only I_DIRTY_INODE flags to ->dirty_inode

From: Eric Biggers <[email protected]>

->dirty_inode is now only called when I_DIRTY_INODE (I_DIRTY_SYNC and/or I_DIRTY_DATASYNC) is set. However it may still be passed other dirty flags at the same time, provided that these other flags happened to be passed to __mark_inode_dirty() at the same time as I_DIRTY_INODE.

This doesn't make sense because there is no reason for filesystems to care about these extra flags. Nor are filesystems notified about all updates to these other flags.

Therefore, mask the flags before passing them to ->dirty_inode.

Also properly document ->dirty_inode in vfs.rst.

---
 Documentation/filesystems/vfs.rst | 5 ++++-
 fs/fs-writeback.c                 | 2 +-
 2 files changed, 5 insertions(+), 2 deletions(-)

Documentation/filesystems/vfs.rst +4 -1

index ca52c82e5bb54..287b80948a40b 100644
@@ -270,7 +270,10 @@ or bottom half).
 	->alloc_inode.
 
 ``dirty_inode``
-	this method is called by the VFS to mark an inode dirty.
+	this method is called by the VFS when an inode is marked dirty.
+	This is specifically for the inode itself being marked dirty,
+	not its data.  If the update needs to be persisted by fdatasync(),
+	then I_DIRTY_DATASYNC will be set in the flags argument.
 
 ``write_inode``
 	this method is called when the VFS needs to write an inode to

fs/fs-writeback.c +1 -1

index b7616bbd55336..2e6064012f7d3 100644
@@ -2259,7 +2259,7 @@ void __mark_inode_dirty(struct inode *inode, int flags)
 		trace_writeback_dirty_inode_start(inode, flags);
 
 		if (sb->s_op->dirty_inode)
-			sb->s_op->dirty_inode(inode, flags);
+			sb->s_op->dirty_inode(inode, flags & I_DIRTY_INODE);
 
 		trace_writeback_dirty_inode(inode, flags);

Signature · 8 lines

2.30.0



_______________________________________________
Linux-f2fs-devel mailing list
[email protected]
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel

Jan Kara

maintainer Reviewed-by on v2 6/12

2021-01-11 14:56 UTC #17 lore raw

Re: [PATCH v2 06/12] fs: pass only I_DIRTY_INODE flags to ->dirty_inode

Quoted 11 lines · Eric Biggers

From: Eric Biggers <[email protected]>
->dirty_inode is now only called when I_DIRTY_INODE (I_DIRTY_SYNC and/or I_DIRTY_DATASYNC) is set. However it may still be passed other dirty flags at the same time, provided that these other flags happened to be passed to __mark_inode_dirty() at the same time as I_DIRTY_INODE.
This doesn't make sense because there is no reason for filesystems to care about these extra flags. Nor are filesystems notified about all updates to these other flags.

Quoted 6 lines

Therefore, mask the flags before passing them to ->dirty_inode.
Also properly document ->dirty_inode in vfs.rst.
Reviewed-by Christoph Hellwig <[email protected]>
Signed-off-by Eric Biggers <[email protected]>

Looks good to me. Feel free to add:

Honza

Quoted 37 lines

---
 Documentation/filesystems/vfs.rst | 5 ++++-
 fs/fs-writeback.c                 | 2 +-
 2 files changed, 5 insertions(+), 2 deletions(-)

Documentation/filesystems/vfs.rst +4 -1

index ca52c82e5bb54..287b80948a40b 100644
@@ -270,7 +270,10 @@ or bottom half).
 	->alloc_inode.
 
 ``dirty_inode``
-	this method is called by the VFS to mark an inode dirty.
+	this method is called by the VFS when an inode is marked dirty.
+	This is specifically for the inode itself being marked dirty,
+	not its data.  If the update needs to be persisted by fdatasync(),
+	then I_DIRTY_DATASYNC will be set in the flags argument.
 
 ``write_inode``
 	this method is called when the VFS needs to write an inode to

fs/fs-writeback.c +1 -1

index b7616bbd55336..2e6064012f7d3 100644
@@ -2259,7 +2259,7 @@ void __mark_inode_dirty(struct inode *inode, int flags)
 		trace_writeback_dirty_inode_start(inode, flags);
 
 		if (sb->s_op->dirty_inode)
-			sb->s_op->dirty_inode(inode, flags);
+			sb->s_op->dirty_inode(inode, flags & I_DIRTY_INODE);
 
 		trace_writeback_dirty_inode(inode, flags);

Signature · 1 line

2.30.0

Signature · 8 lines

Jan Kara <[email protected]>
SUSE Labs, CR


_______________________________________________
Linux-f2fs-devel mailing list
[email protected]
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel

Eric Biggers

author

2021-01-09 08:00 UTC #18 lore raw

PATCH v2 7/12 fs: clean up __mark_inode_dirty() a bit

From: Eric Biggers <[email protected]>

Improve some comments, and don't bother checking for the I_DIRTY_TIME flag in the case where we just cleared it.

Also, warn if I_DIRTY_TIME and I_DIRTY_PAGES are passed to __mark_inode_dirty() at the same time, as this case isn't handled.

---
 fs/fs-writeback.c | 49 +++++++++++++++++++++++++++++------------------
 1 file changed, 30 insertions(+), 19 deletions(-)

fs/fs-writeback.c +30 ~~-19~~

index 2e6064012f7d3..80ee9816d9df5 100644
@@ -2219,23 +2219,24 @@ static noinline void block_dump___mark_inode_dirty(struct inode *inode)
 }
 
 /**
- * __mark_inode_dirty -	internal function
+ * __mark_inode_dirty -	internal function to mark an inode dirty
  *
  * @inode: inode to mark
- * @flags: what kind of dirty (i.e. I_DIRTY_SYNC)
+ * @flags: what kind of dirty, e.g. I_DIRTY_SYNC.  This can be a combination of
+ *	   multiple I_DIRTY_* flags, except that I_DIRTY_TIME can't be combined
+ *	   with I_DIRTY_PAGES.
  *
- * Mark an inode as dirty. Callers should use mark_inode_dirty or
- * mark_inode_dirty_sync.
+ * Mark an inode as dirty.  We notify the filesystem, then update the inode's
+ * dirty flags.  Then, if needed we add the inode to the appropriate dirty list.
  *
- * Put the inode on the super block's dirty list.
+ * Most callers should use mark_inode_dirty() or mark_inode_dirty_sync()
+ * instead of calling this directly.
  *
- * CAREFUL! We mark it dirty unconditionally, but move it onto the
- * dirty list only if it is hashed or if it refers to a blockdev.
- * If it was not hashed, it will never be added to the dirty list
- * even if it is later hashed, as it will have been marked dirty already.
+ * CAREFUL!  We only add the inode to the dirty list if it is hashed or if it
+ * refers to a blockdev.  Unhashed inodes will never be added to the dirty list
+ * even if they are later hashed, as they will have been marked dirty already.
  *
- * In short, make sure you hash any inodes _before_ you start marking
- * them dirty.
+ * In short, ensure you hash any inodes _before_ you start marking them dirty.
  *
  * Note that for blockdevs, inode->dirtied_when represents the dirtying time of
  * the block-special inode (/dev/hda1) itself.  And the ->dirtied_when field of

@@ -2247,25 +2248,34 @@ static noinline void block_dump___mark_inode_dirty(struct inode *inode)
 void __mark_inode_dirty(struct inode *inode, int flags)
 {
 	struct super_block *sb = inode->i_sb;
-	int dirtytime;
+	int dirtytime = 0;
 
 	trace_writeback_mark_inode_dirty(inode, flags);
 
-	/*
-	 * Don't do this for I_DIRTY_PAGES - that doesn't actually
-	 * dirty the inode itself
-	 */
 	if (flags & I_DIRTY_INODE) {
+		/*
+		 * Notify the filesystem about the inode being dirtied, so that
+		 * (if needed) it can update on-disk fields and journal the
+		 * inode.  This is only needed when the inode itself is being
+		 * dirtied now.  I.e. it's only needed for I_DIRTY_INODE, not
+		 * for just I_DIRTY_PAGES or I_DIRTY_TIME.
+		 */
 		trace_writeback_dirty_inode_start(inode, flags);
-
 		if (sb->s_op->dirty_inode)
 			sb->s_op->dirty_inode(inode, flags & I_DIRTY_INODE);
-
 		trace_writeback_dirty_inode(inode, flags);
 
+		/* I_DIRTY_INODE supersedes I_DIRTY_TIME. */
 		flags &= ~I_DIRTY_TIME;
+	} else {
+		/*
+		 * Else it's either I_DIRTY_PAGES, I_DIRTY_TIME, or nothing.
+		 * (We don't support setting both I_DIRTY_PAGES and I_DIRTY_TIME
+		 * in one call to __mark_inode_dirty().)
+		 */
+		dirtytime = flags & I_DIRTY_TIME;
+		WARN_ON_ONCE(dirtytime && flags != I_DIRTY_TIME);
 	}
-	dirtytime = flags & I_DIRTY_TIME;
 
 	/*
 	 * Paired with smp_mb() in __writeback_single_inode() for the

@@ -2288,6 +2298,7 @@ void __mark_inode_dirty(struct inode *inode, int flags)
 
 		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;

Signature · 8 lines

2.30.0



_______________________________________________
Linux-f2fs-devel mailing list
[email protected]
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel

Jan Kara

maintainer Reviewed-by on v2 7/12

2021-01-11 14:59 UTC #19 lore raw

Re: [PATCH v2 07/12] fs: clean up __mark_inode_dirty() a bit

Quoted 10 lines · Eric Biggers

From: Eric Biggers <[email protected]>
Improve some comments, and don't bother checking for the I_DIRTY_TIME flag in the case where we just cleared it.
Also, warn if I_DIRTY_TIME and I_DIRTY_PAGES are passed to __mark_inode_dirty() at the same time, as this case isn't handled.
Reviewed-by Christoph Hellwig <[email protected]>
Signed-off-by Eric Biggers <[email protected]>

Looks good. Feel free to add:

Honza

Quoted 98 lines

---
 fs/fs-writeback.c | 49 +++++++++++++++++++++++++++++------------------
 1 file changed, 30 insertions(+), 19 deletions(-)

fs/fs-writeback.c +30 ~~-19~~

index 2e6064012f7d3..80ee9816d9df5 100644
@@ -2219,23 +2219,24 @@ static noinline void block_dump___mark_inode_dirty(struct inode *inode)
 }
 
 /**
- * __mark_inode_dirty -	internal function
+ * __mark_inode_dirty -	internal function to mark an inode dirty
  *
  * @inode: inode to mark
- * @flags: what kind of dirty (i.e. I_DIRTY_SYNC)
+ * @flags: what kind of dirty, e.g. I_DIRTY_SYNC.  This can be a combination of
+ *	   multiple I_DIRTY_* flags, except that I_DIRTY_TIME can't be combined
+ *	   with I_DIRTY_PAGES.
  *
- * Mark an inode as dirty. Callers should use mark_inode_dirty or
- * mark_inode_dirty_sync.
+ * Mark an inode as dirty.  We notify the filesystem, then update the inode's
+ * dirty flags.  Then, if needed we add the inode to the appropriate dirty list.
  *
- * Put the inode on the super block's dirty list.
+ * Most callers should use mark_inode_dirty() or mark_inode_dirty_sync()
+ * instead of calling this directly.
  *
- * CAREFUL! We mark it dirty unconditionally, but move it onto the
- * dirty list only if it is hashed or if it refers to a blockdev.
- * If it was not hashed, it will never be added to the dirty list
- * even if it is later hashed, as it will have been marked dirty already.
+ * CAREFUL!  We only add the inode to the dirty list if it is hashed or if it
+ * refers to a blockdev.  Unhashed inodes will never be added to the dirty list
+ * even if they are later hashed, as they will have been marked dirty already.
  *
- * In short, make sure you hash any inodes _before_ you start marking
- * them dirty.
+ * In short, ensure you hash any inodes _before_ you start marking them dirty.
  *
  * Note that for blockdevs, inode->dirtied_when represents the dirtying time of
  * the block-special inode (/dev/hda1) itself.  And the ->dirtied_when field of

@@ -2247,25 +2248,34 @@ static noinline void block_dump___mark_inode_dirty(struct inode *inode)
 void __mark_inode_dirty(struct inode *inode, int flags)
 {
 	struct super_block *sb = inode->i_sb;
-	int dirtytime;
+	int dirtytime = 0;
 
 	trace_writeback_mark_inode_dirty(inode, flags);
 
-	/*
-	 * Don't do this for I_DIRTY_PAGES - that doesn't actually
-	 * dirty the inode itself
-	 */
 	if (flags & I_DIRTY_INODE) {
+		/*
+		 * Notify the filesystem about the inode being dirtied, so that
+		 * (if needed) it can update on-disk fields and journal the
+		 * inode.  This is only needed when the inode itself is being
+		 * dirtied now.  I.e. it's only needed for I_DIRTY_INODE, not
+		 * for just I_DIRTY_PAGES or I_DIRTY_TIME.
+		 */
 		trace_writeback_dirty_inode_start(inode, flags);
-
 		if (sb->s_op->dirty_inode)
 			sb->s_op->dirty_inode(inode, flags & I_DIRTY_INODE);
-
 		trace_writeback_dirty_inode(inode, flags);
 
+		/* I_DIRTY_INODE supersedes I_DIRTY_TIME. */
 		flags &= ~I_DIRTY_TIME;
+	} else {
+		/*
+		 * Else it's either I_DIRTY_PAGES, I_DIRTY_TIME, or nothing.
+		 * (We don't support setting both I_DIRTY_PAGES and I_DIRTY_TIME
+		 * in one call to __mark_inode_dirty().)
+		 */
+		dirtytime = flags & I_DIRTY_TIME;
+		WARN_ON_ONCE(dirtytime && flags != I_DIRTY_TIME);
 	}
-	dirtytime = flags & I_DIRTY_TIME;
 
 	/*
 	 * Paired with smp_mb() in __writeback_single_inode() for the

@@ -2288,6 +2298,7 @@ void __mark_inode_dirty(struct inode *inode, int flags)
 
 		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;

Signature · 1 line

2.30.0

Signature · 8 lines

Jan Kara <[email protected]>
SUSE Labs, CR


_______________________________________________
Linux-f2fs-devel mailing list
[email protected]
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel

Eric Biggers

author

2021-01-09 08:00 UTC #20 lore raw

PATCH v2 8/12 fs: drop redundant check from __writeback_single_inode()

From: Eric Biggers <[email protected]>

wbc->for_sync implies wbc->sync_mode == WB_SYNC_ALL, so there's no need to check for both. Just check for WB_SYNC_ALL.

---
 fs/fs-writeback.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

fs/fs-writeback.c +1 -1

index 80ee9816d9df5..cee1df6e3bd43 100644
@@ -1479,7 +1479,7 @@ __writeback_single_inode(struct inode *inode, struct writeback_control *wbc)
 	 * change I_DIRTY_TIME into I_DIRTY_SYNC.
 	 */
 	if ((inode->i_state & I_DIRTY_TIME) &&
-	    (wbc->sync_mode == WB_SYNC_ALL || wbc->for_sync ||
+	    (wbc->sync_mode == WB_SYNC_ALL ||
 	     time_after(jiffies, inode->dirtied_time_when +
 			dirtytime_expire_interval * HZ))) {
 		trace_writeback_lazytime(inode);

Signature · 8 lines

2.30.0



_______________________________________________
Linux-f2fs-devel mailing list
[email protected]
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel

Christoph Hellwig

Reviewed-by on v2 8/12

2021-01-11 10:52 UTC #21 lore raw

Re: [PATCH v2 08/12] fs: drop redundant check from __writeback_single_inode()

Quoted 6 lines · Eric Biggers

From: Eric Biggers <[email protected]>
wbc->for_sync implies wbc->sync_mode == WB_SYNC_ALL, so there's no need to check for both. Just check for WB_SYNC_ALL.
Signed-off-by Eric Biggers <[email protected]>

Looks good,

_______________________________________________
Linux-f2fs-devel mailing list
[email protected]
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel

Jan Kara

maintainer Reviewed-by on v2 8/12

2021-01-11 15:00 UTC #22 lore raw

Re: [PATCH v2 08/12] fs: drop redundant check from __writeback_single_inode()

Quoted 6 lines · Eric Biggers

From: Eric Biggers <[email protected]>
wbc->for_sync implies wbc->sync_mode == WB_SYNC_ALL, so there's no need to check for both. Just check for WB_SYNC_ALL.
Signed-off-by Eric Biggers <[email protected]>

Looks good to me. Feel free to add:

Honza

Quoted 20 lines

---
 fs/fs-writeback.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

fs/fs-writeback.c +1 -1

index 80ee9816d9df5..cee1df6e3bd43 100644
@@ -1479,7 +1479,7 @@ __writeback_single_inode(struct inode *inode, struct writeback_control *wbc)
 	 * change I_DIRTY_TIME into I_DIRTY_SYNC.
 	 */
 	if ((inode->i_state & I_DIRTY_TIME) &&
-	    (wbc->sync_mode == WB_SYNC_ALL || wbc->for_sync ||
+	    (wbc->sync_mode == WB_SYNC_ALL ||
 	     time_after(jiffies, inode->dirtied_time_when +
 			dirtytime_expire_interval * HZ))) {
 		trace_writeback_lazytime(inode);

Signature · 1 line

2.30.0

Signature · 8 lines

Jan Kara <[email protected]>
SUSE Labs, CR


_______________________________________________
Linux-f2fs-devel mailing list
[email protected]
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel

Eric Biggers

author

2021-01-09 08:00 UTC #23 lore raw

PATCH v2 9/12 fs: improve comments for writeback_single_inode()

From: Eric Biggers <[email protected]>

Some comments for writeback_single_inode() and __writeback_single_inode() are outdated or not very helpful, especially with regards to writeback list handling. Update them.

---
 fs/fs-writeback.c | 57 +++++++++++++++++++++++++++--------------------
 1 file changed, 33 insertions(+), 24 deletions(-)

fs/fs-writeback.c +33 ~~-24~~

index cee1df6e3bd43..e91980f493884 100644
@@ -1442,9 +1442,15 @@ static void requeue_inode(struct inode *inode, struct bdi_writeback *wb,
 }
 
 /*
- * Write out an inode and its dirty pages. Do not update the writeback list
- * linkage. That is left to the caller. The caller is also responsible for
- * setting I_SYNC flag and calling inode_sync_complete() to clear it.
+ * Write out an inode and its dirty pages (or some of its dirty pages, depending
+ * on @wbc->nr_to_write), and clear the relevant dirty flags from i_state.
+ *
+ * This doesn't remove the inode from the writeback list it is on, except
+ * potentially to move it from b_dirty_time to b_dirty due to timestamp
+ * expiration.  The caller is otherwise responsible for writeback list handling.
+ *
+ * The caller is also responsible for setting the I_SYNC flag beforehand and
+ * calling inode_sync_complete() to clear it afterwards.
  */
 static int
 __writeback_single_inode(struct inode *inode, struct writeback_control *wbc)

@@ -1487,9 +1493,10 @@ __writeback_single_inode(struct inode *inode, struct writeback_control *wbc)
 	}
 
 	/*
-	 * Some filesystems may redirty the inode during the writeback
-	 * due to delalloc, clear dirty metadata flags right before
-	 * write_inode()
+	 * Get and clear the dirty flags from i_state.  This needs to be done
+	 * after calling writepages because some filesystems may redirty the
+	 * inode during writepages due to delalloc.  It also needs to be done
+	 * after handling timestamp expiration, as that may dirty the inode too.
 	 */
 	spin_lock(&inode->i_lock);
 	dirty = inode->i_state & I_DIRTY;

@@ -1524,12 +1531,13 @@ __writeback_single_inode(struct inode *inode, struct writeback_control *wbc)
 }
 
 /*
- * Write out an inode's dirty pages. Either the caller has an active reference
- * on the inode or the inode has I_WILL_FREE set.
+ * Write out an inode's dirty data and metadata on-demand, i.e. separately from
+ * the regular batched writeback done by the flusher threads in
+ * writeback_sb_inodes().  @wbc controls various aspects of the write, such as
+ * whether it is a data-integrity sync (%WB_SYNC_ALL) or not (%WB_SYNC_NONE).
  *
- * This function is designed to be called for writing back one inode which
- * we go e.g. from filesystem. Flusher thread uses __writeback_single_inode()
- * and does more profound writeback list handling in writeback_sb_inodes().
+ * To prevent the inode from going away, either the caller must have a reference
+ * to the inode, or the inode must have I_WILL_FREE or I_FREEING set.
  */
 static int writeback_single_inode(struct inode *inode,
 				  struct writeback_control *wbc)

@@ -1544,23 +1552,23 @@ static int writeback_single_inode(struct inode *inode,
 		WARN_ON(inode->i_state & I_WILL_FREE);
 
 	if (inode->i_state & I_SYNC) {
-		if (wbc->sync_mode != WB_SYNC_ALL)
-			goto out;
 		/*
-		 * It's a data-integrity sync. We must wait. Since callers hold
-		 * inode reference or inode has I_WILL_FREE set, it cannot go
-		 * away under us.
+		 * Writeback is already running on the inode.  For WB_SYNC_NONE,
+		 * that's enough and we can just return.  For WB_SYNC_ALL, we
+		 * must wait for the existing writeback to complete, then do
+		 * writeback again if there's anything left.
 		 */
+		if (wbc->sync_mode != WB_SYNC_ALL)
+			goto out;
 		__inode_wait_for_writeback(inode);
 	}
 	WARN_ON(inode->i_state & I_SYNC);
 	/*
-	 * Skip inode if it is clean and we have no outstanding writeback in
-	 * WB_SYNC_ALL mode. We don't want to mess with writeback lists in this
-	 * function since flusher thread may be doing for example sync in
-	 * parallel and if we move the inode, it could get skipped. So here we
-	 * make sure inode is on some writeback list and leave it there unless
-	 * we have completely cleaned the inode.
+	 * If the inode is already fully clean, then there's nothing to do.
+	 *
+	 * For data-integrity syncs we also need to check whether any pages are
+	 * still under writeback, e.g. due to prior WB_SYNC_NONE writeback.  If
+	 * there are any such pages, we'll need to wait for them.
 	 */
 	if (!(inode->i_state & I_DIRTY_ALL) &&
 	    (wbc->sync_mode != WB_SYNC_ALL ||

@@ -1576,8 +1584,9 @@ static int writeback_single_inode(struct inode *inode,
 	wb = inode_to_wb_and_lock_list(inode);
 	spin_lock(&inode->i_lock);
 	/*
-	 * If inode is clean, remove it from writeback lists. Otherwise don't
-	 * touch it. See comment above for explanation.
+	 * If the inode is now fully clean, then it can be safely removed from
+	 * its writeback list (if any).  Otherwise the flusher threads are
+	 * responsible for the writeback lists.
 	 */
 	if (!(inode->i_state & I_DIRTY_ALL))
 		inode_io_list_del_locked(inode, wb);

Signature · 8 lines

2.30.0



_______________________________________________
Linux-f2fs-devel mailing list
[email protected]
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel

Christoph Hellwig

Reviewed-by on v2 9/12

2021-01-11 10:53 UTC #24 lore raw

Re: [PATCH v2 09/12] fs: improve comments for writeback_single_inode()

Looks good,

_______________________________________________
Linux-f2fs-devel mailing list
[email protected]
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel

Jan Kara

maintainer Reviewed-by on v2 9/12

2021-01-11 15:05 UTC #25 lore raw

Re: [PATCH v2 09/12] fs: improve comments for writeback_single_inode()

Quoted 7 lines · Eric Biggers

From: Eric Biggers <[email protected]>
Some comments for writeback_single_inode() and __writeback_single_inode() are outdated or not very helpful, especially with regards to writeback list handling. Update them.
Signed-off-by Eric Biggers <[email protected]>

Yeah, looks more comprehensible :). Thanks for the cleanup. Feel free to add:

Honza

Quoted 110 lines

---
 fs/fs-writeback.c | 57 +++++++++++++++++++++++++++--------------------
 1 file changed, 33 insertions(+), 24 deletions(-)

fs/fs-writeback.c +33 ~~-24~~

index cee1df6e3bd43..e91980f493884 100644
@@ -1442,9 +1442,15 @@ static void requeue_inode(struct inode *inode, struct bdi_writeback *wb,
 }
 
 /*
- * Write out an inode and its dirty pages. Do not update the writeback list
- * linkage. That is left to the caller. The caller is also responsible for
- * setting I_SYNC flag and calling inode_sync_complete() to clear it.
+ * Write out an inode and its dirty pages (or some of its dirty pages, depending
+ * on @wbc->nr_to_write), and clear the relevant dirty flags from i_state.
+ *
+ * This doesn't remove the inode from the writeback list it is on, except
+ * potentially to move it from b_dirty_time to b_dirty due to timestamp
+ * expiration.  The caller is otherwise responsible for writeback list handling.
+ *
+ * The caller is also responsible for setting the I_SYNC flag beforehand and
+ * calling inode_sync_complete() to clear it afterwards.
  */
 static int
 __writeback_single_inode(struct inode *inode, struct writeback_control *wbc)

@@ -1487,9 +1493,10 @@ __writeback_single_inode(struct inode *inode, struct writeback_control *wbc)
 	}
 
 	/*
-	 * Some filesystems may redirty the inode during the writeback
-	 * due to delalloc, clear dirty metadata flags right before
-	 * write_inode()
+	 * Get and clear the dirty flags from i_state.  This needs to be done
+	 * after calling writepages because some filesystems may redirty the
+	 * inode during writepages due to delalloc.  It also needs to be done
+	 * after handling timestamp expiration, as that may dirty the inode too.
 	 */
 	spin_lock(&inode->i_lock);
 	dirty = inode->i_state & I_DIRTY;

@@ -1524,12 +1531,13 @@ __writeback_single_inode(struct inode *inode, struct writeback_control *wbc)
 }
 
 /*
- * Write out an inode's dirty pages. Either the caller has an active reference
- * on the inode or the inode has I_WILL_FREE set.
+ * Write out an inode's dirty data and metadata on-demand, i.e. separately from
+ * the regular batched writeback done by the flusher threads in
+ * writeback_sb_inodes().  @wbc controls various aspects of the write, such as
+ * whether it is a data-integrity sync (%WB_SYNC_ALL) or not (%WB_SYNC_NONE).
  *
- * This function is designed to be called for writing back one inode which
- * we go e.g. from filesystem. Flusher thread uses __writeback_single_inode()
- * and does more profound writeback list handling in writeback_sb_inodes().
+ * To prevent the inode from going away, either the caller must have a reference
+ * to the inode, or the inode must have I_WILL_FREE or I_FREEING set.
  */
 static int writeback_single_inode(struct inode *inode,
 				  struct writeback_control *wbc)

@@ -1544,23 +1552,23 @@ static int writeback_single_inode(struct inode *inode,
 		WARN_ON(inode->i_state & I_WILL_FREE);
 
 	if (inode->i_state & I_SYNC) {
-		if (wbc->sync_mode != WB_SYNC_ALL)
-			goto out;
 		/*
-		 * It's a data-integrity sync. We must wait. Since callers hold
-		 * inode reference or inode has I_WILL_FREE set, it cannot go
-		 * away under us.
+		 * Writeback is already running on the inode.  For WB_SYNC_NONE,
+		 * that's enough and we can just return.  For WB_SYNC_ALL, we
+		 * must wait for the existing writeback to complete, then do
+		 * writeback again if there's anything left.
 		 */
+		if (wbc->sync_mode != WB_SYNC_ALL)
+			goto out;
 		__inode_wait_for_writeback(inode);
 	}
 	WARN_ON(inode->i_state & I_SYNC);
 	/*
-	 * Skip inode if it is clean and we have no outstanding writeback in
-	 * WB_SYNC_ALL mode. We don't want to mess with writeback lists in this
-	 * function since flusher thread may be doing for example sync in
-	 * parallel and if we move the inode, it could get skipped. So here we
-	 * make sure inode is on some writeback list and leave it there unless
-	 * we have completely cleaned the inode.
+	 * If the inode is already fully clean, then there's nothing to do.
+	 *
+	 * For data-integrity syncs we also need to check whether any pages are
+	 * still under writeback, e.g. due to prior WB_SYNC_NONE writeback.  If
+	 * there are any such pages, we'll need to wait for them.
 	 */
 	if (!(inode->i_state & I_DIRTY_ALL) &&
 	    (wbc->sync_mode != WB_SYNC_ALL ||

@@ -1576,8 +1584,9 @@ static int writeback_single_inode(struct inode *inode,
 	wb = inode_to_wb_and_lock_list(inode);
 	spin_lock(&inode->i_lock);
 	/*
-	 * If inode is clean, remove it from writeback lists. Otherwise don't
-	 * touch it. See comment above for explanation.
+	 * If the inode is now fully clean, then it can be safely removed from
+	 * its writeback list (if any).  Otherwise the flusher threads are
+	 * responsible for the writeback lists.
 	 */
 	if (!(inode->i_state & I_DIRTY_ALL))
 		inode_io_list_del_locked(inode, wb);

Signature · 1 line

2.30.0

Signature · 8 lines

Jan Kara <[email protected]>
SUSE Labs, CR


_______________________________________________
Linux-f2fs-devel mailing list
[email protected]
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel

Eric Biggers

author

2021-01-09 08:00 UTC #26 lore raw

PATCH v2 10/12 gfs2: don't worry about I_DIRTY_TIME in gfs2_fsync()

From: Eric Biggers <[email protected]>

The I_DIRTY_TIME flag is primary used within the VFS, and there's no reason for ->fsync() implementations to do anything with it. This is because when !datasync, the VFS will expire dirty timestamps before calling ->fsync(). (See vfs_fsync_range().) This turns I_DIRTY_TIME into I_DIRTY_SYNC.

Therefore, change gfs2_fsync() to not check for I_DIRTY_TIME.

---
 fs/gfs2/file.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

fs/gfs2/file.c +2 -2

index b39b339feddc9..7fe2497755a37 100644
@@ -749,7 +749,7 @@ static int gfs2_fsync(struct file *file, loff_t start, loff_t end,
 {
 	struct address_space *mapping = file->f_mapping;
 	struct inode *inode = mapping->host;
-	int sync_state = inode->i_state & I_DIRTY_ALL;
+	int sync_state = inode->i_state & I_DIRTY;
 	struct gfs2_inode *ip = GFS2_I(inode);
 	int ret = 0, ret1 = 0;

@@ -762,7 +762,7 @@ static int gfs2_fsync(struct file *file, loff_t start, loff_t end,
 	if (!gfs2_is_jdata(ip))
 		sync_state &= ~I_DIRTY_PAGES;
 	if (datasync)
-		sync_state &= ~(I_DIRTY_SYNC | I_DIRTY_TIME);
+		sync_state &= ~I_DIRTY_SYNC;
 
 	if (sync_state) {
 		ret = sync_inode_metadata(inode, 1);

Signature · 8 lines

2.30.0



_______________________________________________
Linux-f2fs-devel mailing list
[email protected]
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel

Jan Kara

maintainer Reviewed-by on v2 10/12

2021-01-11 15:06 UTC #27 lore raw

Re: [PATCH v2 10/12] gfs2: don't worry about I_DIRTY_TIME in gfs2_fsync()

Quoted 8 lines · Eric Biggers

From: Eric Biggers <[email protected]>
The I_DIRTY_TIME flag is primary used within the VFS, and there's no reason for ->fsync() implementations to do anything with it. This is because when !datasync, the VFS will expire dirty timestamps before calling ->fsync(). (See vfs_fsync_range().) This turns I_DIRTY_TIME into I_DIRTY_SYNC.

Quoted 4 lines

Therefore, change gfs2_fsync() to not check for I_DIRTY_TIME.
Reviewed-by Christoph Hellwig <[email protected]>
Signed-off-by Eric Biggers <[email protected]>

Looks good to me. Feel free to add:

Honza

Quoted 29 lines

---
 fs/gfs2/file.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

fs/gfs2/file.c +2 -2

index b39b339feddc9..7fe2497755a37 100644
@@ -749,7 +749,7 @@ static int gfs2_fsync(struct file *file, loff_t start, loff_t end,
 {
 	struct address_space *mapping = file->f_mapping;
 	struct inode *inode = mapping->host;
-	int sync_state = inode->i_state & I_DIRTY_ALL;
+	int sync_state = inode->i_state & I_DIRTY;
 	struct gfs2_inode *ip = GFS2_I(inode);
 	int ret = 0, ret1 = 0;

@@ -762,7 +762,7 @@ static int gfs2_fsync(struct file *file, loff_t start, loff_t end,
 	if (!gfs2_is_jdata(ip))
 		sync_state &= ~I_DIRTY_PAGES;
 	if (datasync)
-		sync_state &= ~(I_DIRTY_SYNC | I_DIRTY_TIME);
+		sync_state &= ~I_DIRTY_SYNC;
 
 	if (sync_state) {
 		ret = sync_inode_metadata(inode, 1);

Signature · 1 line

2.30.0

Signature · 8 lines

Jan Kara <[email protected]>
SUSE Labs, CR


_______________________________________________
Linux-f2fs-devel mailing list
[email protected]
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel

Eric Biggers

author

2021-01-09 08:00 UTC #28 lore raw

PATCH v2 11/12 ext4: simplify i_state checks in __ext4_update_other_inode_time()

From: Eric Biggers <[email protected]>

Since I_DIRTY_TIME and I_DIRTY_INODE are mutually exclusive in i_state, there's no need to check for I_DIRTY_TIME && !I_DIRTY_INODE. Just check for I_DIRTY_TIME.

---
 fs/ext4/inode.c | 8 +++-----
 1 file changed, 3 insertions(+), 5 deletions(-)

fs/ext4/inode.c +3 -5

index 4cc6c7834312f..00bca5c18eb65 100644
@@ -4962,14 +4962,12 @@ static void __ext4_update_other_inode_time(struct super_block *sb,
 		return;
 
 	if ((inode->i_state & (I_FREEING | I_WILL_FREE | I_NEW |
-			       I_DIRTY_INODE)) ||
-	    ((inode->i_state & I_DIRTY_TIME) == 0))
+			       I_DIRTY_TIME)) != I_DIRTY_TIME)
 		return;
 
 	spin_lock(&inode->i_lock);
-	if (((inode->i_state & (I_FREEING | I_WILL_FREE | I_NEW |
-				I_DIRTY_INODE)) == 0) &&
-	    (inode->i_state & I_DIRTY_TIME)) {
+	if ((inode->i_state & (I_FREEING | I_WILL_FREE | I_NEW |
+			       I_DIRTY_TIME)) == I_DIRTY_TIME) {
 		struct ext4_inode_info	*ei = EXT4_I(inode);
 
 		inode->i_state &= ~I_DIRTY_TIME;

Signature · 8 lines

2.30.0



_______________________________________________
Linux-f2fs-devel mailing list
[email protected]
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel

Christoph Hellwig

2021-01-11 10:54 UTC #29 lore raw

Re: [PATCH v2 11/12] ext4: simplify i_state checks in __ext4_update_other_inode_time()

fs/ext4/inode.c static void __ext4_update_other_inode_time(struct super_block *sb, · line 4970

Quoted 12 lines · Eric Biggers

fs/ext4/inode.c +3 -5

 	if ((inode->i_state & (I_FREEING | I_WILL_FREE | I_NEW |
-			       I_DIRTY_INODE)) ||
-	    ((inode->i_state & I_DIRTY_TIME) == 0))
+			       I_DIRTY_TIME)) != I_DIRTY_TIME)
 		return;
 
 	spin_lock(&inode->i_lock);
-	if (((inode->i_state & (I_FREEING | I_WILL_FREE | I_NEW |
-				I_DIRTY_INODE)) == 0) &&
-	    (inode->i_state & I_DIRTY_TIME)) {
+	if ((inode->i_state & (I_FREEING | I_WILL_FREE | I_NEW |
+			       I_DIRTY_TIME)) == I_DIRTY_TIME) {

I think a descriptively named inline helper in fs.h would really improve this..

_______________________________________________
Linux-f2fs-devel mailing list
[email protected]
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel

Eric Biggers

author

2021-01-11 20:24 UTC #30 lore raw

Re: [PATCH v2 11/12] ext4: simplify i_state checks in __ext4_update_other_inode_time()

Quoted 7 lines · Christoph Hellwig

Quoted 6 lines · Eric Biggers

fs/ext4/inode.c +1 -2

 	if ((inode->i_state & (I_FREEING | I_WILL_FREE | I_NEW |
-			       I_DIRTY_INODE)) ||
-	    ((inode->i_state & I_DIRTY_TIME) == 0))
+			       I_DIRTY_TIME)) != I_DIRTY_TIME)
 		return;

Quoted 9 lines

fs/ext4/inode.c static void __ext4_update_other_inode_time(struct super_block *sb, · line 4970
Quoted 6 lines
fs/ext4/inode.c +2 -3
 	spin_lock(&inode->i_lock);
-	if (((inode->i_state & (I_FREEING | I_WILL_FREE | I_NEW |
-				I_DIRTY_INODE)) == 0) &&
-	    (inode->i_state & I_DIRTY_TIME)) {
+	if ((inode->i_state & (I_FREEING | I_WILL_FREE | I_NEW |
+			       I_DIRTY_TIME)) == I_DIRTY_TIME) {
I think a descriptively named inline helper in fs.h would really improve this..

Do you want this even though it is specific to how ext4 opportunisticly updates other inodes in the same inode block as the inode being updated? That's the only reason that I_FREEING|I_WILL_FREE|I_NEW need to be checked; everywhere else justs want I_DIRTY_TIME.

We could add:

	static inline bool other_inode_has_dirtytime(struct inode *inode)
	{
		return (inode->state & (I_FREEING | I_WILL_FREE |
					I_NEW | I_DIRTY_TIME)) == I_DIRTY_TIME;
	}

But it seems a bit weird when it's specific to ext4 at the moment.

Are you thinking that other filesystems will implement the same sort of opportunistic update, so we should add the helper now?

- Eric

_______________________________________________
Linux-f2fs-devel mailing list
[email protected]
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel

Christoph Hellwig

2021-01-12 13:25 UTC #31 lore raw

Re: [PATCH v2 11/12] ext4: simplify i_state checks in __ext4_update_other_inode_time()

Quoted 16 lines · Eric Biggers

Quoted 2 lines
I think a descriptively named inline helper in fs.h would really improve this..
Do you want this even though it is specific to how ext4 opportunisticly updates other inodes in the same inode block as the inode being updated? That's the only reason that I_FREEING|I_WILL_FREE|I_NEW need to be checked; everywhere else justs want I_DIRTY_TIME.
We could add:
	static inline bool other_inode_has_dirtytime(struct inode *inode)
	{
		return (inode->state & (I_FREEING | I_WILL_FREE |
					I_NEW | I_DIRTY_TIME)) == I_DIRTY_TIME;
	}

Quoted 4 lines

But it seems a bit weird when it's specific to ext4 at the moment.
Are you thinking that other filesystems will implement the same sort of opportunistic update, so we should add the helper now?

For my taste these checks for flags is way too much black magic and will trivially break when people add new flags. So having a helper next to the definition of the I_* flags that is well documented would be very, very helpful. My preferred naming would be something along the lines of 'inode_is_dirty_lazytime_only()'.

_______________________________________________
Linux-f2fs-devel mailing list
[email protected]
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel

Theodore Ts'o

maintainer

2021-02-03 05:17 UTC #32 lore raw

Re: [PATCH v2 11/12] ext4: simplify i_state checks in __ext4_update_other_inode_time()

Quoted 10 lines · Christoph Hellwig

Quoted 10 lines
We could add:
	static inline bool other_inode_has_dirtytime(struct inode *inode)
	{
		return (inode->state & (I_FREEING | I_WILL_FREE |
					I_NEW | I_DIRTY_TIME)) == I_DIRTY_TIME;
	}
But it seems a bit weird when it's specific to ext4 at the moment.

Quoted 8 lines

Quoted 2 lines
Are you thinking that other filesystems will implement the same sort of opportunistic update, so we should add the helper now?
For my taste these checks for flags is way too much black magic and will trivially break when people add new flags. So having a helper next to the definition of the I_* flags that is well documented would be very, very helpful. My preferred naming would be something along the lines of 'inode_is_dirty_lazytime_only()'.

The name makes sense to me. I'm not sure it's likely that there will be new types of dirtiness --- as near I can tell the I_DIRTY_TIME was the first time there has been any changes in a _really_ long time, but I agree that how the flags interact (even before we added I_DIRTY_TIME) involved no small amount of black magic, and it's the kind of thing that requires deep meditation before trying to make any changes, and then immediately slips out of one's L1 cache very shortly afterwards. :-)

- Ted

_______________________________________________
Linux-f2fs-devel mailing list
[email protected]
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel

Jan Kara

maintainer Reviewed-by on v2 11/12

2021-01-11 15:11 UTC #33 lore raw

Re: [PATCH v2 11/12] ext4: simplify i_state checks in __ext4_update_other_inode_time()

Quoted 7 lines · Eric Biggers

From: Eric Biggers <[email protected]>
Since I_DIRTY_TIME and I_DIRTY_INODE are mutually exclusive in i_state, there's no need to check for I_DIRTY_TIME && !I_DIRTY_INODE. Just check for I_DIRTY_TIME.
Signed-off-by Eric Biggers <[email protected]>

Looks good to me. Feel free to add:

Honza

Quoted 29 lines

---
 fs/ext4/inode.c | 8 +++-----
 1 file changed, 3 insertions(+), 5 deletions(-)

fs/ext4/inode.c +3 -5

index 4cc6c7834312f..00bca5c18eb65 100644
@@ -4962,14 +4962,12 @@ static void __ext4_update_other_inode_time(struct super_block *sb,
 		return;
 
 	if ((inode->i_state & (I_FREEING | I_WILL_FREE | I_NEW |
-			       I_DIRTY_INODE)) ||
-	    ((inode->i_state & I_DIRTY_TIME) == 0))
+			       I_DIRTY_TIME)) != I_DIRTY_TIME)
 		return;
 
 	spin_lock(&inode->i_lock);
-	if (((inode->i_state & (I_FREEING | I_WILL_FREE | I_NEW |
-				I_DIRTY_INODE)) == 0) &&
-	    (inode->i_state & I_DIRTY_TIME)) {
+	if ((inode->i_state & (I_FREEING | I_WILL_FREE | I_NEW |
+			       I_DIRTY_TIME)) == I_DIRTY_TIME) {
 		struct ext4_inode_info	*ei = EXT4_I(inode);
 
 		inode->i_state &= ~I_DIRTY_TIME;

Signature · 1 line

2.30.0

Signature · 8 lines

Jan Kara <[email protected]>
SUSE Labs, CR


_______________________________________________
Linux-f2fs-devel mailing list
[email protected]
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel

Eric Biggers

author

2021-01-09 08:00 UTC #34 lore raw

PATCH v2 12/12 xfs: remove a stale comment from xfs_file_aio_write_checks()

From: Eric Biggers <[email protected]>

The comment in xfs_file_aio_write_checks() about calling file_modified() after dropping the ilock doesn't make sense, because the code that unconditionally acquires and drops the ilock was removed by commit 467f78992a07 ("xfs: reduce ilock hold times in xfs_file_aio_write_checks").

Remove this outdated comment.

---
 fs/xfs/xfs_file.c | 6 ------
 1 file changed, 6 deletions(-)

fs/xfs/xfs_file.c +0 -6

index 5b0f93f738372..4927c6653f15d 100644
@@ -389,12 +389,6 @@ xfs_file_aio_write_checks(
 	} else
 		spin_unlock(&ip->i_flags_lock);
 
-	/*
-	 * Updating the timestamps will grab the ilock again from
-	 * xfs_fs_dirty_inode, so we have to call it after dropping the
-	 * lock above.  Eventually we should look into a way to avoid
-	 * the pointless lock roundtrip.
-	 */
 	return file_modified(file);
 }

Signature · 8 lines

2.30.0



_______________________________________________
Linux-f2fs-devel mailing list
[email protected]
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel

Darrick J. Wong

Reviewed-by on v2 12/12

2021-01-12 17:31 UTC #35 lore raw

Re: [PATCH v2 12/12] xfs: remove a stale comment from xfs_file_aio_write_checks()

Quoted 8 lines · Eric Biggers

From: Eric Biggers <[email protected]>
The comment in xfs_file_aio_write_checks() about calling file_modified() after dropping the ilock doesn't make sense, because the code that unconditionally acquires and drops the ilock was removed by commit 467f78992a07 ("xfs: reduce ilock hold times in xfs_file_aio_write_checks").

Quoted 4 lines

Remove this outdated comment.
Reviewed-by Christoph Hellwig <[email protected]>
Signed-off-by Eric Biggers <[email protected]>

Yep, thanks for the update. :)

Reviewed-by Darrick J. Wong <[email protected]>

--D

Quoted 24 lines

---
 fs/xfs/xfs_file.c | 6 ------
 1 file changed, 6 deletions(-)

fs/xfs/xfs_file.c +0 -6

index 5b0f93f738372..4927c6653f15d 100644
@@ -389,12 +389,6 @@ xfs_file_aio_write_checks(
 	} else
 		spin_unlock(&ip->i_flags_lock);
 
-	/*
-	 * Updating the timestamps will grab the ilock again from
-	 * xfs_fs_dirty_inode, so we have to call it after dropping the
-	 * lock above.  Eventually we should look into a way to avoid
-	 * the pointless lock roundtrip.
-	 */
 	return file_modified(file);
 }

Signature · 1 line

2.30.0

_______________________________________________
Linux-f2fs-devel mailing list
[email protected]
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel

Jan Kara

maintainer

2021-01-11 15:15 UTC #36 lore raw