From: Eric Biggers <ebiggers@kernel.org> Date: 2021-01-09 08:00:40
Hello,
Patch 1 fixes a bug in how __writeback_single_inode() handles lazytime
expirations. I originally reported this last year
(https://lore.kernel.org/r/20200306004555.GB225345@gmail.com) because it
causes the FS_IOC_REMOVE_ENCRYPTION_KEY ioctl to not work properly, as
the bug causes inodes to remain dirty after a sync.
It also turns out that lazytime on XFS is partially broken because it
doesn't actually write timestamps to disk after a sync() or after
dirtytime_expire_interval. This is fixed by the same fix.
This supersedes previously proposed fixes, including
https://lore.kernel.org/r/20200307020043.60118-1-tytso@mit.edu and
https://lore.kernel.org/r/20200325122825.1086872-3-hch@lst.de from last
year (which had some issues and didn't fix the XFS bug), and v1 of this
patchset which took a different approach
(https://lore.kernel.org/r/20210105005452.92521-1-ebiggers@kernel.org).
Patches 2-12 then clean up various things related to lazytime and
writeback, such as clarifying the semantics of ->dirty_inode() and the
inode dirty flags, and improving comments. Most of these patches could
be applied independently if needed.
This patchset applies to v5.11-rc2.
Changed since v1:
- Switched to the fix suggested by Jan Kara, and dropped the
patches which introduced ->lazytime_expired().
- Fixed bugs in the fat and ext4 patches.
- Added patch "fs: improve comments for writeback_single_inode()".
- Reordered the patches a bit.
- Added Reviewed-by's.
Eric Biggers (12):
fs: fix lazytime expiration handling in __writeback_single_inode()
fs: correctly document the inode dirty flags
fs: only specify I_DIRTY_TIME when needed in generic_update_time()
fat: only specify I_DIRTY_TIME when needed in fat_update_time()
fs: don't call ->dirty_inode for lazytime timestamp updates
fs: pass only I_DIRTY_INODE flags to ->dirty_inode
fs: clean up __mark_inode_dirty() a bit
fs: drop redundant check from __writeback_single_inode()
fs: improve comments for writeback_single_inode()
gfs2: don't worry about I_DIRTY_TIME in gfs2_fsync()
ext4: simplify i_state checks in __ext4_update_other_inode_time()
xfs: remove a stale comment from xfs_file_aio_write_checks()
Documentation/filesystems/vfs.rst | 5 +-
fs/ext4/inode.c | 20 +----
fs/f2fs/super.c | 3 -
fs/fat/misc.c | 23 +++---
fs/fs-writeback.c | 132 +++++++++++++++++-------------
fs/gfs2/file.c | 4 +-
fs/gfs2/super.c | 2 -
fs/inode.c | 38 +++++----
fs/xfs/xfs_file.c | 6 --
include/linux/fs.h | 18 ++--
10 files changed, 132 insertions(+), 119 deletions(-)
base-commit: e71ba9452f0b5b2e8dc8aa5445198cd9214a6a62
--
2.30.0
From: Eric Biggers <ebiggers@kernel.org> Date: 2021-01-09 08:00:41
From: Eric Biggers <redacted>
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.
Reviewed-by: Christoph Hellwig <hch@lst.de>
Signed-off-by: Eric Biggers <redacted>
---
fs/inode.c | 38 ++++++++++++++++++++------------------
1 file changed, 20 insertions(+), 18 deletions(-)
From: Eric Biggers <ebiggers@kernel.org> Date: 2021-01-09 08:00:41
From: Eric Biggers <redacted>
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 <hch@lst.de>
Signed-off-by: Eric Biggers <redacted>
---
include/linux/fs.h | 18 +++++++++++++-----
1 file changed, 13 insertions(+), 5 deletions(-)
From: Eric Biggers <ebiggers@kernel.org> Date: 2021-01-09 08:00:41
From: Eric Biggers <redacted>
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/20200306004555.GB225345@gmail.com), 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/20210105005818.92978-1-ebiggers@kernel.org).
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.
Fixes: 0ae45f63d4ef ("vfs: add support for a lazytime mount option")
Cc: stable@vger.kernel.org
Depends-on: 5afced3bf281 ("writeback: Avoid skipping inode writeback")
Suggested-by: Jan Kara <jack@suse.cz>
Signed-off-by: Eric Biggers <redacted>
---
fs/fs-writeback.c | 24 +++++++++++++-----------
1 file changed, 13 insertions(+), 11 deletions(-)
@@ -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){interr=write_inode(inode,wbc);
From: Eric Biggers <ebiggers@kernel.org> Date: 2021-01-09 08:00:42
From: Eric Biggers <redacted>
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 <redacted>
---
fs/fat/misc.c | 23 ++++++++++++-----------
1 file changed, 12 insertions(+), 11 deletions(-)
From: Eric Biggers <ebiggers@kernel.org> Date: 2021-01-09 08:01:21
From: Eric Biggers <redacted>
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.
Reviewed-by: Christoph Hellwig <hch@lst.de>
Signed-off-by: Eric Biggers <redacted>
---
fs/gfs2/file.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
From: Eric Biggers <ebiggers@kernel.org> Date: 2021-01-09 08:01:22
From: Eric Biggers <redacted>
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 <redacted>
---
fs/ext4/inode.c | 8 +++-----
1 file changed, 3 insertions(+), 5 deletions(-)
From: Eric Biggers <ebiggers@kernel.org> Date: 2021-01-09 08:01:22
From: Eric Biggers <redacted>
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 <redacted>
---
fs/fs-writeback.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
From: Eric Biggers <ebiggers@kernel.org> Date: 2021-01-09 08:01:22
From: Eric Biggers <redacted>
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.
Reviewed-by: Christoph Hellwig <hch@lst.de>
Signed-off-by: Eric Biggers <redacted>
---
fs/xfs/xfs_file.c | 6 ------
1 file changed, 6 deletions(-)
From: Eric Biggers <ebiggers@kernel.org> Date: 2021-01-09 08:01:22
From: Eric Biggers <redacted>
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 <hch@lst.de>
Signed-off-by: Eric Biggers <redacted>
---
fs/fs-writeback.c | 49 +++++++++++++++++++++++++++++------------------
1 file changed, 30 insertions(+), 19 deletions(-)
From: Eric Biggers <ebiggers@kernel.org> Date: 2021-01-09 08:01:23
From: Eric Biggers <redacted>
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 <redacted>
---
fs/fs-writeback.c | 57 +++++++++++++++++++++++++++--------------------
1 file changed, 33 insertions(+), 24 deletions(-)
From: Eric Biggers <ebiggers@kernel.org> Date: 2021-01-09 08:01:23
From: Eric Biggers <redacted>
->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.
Reviewed-by: Christoph Hellwig <hch@lst.de>
Signed-off-by: Eric Biggers <redacted>
---
Documentation/filesystems/vfs.rst | 5 ++++-
fs/fs-writeback.c | 2 +-
2 files changed, 5 insertions(+), 2 deletions(-)
@@ -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
From: Eric Biggers <ebiggers@kernel.org> Date: 2021-01-09 08:01:23
From: Eric Biggers <redacted>
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/20200325122825.1086872-4-hch@lst.de
Reviewed-by: Christoph Hellwig <hch@lst.de>
Signed-off-by: Eric Biggers <redacted>
---
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(-)
From: Christoph Hellwig <hch@lst.de> Date: 2021-01-11 10:53:32
On Fri, Jan 08, 2021 at 11:58:59PM -0800, Eric Biggers wrote:
From: Eric Biggers <redacted>
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 <redacted>
Looks good,
Reviewed-by: Christoph Hellwig <hch@lst.de>
From: Jan Kara <jack@suse.cz> Date: 2021-01-11 14:46:59
On Fri 08-01-21 23:58:52, Eric Biggers wrote:
From: Eric Biggers <redacted>
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/20200306004555.GB225345@gmail.com), 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/20210105005818.92978-1-ebiggers@kernel.org).
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.
Fixes: 0ae45f63d4ef ("vfs: add support for a lazytime mount option")
Cc: stable@vger.kernel.org
Depends-on: 5afced3bf281 ("writeback: Avoid skipping inode writeback")
Suggested-by: Jan Kara <jack@suse.cz>
Signed-off-by: Eric Biggers <redacted>
Thanks for writing this fix! It looks good to me. You can add:
Reviewed-by: Jan Kara <jack@suse.cz>
Honza
@@ -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){interr=write_inode(inode,wbc);
From: Jan Kara <jack@suse.cz> Date: 2021-01-11 14:49:40
On Fri 08-01-21 23:58:53, Eric Biggers wrote:
From: Eric Biggers <redacted>
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 <hch@lst.de>
Signed-off-by: Eric Biggers <redacted>
Looks good to me. You can add:
Reviewed-by: Jan Kara <jack@suse.cz>
Honza
From: Jan Kara <jack@suse.cz> Date: 2021-01-11 14:51:44
On Fri 08-01-21 23:58:54, Eric Biggers wrote:
From: Eric Biggers <redacted>
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.
Reviewed-by: Christoph Hellwig <hch@lst.de>
Signed-off-by: Eric Biggers <redacted>
Looks good to me. You can add:
Reviewed-by: Jan Kara <jack@suse.cz>
Honza
From: Jan Kara <jack@suse.cz> Date: 2021-01-11 14:53:39
On Fri 08-01-21 23:58:55, Eric Biggers wrote:
From: Eric Biggers <redacted>
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 <redacted>
Looks good to me. You can add:
Reviewed-by: Jan Kara <jack@suse.cz>
Honza
From: Jan Kara <jack@suse.cz> Date: 2021-01-11 14:56:04
On Fri 08-01-21 23:58:56, Eric Biggers wrote:
From: Eric Biggers <redacted>
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/20200325122825.1086872-4-hch@lst.de
Reviewed-by: Christoph Hellwig <hch@lst.de>
Signed-off-by: Eric Biggers <redacted>
Looks good to me. You can add:
Reviewed-by: Jan Kara <jack@suse.cz>
Honza
From: Jan Kara <jack@suse.cz> Date: 2021-01-11 14:57:32
On Fri 08-01-21 23:58:57, Eric Biggers wrote:
From: Eric Biggers <redacted>
->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.
Reviewed-by: Christoph Hellwig <hch@lst.de>
Signed-off-by: Eric Biggers <redacted>
Looks good to me. Feel free to add:
Reviewed-by: Jan Kara <jack@suse.cz>
Honza
@@ -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
From: Jan Kara <jack@suse.cz> Date: 2021-01-11 15:00:27
On Fri 08-01-21 23:58:58, Eric Biggers wrote:
From: Eric Biggers <redacted>
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 <hch@lst.de>
Signed-off-by: Eric Biggers <redacted>
Looks good. Feel free to add:
Reviewed-by: Jan Kara <jack@suse.cz>
Honza
From: Jan Kara <jack@suse.cz> Date: 2021-01-11 15:01:18
On Fri 08-01-21 23:58:59, Eric Biggers wrote:
From: Eric Biggers <redacted>
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 <redacted>
Looks good to me. Feel free to add:
Reviewed-by: Jan Kara <jack@suse.cz>
Honza
From: Jan Kara <jack@suse.cz> Date: 2021-01-11 15:06:18
On Fri 08-01-21 23:59:00, Eric Biggers wrote:
From: Eric Biggers <redacted>
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 <redacted>
Yeah, looks more comprehensible :). Thanks for the cleanup. Feel free to
add:
Reviewed-by: Jan Kara <jack@suse.cz>
Honza
From: Jan Kara <jack@suse.cz> Date: 2021-01-11 15:07:02
On Fri 08-01-21 23:59:01, Eric Biggers wrote:
From: Eric Biggers <redacted>
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.
Reviewed-by: Christoph Hellwig <hch@lst.de>
Signed-off-by: Eric Biggers <redacted>
Looks good to me. Feel free to add:
Reviewed-by: Jan Kara <jack@suse.cz>
Honza
From: Jan Kara <jack@suse.cz> Date: 2021-01-11 15:12:18
On Fri 08-01-21 23:59:02, Eric Biggers wrote:
From: Eric Biggers <redacted>
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 <redacted>
Looks good to me. Feel free to add:
Reviewed-by: Jan Kara <jack@suse.cz>
Honza
From: Jan Kara <jack@suse.cz> Date: 2021-01-11 15:16:03
Hi!
On Fri 08-01-21 23:58:51, Eric Biggers wrote:
Hello,
Patch 1 fixes a bug in how __writeback_single_inode() handles lazytime
expirations. I originally reported this last year
(https://lore.kernel.org/r/20200306004555.GB225345@gmail.com) because it
causes the FS_IOC_REMOVE_ENCRYPTION_KEY ioctl to not work properly, as
the bug causes inodes to remain dirty after a sync.
It also turns out that lazytime on XFS is partially broken because it
doesn't actually write timestamps to disk after a sync() or after
dirtytime_expire_interval. This is fixed by the same fix.
This supersedes previously proposed fixes, including
https://lore.kernel.org/r/20200307020043.60118-1-tytso@mit.edu and
https://lore.kernel.org/r/20200325122825.1086872-3-hch@lst.de from last
year (which had some issues and didn't fix the XFS bug), and v1 of this
patchset which took a different approach
(https://lore.kernel.org/r/20210105005452.92521-1-ebiggers@kernel.org).
Patches 2-12 then clean up various things related to lazytime and
writeback, such as clarifying the semantics of ->dirty_inode() and the
inode dirty flags, and improving comments. Most of these patches could
be applied independently if needed.
This patchset applies to v5.11-rc2.
The series look good to me. How do you plan to merge it (after resolving
Christoph's remarks)? I guess either Ted can take it through the ext4 tree
or I can take it through my tree...
Honza
Changed since v1:
- Switched to the fix suggested by Jan Kara, and dropped the
patches which introduced ->lazytime_expired().
- Fixed bugs in the fat and ext4 patches.
- Added patch "fs: improve comments for writeback_single_inode()".
- Reordered the patches a bit.
- Added Reviewed-by's.
Eric Biggers (12):
fs: fix lazytime expiration handling in __writeback_single_inode()
fs: correctly document the inode dirty flags
fs: only specify I_DIRTY_TIME when needed in generic_update_time()
fat: only specify I_DIRTY_TIME when needed in fat_update_time()
fs: don't call ->dirty_inode for lazytime timestamp updates
fs: pass only I_DIRTY_INODE flags to ->dirty_inode
fs: clean up __mark_inode_dirty() a bit
fs: drop redundant check from __writeback_single_inode()
fs: improve comments for writeback_single_inode()
gfs2: don't worry about I_DIRTY_TIME in gfs2_fsync()
ext4: simplify i_state checks in __ext4_update_other_inode_time()
xfs: remove a stale comment from xfs_file_aio_write_checks()
Documentation/filesystems/vfs.rst | 5 +-
fs/ext4/inode.c | 20 +----
fs/f2fs/super.c | 3 -
fs/fat/misc.c | 23 +++---
fs/fs-writeback.c | 132 +++++++++++++++++-------------
fs/gfs2/file.c | 4 +-
fs/gfs2/super.c | 2 -
fs/inode.c | 38 +++++----
fs/xfs/xfs_file.c | 6 --
include/linux/fs.h | 18 ++--
10 files changed, 132 insertions(+), 119 deletions(-)
base-commit: e71ba9452f0b5b2e8dc8aa5445198cd9214a6a62
--
2.30.0
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
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
From: Eric Biggers <ebiggers@kernel.org> Date: 2021-01-11 20:45:41
On Mon, Jan 11, 2021 at 04:15:17PM +0100, Jan Kara wrote:
Hi!
On Fri 08-01-21 23:58:51, Eric Biggers wrote:
quoted
Hello,
Patch 1 fixes a bug in how __writeback_single_inode() handles lazytime
expirations. I originally reported this last year
(https://lore.kernel.org/r/20200306004555.GB225345@gmail.com) because it
causes the FS_IOC_REMOVE_ENCRYPTION_KEY ioctl to not work properly, as
the bug causes inodes to remain dirty after a sync.
It also turns out that lazytime on XFS is partially broken because it
doesn't actually write timestamps to disk after a sync() or after
dirtytime_expire_interval. This is fixed by the same fix.
This supersedes previously proposed fixes, including
https://lore.kernel.org/r/20200307020043.60118-1-tytso@mit.edu and
https://lore.kernel.org/r/20200325122825.1086872-3-hch@lst.de from last
year (which had some issues and didn't fix the XFS bug), and v1 of this
patchset which took a different approach
(https://lore.kernel.org/r/20210105005452.92521-1-ebiggers@kernel.org).
Patches 2-12 then clean up various things related to lazytime and
writeback, such as clarifying the semantics of ->dirty_inode() and the
inode dirty flags, and improving comments. Most of these patches could
be applied independently if needed.
This patchset applies to v5.11-rc2.
The series look good to me. How do you plan to merge it (after resolving
Christoph's remarks)? I guess either Ted can take it through the ext4 tree
or I can take it through my tree...
I think taking it through your tree would be best, unless Al or Ted wants to
take it.
I'll probably separate out
"xfs: remove a stale comment from xfs_file_aio_write_checks()",
since it isn't really related anymore and could go in through the XFS tree.
- Eric
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.
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.
--
Dave Chinner
david@fromorbit.com
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?
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:
Reviewed-by: Christoph Hellwig <hch@lst.de>
From: Christoph Hellwig <hch@lst.de> Date: 2021-01-12 13:26:37
On Mon, Jan 11, 2021 at 12:23:40PM -0800, Eric Biggers wrote:
quoted
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?
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()'.
From: "Darrick J. Wong" <djwong@kernel.org> Date: 2021-01-12 17:32:17
On Fri, Jan 08, 2021 at 11:59:03PM -0800, Eric Biggers wrote:
From: Eric Biggers <redacted>
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.
Reviewed-by: Christoph Hellwig <hch@lst.de>
Signed-off-by: Eric Biggers <redacted>
Yep, thanks for the update. :)
Reviewed-by: Darrick J. Wong <redacted>
--D
On Mon, Jan 11, 2021 at 12:44:35PM -0800, Eric Biggers wrote:
quoted
The series look good to me. How do you plan to merge it (after resolving
Christoph's remarks)? I guess either Ted can take it through the ext4 tree
or I can take it through my tree...
I think taking it through your tree would be best, unless Al or Ted wants to
take it.
I'm happy to take it through the ext4 tree. Are you planning on
issuing a newer version of this patch series to resolve Christoph's
comments?
- Ted
On Tue, Jan 12, 2021 at 02:25:21PM +0100, Christoph Hellwig wrote:
quoted
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?
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
From: Eric Biggers <ebiggers@kernel.org> Date: 2021-02-03 05:23:53
On Wed, Feb 03, 2021 at 12:11:52AM -0500, Theodore Ts'o wrote:
On Mon, Jan 11, 2021 at 12:44:35PM -0800, Eric Biggers wrote:
quoted
quoted
The series look good to me. How do you plan to merge it (after resolving
Christoph's remarks)? I guess either Ted can take it through the ext4 tree
or I can take it through my tree...
I think taking it through your tree would be best, unless Al or Ted wants to
take it.
I'm happy to take it through the ext4 tree. Are you planning on
issuing a newer version of this patch series to resolve Christoph's
comments?
- Ted