[RESEND PATCH 0/3] cleanup rdonly flag and BTRFS_FS_OPEN flag

[RESEND PATCH 0/3] cleanup rdonly flag and BTRFS_FS_OPEN flag

[Anand Jain]

Patch 1/3 brings the check for the rdonly to a new function
btrfs_fs_is_rdonly().

Patch 2/3 wraps the check for BTRFS_FS_OPEN flag into
btrfs_fs_state_is_open_rw().

Patch 3/3 removes the rdonly part from the BTRFS_FS_OPEN flag is true
for both rdonly and rw.


Anand Jain (3):
  btrfs: wrap rdonly check into btrfs_fs_is_rdonly
  btrfs: wrap check for BTRFS_FS_OPEN flag into function
  btrfs: set BTRFS_FS_OPEN flag for both rdonly and rw

 fs/btrfs/block-group.c  |  6 ++---
 fs/btrfs/ctree.h        | 19 ++++++++++++++--
 fs/btrfs/dev-replace.c  |  2 +-
 fs/btrfs/disk-io.c      | 50 +++++++++++++++++++++--------------------
 fs/btrfs/extent_io.c    |  4 ++--
 fs/btrfs/inode.c        |  2 +-
 fs/btrfs/ioctl.c        |  2 +-
 fs/btrfs/super.c        | 14 +++++-------
 fs/btrfs/sysfs.c        |  4 ++--
 fs/btrfs/tree-checker.c |  2 +-
 fs/btrfs/volumes.c      |  6 ++---
 11 files changed, 63 insertions(+), 48 deletions(-)

-- 
2.33.1

[RESEND PATCH 1/3] btrfs: wrap rdonly check into btrfs_fs_is_rdonly

[Anand Jain]

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);
+
+	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;
 	}
-- 
2.33.1

[RESEND PATCH 2/3] btrfs: wrap check for BTRFS_FS_OPEN flag into function

[Anand Jain]

2nd patch in preparation to make the BTRFS_FS_OPEN flag work irrespective
of the type of mount (rdonly or read-write-able). Bring the check for the
BTRFS_FS_OPEN flag into an inline function btrfs_fs_state_is_open_rw().

Signed-off-by: Anand Jain <anand.jain@oracle.com>
---
 fs/btrfs/block-group.c | 4 ++--
 fs/btrfs/ctree.h       | 5 +++++
 fs/btrfs/disk-io.c     | 2 +-
 fs/btrfs/volumes.c     | 2 +-
 4 files changed, 9 insertions(+), 4 deletions(-)

diff --git a/fs/btrfs/block-group.c b/fs/btrfs/block-group.c
index 8f73dc120290..bdb4007ba15f 100644
--- a/fs/btrfs/block-group.c
+++ b/fs/btrfs/block-group.c
@@ -1313,7 +1313,7 @@ void btrfs_delete_unused_bgs(struct btrfs_fs_info *fs_info)
 	const bool async_trim_enabled = btrfs_test_opt(fs_info, DISCARD_ASYNC);
 	int ret = 0;
 
-	if (!test_bit(BTRFS_FS_OPEN, &fs_info->flags))
+	if (!btrfs_fs_state_is_open_rw(fs_info))
 		return;
 
 	/*
@@ -1552,7 +1552,7 @@ void btrfs_reclaim_bgs_work(struct work_struct *work)
 	struct btrfs_block_group *bg;
 	struct btrfs_space_info *space_info;
 
-	if (!test_bit(BTRFS_FS_OPEN, &fs_info->flags))
+	if (!btrfs_fs_state_is_open_rw(fs_info))
 		return;
 
 	if (!btrfs_should_reclaim(fs_info))
diff --git a/fs/btrfs/ctree.h b/fs/btrfs/ctree.h
index 171dd25def8b..1a77a0123c44 100644
--- a/fs/btrfs/ctree.h
+++ b/fs/btrfs/ctree.h
@@ -4039,6 +4039,11 @@ static inline bool btrfs_is_data_reloc_root(const struct btrfs_root *root)
 	return root->root_key.objectid == BTRFS_DATA_RELOC_TREE_OBJECTID;
 }
 
+static inline bool btrfs_fs_state_is_open_rw(const struct btrfs_fs_info *fs_info)
+{
+	return test_bit(BTRFS_FS_OPEN, &fs_info->flags);
+}
+
 /*
  * We use page status Private2 to indicate there is an ordered extent with
  * unfinished IO.
diff --git a/fs/btrfs/disk-io.c b/fs/btrfs/disk-io.c
index 624070f144d0..e25e0104a9f0 100644
--- a/fs/btrfs/disk-io.c
+++ b/fs/btrfs/disk-io.c
@@ -1984,7 +1984,7 @@ static int cleaner_kthread(void *arg)
 		 * Do not do anything if we might cause open_ctree() to block
 		 * before we have finished mounting the filesystem.
 		 */
-		if (!test_bit(BTRFS_FS_OPEN, &fs_info->flags))
+		if (!btrfs_fs_state_is_open_rw(fs_info))
 			goto sleep;
 
 		if (!mutex_trylock(&fs_info->cleaner_mutex))
diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
index 35b6b87464f7..a06a35dc8156 100644
--- a/fs/btrfs/volumes.c
+++ b/fs/btrfs/volumes.c
@@ -7610,7 +7610,7 @@ int btrfs_read_chunk_tree(struct btrfs_fs_info *fs_info)
 	 * and at this point there can't be any concurrent task modifying the
 	 * chunk tree, to keep it simple, just skip locking on the chunk tree.
 	 */
-	ASSERT(!test_bit(BTRFS_FS_OPEN, &fs_info->flags));
+	ASSERT(!btrfs_fs_state_is_open_rw(fs_info));
 	path->skip_locking = 1;
 
 	/*
-- 
2.33.1

[RESEND PATCH 3/3] btrfs: set BTRFS_FS_OPEN flag for both rdonly and rw

[Anand Jain]

It is good to have the BTRFS_FS_OPEN flag set irrespective of rdonly or rw
mount. Setting the BTRFS_FS_OPEN  flag only for the rw-mount does not make
sense as it lessens its utility. Besides, we have the BTRFS_FS_STATE_RO
state for mount type rdonly or rw-able.

Signed-off-by: Anand Jain <anand.jain@oracle.com>
---
 fs/btrfs/ctree.h   |  3 ++-
 fs/btrfs/disk-io.c | 39 ++++++++++++++++++++-------------------
 fs/btrfs/super.c   |  2 --
 3 files changed, 22 insertions(+), 22 deletions(-)

diff --git a/fs/btrfs/ctree.h b/fs/btrfs/ctree.h
index 1a77a0123c44..1b46b197fb61 100644
--- a/fs/btrfs/ctree.h
+++ b/fs/btrfs/ctree.h
@@ -4041,7 +4041,8 @@ static inline bool btrfs_is_data_reloc_root(const struct btrfs_root *root)
 
 static inline bool btrfs_fs_state_is_open_rw(const struct btrfs_fs_info *fs_info)
 {
-	return test_bit(BTRFS_FS_OPEN, &fs_info->flags);
+	return test_bit(BTRFS_FS_OPEN, &fs_info->flags) &&
+	       !btrfs_fs_is_rdonly(fs_info);
 }
 
 /*
diff --git a/fs/btrfs/disk-io.c b/fs/btrfs/disk-io.c
index e25e0104a9f0..16c66f71a3bc 100644
--- a/fs/btrfs/disk-io.c
+++ b/fs/btrfs/disk-io.c
@@ -3891,36 +3891,37 @@ int __cold open_ctree(struct super_block *sb, struct btrfs_fs_devices *fs_device
 		goto fail_qgroup;
 	}
 
-	if (btrfs_fs_is_rdonly(fs_info))
-		goto clear_oneshot;
-
-	ret = btrfs_start_pre_rw_mount(fs_info);
-	if (ret) {
-		close_ctree(fs_info);
-		return ret;
-	}
-	btrfs_discard_resume(fs_info);
-
-	if (fs_info->uuid_root &&
-	    (btrfs_test_opt(fs_info, RESCAN_UUID_TREE) ||
-	     fs_info->generation != btrfs_super_uuid_tree_generation(disk_super))) {
-		btrfs_info(fs_info, "checking UUID tree");
-		ret = btrfs_check_uuid_tree(fs_info);
+	if (!btrfs_fs_is_rdonly(fs_info)) {
+		ret = btrfs_start_pre_rw_mount(fs_info);
 		if (ret) {
-			btrfs_warn(fs_info,
-				"failed to check the UUID tree: %d", ret);
 			close_ctree(fs_info);
 			return ret;
 		}
+		btrfs_discard_resume(fs_info);
+
+		if (fs_info->uuid_root &&
+		    (btrfs_test_opt(fs_info, RESCAN_UUID_TREE) ||
+		     fs_info->generation !=
+		     btrfs_super_uuid_tree_generation(disk_super))) {
+			btrfs_info(fs_info, "checking UUID tree");
+			ret = btrfs_check_uuid_tree(fs_info);
+			if (ret) {
+				btrfs_warn(fs_info,
+				"failed to check the UUID tree: %d", ret);
+				close_ctree(fs_info);
+				return ret;
+			}
+		}
 	}
 
+	/* set open-ed flag for both rdonly and rw mounts */
 	set_bit(BTRFS_FS_OPEN, &fs_info->flags);
 
 	/* Kick the cleaner thread so it'll start deleting snapshots. */
-	if (test_bit(BTRFS_FS_UNFINISHED_DROPS, &fs_info->flags))
+	if (test_bit(BTRFS_FS_UNFINISHED_DROPS, &fs_info->flags) &&
+	    !btrfs_fs_is_rdonly(fs_info))
 		wake_up_process(fs_info->cleaner_kthread);
 
-clear_oneshot:
 	btrfs_clear_oneshot_options(fs_info);
 	return 0;
 
diff --git a/fs/btrfs/super.c b/fs/btrfs/super.c
index 9ace5ac8a688..96b204bdd49c 100644
--- a/fs/btrfs/super.c
+++ b/fs/btrfs/super.c
@@ -2118,8 +2118,6 @@ static int btrfs_remount(struct super_block *sb, int *flags, char *data)
 			goto restore;
 
 		btrfs_clear_sb_rdonly(sb);
-
-		set_bit(BTRFS_FS_OPEN, &fs_info->flags);
 	}
 out:
 	/*
-- 
2.33.1

Re: [RESEND PATCH 1/3] btrfs: wrap rdonly check into btrfs_fs_is_rdonly

[Johannes Thumshirn]

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>

Re: [RESEND PATCH 1/3] btrfs: wrap rdonly check into btrfs_fs_is_rdonly

[Anand Jain]

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>

Re: [RESEND PATCH 1/3] btrfs: wrap rdonly check into btrfs_fs_is_rdonly

[Johannes Thumshirn]

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?

Re: [RESEND PATCH 1/3] btrfs: wrap rdonly check into btrfs_fs_is_rdonly

[David Sterba]

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.