[PATCH 0/11] ext4: Cleanup read-only and fs aborted checks

[PATCH 0/11] ext4: Cleanup read-only and fs aborted checks

[Jan Kara]

Hello,

This series arised from me trying to fix races when the ext4 filesystem gets
remounted read-write and users can race in writes before quota subsystem is
prepared to take them. This particular problem got fixed in VFS in the end
but the cleanups are still good in my opinion so I'm submitting them. They
get rid of EXT4_MF_ABORTED flag and cleanup some sb_rdonly() checks.

								Honza

[PATCH 01/11] ext4: Remove pointless sb_rdonly() checks from freezing code

[Jan Kara]

ext4_freeze() and ext4_unfreeze() checks for sb_rdonly(). However this
check is pointless as VFS already checks for read-only filesystem before
calling filesystem specific methods. Remove the pointless checks.

Signed-off-by: Jan Kara <jack@suse.cz>
---
 fs/ext4/super.c | 9 ++-------
 1 file changed, 2 insertions(+), 7 deletions(-)

diff --git a/fs/ext4/super.c b/fs/ext4/super.c
index 05fcecc36244..f24a7919a328 100644
--- a/fs/ext4/super.c
+++ b/fs/ext4/super.c
@@ -6310,12 +6310,7 @@ static int ext4_sync_fs(struct super_block *sb, int wait)
 static int ext4_freeze(struct super_block *sb)
 {
 	int error = 0;
-	journal_t *journal;
-
-	if (sb_rdonly(sb))
-		return 0;
-
-	journal = EXT4_SB(sb)->s_journal;
+	journal_t *journal = EXT4_SB(sb)->s_journal;
 
 	if (journal) {
 		/* Now we set up the journal barrier. */
@@ -6349,7 +6344,7 @@ static int ext4_freeze(struct super_block *sb)
  */
 static int ext4_unfreeze(struct super_block *sb)
 {
-	if (sb_rdonly(sb) || ext4_forced_shutdown(EXT4_SB(sb)))
+	if (ext4_forced_shutdown(EXT4_SB(sb)))
 		return 0;
 
 	if (EXT4_SB(sb)->s_journal) {
-- 
2.35.3

[PATCH 02/11] ext4: Use sb_rdonly() helper for checking read-only flag

[Jan Kara]

sb_rdonly() helper instead of directly checking sb->s_flags.

Signed-off-by: Jan Kara <jack@suse.cz>
---
 fs/ext4/super.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/fs/ext4/super.c b/fs/ext4/super.c
index f24a7919a328..8c843ceb3cbb 100644
--- a/fs/ext4/super.c
+++ b/fs/ext4/super.c
@@ -6047,7 +6047,7 @@ static void ext4_update_super(struct super_block *sb)
 	 * the clock is set in the future, and this will cause e2fsck
 	 * to complain and force a full file system check.
 	 */
-	if (!(sb->s_flags & SB_RDONLY))
+	if (!sb_rdonly(sb))
 		ext4_update_tstamp(es, s_wtime);
 	es->s_kbytes_written =
 		cpu_to_le64(sbi->s_kbytes_written +
@@ -6638,7 +6638,7 @@ static int __ext4_remount(struct fs_context *fc, struct super_block *sb)
 	 * If there was a failing r/w to ro transition, we may need to
 	 * re-enable quota
 	 */
-	if ((sb->s_flags & SB_RDONLY) && !(old_sb_flags & SB_RDONLY) &&
+	if (sb_rdonly(sb) && !(old_sb_flags & SB_RDONLY) &&
 	    sb_any_quota_suspended(sb))
 		dquot_resume(sb, -1);
 	sb->s_flags = old_sb_flags;
-- 
2.35.3

[PATCH 03/11] ext4: Make ext4_forced_shutdown() take struct super_block

[Jan Kara]

Currently ext4_forced_shutdown() takes struct ext4_sb_info but most
callers need to get it from struct super_block anyway. So just pass in
struct super_block to save all callers from some boilerplate code. No
functional changes.

Signed-off-by: Jan Kara <jack@suse.cz>
---
 fs/ext4/ext4.h      |  4 ++--
 fs/ext4/ext4_jbd2.c |  2 +-
 fs/ext4/file.c      | 11 +++++------
 fs/ext4/fsync.c     |  2 +-
 fs/ext4/ialloc.c    |  2 +-
 fs/ext4/inline.c    |  2 +-
 fs/ext4/inode.c     | 24 ++++++++++++------------
 fs/ext4/ioctl.c     |  2 +-
 fs/ext4/namei.c     |  8 ++++----
 fs/ext4/page-io.c   |  2 +-
 fs/ext4/super.c     | 14 +++++++-------
 fs/ext4/xattr.c     |  2 +-
 12 files changed, 37 insertions(+), 38 deletions(-)

diff --git a/fs/ext4/ext4.h b/fs/ext4/ext4.h
index 8104a21b001a..f0b6aa79dcc4 100644
--- a/fs/ext4/ext4.h
+++ b/fs/ext4/ext4.h
@@ -2163,9 +2163,9 @@ extern int ext4_feature_set_ok(struct super_block *sb, int readonly);
 #define EXT4_FLAGS_SHUTDOWN	1
 #define EXT4_FLAGS_BDEV_IS_DAX	2
 
-static inline int ext4_forced_shutdown(struct ext4_sb_info *sbi)
+static inline int ext4_forced_shutdown(struct super_block *sb)
 {
-	return test_bit(EXT4_FLAGS_SHUTDOWN, &sbi->s_ext4_flags);
+	return test_bit(EXT4_FLAGS_SHUTDOWN, &EXT4_SB(sb)->s_ext4_flags);
 }
 
 /*
diff --git a/fs/ext4/ext4_jbd2.c b/fs/ext4/ext4_jbd2.c
index 77f318ec8abb..b72a22a57d20 100644
--- a/fs/ext4/ext4_jbd2.c
+++ b/fs/ext4/ext4_jbd2.c
@@ -67,7 +67,7 @@ static int ext4_journal_check_start(struct super_block *sb)
 
 	might_sleep();
 
-	if (unlikely(ext4_forced_shutdown(EXT4_SB(sb))))
+	if (unlikely(ext4_forced_shutdown(sb)))
 		return -EIO;
 
 	if (sb_rdonly(sb))
diff --git a/fs/ext4/file.c b/fs/ext4/file.c
index d101b3b0c7da..2a8a780c856b 100644
--- a/fs/ext4/file.c
+++ b/fs/ext4/file.c
@@ -131,7 +131,7 @@ static ssize_t ext4_file_read_iter(struct kiocb *iocb, struct iov_iter *to)
 {
 	struct inode *inode = file_inode(iocb->ki_filp);
 
-	if (unlikely(ext4_forced_shutdown(EXT4_SB(inode->i_sb))))
+	if (unlikely(ext4_forced_shutdown(inode->i_sb)))
 		return -EIO;
 
 	if (!iov_iter_count(to))
@@ -697,7 +697,7 @@ ext4_file_write_iter(struct kiocb *iocb, struct iov_iter *from)
 {
 	struct inode *inode = file_inode(iocb->ki_filp);
 
-	if (unlikely(ext4_forced_shutdown(EXT4_SB(inode->i_sb))))
+	if (unlikely(ext4_forced_shutdown(inode->i_sb)))
 		return -EIO;
 
 #ifdef CONFIG_FS_DAX
@@ -795,10 +795,9 @@ static const struct vm_operations_struct ext4_file_vm_ops = {
 static int ext4_file_mmap(struct file *file, struct vm_area_struct *vma)
 {
 	struct inode *inode = file->f_mapping->host;
-	struct ext4_sb_info *sbi = EXT4_SB(inode->i_sb);
-	struct dax_device *dax_dev = sbi->s_daxdev;
+	struct dax_device *dax_dev = EXT4_SB(inode->i_sb)->s_daxdev;
 
-	if (unlikely(ext4_forced_shutdown(sbi)))
+	if (unlikely(ext4_forced_shutdown(inode->i_sb)))
 		return -EIO;
 
 	/*
@@ -874,7 +873,7 @@ static int ext4_file_open(struct inode *inode, struct file *filp)
 {
 	int ret;
 
-	if (unlikely(ext4_forced_shutdown(EXT4_SB(inode->i_sb))))
+	if (unlikely(ext4_forced_shutdown(inode->i_sb)))
 		return -EIO;
 
 	ret = ext4_sample_last_mounted(inode->i_sb, filp->f_path.mnt);
diff --git a/fs/ext4/fsync.c b/fs/ext4/fsync.c
index 2a143209aa0c..958bcaedcff6 100644
--- a/fs/ext4/fsync.c
+++ b/fs/ext4/fsync.c
@@ -140,7 +140,7 @@ int ext4_sync_file(struct file *file, loff_t start, loff_t end, int datasync)
 	struct inode *inode = file->f_mapping->host;
 	struct ext4_sb_info *sbi = EXT4_SB(inode->i_sb);
 
-	if (unlikely(ext4_forced_shutdown(sbi)))
+	if (unlikely(ext4_forced_shutdown(inode->i_sb)))
 		return -EIO;
 
 	ASSERT(ext4_journal_current_handle() == NULL);
diff --git a/fs/ext4/ialloc.c b/fs/ext4/ialloc.c
index 754f961cd9fd..060630c0b0ca 100644
--- a/fs/ext4/ialloc.c
+++ b/fs/ext4/ialloc.c
@@ -950,7 +950,7 @@ struct inode *__ext4_new_inode(struct mnt_idmap *idmap,
 	sb = dir->i_sb;
 	sbi = EXT4_SB(sb);
 
-	if (unlikely(ext4_forced_shutdown(sbi)))
+	if (unlikely(ext4_forced_shutdown(sb)))
 		return ERR_PTR(-EIO);
 
 	ngroups = ext4_get_groups_count(sb);
diff --git a/fs/ext4/inline.c b/fs/ext4/inline.c
index 5854bd5a3352..2aa3ecbdbb9f 100644
--- a/fs/ext4/inline.c
+++ b/fs/ext4/inline.c
@@ -228,7 +228,7 @@ static void ext4_write_inline_data(struct inode *inode, struct ext4_iloc *iloc,
 	struct ext4_inode *raw_inode;
 	int cp_len = 0;
 
-	if (unlikely(ext4_forced_shutdown(EXT4_SB(inode->i_sb))))
+	if (unlikely(ext4_forced_shutdown(inode->i_sb)))
 		return;
 
 	BUG_ON(!EXT4_I(inode)->i_inline_off);
diff --git a/fs/ext4/inode.c b/fs/ext4/inode.c
index 02de439bf1f0..da3aaaea5f1c 100644
--- a/fs/ext4/inode.c
+++ b/fs/ext4/inode.c
@@ -1130,7 +1130,7 @@ static int ext4_write_begin(struct file *file, struct address_space *mapping,
 	pgoff_t index;
 	unsigned from, to;
 
-	if (unlikely(ext4_forced_shutdown(EXT4_SB(inode->i_sb))))
+	if (unlikely(ext4_forced_shutdown(inode->i_sb)))
 		return -EIO;
 
 	trace_ext4_write_begin(inode, pos, len);
@@ -2237,7 +2237,7 @@ static int mpage_map_and_submit_extent(handle_t *handle,
 		if (err < 0) {
 			struct super_block *sb = inode->i_sb;
 
-			if (ext4_forced_shutdown(EXT4_SB(sb)) ||
+			if (ext4_forced_shutdown(sb) ||
 			    ext4_test_mount_flag(sb, EXT4_MF_FS_ABORTED))
 				goto invalidate_dirty_pages;
 			/*
@@ -2566,7 +2566,7 @@ static int ext4_do_writepages(struct mpage_da_data *mpd)
 	 * *never* be called, so if that ever happens, we would want
 	 * the stack trace.
 	 */
-	if (unlikely(ext4_forced_shutdown(EXT4_SB(mapping->host->i_sb)) ||
+	if (unlikely(ext4_forced_shutdown(mapping->host->i_sb) ||
 		     ext4_test_mount_flag(inode->i_sb, EXT4_MF_FS_ABORTED))) {
 		ret = -EROFS;
 		goto out_writepages;
@@ -2785,7 +2785,7 @@ static int ext4_writepages(struct address_space *mapping,
 	int ret;
 	int alloc_ctx;
 
-	if (unlikely(ext4_forced_shutdown(EXT4_SB(sb))))
+	if (unlikely(ext4_forced_shutdown(sb)))
 		return -EIO;
 
 	alloc_ctx = ext4_writepages_down_read(sb);
@@ -2824,16 +2824,16 @@ static int ext4_dax_writepages(struct address_space *mapping,
 	int ret;
 	long nr_to_write = wbc->nr_to_write;
 	struct inode *inode = mapping->host;
-	struct ext4_sb_info *sbi = EXT4_SB(mapping->host->i_sb);
 	int alloc_ctx;
 
-	if (unlikely(ext4_forced_shutdown(EXT4_SB(inode->i_sb))))
+	if (unlikely(ext4_forced_shutdown(inode->i_sb)))
 		return -EIO;
 
 	alloc_ctx = ext4_writepages_down_read(inode->i_sb);
 	trace_ext4_writepages(inode, wbc);
 
-	ret = dax_writeback_mapping_range(mapping, sbi->s_daxdev, wbc);
+	ret = dax_writeback_mapping_range(mapping,
+					  EXT4_SB(inode->i_sb)->s_daxdev, wbc);
 	trace_ext4_writepages_result(inode, wbc, ret,
 				     nr_to_write - wbc->nr_to_write);
 	ext4_writepages_up_read(inode->i_sb, alloc_ctx);
@@ -2883,7 +2883,7 @@ static int ext4_da_write_begin(struct file *file, struct address_space *mapping,
 	pgoff_t index;
 	struct inode *inode = mapping->host;
 
-	if (unlikely(ext4_forced_shutdown(EXT4_SB(inode->i_sb))))
+	if (unlikely(ext4_forced_shutdown(inode->i_sb)))
 		return -EIO;
 
 	index = pos >> PAGE_SHIFT;
@@ -5161,7 +5161,7 @@ int ext4_write_inode(struct inode *inode, struct writeback_control *wbc)
 	    sb_rdonly(inode->i_sb))
 		return 0;
 
-	if (unlikely(ext4_forced_shutdown(EXT4_SB(inode->i_sb))))
+	if (unlikely(ext4_forced_shutdown(inode->i_sb)))
 		return -EIO;
 
 	if (EXT4_SB(inode->i_sb)->s_journal) {
@@ -5281,7 +5281,7 @@ int ext4_setattr(struct mnt_idmap *idmap, struct dentry *dentry,
 	const unsigned int ia_valid = attr->ia_valid;
 	bool inc_ivers = true;
 
-	if (unlikely(ext4_forced_shutdown(EXT4_SB(inode->i_sb))))
+	if (unlikely(ext4_forced_shutdown(inode->i_sb)))
 		return -EIO;
 
 	if (unlikely(IS_IMMUTABLE(inode)))
@@ -5702,7 +5702,7 @@ int ext4_mark_iloc_dirty(handle_t *handle,
 {
 	int err = 0;
 
-	if (unlikely(ext4_forced_shutdown(EXT4_SB(inode->i_sb)))) {
+	if (unlikely(ext4_forced_shutdown(inode->i_sb))) {
 		put_bh(iloc->bh);
 		return -EIO;
 	}
@@ -5728,7 +5728,7 @@ ext4_reserve_inode_write(handle_t *handle, struct inode *inode,
 {
 	int err;
 
-	if (unlikely(ext4_forced_shutdown(EXT4_SB(inode->i_sb))))
+	if (unlikely(ext4_forced_shutdown(inode->i_sb)))
 		return -EIO;
 
 	err = ext4_get_inode_loc(inode, iloc);
diff --git a/fs/ext4/ioctl.c b/fs/ext4/ioctl.c
index f9a430152063..c07417986ed6 100644
--- a/fs/ext4/ioctl.c
+++ b/fs/ext4/ioctl.c
@@ -807,7 +807,7 @@ static int ext4_shutdown(struct super_block *sb, unsigned long arg)
 	if (flags > EXT4_GOING_FLAGS_NOLOGFLUSH)
 		return -EINVAL;
 
-	if (ext4_forced_shutdown(sbi))
+	if (ext4_forced_shutdown(sb))
 		return 0;
 
 	ext4_msg(sb, KERN_ALERT, "shut down requested (%d)", flags);
diff --git a/fs/ext4/namei.c b/fs/ext4/namei.c
index 45b579805c95..437d76e98c0a 100644
--- a/fs/ext4/namei.c
+++ b/fs/ext4/namei.c
@@ -3142,7 +3142,7 @@ static int ext4_rmdir(struct inode *dir, struct dentry *dentry)
 	struct ext4_dir_entry_2 *de;
 	handle_t *handle = NULL;
 
-	if (unlikely(ext4_forced_shutdown(EXT4_SB(dir->i_sb))))
+	if (unlikely(ext4_forced_shutdown(dir->i_sb)))
 		return -EIO;
 
 	/* Initialize quotas before so that eventual writes go in
@@ -3301,7 +3301,7 @@ static int ext4_unlink(struct inode *dir, struct dentry *dentry)
 {
 	int retval;
 
-	if (unlikely(ext4_forced_shutdown(EXT4_SB(dir->i_sb))))
+	if (unlikely(ext4_forced_shutdown(dir->i_sb)))
 		return -EIO;
 
 	trace_ext4_unlink_enter(dir, dentry);
@@ -3369,7 +3369,7 @@ static int ext4_symlink(struct mnt_idmap *idmap, struct inode *dir,
 	struct fscrypt_str disk_link;
 	int retries = 0;
 
-	if (unlikely(ext4_forced_shutdown(EXT4_SB(dir->i_sb))))
+	if (unlikely(ext4_forced_shutdown(dir->i_sb)))
 		return -EIO;
 
 	err = fscrypt_prepare_symlink(dir, symname, len, dir->i_sb->s_blocksize,
@@ -4202,7 +4202,7 @@ static int ext4_rename2(struct mnt_idmap *idmap,
 {
 	int err;
 
-	if (unlikely(ext4_forced_shutdown(EXT4_SB(old_dir->i_sb))))
+	if (unlikely(ext4_forced_shutdown(old_dir->i_sb)))
 		return -EIO;
 
 	if (flags & ~(RENAME_NOREPLACE | RENAME_EXCHANGE | RENAME_WHITEOUT))
diff --git a/fs/ext4/page-io.c b/fs/ext4/page-io.c
index 3621f29ec671..dfdd7e5cf038 100644
--- a/fs/ext4/page-io.c
+++ b/fs/ext4/page-io.c
@@ -184,7 +184,7 @@ static int ext4_end_io_end(ext4_io_end_t *io_end)
 
 	io_end->handle = NULL;	/* Following call will use up the handle */
 	ret = ext4_convert_unwritten_io_end_vec(handle, io_end);
-	if (ret < 0 && !ext4_forced_shutdown(EXT4_SB(inode->i_sb))) {
+	if (ret < 0 && !ext4_forced_shutdown(inode->i_sb)) {
 		ext4_msg(inode->i_sb, KERN_EMERG,
 			 "failed to convert unwritten extents to written "
 			 "extents -- potential data loss!  "
diff --git a/fs/ext4/super.c b/fs/ext4/super.c
index 8c843ceb3cbb..d4978e705718 100644
--- a/fs/ext4/super.c
+++ b/fs/ext4/super.c
@@ -758,7 +758,7 @@ void __ext4_error(struct super_block *sb, const char *function,
 	struct va_format vaf;
 	va_list args;
 
-	if (unlikely(ext4_forced_shutdown(EXT4_SB(sb))))
+	if (unlikely(ext4_forced_shutdown(sb)))
 		return;
 
 	trace_ext4_error(sb, function, line);
@@ -783,7 +783,7 @@ void __ext4_error_inode(struct inode *inode, const char *function,
 	va_list args;
 	struct va_format vaf;
 
-	if (unlikely(ext4_forced_shutdown(EXT4_SB(inode->i_sb))))
+	if (unlikely(ext4_forced_shutdown(inode->i_sb)))
 		return;
 
 	trace_ext4_error(inode->i_sb, function, line);
@@ -818,7 +818,7 @@ void __ext4_error_file(struct file *file, const char *function,
 	struct inode *inode = file_inode(file);
 	char pathname[80], *path;
 
-	if (unlikely(ext4_forced_shutdown(EXT4_SB(inode->i_sb))))
+	if (unlikely(ext4_forced_shutdown(inode->i_sb)))
 		return;
 
 	trace_ext4_error(inode->i_sb, function, line);
@@ -898,7 +898,7 @@ void __ext4_std_error(struct super_block *sb, const char *function,
 	char nbuf[16];
 	const char *errstr;
 
-	if (unlikely(ext4_forced_shutdown(EXT4_SB(sb))))
+	if (unlikely(ext4_forced_shutdown(sb)))
 		return;
 
 	/* Special case: if the error is EROFS, and we're not already
@@ -992,7 +992,7 @@ __acquires(bitlock)
 	struct va_format vaf;
 	va_list args;
 
-	if (unlikely(ext4_forced_shutdown(EXT4_SB(sb))))
+	if (unlikely(ext4_forced_shutdown(sb)))
 		return;
 
 	trace_ext4_error(sb, function, line);
@@ -6261,7 +6261,7 @@ static int ext4_sync_fs(struct super_block *sb, int wait)
 	bool needs_barrier = false;
 	struct ext4_sb_info *sbi = EXT4_SB(sb);
 
-	if (unlikely(ext4_forced_shutdown(sbi)))
+	if (unlikely(ext4_forced_shutdown(sb)))
 		return 0;
 
 	trace_ext4_sync_fs(sb, wait);
@@ -6344,7 +6344,7 @@ static int ext4_freeze(struct super_block *sb)
  */
 static int ext4_unfreeze(struct super_block *sb)
 {
-	if (ext4_forced_shutdown(EXT4_SB(sb)))
+	if (ext4_forced_shutdown(sb))
 		return 0;
 
 	if (EXT4_SB(sb)->s_journal) {
diff --git a/fs/ext4/xattr.c b/fs/ext4/xattr.c
index 321e3a888c20..5ba42b1a3d46 100644
--- a/fs/ext4/xattr.c
+++ b/fs/ext4/xattr.c
@@ -701,7 +701,7 @@ ext4_xattr_get(struct inode *inode, int name_index, const char *name,
 {
 	int error;
 
-	if (unlikely(ext4_forced_shutdown(EXT4_SB(inode->i_sb))))
+	if (unlikely(ext4_forced_shutdown(inode->i_sb)))
 		return -EIO;
 
 	if (strlen(name) > 255)
-- 
2.35.3

[PATCH 04/11] ext4: Make 'abort' mount option handling standard

[Jan Kara]

'abort' mount option is the only mount option that has special handling
and sets a bit in sbi->s_mount_flags. There is not strong reason for
that so just simplify the code and make 'abort' set a bit in
sbi->s_mount_opt2 as any other mount option. This simplifies the code
and will allow us to drop EXT4_MF_FS_ABORTED completely in the following
patch.

Signed-off-by: Jan Kara <jack@suse.cz>
---
 fs/ext4/ext4.h  |  1 +
 fs/ext4/super.c | 16 ++--------------
 2 files changed, 3 insertions(+), 14 deletions(-)

diff --git a/fs/ext4/ext4.h b/fs/ext4/ext4.h
index f0b6aa79dcc4..ae0c3a148c7b 100644
--- a/fs/ext4/ext4.h
+++ b/fs/ext4/ext4.h
@@ -1180,6 +1180,7 @@ struct ext4_inode_info {
 #define EXT4_MOUNT2_MB_OPTIMIZE_SCAN	0x00000080 /* Optimize group
 						    * scanning in mballoc
 						    */
+#define EXT4_MOUNT2_ABORT		0x00000100 /* Abort filesystem */
 
 #define clear_opt(sb, opt)		EXT4_SB(sb)->s_mount_opt &= \
 						~EXT4_MOUNT_##opt
diff --git a/fs/ext4/super.c b/fs/ext4/super.c
index d4978e705718..d57b135c8e54 100644
--- a/fs/ext4/super.c
+++ b/fs/ext4/super.c
@@ -1886,6 +1886,7 @@ static const struct mount_opts {
 	{Opt_fc_debug_force, EXT4_MOUNT2_JOURNAL_FAST_COMMIT,
 	 MOPT_SET | MOPT_2 | MOPT_EXT4_ONLY},
 #endif
+	{Opt_abort, EXT4_MOUNT2_ABORT, MOPT_SET | MOPT_2},
 	{Opt_err, 0, 0}
 };
 
@@ -1954,8 +1955,6 @@ struct ext4_fs_context {
 	unsigned int	mask_s_mount_opt;
 	unsigned int	vals_s_mount_opt2;
 	unsigned int	mask_s_mount_opt2;
-	unsigned long	vals_s_mount_flags;
-	unsigned long	mask_s_mount_flags;
 	unsigned int	opt_flags;	/* MOPT flags */
 	unsigned int	spec;
 	u32		s_max_batch_time;
@@ -2106,12 +2105,6 @@ EXT4_SET_CTX(mount_opt2);
 EXT4_CLEAR_CTX(mount_opt2);
 EXT4_TEST_CTX(mount_opt2);
 
-static inline void ctx_set_mount_flag(struct ext4_fs_context *ctx, int bit)
-{
-	set_bit(bit, &ctx->mask_s_mount_flags);
-	set_bit(bit, &ctx->vals_s_mount_flags);
-}
-
 static int ext4_parse_param(struct fs_context *fc, struct fs_parameter *param)
 {
 	struct ext4_fs_context *ctx = fc->fs_private;
@@ -2175,9 +2168,6 @@ static int ext4_parse_param(struct fs_context *fc, struct fs_parameter *param)
 		ext4_msg(NULL, KERN_WARNING, "Ignoring removed %s option",
 			 param->key);
 		return 0;
-	case Opt_abort:
-		ctx_set_mount_flag(ctx, EXT4_MF_FS_ABORTED);
-		return 0;
 	case Opt_inlinecrypt:
 #ifdef CONFIG_FS_ENCRYPTION_INLINE_CRYPT
 		ctx_set_flags(ctx, SB_INLINECRYPT);
@@ -2831,8 +2821,6 @@ static void ext4_apply_options(struct fs_context *fc, struct super_block *sb)
 	sbi->s_mount_opt |= ctx->vals_s_mount_opt;
 	sbi->s_mount_opt2 &= ~ctx->mask_s_mount_opt2;
 	sbi->s_mount_opt2 |= ctx->vals_s_mount_opt2;
-	sbi->s_mount_flags &= ~ctx->mask_s_mount_flags;
-	sbi->s_mount_flags |= ctx->vals_s_mount_flags;
 	sb->s_flags &= ~ctx->mask_s_flags;
 	sb->s_flags |= ctx->vals_s_flags;
 
@@ -6460,7 +6448,7 @@ static int __ext4_remount(struct fs_context *fc, struct super_block *sb)
 		goto restore_opts;
 	}
 
-	if (ext4_test_mount_flag(sb, EXT4_MF_FS_ABORTED))
+	if (test_opt2(sb, ABORT))
 		ext4_abort(sb, ESHUTDOWN, "Abort forced by user");
 
 	sb->s_flags = (sb->s_flags & ~SB_POSIXACL) |
-- 
2.35.3

[PATCH 05/11] ext4: Drop EXT4_MF_FS_ABORTED flag

[Jan Kara]

EXT4_MF_FS_ABORTED flag has practically the same intent as
EXT4_FLAGS_SHUTDOWN flag. The shutdown flag is checked in many more
places than the aborted flag which is mostly the historical artifact
where we were relying on SB_RDONLY checks instead of the aborted flag
checks. There are only three places - ext4_sync_file(),
__ext4_remount(), and mballoc debug code - which check aborted flag and
not shutdown flag and this is arguably a bug. Avoid these
inconsistencies by removing EXT4_MF_FS_ABORTED flag and using
EXT4_FLAGS_SHUTDOWN everywhere.

Signed-off-by: Jan Kara <jack@suse.cz>
---
 fs/ext4/ext4.h    | 1 -
 fs/ext4/fsync.c   | 7 +++----
 fs/ext4/inode.c   | 8 +++-----
 fs/ext4/mballoc.c | 4 ++--
 fs/ext4/super.c   | 4 ++--
 5 files changed, 10 insertions(+), 14 deletions(-)

diff --git a/fs/ext4/ext4.h b/fs/ext4/ext4.h
index ae0c3a148c7b..46d359f5615d 100644
--- a/fs/ext4/ext4.h
+++ b/fs/ext4/ext4.h
@@ -1740,7 +1740,6 @@ static inline int ext4_valid_inum(struct super_block *sb, unsigned long ino)
  */
 enum {
 	EXT4_MF_MNTDIR_SAMPLED,
-	EXT4_MF_FS_ABORTED,	/* Fatal error detected */
 	EXT4_MF_FC_INELIGIBLE	/* Fast commit ineligible */
 };
 
diff --git a/fs/ext4/fsync.c b/fs/ext4/fsync.c
index 958bcaedcff6..b8e15b9f86c8 100644
--- a/fs/ext4/fsync.c
+++ b/fs/ext4/fsync.c
@@ -138,7 +138,6 @@ int ext4_sync_file(struct file *file, loff_t start, loff_t end, int datasync)
 	int ret = 0, err;
 	bool needs_barrier = false;
 	struct inode *inode = file->f_mapping->host;
-	struct ext4_sb_info *sbi = EXT4_SB(inode->i_sb);
 
 	if (unlikely(ext4_forced_shutdown(inode->i_sb)))
 		return -EIO;
@@ -148,9 +147,9 @@ int ext4_sync_file(struct file *file, loff_t start, loff_t end, int datasync)
 	trace_ext4_sync_file_enter(file, datasync);
 
 	if (sb_rdonly(inode->i_sb)) {
-		/* Make sure that we read updated s_mount_flags value */
+		/* Make sure that we read updated s_ext4_flags value */
 		smp_rmb();
-		if (ext4_test_mount_flag(inode->i_sb, EXT4_MF_FS_ABORTED))
+		if (ext4_forced_shutdown(inode->i_sb))
 			ret = -EROFS;
 		goto out;
 	}
@@ -164,7 +163,7 @@ int ext4_sync_file(struct file *file, loff_t start, loff_t end, int datasync)
 	 *  Metadata is in the journal, we wait for proper transaction to
 	 *  commit here.
 	 */
-	if (!sbi->s_journal)
+	if (!EXT4_SB(inode->i_sb)->s_journal)
 		ret = ext4_fsync_nojournal(inode, datasync, &needs_barrier);
 	else
 		ret = ext4_fsync_journal(inode, datasync, &needs_barrier);
diff --git a/fs/ext4/inode.c b/fs/ext4/inode.c
index da3aaaea5f1c..fc6abafcc3fc 100644
--- a/fs/ext4/inode.c
+++ b/fs/ext4/inode.c
@@ -2237,8 +2237,7 @@ static int mpage_map_and_submit_extent(handle_t *handle,
 		if (err < 0) {
 			struct super_block *sb = inode->i_sb;
 
-			if (ext4_forced_shutdown(sb) ||
-			    ext4_test_mount_flag(sb, EXT4_MF_FS_ABORTED))
+			if (ext4_forced_shutdown(sb))
 				goto invalidate_dirty_pages;
 			/*
 			 * Let the uper layers retry transient errors.
@@ -2560,14 +2559,13 @@ static int ext4_do_writepages(struct mpage_da_data *mpd)
 	 * If the filesystem has aborted, it is read-only, so return
 	 * right away instead of dumping stack traces later on that
 	 * will obscure the real source of the problem.  We test
-	 * EXT4_MF_FS_ABORTED instead of sb->s_flag's SB_RDONLY because
+	 * fs shutdown state instead of sb->s_flag's SB_RDONLY because
 	 * the latter could be true if the filesystem is mounted
 	 * read-only, and in that case, ext4_writepages should
 	 * *never* be called, so if that ever happens, we would want
 	 * the stack trace.
 	 */
-	if (unlikely(ext4_forced_shutdown(mapping->host->i_sb) ||
-		     ext4_test_mount_flag(inode->i_sb, EXT4_MF_FS_ABORTED))) {
+	if (unlikely(ext4_forced_shutdown(mapping->host->i_sb))) {
 		ret = -EROFS;
 		goto out_writepages;
 	}
diff --git a/fs/ext4/mballoc.c b/fs/ext4/mballoc.c
index 20f67a260df5..5bcccf6908ea 100644
--- a/fs/ext4/mballoc.c
+++ b/fs/ext4/mballoc.c
@@ -5341,7 +5341,7 @@ static inline void ext4_mb_show_pa(struct super_block *sb)
 {
 	ext4_group_t i, ngroups;
 
-	if (ext4_test_mount_flag(sb, EXT4_MF_FS_ABORTED))
+	if (ext4_forced_shutdown(sb))
 		return;
 
 	ngroups = ext4_get_groups_count(sb);
@@ -5375,7 +5375,7 @@ static void ext4_mb_show_ac(struct ext4_allocation_context *ac)
 {
 	struct super_block *sb = ac->ac_sb;
 
-	if (ext4_test_mount_flag(sb, EXT4_MF_FS_ABORTED))
+	if (ext4_forced_shutdown(sb))
 		return;
 
 	mb_debug(sb, "Can't allocate:"
diff --git a/fs/ext4/super.c b/fs/ext4/super.c
index d57b135c8e54..f883f3fce066 100644
--- a/fs/ext4/super.c
+++ b/fs/ext4/super.c
@@ -657,7 +657,7 @@ static void ext4_handle_error(struct super_block *sb, bool force_ro, int error,
 		WARN_ON_ONCE(1);
 
 	if (!continue_fs && !sb_rdonly(sb)) {
-		ext4_set_mount_flag(sb, EXT4_MF_FS_ABORTED);
+		set_bit(EXT4_FLAGS_SHUTDOWN, &EXT4_SB(sb)->s_ext4_flags);
 		if (journal)
 			jbd2_journal_abort(journal, -EIO);
 	}
@@ -6465,7 +6465,7 @@ static int __ext4_remount(struct fs_context *fc, struct super_block *sb)
 	flush_work(&sbi->s_error_work);
 
 	if ((bool)(fc->sb_flags & SB_RDONLY) != sb_rdonly(sb)) {
-		if (ext4_test_mount_flag(sb, EXT4_MF_FS_ABORTED)) {
+		if (ext4_forced_shutdown(sb)) {
 			err = -EROFS;
 			goto restore_opts;
 		}
-- 
2.35.3

[PATCH 06/11] ext4: Avoid starting transaction on read-only fs in ext4_quota_off()

[Jan Kara]

When the filesystem gets first remounted read-only and then unmounted,
ext4_quota_off() will try to start a transaction (and fail) on read-only
filesystem to cleanup inode flags for legacy quota files. Just bail
before trying to start a transaction instead since that is going to
issue a warning.

Signed-off-by: Jan Kara <jack@suse.cz>
---
 fs/ext4/super.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/fs/ext4/super.c b/fs/ext4/super.c
index f883f3fce066..7dc6750be978 100644
--- a/fs/ext4/super.c
+++ b/fs/ext4/super.c
@@ -7047,6 +7047,13 @@ static int ext4_quota_off(struct super_block *sb, int type)
 	err = dquot_quota_off(sb, type);
 	if (err || ext4_has_feature_quota(sb))
 		goto out_put;
+	/*
+	 * When the filesystem was remounted read-only first, we cannot cleanup
+ 	 * inode flags here. Bad luck but people should be using QUOTA feature
+	 * these days anyway.
+	 */
+	if (sb_rdonly(sb))
+		goto out_put;
 
 	inode_lock(inode);
 	/*
-- 
2.35.3

[PATCH 07/11] ext4: Warn on read-only filesystem in ext4_journal_check_start()

[Jan Kara]

Now that filesystem abort marks the filesystem as shutdown, we shouldn't
be ever hitting the sb_rdonly() check in ext4_journal_check_start().
Since this is a suitable place for catching all sorts of programming
errors, convert the check to WARN_ON instead of dropping it.

Signed-off-by: Jan Kara <jack@suse.cz>
---
 fs/ext4/ext4_jbd2.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/fs/ext4/ext4_jbd2.c b/fs/ext4/ext4_jbd2.c
index b72a22a57d20..ca0eaf2147b0 100644
--- a/fs/ext4/ext4_jbd2.c
+++ b/fs/ext4/ext4_jbd2.c
@@ -70,8 +70,9 @@ static int ext4_journal_check_start(struct super_block *sb)
 	if (unlikely(ext4_forced_shutdown(sb)))
 		return -EIO;
 
-	if (sb_rdonly(sb))
+	if (WARN_ON_ONCE(sb_rdonly(sb)))
 		return -EROFS;
+
 	WARN_ON(sb->s_writers.frozen == SB_FREEZE_COMPLETE);
 	journal = EXT4_SB(sb)->s_journal;
 	/*
-- 
2.35.3

[PATCH 08/11] ext4: Drop read-only check in ext4_init_inode_table()

[Jan Kara]

We better should not be initializing inode tables on read-only
filesystem. The following transaction start will warn us and make the
function bail anyway so drop the pointless check.

Signed-off-by: Jan Kara <jack@suse.cz>
---
 fs/ext4/ialloc.c | 6 ------
 1 file changed, 6 deletions(-)

diff --git a/fs/ext4/ialloc.c b/fs/ext4/ialloc.c
index 060630c0b0ca..e0698f54e17a 100644
--- a/fs/ext4/ialloc.c
+++ b/fs/ext4/ialloc.c
@@ -1523,12 +1523,6 @@ int ext4_init_inode_table(struct super_block *sb, ext4_group_t group,
 	int num, ret = 0, used_blks = 0;
 	unsigned long used_inos = 0;
 
-	/* This should not happen, but just to be sure check this */
-	if (sb_rdonly(sb)) {
-		ret = 1;
-		goto out;
-	}
-
 	gdp = ext4_get_group_desc(sb, group, &group_desc_bh);
 	if (!gdp || !grp)
 		goto out;
-- 
2.35.3

[PATCH 09/11] ext4: Drop read-only check in ext4_write_inode()

[Jan Kara]

We should not have dirty inodes on read-only filesystem. Also silently
bailing without writing anything would be a problem when we enable
quotas during remount while the filesystem is read-only. So drop the
read-only check.

Signed-off-by: Jan Kara <jack@suse.cz>
---
 fs/ext4/inode.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/fs/ext4/inode.c b/fs/ext4/inode.c
index fc6abafcc3fc..e0fe1895a20f 100644
--- a/fs/ext4/inode.c
+++ b/fs/ext4/inode.c
@@ -5155,8 +5155,7 @@ int ext4_write_inode(struct inode *inode, struct writeback_control *wbc)
 {
 	int err;
 
-	if (WARN_ON_ONCE(current->flags & PF_MEMALLOC) ||
-	    sb_rdonly(inode->i_sb))
+	if (WARN_ON_ONCE(current->flags & PF_MEMALLOC))
 		return 0;
 
 	if (unlikely(ext4_forced_shutdown(inode->i_sb)))
-- 
2.35.3

[PATCH 10/11] ext4: Drop read-only check from ext4_force_commit()

[Jan Kara]

JBD2 code will quickly return without doing anything when there's
nothing to commit so there's no point in the read-only check in
ext4_force_commit(). Just drop it.

Signed-off-by: Jan Kara <jack@suse.cz>
---
 fs/ext4/super.c | 8 +-------
 1 file changed, 1 insertion(+), 7 deletions(-)

diff --git a/fs/ext4/super.c b/fs/ext4/super.c
index 7dc6750be978..5299ef013bcd 100644
--- a/fs/ext4/super.c
+++ b/fs/ext4/super.c
@@ -6233,13 +6233,7 @@ static int ext4_clear_journal_err(struct super_block *sb,
  */
 int ext4_force_commit(struct super_block *sb)
 {
-	journal_t *journal;
-
-	if (sb_rdonly(sb))
-		return 0;
-
-	journal = EXT4_SB(sb)->s_journal;
-	return ext4_journal_force_commit(journal);
+	return ext4_journal_force_commit(EXT4_SB(sb)->s_journal);
 }
 
 static int ext4_sync_fs(struct super_block *sb, int wait)
-- 
2.35.3

[PATCH 11/11] ext4: Replace read-only check for shutdown check in mmp code

[Jan Kara]

The multi-mount protection kthread checks for read-only filesystem and
aborts in that case. The remount code actually handles stopping of the
kthread on remount so the only purpose of the check is in case of
emergency remount read-only. Replace the check for read-only filesystem
with a check for shutdown filesystem as running MMP on such is risky
anyway and it makes ordering of things during remount simpler.

Signed-off-by: Jan Kara <jack@suse.cz>
---
 fs/ext4/mmp.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/fs/ext4/mmp.c b/fs/ext4/mmp.c
index 0aaf38ffcb6e..bd946d0c71b7 100644
--- a/fs/ext4/mmp.c
+++ b/fs/ext4/mmp.c
@@ -162,7 +162,7 @@ static int kmmpd(void *data)
 	memcpy(mmp->mmp_nodename, init_utsname()->nodename,
 	       sizeof(mmp->mmp_nodename));
 
-	while (!kthread_should_stop() && !sb_rdonly(sb)) {
+	while (!kthread_should_stop() && !ext4_forced_shutdown(sb)) {
 		if (!ext4_has_feature_mmp(sb)) {
 			ext4_warning(sb, "kmmpd being stopped since MMP feature"
 				     " has been disabled.");
-- 
2.35.3

Re: [PATCH 0/11] ext4: Cleanup read-only and fs aborted checks

[Theodore Ts'o]

On Fri, 16 Jun 2023 18:50:46 +0200, Jan Kara wrote:
> This series arised from me trying to fix races when the ext4 filesystem gets
> remounted read-write and users can race in writes before quota subsystem is
> prepared to take them. This particular problem got fixed in VFS in the end
> but the cleanups are still good in my opinion so I'm submitting them. They
> get rid of EXT4_MF_ABORTED flag and cleanup some sb_rdonly() checks.
> 
> Honza
> 
> [...]

Applied, thanks!

[01/11] ext4: Remove pointless sb_rdonly() checks from freezing code
        commit: 98175720c9ed3bac857b0364321517cc2d695a3f
[02/11] ext4: Use sb_rdonly() helper for checking read-only flag
        commit: d5d020b3294b69eaf3b8985e7a37ba237849c390
[03/11] ext4: Make ext4_forced_shutdown() take struct super_block
        commit: eb8ab4443aec5ffe923a471b337568a8158cd32b
[04/11] ext4: Make 'abort' mount option handling standard
        commit: 22b8d707b07e6e06f50fe1d9ca8756e1f894eb0d
[05/11] ext4: Drop EXT4_MF_FS_ABORTED flag
        commit: 95257987a6387f02970eda707e55a06cce734e18
[06/11] ext4: Avoid starting transaction on read-only fs in ext4_quota_off()
        commit: e0e985f3f8941438a66ab8abb94cb011b9fb39a7
[07/11] ext4: Warn on read-only filesystem in ext4_journal_check_start()
        commit: e7fc2b31e04c46c9e2098bba710c9951c6b968af
[08/11] ext4: Drop read-only check in ext4_init_inode_table()
        commit: ffb6844e28ef6b9d76bee378774d7afbc3db6da9
[09/11] ext4: Drop read-only check in ext4_write_inode()
        commit: f1128084b40e520bea8bb32b3ff4d03745ab7e64
[10/11] ext4: Drop read-only check from ext4_force_commit()
        commit: 889860e452d7436ca72018b8a03cbd89c38d6384
[11/11] ext4: Replace read-only check for shutdown check in mmp code
        commit: 1e1566b9c85fbd6150657ea17f50fd42b9166d31

Best regards,
-- 
Theodore Ts'o <tytso@mit.edu>

Re: [PATCH 0/11] ext4: Cleanup read-only and fs aborted checks

[Amir Goldstein]

On Thu, Aug 3, 2023 at 6:23 PM Theodore Ts'o <tytso@mit.edu> wrote:
>
>
> On Fri, 16 Jun 2023 18:50:46 +0200, Jan Kara wrote:
> > This series arised from me trying to fix races when the ext4 filesystem gets
> > remounted read-write and users can race in writes before quota subsystem is
> > prepared to take them. This particular problem got fixed in VFS in the end
> > but the cleanups are still good in my opinion so I'm submitting them. They
> > get rid of EXT4_MF_ABORTED flag and cleanup some sb_rdonly() checks.
> >
> > Honza
> >
> > [...]
>
> Applied, thanks!
>
> [01/11] ext4: Remove pointless sb_rdonly() checks from freezing code
>         commit: 98175720c9ed3bac857b0364321517cc2d695a3f
> [02/11] ext4: Use sb_rdonly() helper for checking read-only flag
>         commit: d5d020b3294b69eaf3b8985e7a37ba237849c390
> [03/11] ext4: Make ext4_forced_shutdown() take struct super_block
>         commit: eb8ab4443aec5ffe923a471b337568a8158cd32b
> [04/11] ext4: Make 'abort' mount option handling standard
>         commit: 22b8d707b07e6e06f50fe1d9ca8756e1f894eb0d
> [05/11] ext4: Drop EXT4_MF_FS_ABORTED flag
>         commit: 95257987a6387f02970eda707e55a06cce734e18
> [06/11] ext4: Avoid starting transaction on read-only fs in ext4_quota_off()
>         commit: e0e985f3f8941438a66ab8abb94cb011b9fb39a7
> [07/11] ext4: Warn on read-only filesystem in ext4_journal_check_start()
>         commit: e7fc2b31e04c46c9e2098bba710c9951c6b968af
> [08/11] ext4: Drop read-only check in ext4_init_inode_table()
>         commit: ffb6844e28ef6b9d76bee378774d7afbc3db6da9
> [09/11] ext4: Drop read-only check in ext4_write_inode()
>         commit: f1128084b40e520bea8bb32b3ff4d03745ab7e64
> [10/11] ext4: Drop read-only check from ext4_force_commit()
>         commit: 889860e452d7436ca72018b8a03cbd89c38d6384
> [11/11] ext4: Replace read-only check for shutdown check in mmp code
>         commit: 1e1566b9c85fbd6150657ea17f50fd42b9166d31
>
> Best regards,
> --
> Theodore Ts'o <tytso@mit.edu>

Hi Jan,

Yesterday I ran fanotify LTP tests on linux-next and noticed a regression
with fanotify22 which tests the FAN_FS_ERROR event on ext4.
It's 100% reproducible on my machine (see below).

I've bisected the regression down to this series.
Not sure if this is an acute regression or just the test needs to be adjusted.

Thanks,
Amir.

# ./fanotify/fanotify22
tst_device.c:96: TINFO: Found free device 0 '/dev/loop0'
[   29.672163] loop0: detected capacity change from 0 to 614400
tst_test.c:1093: TINFO: Formatting /dev/loop0 with ext4 opts='' extra opts=''
mke2fs 1.46.5 (30-Dec-2021)
[   30.169795] operation not supported error, dev loop0, sector 614272
op 0x9:(WRITE_ZEROES) flags 0x8000800 phys_seg 0 prio class 2
[   30.172411] operation not supported error, dev loop0, sector 586 op
0x9:(WRITE_ZEROES) flags 0x8000800 phys_seg 0 prio class 2
[   30.176215] operation not supported error, dev loop0, sector 15792
op 0x9:(WRITE_ZEROES) flags 0x8000800 phys_seg 0 prio class 2
[   30.189827] operation not supported error, dev loop0, sector 278530
op 0x9:(WRITE_ZEROES) flags 0x8000800 phys_seg 0 prio class 2
[   30.247427] EXT4-fs (loop0): mounted filesystem
a2b5d123-2a7a-44cc-96d4-47119b66a9a9 r/w with ordered data mode. Quota
mode: none.
tst_test.c:1558: TINFO: Timeout per run is 0h 00m 30s
fanotify.h:129: TINFO: fid(test_mnt/internal_dir/bad_dir) =
32966134.65ed1cb1.7e82.a4cd38.0...
[   30.275088] EXT4-fs (loop0): unmounting filesystem
a2b5d123-2a7a-44cc-96d4-47119b66a9a9.
debugfs 1.46.5 (30-Dec-2021)
[   30.339363] EXT4-fs (loop0): mounted filesystem
a2b5d123-2a7a-44cc-96d4-47119b66a9a9 r/w with ordered data mode. Quota
mode: none.
fanotify.h:129: TINFO: fid(test_mnt) = 32966134.65ed1cb1.2.0.0...
[   30.346145] EXT4-fs error (device loop0): __ext4_remount:6465: comm
fanotify22: Abort forced by user
[   30.348788] Aborting journal on device loop0-8.
[   30.350608] EXT4-fs (loop0): Remounting filesystem read-only
[   30.352403] EXT4-fs (loop0): re-mounted
a2b5d123-2a7a-44cc-96d4-47119b66a9a9 ro. Quota mode: none.
fanotify22.c:232: TPASS: Successfully received: Trigger abort
Test timeouted, sending SIGKILL!
tst_test.c:1612: TINFO: If you are running on slow machine, try
exporting LTP_TIMEOUT_MUL > 1
tst_test.c:1614: TBROK: Test killed! (timeout?)

Re: [PATCH 0/11] ext4: Cleanup read-only and fs aborted checks

[Amir Goldstein]

On Fri, Aug 25, 2023 at 10:15 AM Amir Goldstein <amir73il@gmail.com> wrote:
>
> On Thu, Aug 3, 2023 at 6:23 PM Theodore Ts'o <tytso@mit.edu> wrote:
> >
> >
> > On Fri, 16 Jun 2023 18:50:46 +0200, Jan Kara wrote:
> > > This series arised from me trying to fix races when the ext4 filesystem gets
> > > remounted read-write and users can race in writes before quota subsystem is
> > > prepared to take them. This particular problem got fixed in VFS in the end
> > > but the cleanups are still good in my opinion so I'm submitting them. They
> > > get rid of EXT4_MF_ABORTED flag and cleanup some sb_rdonly() checks.
> > >
> > > Honza
> > >
> > > [...]
> >
> > Applied, thanks!
> >
> > [01/11] ext4: Remove pointless sb_rdonly() checks from freezing code
> >         commit: 98175720c9ed3bac857b0364321517cc2d695a3f
> > [02/11] ext4: Use sb_rdonly() helper for checking read-only flag
> >         commit: d5d020b3294b69eaf3b8985e7a37ba237849c390
> > [03/11] ext4: Make ext4_forced_shutdown() take struct super_block
> >         commit: eb8ab4443aec5ffe923a471b337568a8158cd32b
> > [04/11] ext4: Make 'abort' mount option handling standard
> >         commit: 22b8d707b07e6e06f50fe1d9ca8756e1f894eb0d
> > [05/11] ext4: Drop EXT4_MF_FS_ABORTED flag
> >         commit: 95257987a6387f02970eda707e55a06cce734e18
> > [06/11] ext4: Avoid starting transaction on read-only fs in ext4_quota_off()
> >         commit: e0e985f3f8941438a66ab8abb94cb011b9fb39a7
> > [07/11] ext4: Warn on read-only filesystem in ext4_journal_check_start()
> >         commit: e7fc2b31e04c46c9e2098bba710c9951c6b968af
> > [08/11] ext4: Drop read-only check in ext4_init_inode_table()
> >         commit: ffb6844e28ef6b9d76bee378774d7afbc3db6da9
> > [09/11] ext4: Drop read-only check in ext4_write_inode()
> >         commit: f1128084b40e520bea8bb32b3ff4d03745ab7e64
> > [10/11] ext4: Drop read-only check from ext4_force_commit()
> >         commit: 889860e452d7436ca72018b8a03cbd89c38d6384
> > [11/11] ext4: Replace read-only check for shutdown check in mmp code
> >         commit: 1e1566b9c85fbd6150657ea17f50fd42b9166d31
> >
> > Best regards,
> > --
> > Theodore Ts'o <tytso@mit.edu>
>
> Hi Jan,
>
> Yesterday I ran fanotify LTP tests on linux-next and noticed a regression
> with fanotify22 which tests the FAN_FS_ERROR event on ext4.
> It's 100% reproducible on my machine (see below).
>
> I've bisected the regression down to this series.

Forgot to say that the good baseline for the test is Christian's vfs.all
branch merged into Linus' master and the regression is after merging
commit 1e1566b9c85 from Ted's tree.

> Not sure if this is an acute regression or just the test needs to be adjusted.
>
> Thanks,
> Amir.
>
> # ./fanotify/fanotify22
> tst_device.c:96: TINFO: Found free device 0 '/dev/loop0'
> [   29.672163] loop0: detected capacity change from 0 to 614400
> tst_test.c:1093: TINFO: Formatting /dev/loop0 with ext4 opts='' extra opts=''
> mke2fs 1.46.5 (30-Dec-2021)
> [   30.169795] operation not supported error, dev loop0, sector 614272
> op 0x9:(WRITE_ZEROES) flags 0x8000800 phys_seg 0 prio class 2
> [   30.172411] operation not supported error, dev loop0, sector 586 op
> 0x9:(WRITE_ZEROES) flags 0x8000800 phys_seg 0 prio class 2
> [   30.176215] operation not supported error, dev loop0, sector 15792
> op 0x9:(WRITE_ZEROES) flags 0x8000800 phys_seg 0 prio class 2
> [   30.189827] operation not supported error, dev loop0, sector 278530
> op 0x9:(WRITE_ZEROES) flags 0x8000800 phys_seg 0 prio class 2
> [   30.247427] EXT4-fs (loop0): mounted filesystem
> a2b5d123-2a7a-44cc-96d4-47119b66a9a9 r/w with ordered data mode. Quota
> mode: none.
> tst_test.c:1558: TINFO: Timeout per run is 0h 00m 30s
> fanotify.h:129: TINFO: fid(test_mnt/internal_dir/bad_dir) =
> 32966134.65ed1cb1.7e82.a4cd38.0...
> [   30.275088] EXT4-fs (loop0): unmounting filesystem
> a2b5d123-2a7a-44cc-96d4-47119b66a9a9.
> debugfs 1.46.5 (30-Dec-2021)
> [   30.339363] EXT4-fs (loop0): mounted filesystem
> a2b5d123-2a7a-44cc-96d4-47119b66a9a9 r/w with ordered data mode. Quota
> mode: none.
> fanotify.h:129: TINFO: fid(test_mnt) = 32966134.65ed1cb1.2.0.0...
> [   30.346145] EXT4-fs error (device loop0): __ext4_remount:6465: comm
> fanotify22: Abort forced by user
> [   30.348788] Aborting journal on device loop0-8.
> [   30.350608] EXT4-fs (loop0): Remounting filesystem read-only
> [   30.352403] EXT4-fs (loop0): re-mounted
> a2b5d123-2a7a-44cc-96d4-47119b66a9a9 ro. Quota mode: none.
> fanotify22.c:232: TPASS: Successfully received: Trigger abort
> Test timeouted, sending SIGKILL!
> tst_test.c:1612: TINFO: If you are running on slow machine, try
> exporting LTP_TIMEOUT_MUL > 1
> tst_test.c:1614: TBROK: Test killed! (timeout?)

Re: [PATCH 0/11] ext4: Cleanup read-only and fs aborted checks

[Jan Kara]

On Fri 25-08-23 10:24:52, Amir Goldstein wrote:
> On Fri, Aug 25, 2023 at 10:15 AM Amir Goldstein <amir73il@gmail.com> wrote:
> >
> > On Thu, Aug 3, 2023 at 6:23 PM Theodore Ts'o <tytso@mit.edu> wrote:
> > >
> > >
> > > On Fri, 16 Jun 2023 18:50:46 +0200, Jan Kara wrote:
> > > > This series arised from me trying to fix races when the ext4 filesystem gets
> > > > remounted read-write and users can race in writes before quota subsystem is
> > > > prepared to take them. This particular problem got fixed in VFS in the end
> > > > but the cleanups are still good in my opinion so I'm submitting them. They
> > > > get rid of EXT4_MF_ABORTED flag and cleanup some sb_rdonly() checks.
> > > >
> > > > Honza
> > > >
> > > > [...]
> > >
> > > Applied, thanks!
> > >
> > > [01/11] ext4: Remove pointless sb_rdonly() checks from freezing code
> > >         commit: 98175720c9ed3bac857b0364321517cc2d695a3f
> > > [02/11] ext4: Use sb_rdonly() helper for checking read-only flag
> > >         commit: d5d020b3294b69eaf3b8985e7a37ba237849c390
> > > [03/11] ext4: Make ext4_forced_shutdown() take struct super_block
> > >         commit: eb8ab4443aec5ffe923a471b337568a8158cd32b
> > > [04/11] ext4: Make 'abort' mount option handling standard
> > >         commit: 22b8d707b07e6e06f50fe1d9ca8756e1f894eb0d
> > > [05/11] ext4: Drop EXT4_MF_FS_ABORTED flag
> > >         commit: 95257987a6387f02970eda707e55a06cce734e18
> > > [06/11] ext4: Avoid starting transaction on read-only fs in ext4_quota_off()
> > >         commit: e0e985f3f8941438a66ab8abb94cb011b9fb39a7
> > > [07/11] ext4: Warn on read-only filesystem in ext4_journal_check_start()
> > >         commit: e7fc2b31e04c46c9e2098bba710c9951c6b968af
> > > [08/11] ext4: Drop read-only check in ext4_init_inode_table()
> > >         commit: ffb6844e28ef6b9d76bee378774d7afbc3db6da9
> > > [09/11] ext4: Drop read-only check in ext4_write_inode()
> > >         commit: f1128084b40e520bea8bb32b3ff4d03745ab7e64
> > > [10/11] ext4: Drop read-only check from ext4_force_commit()
> > >         commit: 889860e452d7436ca72018b8a03cbd89c38d6384
> > > [11/11] ext4: Replace read-only check for shutdown check in mmp code
> > >         commit: 1e1566b9c85fbd6150657ea17f50fd42b9166d31
> > >
> > > Best regards,
> > > --
> > > Theodore Ts'o <tytso@mit.edu>
> >
> > Hi Jan,
> >
> > Yesterday I ran fanotify LTP tests on linux-next and noticed a regression
> > with fanotify22 which tests the FAN_FS_ERROR event on ext4.
> > It's 100% reproducible on my machine (see below).
> >
> > I've bisected the regression down to this series.
> 
> Forgot to say that the good baseline for the test is Christian's vfs.all
> branch merged into Linus' master and the regression is after merging
> commit 1e1566b9c85 from Ted's tree.

Thanks for report! I had a look and it is the LTP test that is problematic.

1) It has four testcases, each of which ends up triggering more or less
fatal error on this filesystem. However the filesystem is not unmounted &
mounted again between testcases so it assumes that we continue reporting
further errors after fatal filesystem shutdown. This is a wrong assumption
as after such fatal error it isn't really defined what succeeds and what
not.

2) The patchset in ext4 tree slightly changed the behavior of the 'abort'
mount option by unifying it with the filesystem shutdown functionality
because having two different ways to abort a filesystem led to places
checking one but not the other. As a result once the filesystem is
shutdown using the 'abort' mount option, we don't report any more errors
because it's kind of pointless noise - things are expected to fail on
shutdown filesystem. And this upsets the test.

I'll fix the test.

								Honza
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR