mbox series

[v2,00/12] lazytime fix and cleanups

Message ID 20210109075903.208222-1-ebiggers@kernel.org
Headers show
Series lazytime fix and cleanups | expand

Message

Eric Biggers Jan. 9, 2021, 7:58 a.m. UTC 
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

Comments

Jan Kara Jan. 11, 2021, 3:15 p.m. UTC | #1 
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
>
Eric Biggers Jan. 11, 2021, 8:44 p.m. UTC | #2 
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:
> > 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
Theodore Ts'o Feb. 3, 2021, 5:11 a.m. UTC | #3 
On Mon, Jan 11, 2021 at 12:44:35PM -0800, Eric Biggers wrote:
> > 
> > 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
Eric Biggers Feb. 3, 2021, 5:22 a.m. UTC | #4 
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:
> > > 
> > > 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

I already sent out v3 of this series several weeks ago
(https://lkml.kernel.org/r/20210112190253.64307-1-ebiggers@kernel.org),
and Jan applied it already.

- Eric
Theodore Ts'o Feb. 3, 2021, 3:49 p.m. UTC | #5 
On Tue, Feb 02, 2021 at 09:22:16PM -0800, Eric Biggers wrote:
> 
> I already sent out v3 of this series several weeks ago
> (https://lkml.kernel.org/r/20210112190253.64307-1-ebiggers@kernel.org),
> and Jan applied it already.

Great, thanks.  Sorry, I missed it.

       		       	 	- Ted