| Message ID | 0bd3d3777e34a5322fb4d3ec315df4090ee43399.1654216941.git.anand.jain@oracle.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | cleanup rdonly flag and BTRFS_FS_OPEN flag | expand |
On 03.06.22 04:04, Anand Jain wrote: > +static inline bool btrfs_fs_is_rdonly(const struct btrfs_fs_info *fs_info) > +{ > + bool rdonly = sb_rdonly(fs_info->sb); > + bool fs_rdonly = test_bit(BTRFS_FS_STATE_RO, &fs_info->fs_state); > + > + BUG_ON(rdonly != fs_rdonly); > + > + return rdonly; > +} Do we really need a BUG_ON() here or can we make it an ASSERT()? Apart from that Reviewed-by: Johannes Thumshirn <johannes.thumshirn@wdc.com>
On 6/3/22 14:13, Johannes Thumshirn wrote: > On 03.06.22 04:04, Anand Jain wrote: >> +static inline bool btrfs_fs_is_rdonly(const struct btrfs_fs_info *fs_info) >> +{ >> + bool rdonly = sb_rdonly(fs_info->sb); >> + bool fs_rdonly = test_bit(BTRFS_FS_STATE_RO, &fs_info->fs_state); >> + >> + BUG_ON(rdonly != fs_rdonly); >> + >> + return rdonly; >> +} > > > Do we really need a BUG_ON() here or can we make it an ASSERT()? These two rdonly verification methods should match, but we have never verified them. When this is through a few revisions, we can just remove it instead. I suggest we keep BUG_ON() a couple of revisions. Thanks, Anand > > Apart from that > > Reviewed-by: Johannes Thumshirn <johannes.thumshirn@wdc.com>
On 07.06.22 12:15, Anand Jain wrote: > On 6/3/22 14:13, Johannes Thumshirn wrote: >> On 03.06.22 04:04, Anand Jain wrote: >>> +static inline bool btrfs_fs_is_rdonly(const struct btrfs_fs_info *fs_info) >>> +{ >>> + bool rdonly = sb_rdonly(fs_info->sb); >>> + bool fs_rdonly = test_bit(BTRFS_FS_STATE_RO, &fs_info->fs_state); >>> + >>> + BUG_ON(rdonly != fs_rdonly); >>> + >>> + return rdonly; >>> +} >> >> >> Do we really need a BUG_ON() here or can we make it an ASSERT()? > > > These two rdonly verification methods should match, but we have never > verified them. When this is through a few revisions, we can just remove > it instead. I suggest we keep BUG_ON() a couple of revisions. But isn't this what an ASSERT() is for?
On Fri, Jun 03, 2022 at 07:12:09AM +0530, Anand Jain wrote: > As of now, the BTRFS_FS_OPEN flag is true if the filesystem open is complete > and as well as it is used for the affirmation if the filesystem read-write > able. > > In preparatory to take out the rw affirm part in the above flag, first > consolidate the check for filesystem rdonly into the function > btrfs_fs_is_rdonly(). It makes migration to the new definition of the > flag BTRFS_FS_OPEN cleaner. > > Here I introduce a new function btrfs_fs_is_rdonly(), it consolidates the > current two ways we check for the filesystem in rdonly. > > At all places we use the check > sb_rdonly(fs_info->sb) > however in the funtion btrfs_need_cleaner_sleep() we use the > test_bit(BTRFS_FS_STATE_RO....). > > As per the comment of btrfs_need_cleaner_sleep(), it needs to use > BTRFS_FS_STATE_RO state for the atomicity purpose. > > Signed-off-by: Anand Jain <anand.jain@oracle.com> > --- > Change log reworded. > The same patch was marked as RFC before. To know if there is any better way to > clean up. Now I think there isn't. Removed > > fs/btrfs/block-group.c | 2 +- > fs/btrfs/ctree.h | 13 +++++++++++-- > fs/btrfs/dev-replace.c | 2 +- > fs/btrfs/disk-io.c | 11 ++++++----- > fs/btrfs/extent_io.c | 4 ++-- > fs/btrfs/inode.c | 2 +- > fs/btrfs/ioctl.c | 2 +- > fs/btrfs/super.c | 12 ++++++------ > fs/btrfs/sysfs.c | 4 ++-- > fs/btrfs/tree-checker.c | 2 +- > fs/btrfs/volumes.c | 4 ++-- > 11 files changed, 34 insertions(+), 24 deletions(-) > > diff --git a/fs/btrfs/block-group.c b/fs/btrfs/block-group.c > index ede389f2602d..8f73dc120290 100644 > --- a/fs/btrfs/block-group.c > +++ b/fs/btrfs/block-group.c > @@ -2597,7 +2597,7 @@ int btrfs_inc_block_group_ro(struct btrfs_block_group *cache, > * In that case we should not start a new transaction on read-only fs. > * Thus here we skip all chunk allocations. > */ > - if (sb_rdonly(fs_info->sb)) { > + if (btrfs_fs_is_rdonly(fs_info)) { > mutex_lock(&fs_info->ro_block_group_mutex); > ret = inc_block_group_ro(cache, 0); > mutex_unlock(&fs_info->ro_block_group_mutex); > diff --git a/fs/btrfs/ctree.h b/fs/btrfs/ctree.h > index 777d0b1a0b1e..171dd25def8b 100644 > --- a/fs/btrfs/ctree.h > +++ b/fs/btrfs/ctree.h > @@ -3119,6 +3119,16 @@ static inline int btrfs_fs_closing(struct btrfs_fs_info *fs_info) > return 0; > } > > +static inline bool btrfs_fs_is_rdonly(const struct btrfs_fs_info *fs_info) > +{ > + bool rdonly = sb_rdonly(fs_info->sb); > + bool fs_rdonly = test_bit(BTRFS_FS_STATE_RO, &fs_info->fs_state); > + > + BUG_ON(rdonly != fs_rdonly); I think this is wrong, this would mandate that sb_rdonly is always equal to the state of BTRFS_FS_STATE_RO bit, but this is not always true and was the reason why the separate bit was added in commit a0a1db70df5f ("btrfs: fix race between RO remount and the cleaner task") > + > + return rdonly; > +} > + > /* > * If we remount the fs to be R/O or umount the fs, the cleaner needn't do > * anything except sleeping. This function is used to check the status of > @@ -3129,8 +3139,7 @@ static inline int btrfs_fs_closing(struct btrfs_fs_info *fs_info) > */ > static inline int btrfs_need_cleaner_sleep(struct btrfs_fs_info *fs_info) > { > - return test_bit(BTRFS_FS_STATE_RO, &fs_info->fs_state) || > - btrfs_fs_closing(fs_info); > + return btrfs_fs_is_rdonly(fs_info) || btrfs_fs_closing(fs_info); So here you would effectively switch it from testing BTRFS_FS_STATE_RO to sb_rdonly and obscuring it by a helper. According to a0a1db70df5f the status can get out of sync and can lead to a crash, so the BUG_ON can be triggered and thus it's not even an assertion. It could be possible to return true from btrfs_fs_is_rdonly if either of the flags is set, but this may break in other place than the cleaner.
diff --git a/fs/btrfs/block-group.c b/fs/btrfs/block-group.c index ede389f2602d..8f73dc120290 100644 --- a/fs/btrfs/block-group.c +++ b/fs/btrfs/block-group.c @@ -2597,7 +2597,7 @@ int btrfs_inc_block_group_ro(struct btrfs_block_group *cache, * In that case we should not start a new transaction on read-only fs. * Thus here we skip all chunk allocations. */ - if (sb_rdonly(fs_info->sb)) { + if (btrfs_fs_is_rdonly(fs_info)) { mutex_lock(&fs_info->ro_block_group_mutex); ret = inc_block_group_ro(cache, 0); mutex_unlock(&fs_info->ro_block_group_mutex); diff --git a/fs/btrfs/ctree.h b/fs/btrfs/ctree.h index 777d0b1a0b1e..171dd25def8b 100644 --- a/fs/btrfs/ctree.h +++ b/fs/btrfs/ctree.h @@ -3119,6 +3119,16 @@ static inline int btrfs_fs_closing(struct btrfs_fs_info *fs_info) return 0; } +static inline bool btrfs_fs_is_rdonly(const struct btrfs_fs_info *fs_info) +{ + bool rdonly = sb_rdonly(fs_info->sb); + bool fs_rdonly = test_bit(BTRFS_FS_STATE_RO, &fs_info->fs_state); + + BUG_ON(rdonly != fs_rdonly); + + return rdonly; +} + /* * If we remount the fs to be R/O or umount the fs, the cleaner needn't do * anything except sleeping. This function is used to check the status of @@ -3129,8 +3139,7 @@ static inline int btrfs_fs_closing(struct btrfs_fs_info *fs_info) */ static inline int btrfs_need_cleaner_sleep(struct btrfs_fs_info *fs_info) { - return test_bit(BTRFS_FS_STATE_RO, &fs_info->fs_state) || - btrfs_fs_closing(fs_info); + return btrfs_fs_is_rdonly(fs_info) || btrfs_fs_closing(fs_info); } static inline void btrfs_set_sb_rdonly(struct super_block *sb) diff --git a/fs/btrfs/dev-replace.c b/fs/btrfs/dev-replace.c index ee92854b3a49..0790e0e49a78 100644 --- a/fs/btrfs/dev-replace.c +++ b/fs/btrfs/dev-replace.c @@ -1082,7 +1082,7 @@ int btrfs_dev_replace_cancel(struct btrfs_fs_info *fs_info) int result; int ret; - if (sb_rdonly(fs_info->sb)) + if (btrfs_fs_is_rdonly(fs_info)) return -EROFS; mutex_lock(&dev_replace->lock_finishing_cancel_unmount); diff --git a/fs/btrfs/disk-io.c b/fs/btrfs/disk-io.c index 5a39e63c7aa4..624070f144d0 100644 --- a/fs/btrfs/disk-io.c +++ b/fs/btrfs/disk-io.c @@ -2570,7 +2570,7 @@ static int btrfs_replay_log(struct btrfs_fs_info *fs_info, return ret; } - if (sb_rdonly(fs_info->sb)) { + if (btrfs_fs_is_rdonly(fs_info)) { ret = btrfs_commit_super(fs_info); if (ret) return ret; @@ -3648,7 +3648,7 @@ int __cold open_ctree(struct super_block *sb, struct btrfs_fs_devices *fs_device features = btrfs_super_compat_ro_flags(disk_super) & ~BTRFS_FEATURE_COMPAT_RO_SUPP; - if (!sb_rdonly(sb) && features) { + if (!btrfs_fs_is_rdonly(fs_info) && features) { btrfs_err(fs_info, "cannot mount read-write because of unsupported optional features (0x%llx)", features); @@ -3823,7 +3823,8 @@ int __cold open_ctree(struct super_block *sb, struct btrfs_fs_devices *fs_device btrfs_free_zone_cache(fs_info); - if (!sb_rdonly(sb) && fs_info->fs_devices->missing_devices && + if (!btrfs_fs_is_rdonly(fs_info) && + fs_info->fs_devices->missing_devices && !btrfs_check_rw_degradable(fs_info, NULL)) { btrfs_warn(fs_info, "writable mount is not allowed due to too many missing devices"); @@ -3890,7 +3891,7 @@ int __cold open_ctree(struct super_block *sb, struct btrfs_fs_devices *fs_device goto fail_qgroup; } - if (sb_rdonly(sb)) + if (btrfs_fs_is_rdonly(fs_info)) goto clear_oneshot; ret = btrfs_start_pre_rw_mount(fs_info); @@ -4687,7 +4688,7 @@ void __cold close_ctree(struct btrfs_fs_info *fs_info) /* Cancel or finish ongoing discard work */ btrfs_discard_cleanup(fs_info); - if (!sb_rdonly(fs_info->sb)) { + if (!btrfs_fs_is_rdonly(fs_info)) { /* * The cleaner kthread is stopped, so do one final pass over * unused block groups. diff --git a/fs/btrfs/extent_io.c b/fs/btrfs/extent_io.c index 4847e0471dbf..70283ebcc3c4 100644 --- a/fs/btrfs/extent_io.c +++ b/fs/btrfs/extent_io.c @@ -2402,7 +2402,7 @@ int btrfs_repair_eb_io_failure(const struct extent_buffer *eb, int mirror_num) int i, num_pages = num_extent_pages(eb); int ret = 0; - if (sb_rdonly(fs_info->sb)) + if (btrfs_fs_is_rdonly(fs_info)) return -EROFS; for (i = 0; i < num_pages; i++) { @@ -2445,7 +2445,7 @@ int clean_io_failure(struct btrfs_fs_info *fs_info, BUG_ON(!failrec->this_mirror); - if (sb_rdonly(fs_info->sb)) + if (btrfs_fs_is_rdonly(fs_info)) goto out; spin_lock(&io_tree->lock); diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c index bb1677f6f201..809aa9d31bb3 100644 --- a/fs/btrfs/inode.c +++ b/fs/btrfs/inode.c @@ -5785,7 +5785,7 @@ struct inode *btrfs_lookup_dentry(struct inode *dir, struct dentry *dentry) if (!IS_ERR(inode) && root != sub_root) { down_read(&fs_info->cleanup_work_sem); - if (!sb_rdonly(inode->i_sb)) + if (!btrfs_fs_is_rdonly(fs_info)) ret = btrfs_orphan_cleanup(sub_root); up_read(&fs_info->cleanup_work_sem); if (ret) { diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c index 840a7d95ab86..8acf54d1962a 100644 --- a/fs/btrfs/ioctl.c +++ b/fs/btrfs/ioctl.c @@ -4152,7 +4152,7 @@ static long btrfs_ioctl_dev_replace(struct btrfs_fs_info *fs_info, switch (p->cmd) { case BTRFS_IOCTL_DEV_REPLACE_CMD_START: - if (sb_rdonly(fs_info->sb)) { + if (btrfs_fs_is_rdonly(fs_info)) { ret = -EROFS; goto out; } diff --git a/fs/btrfs/super.c b/fs/btrfs/super.c index 4d43895504b7..9ace5ac8a688 100644 --- a/fs/btrfs/super.c +++ b/fs/btrfs/super.c @@ -183,7 +183,7 @@ void __btrfs_handle_fs_error(struct btrfs_fs_info *fs_info, const char *function * Special case: if the error is EROFS, and we're already * under SB_RDONLY, then it is safe here. */ - if (errno == -EROFS && sb_rdonly(sb)) + if (errno == -EROFS && btrfs_fs_is_rdonly(fs_info)) return; #ifdef CONFIG_PRINTK @@ -216,7 +216,7 @@ void __btrfs_handle_fs_error(struct btrfs_fs_info *fs_info, const char *function if (!(sb->s_flags & SB_BORN)) return; - if (sb_rdonly(sb)) + if (btrfs_fs_is_rdonly(fs_info)) return; btrfs_discard_stop(fs_info); @@ -1940,9 +1940,9 @@ static inline void btrfs_remount_cleanup(struct btrfs_fs_info *fs_info, * close or the filesystem is read only. */ if (btrfs_raw_test_opt(old_opts, AUTO_DEFRAG) && - (!btrfs_raw_test_opt(fs_info->mount_opt, AUTO_DEFRAG) || sb_rdonly(fs_info->sb))) { + (!btrfs_raw_test_opt(fs_info->mount_opt, AUTO_DEFRAG) || + btrfs_fs_is_rdonly(fs_info))) btrfs_cleanup_defrag_inodes(fs_info); - } /* If we toggled discard async */ if (!btrfs_raw_test_opt(old_opts, DISCARD_ASYNC) && @@ -2000,7 +2000,7 @@ static int btrfs_remount(struct super_block *sb, int *flags, char *data) if ((bool)btrfs_test_opt(fs_info, FREE_SPACE_TREE) != (bool)btrfs_fs_compat_ro(fs_info, FREE_SPACE_TREE) && - (!sb_rdonly(sb) || (*flags & SB_RDONLY))) { + (!btrfs_fs_is_rdonly(fs_info) || (*flags & SB_RDONLY))) { btrfs_warn(fs_info, "remount supports changing free space tree only from ro to rw"); /* Make sure free space cache options match the state on disk */ @@ -2014,7 +2014,7 @@ static int btrfs_remount(struct super_block *sb, int *flags, char *data) } } - if ((bool)(*flags & SB_RDONLY) == sb_rdonly(sb)) + if ((bool)(*flags & SB_RDONLY) == btrfs_fs_is_rdonly(fs_info)) goto out; if (*flags & SB_RDONLY) { diff --git a/fs/btrfs/sysfs.c b/fs/btrfs/sysfs.c index ebe76d7a4a64..857600537c3b 100644 --- a/fs/btrfs/sysfs.c +++ b/fs/btrfs/sysfs.c @@ -200,7 +200,7 @@ static ssize_t btrfs_feature_attr_store(struct kobject *kobj, if (!fs_info) return -EPERM; - if (sb_rdonly(fs_info->sb)) + if (btrfs_fs_is_rdonly(fs_info)) return -EROFS; ret = kstrtoul(skip_spaces(buf), 0, &val); @@ -942,7 +942,7 @@ static ssize_t btrfs_label_store(struct kobject *kobj, if (!fs_info) return -EPERM; - if (sb_rdonly(fs_info->sb)) + if (btrfs_fs_is_rdonly(fs_info)) return -EROFS; /* diff --git a/fs/btrfs/tree-checker.c b/fs/btrfs/tree-checker.c index 9e0e0ae2288c..24aa0e00bf5a 100644 --- a/fs/btrfs/tree-checker.c +++ b/fs/btrfs/tree-checker.c @@ -1104,7 +1104,7 @@ static int check_inode_item(struct extent_buffer *leaf, "unknown incompat flags detected: 0x%x", flags); return -EUCLEAN; } - if (unlikely(!sb_rdonly(fs_info->sb) && + if (unlikely(!btrfs_fs_is_rdonly(fs_info) && (ro_flags & ~BTRFS_INODE_RO_FLAG_MASK))) { inode_item_err(leaf, slot, "unknown ro-compat flags detected on writeable mount: 0x%x", diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c index 982a2417d5bb..35b6b87464f7 100644 --- a/fs/btrfs/volumes.c +++ b/fs/btrfs/volumes.c @@ -2630,7 +2630,7 @@ int btrfs_init_new_device(struct btrfs_fs_info *fs_info, const char *device_path bool seeding_dev = false; bool locked = false; - if (sb_rdonly(sb) && !fs_devices->seeding) + if (btrfs_fs_is_rdonly(fs_info) && !fs_devices->seeding) return -EROFS; bdev = blkdev_get_by_path(device_path, FMODE_WRITE | FMODE_EXCL, @@ -4616,7 +4616,7 @@ int btrfs_cancel_balance(struct btrfs_fs_info *fs_info) * mount time if the mount is read-write. Otherwise it's still paused * and we must not allow cancelling as it deletes the item. */ - if (sb_rdonly(fs_info->sb)) { + if (btrfs_fs_is_rdonly(fs_info)) { mutex_unlock(&fs_info->balance_mutex); return -EROFS; }
As of now, the BTRFS_FS_OPEN flag is true if the filesystem open is complete and as well as it is used for the affirmation if the filesystem read-write able. In preparatory to take out the rw affirm part in the above flag, first consolidate the check for filesystem rdonly into the function btrfs_fs_is_rdonly(). It makes migration to the new definition of the flag BTRFS_FS_OPEN cleaner. Here I introduce a new function btrfs_fs_is_rdonly(), it consolidates the current two ways we check for the filesystem in rdonly. At all places we use the check sb_rdonly(fs_info->sb) however in the funtion btrfs_need_cleaner_sleep() we use the test_bit(BTRFS_FS_STATE_RO....). As per the comment of btrfs_need_cleaner_sleep(), it needs to use BTRFS_FS_STATE_RO state for the atomicity purpose. Signed-off-by: Anand Jain <anand.jain@oracle.com> --- Change log reworded. The same patch was marked as RFC before. To know if there is any better way to clean up. Now I think there isn't. Removed fs/btrfs/block-group.c | 2 +- fs/btrfs/ctree.h | 13 +++++++++++-- fs/btrfs/dev-replace.c | 2 +- fs/btrfs/disk-io.c | 11 ++++++----- fs/btrfs/extent_io.c | 4 ++-- fs/btrfs/inode.c | 2 +- fs/btrfs/ioctl.c | 2 +- fs/btrfs/super.c | 12 ++++++------ fs/btrfs/sysfs.c | 4 ++-- fs/btrfs/tree-checker.c | 2 +- fs/btrfs/volumes.c | 4 ++-- 11 files changed, 34 insertions(+), 24 deletions(-)