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
sb_rdonly() helper instead of directly checking sb->s_flags.
Signed-off-by: Jan Kara <[email protected]>
---
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
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 <[email protected]>
---
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
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 <[email protected]>
---
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
'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 <[email protected]>
---
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
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 <[email protected]>
---
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
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 <[email protected]>
---
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
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 <[email protected]>
---
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
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 <[email protected]>
---
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
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 <[email protected]>
---
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
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 <[email protected]>
---
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
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 <[email protected]>
---
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
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 <[email protected]>