[PATCH RFC v3 0/5] mount options consistent enhancement

[PATCH RFC v3 0/5] mount options consistent enhancement

[Qu Wenruo]

Patchset to solve the previous found deadlock and enhance mount options
consistence.

Unlike previous pending_changes, which uses transaction commits to
ensure mount option doesn't change during transaction.
This patch use the RCU-like concept, which will copy the mount_opt from
fs_info into btrfs_transaction, and each btrfs_test_opt() for specific
mount option bit(SPACE_CACHE/INODE_MAP_CACHE) should be changed to
btrfs_test_trans_opt().
So mount option during transaction won't be changed, and also, no extra
read_only or freeze check is needed, the commit routine is also
untouched.

Qu Wenruo (5):
  Revert "btrfs: add support for processing pending changes" related    
    commits
  btrfs: Make btrfs_parse_options() parse mount option in a atomic way
  btrfs: Introduce per-transaction mount_opt to keep mount option    
    consistent during transaction.
  btrfs: Use btrfs_test_trans_opt() to handle SPACE_CACHE if it's under 
       transaction protect.
  btrfs: Use btrfs_test_trans_opt() to handle INODE_CACHE if it's under 
       transaction protect.

 fs/btrfs/ctree.h            |  64 +++-------------------
 fs/btrfs/disk-io.c          |   6 ---
 fs/btrfs/extent-tree.c      |   2 +-
 fs/btrfs/free-space-cache.c |   2 +-
 fs/btrfs/inode-map.c        |   5 +-
 fs/btrfs/super.c            | 129 ++++++++++++++++++++++----------------------
 fs/btrfs/sysfs.c            |  34 +++++++-----
 fs/btrfs/transaction.c      |  43 ++-------------
 fs/btrfs/transaction.h      |   6 ++-
 9 files changed, 107 insertions(+), 184 deletions(-)

-- 
2.2.2

[PATCH RFC v3 1/5] Revert "btrfs: add support for processing pending changes" related commits

[Qu Wenruo]

This reverts commit 572d9ab7845 ~ a6f69dc8018.

This pending commits patches introduce deadlock with freeze, and fix for
it will introduce extra checks on freeze and read only case.

For mount option change, later patches will introduce copy-n-update
method and rwsem protects to keep mount options consistent during
transaction.

For sysfs interface to change label/features, it will keep the same
behavior as 'btrfs pro set', so pending changes are also not needed.

Revert them to a clean base for later changes.

Signed-off-by: Qu Wenruo <quwenruo@cn.fujitsu.com>

Conflicts:
	fs/btrfs/ctree.h
	fs/btrfs/super.c
---
 fs/btrfs/ctree.h       | 49 +------------------------------------------------
 fs/btrfs/disk-io.c     |  8 +++-----
 fs/btrfs/inode-map.c   |  2 +-
 fs/btrfs/super.c       | 14 +++-----------
 fs/btrfs/sysfs.c       | 34 +++++++++++++++++++++-------------
 fs/btrfs/transaction.c | 38 ++++++--------------------------------
 fs/btrfs/transaction.h |  2 --
 7 files changed, 35 insertions(+), 112 deletions(-)

diff --git a/fs/btrfs/ctree.h b/fs/btrfs/ctree.h
index 7e60741..5f99743 100644
--- a/fs/btrfs/ctree.h
+++ b/fs/btrfs/ctree.h
@@ -1411,11 +1411,6 @@ struct btrfs_fs_info {
 	 */
 	u64 last_trans_log_full_commit;
 	unsigned long mount_opt;
-	/*
-	 * Track requests for actions that need to be done during transaction
-	 * commit (like for some mount options).
-	 */
-	unsigned long pending_changes;
 	unsigned long compress_type:4;
 	int commit_interval;
 	/*
@@ -2113,6 +2108,7 @@ struct btrfs_ioctl_defrag_range_args {
 #define BTRFS_MOUNT_CHECK_INTEGRITY_INCLUDING_EXTENT_DATA (1 << 21)
 #define BTRFS_MOUNT_PANIC_ON_FATAL_ERROR	(1 << 22)
 #define BTRFS_MOUNT_RESCAN_UUID_TREE	(1 << 23)
+#define	BTRFS_MOUNT_CHANGE_INODE_CACHE	(1 << 24)
 
 #define BTRFS_DEFAULT_COMMIT_INTERVAL	(30)
 #define BTRFS_DEFAULT_MAX_INLINE	(8192)
@@ -2138,49 +2134,6 @@ struct btrfs_ioctl_defrag_range_args {
 }
 
 /*
- * Requests for changes that need to be done during transaction commit.
- *
- * Internal mount options that are used for special handling of the real
- * mount options (eg. cannot be set during remount and have to be set during
- * transaction commit)
- */
-
-#define BTRFS_PENDING_SET_INODE_MAP_CACHE	(0)
-#define BTRFS_PENDING_CLEAR_INODE_MAP_CACHE	(1)
-#define BTRFS_PENDING_COMMIT			(2)
-
-#define btrfs_test_pending(info, opt)	\
-	test_bit(BTRFS_PENDING_##opt, &(info)->pending_changes)
-#define btrfs_set_pending(info, opt)	\
-	set_bit(BTRFS_PENDING_##opt, &(info)->pending_changes)
-#define btrfs_clear_pending(info, opt)	\
-	clear_bit(BTRFS_PENDING_##opt, &(info)->pending_changes)
-
-/*
- * Helpers for setting pending mount option changes.
- *
- * Expects corresponding macros
- * BTRFS_PENDING_SET_ and CLEAR_ + short mount option name
- */
-#define btrfs_set_pending_and_info(info, opt, fmt, args...)            \
-do {                                                                   \
-       if (!btrfs_raw_test_opt((info)->mount_opt, opt)) {              \
-               btrfs_info((info), fmt, ##args);                        \
-               btrfs_set_pending((info), SET_##opt);                   \
-               btrfs_clear_pending((info), CLEAR_##opt);               \
-       }                                                               \
-} while(0)
-
-#define btrfs_clear_pending_and_info(info, opt, fmt, args...)          \
-do {                                                                   \
-       if (btrfs_raw_test_opt((info)->mount_opt, opt)) {               \
-               btrfs_info((info), fmt, ##args);                        \
-               btrfs_set_pending((info), CLEAR_##opt);                 \
-               btrfs_clear_pending((info), SET_##opt);                 \
-       }                                                               \
-} while(0)
-
-/*
  * Inode flags
  */
 #define BTRFS_INODE_NODATASUM		(1 << 0)
diff --git a/fs/btrfs/disk-io.c b/fs/btrfs/disk-io.c
index 8c63419..2d3c8b7 100644
--- a/fs/btrfs/disk-io.c
+++ b/fs/btrfs/disk-io.c
@@ -2832,11 +2832,9 @@ retry_root_backup:
 		btrfs_set_opt(fs_info->mount_opt, SSD);
 	}
 
-	/*
-	 * Mount does not set all options immediatelly, we can do it now and do
-	 * not have to wait for transaction commit
-	 */
-	btrfs_apply_pending_changes(fs_info);
+	/* Set the real inode map cache flag */
+	if (btrfs_test_opt(tree_root, CHANGE_INODE_CACHE))
+		btrfs_set_opt(tree_root->fs_info->mount_opt, INODE_MAP_CACHE);
 
 #ifdef CONFIG_BTRFS_FS_CHECK_INTEGRITY
 	if (btrfs_test_opt(tree_root, CHECK_INTEGRITY)) {
diff --git a/fs/btrfs/inode-map.c b/fs/btrfs/inode-map.c
index 74faea3..81efd83 100644
--- a/fs/btrfs/inode-map.c
+++ b/fs/btrfs/inode-map.c
@@ -178,7 +178,7 @@ static void start_caching(struct btrfs_root *root)
 			  root->root_key.objectid);
 	if (IS_ERR(tsk)) {
 		btrfs_warn(root->fs_info, "failed to start inode caching task");
-		btrfs_clear_pending_and_info(root->fs_info, INODE_MAP_CACHE,
+		btrfs_clear_and_info(root, CHANGE_INODE_CACHE,
 				"disabling inode map caching");
 	}
 }
diff --git a/fs/btrfs/super.c b/fs/btrfs/super.c
index 60f7cbe..b0c45b2 100644
--- a/fs/btrfs/super.c
+++ b/fs/btrfs/super.c
@@ -993,17 +993,9 @@ int btrfs_sync_fs(struct super_block *sb, int wait)
 	trans = btrfs_attach_transaction_barrier(root);
 	if (IS_ERR(trans)) {
 		/* no transaction, don't bother */
-		if (PTR_ERR(trans) == -ENOENT) {
-			/*
-			 * Exit unless we have some pending changes
-			 * that need to go through commit
-			 */
-			if (fs_info->pending_changes == 0)
-				return 0;
-			trans = btrfs_start_transaction(root, 0);
-		} else {
-			return PTR_ERR(trans);
-		}
+		if (PTR_ERR(trans) == -ENOENT)
+			return 0;
+		return PTR_ERR(trans);
 	}
 	return btrfs_commit_transaction(trans, root);
 }
diff --git a/fs/btrfs/sysfs.c b/fs/btrfs/sysfs.c
index 92db3f6..b2e7bb4 100644
--- a/fs/btrfs/sysfs.c
+++ b/fs/btrfs/sysfs.c
@@ -111,6 +111,7 @@ static ssize_t btrfs_feature_attr_store(struct kobject *kobj,
 {
 	struct btrfs_fs_info *fs_info;
 	struct btrfs_feature_attr *fa = to_btrfs_feature_attr(a);
+	struct btrfs_trans_handle *trans;
 	u64 features, set, clear;
 	unsigned long val;
 	int ret;
@@ -152,6 +153,10 @@ static ssize_t btrfs_feature_attr_store(struct kobject *kobj,
 	btrfs_info(fs_info, "%s %s feature flag",
 		   val ? "Setting" : "Clearing", fa->kobj_attr.attr.name);
 
+	trans = btrfs_start_transaction(fs_info->fs_root, 0);
+	if (IS_ERR(trans))
+		return PTR_ERR(trans);
+
 	spin_lock(&fs_info->super_lock);
 	features = get_features(fs_info, fa->feature_set);
 	if (val)
@@ -161,11 +166,9 @@ static ssize_t btrfs_feature_attr_store(struct kobject *kobj,
 	set_features(fs_info, fa->feature_set, features);
 	spin_unlock(&fs_info->super_lock);
 
-	/*
-	 * We don't want to do full transaction commit from inside sysfs
-	 */
-	btrfs_set_pending(fs_info, COMMIT);
-	wake_up_process(fs_info->transaction_kthread);
+	ret = btrfs_commit_transaction(trans, fs_info->fs_root);
+	if (ret)
+		return ret;
 
 	return count;
 }
@@ -369,6 +372,9 @@ static ssize_t btrfs_label_store(struct kobject *kobj,
 				 const char *buf, size_t len)
 {
 	struct btrfs_fs_info *fs_info = to_fs_info(kobj);
+	struct btrfs_trans_handle *trans;
+	struct btrfs_root *root = fs_info->fs_root;
+	int ret;
 	size_t p_len;
 
 	if (fs_info->sb->s_flags & MS_RDONLY)
@@ -383,18 +389,20 @@ static ssize_t btrfs_label_store(struct kobject *kobj,
 	if (p_len >= BTRFS_LABEL_SIZE)
 		return -EINVAL;
 
-	spin_lock(&fs_info->super_lock);
+	trans = btrfs_start_transaction(root, 0);
+	if (IS_ERR(trans))
+		return PTR_ERR(trans);
+
+	spin_lock(&root->fs_info->super_lock);
 	memset(fs_info->super_copy->label, 0, BTRFS_LABEL_SIZE);
 	memcpy(fs_info->super_copy->label, buf, p_len);
-	spin_unlock(&fs_info->super_lock);
+	spin_unlock(&root->fs_info->super_lock);
+	ret = btrfs_commit_transaction(trans, root);
 
-	/*
-	 * We don't want to do full transaction commit from inside sysfs
-	 */
-	btrfs_set_pending(fs_info, COMMIT);
-	wake_up_process(fs_info->transaction_kthread);
+	if (!ret)
+		return len;
 
-	return len;
+	return ret;
 }
 BTRFS_ATTR_RW(label, btrfs_label_show, btrfs_label_store);
 
diff --git a/fs/btrfs/transaction.c b/fs/btrfs/transaction.c
index a605d4e..295a135 100644
--- a/fs/btrfs/transaction.c
+++ b/fs/btrfs/transaction.c
@@ -1938,10 +1938,13 @@ int btrfs_commit_transaction(struct btrfs_trans_handle *trans,
 	}
 
 	/*
-	 * Since the transaction is done, we can apply the pending changes
-	 * before the next transaction.
+	 * Since the transaction is done, we should set the inode map cache flag
+	 * before any other comming transaction.
 	 */
-	btrfs_apply_pending_changes(root->fs_info);
+	if (btrfs_test_opt(root, CHANGE_INODE_CACHE))
+		btrfs_set_opt(root->fs_info->mount_opt, INODE_MAP_CACHE);
+	else
+		btrfs_clear_opt(root->fs_info->mount_opt, INODE_MAP_CACHE);
 
 	/* commit_fs_roots gets rid of all the tree log roots, it is now
 	 * safe to free the root of tree log roots
@@ -2112,32 +2115,3 @@ int btrfs_clean_one_deleted_snapshot(struct btrfs_root *root)
 
 	return (ret < 0) ? 0 : 1;
 }
-
-void btrfs_apply_pending_changes(struct btrfs_fs_info *fs_info)
-{
-	unsigned long prev;
-	unsigned long bit;
-
-	prev = cmpxchg(&fs_info->pending_changes, 0, 0);
-	if (!prev)
-		return;
-
-	bit = 1 << BTRFS_PENDING_SET_INODE_MAP_CACHE;
-	if (prev & bit)
-		btrfs_set_opt(fs_info->mount_opt, INODE_MAP_CACHE);
-	prev &= ~bit;
-
-	bit = 1 << BTRFS_PENDING_CLEAR_INODE_MAP_CACHE;
-	if (prev & bit)
-		btrfs_clear_opt(fs_info->mount_opt, INODE_MAP_CACHE);
-	prev &= ~bit;
-
-	bit = 1 << BTRFS_PENDING_COMMIT;
-	if (prev & bit)
-		btrfs_debug(fs_info, "pending commit done");
-	prev &= ~bit;
-
-	if (prev)
-		btrfs_warn(fs_info,
-			"unknown pending changes left 0x%lx, ignoring", prev);
-}
diff --git a/fs/btrfs/transaction.h b/fs/btrfs/transaction.h
index 00ed29c..fd400a3 100644
--- a/fs/btrfs/transaction.h
+++ b/fs/btrfs/transaction.h
@@ -170,6 +170,4 @@ int btrfs_wait_marked_extents(struct btrfs_root *root,
 int btrfs_transaction_blocked(struct btrfs_fs_info *info);
 int btrfs_transaction_in_commit(struct btrfs_fs_info *info);
 void btrfs_put_transaction(struct btrfs_transaction *transaction);
-void btrfs_apply_pending_changes(struct btrfs_fs_info *fs_info);
-
 #endif
-- 
2.2.2

[PATCH RFC v3 2/5] btrfs: Make btrfs_parse_options() parse mount option in a atomic way

[Qu Wenruo]

Current btrfs_parse_options() is not atomic, which can set and clear a
bit, especially for nospace_cache case.

For example, if a fs is mounted with nospace_cache,
btrfs_parse_options() will set SPACE_CACHE bit first(since
cache_generation is non-zeo) and clear the SPACE_CACHE bit due to
nospace_cache mount option.
So under heavy operations and remount a nospace_cache btrfs, there is a
windows for commit to create space cache.

This bug can be reproduced by fstest/btrfs/071 073 074 with
nospace_cache mount option. It has about 50% chance to create space
cache, and about 10% chance to create wrong space cache, which can't
pass btrfsck.

This patch will do the mount option parse in a copy-and-update method.
First copy the mount_opt from fs_info to new_opt, and only update
options in new_opt. At last, copy the new_opt back to
fs_info->mount_opt.

This patch is already good enough to fix the above nospace_cache +
remount bug, but need later patch to make sure mount options does not
change during transaction.

Signed-off-by: Qu Wenruo <quwenruo@cn.fujitsu.com>
---
 fs/btrfs/ctree.h     |  16 +++----
 fs/btrfs/inode-map.c |   3 +-
 fs/btrfs/super.c     | 115 +++++++++++++++++++++++++++------------------------
 3 files changed, 71 insertions(+), 63 deletions(-)

diff --git a/fs/btrfs/ctree.h b/fs/btrfs/ctree.h
index 5f99743..26bb47b 100644
--- a/fs/btrfs/ctree.h
+++ b/fs/btrfs/ctree.h
@@ -2119,18 +2119,18 @@ struct btrfs_ioctl_defrag_range_args {
 #define btrfs_test_opt(root, opt)	((root)->fs_info->mount_opt & \
 					 BTRFS_MOUNT_##opt)
 
-#define btrfs_set_and_info(root, opt, fmt, args...)			\
+#define btrfs_set_and_info(fs_info, val, opt, fmt, args...)		\
 {									\
-	if (!btrfs_test_opt(root, opt))					\
-		btrfs_info(root->fs_info, fmt, ##args);			\
-	btrfs_set_opt(root->fs_info->mount_opt, opt);			\
+	if (!btrfs_raw_test_opt(val, opt))				\
+		btrfs_info(fs_info, fmt, ##args);			\
+	btrfs_set_opt(val, opt);					\
 }
 
-#define btrfs_clear_and_info(root, opt, fmt, args...)			\
+#define btrfs_clear_and_info(fs_info, val, opt, fmt, args...)		\
 {									\
-	if (btrfs_test_opt(root, opt))					\
-		btrfs_info(root->fs_info, fmt, ##args);			\
-	btrfs_clear_opt(root->fs_info->mount_opt, opt);			\
+	if (btrfs_raw_test_opt(val, opt))				\
+		btrfs_info(fs_info, fmt, ##args);			\
+	btrfs_clear_opt(val, opt);					\
 }
 
 /*
diff --git a/fs/btrfs/inode-map.c b/fs/btrfs/inode-map.c
index 81efd83..d1edab5 100644
--- a/fs/btrfs/inode-map.c
+++ b/fs/btrfs/inode-map.c
@@ -178,7 +178,8 @@ static void start_caching(struct btrfs_root *root)
 			  root->root_key.objectid);
 	if (IS_ERR(tsk)) {
 		btrfs_warn(root->fs_info, "failed to start inode caching task");
-		btrfs_clear_and_info(root, CHANGE_INODE_CACHE,
+		btrfs_clear_and_info(root->fs_info, root->fs_info->mount_opt,
+				CHANGE_INODE_CACHE,
 				"disabling inode map caching");
 	}
 }
diff --git a/fs/btrfs/super.c b/fs/btrfs/super.c
index b0c45b2..490fe1f 100644
--- a/fs/btrfs/super.c
+++ b/fs/btrfs/super.c
@@ -395,10 +395,13 @@ int btrfs_parse_options(struct btrfs_root *root, char *options)
 	int ret = 0;
 	char *compress_type;
 	bool compress_force = false;
+	unsigned long new_opt;
+
+	new_opt = info->mount_opt;
 
 	cache_gen = btrfs_super_cache_generation(root->fs_info->super_copy);
 	if (cache_gen)
-		btrfs_set_opt(info->mount_opt, SPACE_CACHE);
+		btrfs_set_opt(new_opt, SPACE_CACHE);
 
 	if (!options)
 		goto out;
@@ -422,7 +425,7 @@ int btrfs_parse_options(struct btrfs_root *root, char *options)
 		switch (token) {
 		case Opt_degraded:
 			btrfs_info(root->fs_info, "allowing degraded mounts");
-			btrfs_set_opt(info->mount_opt, DEGRADED);
+			btrfs_set_opt(new_opt, DEGRADED);
 			break;
 		case Opt_subvol:
 		case Opt_subvolid:
@@ -434,7 +437,7 @@ int btrfs_parse_options(struct btrfs_root *root, char *options)
 			 */
 			break;
 		case Opt_nodatasum:
-			btrfs_set_and_info(root, NODATASUM,
+			btrfs_set_and_info(info, new_opt, NODATASUM,
 					   "setting nodatasum");
 			break;
 		case Opt_datasum:
@@ -444,8 +447,8 @@ int btrfs_parse_options(struct btrfs_root *root, char *options)
 				else
 					btrfs_info(root->fs_info, "setting datasum");
 			}
-			btrfs_clear_opt(info->mount_opt, NODATACOW);
-			btrfs_clear_opt(info->mount_opt, NODATASUM);
+			btrfs_clear_opt(new_opt, NODATACOW);
+			btrfs_clear_opt(new_opt, NODATASUM);
 			break;
 		case Opt_nodatacow:
 			if (!btrfs_test_opt(root, NODATACOW)) {
@@ -457,13 +460,13 @@ int btrfs_parse_options(struct btrfs_root *root, char *options)
 					btrfs_info(root->fs_info, "setting nodatacow");
 				}
 			}
-			btrfs_clear_opt(info->mount_opt, COMPRESS);
-			btrfs_clear_opt(info->mount_opt, FORCE_COMPRESS);
-			btrfs_set_opt(info->mount_opt, NODATACOW);
-			btrfs_set_opt(info->mount_opt, NODATASUM);
+			btrfs_clear_opt(new_opt, COMPRESS);
+			btrfs_clear_opt(new_opt, FORCE_COMPRESS);
+			btrfs_set_opt(new_opt, NODATACOW);
+			btrfs_set_opt(new_opt, NODATASUM);
 			break;
 		case Opt_datacow:
-			btrfs_clear_and_info(root, NODATACOW,
+			btrfs_clear_and_info(info, new_opt, NODATACOW,
 					     "setting datacow");
 			break;
 		case Opt_compress_force:
@@ -477,20 +480,20 @@ int btrfs_parse_options(struct btrfs_root *root, char *options)
 			    strcmp(args[0].from, "zlib") == 0) {
 				compress_type = "zlib";
 				info->compress_type = BTRFS_COMPRESS_ZLIB;
-				btrfs_set_opt(info->mount_opt, COMPRESS);
-				btrfs_clear_opt(info->mount_opt, NODATACOW);
-				btrfs_clear_opt(info->mount_opt, NODATASUM);
+				btrfs_set_opt(new_opt, COMPRESS);
+				btrfs_clear_opt(new_opt, NODATACOW);
+				btrfs_clear_opt(new_opt, NODATASUM);
 			} else if (strcmp(args[0].from, "lzo") == 0) {
 				compress_type = "lzo";
 				info->compress_type = BTRFS_COMPRESS_LZO;
-				btrfs_set_opt(info->mount_opt, COMPRESS);
-				btrfs_clear_opt(info->mount_opt, NODATACOW);
-				btrfs_clear_opt(info->mount_opt, NODATASUM);
+				btrfs_set_opt(new_opt, COMPRESS);
+				btrfs_clear_opt(new_opt, NODATACOW);
+				btrfs_clear_opt(new_opt, NODATASUM);
 				btrfs_set_fs_incompat(info, COMPRESS_LZO);
 			} else if (strncmp(args[0].from, "no", 2) == 0) {
 				compress_type = "no";
-				btrfs_clear_opt(info->mount_opt, COMPRESS);
-				btrfs_clear_opt(info->mount_opt, FORCE_COMPRESS);
+				btrfs_clear_opt(new_opt, COMPRESS);
+				btrfs_clear_opt(new_opt, FORCE_COMPRESS);
 				compress_force = false;
 			} else {
 				ret = -EINVAL;
@@ -498,7 +501,8 @@ int btrfs_parse_options(struct btrfs_root *root, char *options)
 			}
 
 			if (compress_force) {
-				btrfs_set_and_info(root, FORCE_COMPRESS,
+				btrfs_set_and_info(info, new_opt,
+						   FORCE_COMPRESS,
 						   "force %s compression",
 						   compress_type);
 			} else {
@@ -512,29 +516,29 @@ int btrfs_parse_options(struct btrfs_root *root, char *options)
 				 * flag, otherwise, there is no way for users
 				 * to disable forcible compression separately.
 				 */
-				btrfs_clear_opt(info->mount_opt, FORCE_COMPRESS);
+				btrfs_clear_opt(new_opt, FORCE_COMPRESS);
 			}
 			break;
 		case Opt_ssd:
-			btrfs_set_and_info(root, SSD,
+			btrfs_set_and_info(info, new_opt, SSD,
 					   "use ssd allocation scheme");
 			break;
 		case Opt_ssd_spread:
-			btrfs_set_and_info(root, SSD_SPREAD,
+			btrfs_set_and_info(info, new_opt, SSD_SPREAD,
 					   "use spread ssd allocation scheme");
-			btrfs_set_opt(info->mount_opt, SSD);
+			btrfs_set_opt(new_opt, SSD);
 			break;
 		case Opt_nossd:
-			btrfs_set_and_info(root, NOSSD,
-					     "not using ssd allocation scheme");
-			btrfs_clear_opt(info->mount_opt, SSD);
+			btrfs_set_and_info(info, new_opt, NOSSD,
+					   "not using ssd allocation scheme");
+			btrfs_clear_opt(new_opt, SSD);
 			break;
 		case Opt_barrier:
-			btrfs_clear_and_info(root, NOBARRIER,
+			btrfs_clear_and_info(info, new_opt, NOBARRIER,
 					     "turning on barriers");
 			break;
 		case Opt_nobarrier:
-			btrfs_set_and_info(root, NOBARRIER,
+			btrfs_set_and_info(info, new_opt, NOBARRIER,
 					   "turning off barriers");
 			break;
 		case Opt_thread_pool:
@@ -594,19 +598,19 @@ int btrfs_parse_options(struct btrfs_root *root, char *options)
 			root->fs_info->sb->s_flags &= ~MS_POSIXACL;
 			break;
 		case Opt_notreelog:
-			btrfs_set_and_info(root, NOTREELOG,
+			btrfs_set_and_info(info, new_opt, NOTREELOG,
 					   "disabling tree log");
 			break;
 		case Opt_treelog:
-			btrfs_clear_and_info(root, NOTREELOG,
+			btrfs_clear_and_info(info, new_opt, NOTREELOG,
 					     "enabling tree log");
 			break;
 		case Opt_flushoncommit:
-			btrfs_set_and_info(root, FLUSHONCOMMIT,
+			btrfs_set_and_info(info, new_opt, FLUSHONCOMMIT,
 					   "turning on flush-on-commit");
 			break;
 		case Opt_noflushoncommit:
-			btrfs_clear_and_info(root, FLUSHONCOMMIT,
+			btrfs_clear_and_info(info, new_opt, FLUSHONCOMMIT,
 					     "turning off flush-on-commit");
 			break;
 		case Opt_ratio:
@@ -623,71 +627,71 @@ int btrfs_parse_options(struct btrfs_root *root, char *options)
 			}
 			break;
 		case Opt_discard:
-			btrfs_set_and_info(root, DISCARD,
+			btrfs_set_and_info(info, new_opt, DISCARD,
 					   "turning on discard");
 			break;
 		case Opt_nodiscard:
-			btrfs_clear_and_info(root, DISCARD,
+			btrfs_clear_and_info(info, new_opt, DISCARD,
 					     "turning off discard");
 			break;
 		case Opt_space_cache:
-			btrfs_set_and_info(root, SPACE_CACHE,
+			btrfs_set_and_info(info, new_opt, SPACE_CACHE,
 					   "enabling disk space caching");
 			break;
 		case Opt_rescan_uuid_tree:
-			btrfs_set_opt(info->mount_opt, RESCAN_UUID_TREE);
+			btrfs_set_opt(new_opt, RESCAN_UUID_TREE);
 			break;
 		case Opt_no_space_cache:
-			btrfs_clear_and_info(root, SPACE_CACHE,
+			btrfs_clear_and_info(info, new_opt, SPACE_CACHE,
 					     "disabling disk space caching");
 			break;
 		case Opt_inode_cache:
-			btrfs_set_pending_and_info(info, INODE_MAP_CACHE,
+			btrfs_set_and_info(info, new_opt, INODE_MAP_CACHE,
 					   "enabling inode map caching");
 			break;
 		case Opt_noinode_cache:
-			btrfs_clear_pending_and_info(info, INODE_MAP_CACHE,
+			btrfs_clear_and_info(info, new_opt, INODE_MAP_CACHE,
 					     "disabling inode map caching");
 			break;
 		case Opt_clear_cache:
-			btrfs_set_and_info(root, CLEAR_CACHE,
+			btrfs_set_and_info(info, new_opt, CLEAR_CACHE,
 					   "force clearing of disk cache");
 			break;
 		case Opt_user_subvol_rm_allowed:
-			btrfs_set_opt(info->mount_opt, USER_SUBVOL_RM_ALLOWED);
+			btrfs_set_opt(new_opt, USER_SUBVOL_RM_ALLOWED);
 			break;
 		case Opt_enospc_debug:
-			btrfs_set_opt(info->mount_opt, ENOSPC_DEBUG);
+			btrfs_set_opt(new_opt, ENOSPC_DEBUG);
 			break;
 		case Opt_noenospc_debug:
-			btrfs_clear_opt(info->mount_opt, ENOSPC_DEBUG);
+			btrfs_clear_opt(new_opt, ENOSPC_DEBUG);
 			break;
 		case Opt_defrag:
-			btrfs_set_and_info(root, AUTO_DEFRAG,
+			btrfs_set_and_info(info, new_opt, AUTO_DEFRAG,
 					   "enabling auto defrag");
 			break;
 		case Opt_nodefrag:
-			btrfs_clear_and_info(root, AUTO_DEFRAG,
+			btrfs_clear_and_info(info, new_opt, AUTO_DEFRAG,
 					     "disabling auto defrag");
 			break;
 		case Opt_recovery:
 			btrfs_info(root->fs_info, "enabling auto recovery");
-			btrfs_set_opt(info->mount_opt, RECOVERY);
+			btrfs_set_opt(new_opt, RECOVERY);
 			break;
 		case Opt_skip_balance:
-			btrfs_set_opt(info->mount_opt, SKIP_BALANCE);
+			btrfs_set_opt(new_opt, SKIP_BALANCE);
 			break;
 #ifdef CONFIG_BTRFS_FS_CHECK_INTEGRITY
 		case Opt_check_integrity_including_extent_data:
 			btrfs_info(root->fs_info,
 				   "enabling check integrity including extent data");
-			btrfs_set_opt(info->mount_opt,
+			btrfs_set_opt(new_opt,
 				      CHECK_INTEGRITY_INCLUDING_EXTENT_DATA);
-			btrfs_set_opt(info->mount_opt, CHECK_INTEGRITY);
+			btrfs_set_opt(new_opt, CHECK_INTEGRITY);
 			break;
 		case Opt_check_integrity:
 			btrfs_info(root->fs_info, "enabling check integrity");
-			btrfs_set_opt(info->mount_opt, CHECK_INTEGRITY);
+			btrfs_set_opt(new_opt, CHECK_INTEGRITY);
 			break;
 		case Opt_check_integrity_print_mask:
 			ret = match_int(&args[0], &intarg);
@@ -713,10 +717,10 @@ int btrfs_parse_options(struct btrfs_root *root, char *options)
 #endif
 		case Opt_fatal_errors:
 			if (strcmp(args[0].from, "panic") == 0)
-				btrfs_set_opt(info->mount_opt,
+				btrfs_set_opt(new_opt,
 					      PANIC_ON_FATAL_ERROR);
 			else if (strcmp(args[0].from, "bug") == 0)
-				btrfs_clear_opt(info->mount_opt,
+				btrfs_clear_opt(new_opt,
 					      PANIC_ON_FATAL_ERROR);
 			else {
 				ret = -EINVAL;
@@ -752,8 +756,11 @@ int btrfs_parse_options(struct btrfs_root *root, char *options)
 		}
 	}
 out:
-	if (!ret && btrfs_test_opt(root, SPACE_CACHE))
-		btrfs_info(root->fs_info, "disk space caching is enabled");
+	if (!ret) {
+		if (btrfs_raw_test_opt(new_opt, SPACE_CACHE))
+			btrfs_info(info, "disk space caching is enabled");
+		info->mount_opt = new_opt;
+	}
 	kfree(orig);
 	return ret;
 }
-- 
2.2.2

[PATCH RFC v3 3/5] btrfs: Introduce per-transaction mount_opt to keep mount option consistent during transaction.

[Qu Wenruo]

Before this patch, mount_opt is not consistent during a transaction.
btrfs_parse_options() can race with transaction.

Now each transaction will keep a copy of fs_info->mount_opt upon
creation, and new btrfs_test_trans_opt() macro is introduced to get the
mount_opt in the transaction.

Signed-off-by: Qu Wenruo <quwenruo@cn.fujitsu.com>
---
 fs/btrfs/transaction.c | 1 +
 fs/btrfs/transaction.h | 4 ++++
 2 files changed, 5 insertions(+)

diff --git a/fs/btrfs/transaction.c b/fs/btrfs/transaction.c
index 295a135..846e1b8 100644
--- a/fs/btrfs/transaction.c
+++ b/fs/btrfs/transaction.c
@@ -210,6 +210,7 @@ loop:
 		return -EROFS;
 	}
 
+	cur_trans->mount_opt = fs_info->mount_opt;
 	atomic_set(&cur_trans->num_writers, 1);
 	extwriter_counter_init(cur_trans, type);
 	init_waitqueue_head(&cur_trans->writer_wait);
diff --git a/fs/btrfs/transaction.h b/fs/btrfs/transaction.h
index fd400a3..4052879 100644
--- a/fs/btrfs/transaction.h
+++ b/fs/btrfs/transaction.h
@@ -52,6 +52,7 @@ struct btrfs_transaction {
 	struct list_head list;
 	struct extent_io_tree dirty_pages;
 	unsigned long start_time;
+	unsigned long mount_opt;
 	wait_queue_head_t writer_wait;
 	wait_queue_head_t commit_wait;
 	struct list_head pending_snapshots;
@@ -126,6 +127,9 @@ struct btrfs_pending_snapshot {
 	struct list_head list;
 };
 
+#define btrfs_test_trans_opt(trans, opt)	\
+	(btrfs_raw_test_opt(trans->transaction->mount_opt, opt))
+
 static inline void btrfs_set_inode_last_trans(struct btrfs_trans_handle *trans,
 					      struct inode *inode)
 {
-- 
2.2.2

[PATCH RFC v3 4/5] btrfs: Use btrfs_test_trans_opt() to handle SPACE_CACHE if it's under transaction protect.

[Qu Wenruo]

Convert btrfs_test_opt() to btrfs_test_trans_opt() if it's called under
transaction protection.
This will ensure SPACE_CACHE bit is consistent during transaction.

Signed-off-by: Qu Wenruo <quwenruo@cn.fujitsu.com>
---
 fs/btrfs/extent-tree.c | 2 +-
 fs/btrfs/transaction.c | 7 ++++---
 2 files changed, 5 insertions(+), 4 deletions(-)

diff --git a/fs/btrfs/extent-tree.c b/fs/btrfs/extent-tree.c
index 1511658..c968d12 100644
--- a/fs/btrfs/extent-tree.c
+++ b/fs/btrfs/extent-tree.c
@@ -3260,7 +3260,7 @@ again:
 
 	spin_lock(&block_group->lock);
 	if (block_group->cached != BTRFS_CACHE_FINISHED ||
-	    !btrfs_test_opt(root, SPACE_CACHE) ||
+	    !btrfs_test_trans_opt(trans, SPACE_CACHE) ||
 	    block_group->delalloc_bytes) {
 		/*
 		 * don't bother trying to write stuff out _if_
diff --git a/fs/btrfs/transaction.c b/fs/btrfs/transaction.c
index 846e1b8..aec5a5a 100644
--- a/fs/btrfs/transaction.c
+++ b/fs/btrfs/transaction.c
@@ -1507,7 +1507,8 @@ static noinline int create_pending_snapshots(struct btrfs_trans_handle *trans,
 	return ret;
 }
 
-static void update_super_roots(struct btrfs_root *root)
+static void update_super_roots(struct btrfs_trans_handle *trans,
+			       struct btrfs_root *root)
 {
 	struct btrfs_root_item *root_item;
 	struct btrfs_super_block *super;
@@ -1523,7 +1524,7 @@ static void update_super_roots(struct btrfs_root *root)
 	super->root = root_item->bytenr;
 	super->generation = root_item->generation;
 	super->root_level = root_item->level;
-	if (btrfs_test_opt(root, SPACE_CACHE))
+	if (btrfs_test_trans_opt(trans, SPACE_CACHE))
 		super->cache_generation = root_item->generation;
 	if (root->fs_info->update_uuid_tree_gen)
 		super->uuid_tree_generation = root_item->generation;
@@ -1987,7 +1988,7 @@ int btrfs_commit_transaction(struct btrfs_trans_handle *trans,
 	switch_commit_roots(cur_trans, root->fs_info);
 
 	assert_qgroups_uptodate(trans);
-	update_super_roots(root);
+	update_super_roots(trans, root);
 
 	btrfs_set_super_log_root(root->fs_info->super_copy, 0);
 	btrfs_set_super_log_root_level(root->fs_info->super_copy, 0);
-- 
2.2.2

[PATCH RFC v3 5/5] btrfs: Use btrfs_test_trans_opt() to handle INODE_CACHE if it's under transaction protect.

[Qu Wenruo]

Convert btrfs_test_opt() to btrfs_test_trans_opt() if it's called under
transaction protection.

This will ensure SPACE_CACHE bit is consistent during transaction.

Signed-off-by: Qu Wenruo <quwenruo@cn.fujitsu.com>
---
 fs/btrfs/ctree.h            | 1 -
 fs/btrfs/disk-io.c          | 4 ----
 fs/btrfs/free-space-cache.c | 2 +-
 fs/btrfs/inode-map.c        | 4 ++--
 fs/btrfs/transaction.c      | 9 ---------
 5 files changed, 3 insertions(+), 17 deletions(-)

diff --git a/fs/btrfs/ctree.h b/fs/btrfs/ctree.h
index 26bb47b..fa04db9 100644
--- a/fs/btrfs/ctree.h
+++ b/fs/btrfs/ctree.h
@@ -2108,7 +2108,6 @@ struct btrfs_ioctl_defrag_range_args {
 #define BTRFS_MOUNT_CHECK_INTEGRITY_INCLUDING_EXTENT_DATA (1 << 21)
 #define BTRFS_MOUNT_PANIC_ON_FATAL_ERROR	(1 << 22)
 #define BTRFS_MOUNT_RESCAN_UUID_TREE	(1 << 23)
-#define	BTRFS_MOUNT_CHANGE_INODE_CACHE	(1 << 24)
 
 #define BTRFS_DEFAULT_COMMIT_INTERVAL	(30)
 #define BTRFS_DEFAULT_MAX_INLINE	(8192)
diff --git a/fs/btrfs/disk-io.c b/fs/btrfs/disk-io.c
index 2d3c8b7..f4d168d 100644
--- a/fs/btrfs/disk-io.c
+++ b/fs/btrfs/disk-io.c
@@ -2832,10 +2832,6 @@ retry_root_backup:
 		btrfs_set_opt(fs_info->mount_opt, SSD);
 	}
 
-	/* Set the real inode map cache flag */
-	if (btrfs_test_opt(tree_root, CHANGE_INODE_CACHE))
-		btrfs_set_opt(tree_root->fs_info->mount_opt, INODE_MAP_CACHE);
-
 #ifdef CONFIG_BTRFS_FS_CHECK_INTEGRITY
 	if (btrfs_test_opt(tree_root, CHECK_INTEGRITY)) {
 		ret = btrfsic_mount(tree_root, fs_devices,
diff --git a/fs/btrfs/free-space-cache.c b/fs/btrfs/free-space-cache.c
index d6c03f7..2b9cabf 100644
--- a/fs/btrfs/free-space-cache.c
+++ b/fs/btrfs/free-space-cache.c
@@ -3348,7 +3348,7 @@ int btrfs_write_out_ino_cache(struct btrfs_root *root,
 	struct btrfs_free_space_ctl *ctl = root->free_ino_ctl;
 	int ret;
 
-	if (!btrfs_test_opt(root, INODE_MAP_CACHE))
+	if (!btrfs_test_trans_opt(trans, INODE_MAP_CACHE))
 		return 0;
 
 	ret = __btrfs_write_out_cache(root, inode, ctl, NULL, trans, path, 0);
diff --git a/fs/btrfs/inode-map.c b/fs/btrfs/inode-map.c
index d1edab5..49b089c 100644
--- a/fs/btrfs/inode-map.c
+++ b/fs/btrfs/inode-map.c
@@ -179,7 +179,7 @@ static void start_caching(struct btrfs_root *root)
 	if (IS_ERR(tsk)) {
 		btrfs_warn(root->fs_info, "failed to start inode caching task");
 		btrfs_clear_and_info(root->fs_info, root->fs_info->mount_opt,
-				CHANGE_INODE_CACHE,
+				INODE_MAP_CACHE,
 				"disabling inode map caching");
 	}
 }
@@ -406,7 +406,7 @@ int btrfs_save_ino_cache(struct btrfs_root *root,
 	if (btrfs_root_refs(&root->root_item) == 0)
 		return 0;
 
-	if (!btrfs_test_opt(root, INODE_MAP_CACHE))
+	if (!btrfs_test_trans_opt(trans, INODE_MAP_CACHE))
 		return 0;
 
 	path = btrfs_alloc_path();
diff --git a/fs/btrfs/transaction.c b/fs/btrfs/transaction.c
index aec5a5a..dd9e7af 100644
--- a/fs/btrfs/transaction.c
+++ b/fs/btrfs/transaction.c
@@ -1939,15 +1939,6 @@ int btrfs_commit_transaction(struct btrfs_trans_handle *trans,
 		goto scrub_continue;
 	}
 
-	/*
-	 * Since the transaction is done, we should set the inode map cache flag
-	 * before any other comming transaction.
-	 */
-	if (btrfs_test_opt(root, CHANGE_INODE_CACHE))
-		btrfs_set_opt(root->fs_info->mount_opt, INODE_MAP_CACHE);
-	else
-		btrfs_clear_opt(root->fs_info->mount_opt, INODE_MAP_CACHE);
-
 	/* commit_fs_roots gets rid of all the tree log roots, it is now
 	 * safe to free the root of tree log roots
 	 */
-- 
2.2.2

Re: [PATCH RFC v3 1/5] Revert "btrfs: add support for processing pending changes" related commits

[David Sterba]

On Fri, Jan 23, 2015 at 05:31:41PM +0800, Qu Wenruo wrote:
> For mount option change, later patches will introduce copy-n-update
> method and rwsem protects to keep mount options consistent during
> transaction.

That's a better approach, for the mount options.

> For sysfs interface to change label/features, it will keep the same
> behavior as 'btrfs pro set', so pending changes are also not needed.

This still leaves the transaction commit inside the syfs handler, that
was one of the points not to do that.

The callstack looks safe from, eg. the label handler:

[169148.523158] WARNING: CPU: 1 PID: 2044 at fs/btrfs/sysfs.c:394 btrfs_label_store+0x135/0x190 [btrfs]()
[169148.533925] Modules linked in: btrfs dm_flakey rpcsec_gss_krb5 loop [last unloaded: btrfs]
[169148.536950] CPU: 1 PID: 2044 Comm: bash Tainted: G        W      3.19.0-rc5-default+ #211
[169148.536952] Hardware name: Intel Corporation Santa Rosa platform/Matanzas, BIOS TSRSCRB1.86C.0047.B00.0610170821 10/17/06
[169148.536954]  000000000000018a ffff88007a753dc8 ffffffff81a9898b 000000000000018a
[169148.536963]  0000000000000000 ffff88007a753e08 ffffffff81077f65 ffff880077fb0100
[169148.536972]  ffff880075dc0000 ffff880077fbff00 0000000000000009 ffff880075dc06d0
[169148.536980] Call Trace:
[169148.536983]  [<ffffffff81a9898b>] dump_stack+0x4f/0x6c
[169148.536991]  [<ffffffff81077f65>] warn_slowpath_common+0x95/0xe0
[169148.537000]  [<ffffffff81077fca>] warn_slowpath_null+0x1a/0x20
[169148.537005]  [<ffffffffa0052b65>] btrfs_label_store+0x135/0x190 [btrfs]
[169148.537030]  [<ffffffff813ed8b7>] kobj_attr_store+0x17/0x20
[169148.537037]  [<ffffffff812147ff>] sysfs_kf_write+0x4f/0x70
[169148.537044]  [<ffffffff81213cc8>] kernfs_fop_write+0x128/0x180
[169148.537051]  [<ffffffff8119f404>] vfs_write+0xd4/0x1d0
[169148.537059]  [<ffffffff8119f7b9>] SyS_write+0x59/0xd0
[169148.537070]  [<ffffffff81a9f9d2>] system_call_fastpath+0x12/0x17

Lockep shows these locks held:

[169148.537296] 4 locks held by bash/2044:
[169148.537309]  #0:  (sb_writers#5){.+.+.+}, at: [<ffffffff8119f4e0>] vfs_write+0x1b0/0x1d0
[169148.537319]  #1:  (&of->mutex){+.+.+.}, at: [<ffffffff81213c2e>] kernfs_fop_write+0x8e/0x180
[169148.537330]  #2:  (s_active#214){.+.+.+}, at: [<ffffffff81213c36>] kernfs_fop_write+0x96/0x180
[169148.537342]  #3:  (tasklist_lock){.+.+..}, at: [<ffffffff810b9ed4>] debug_show_all_locks+0x44/0x1e0

#3 is from lockdep
#2 is not really a lock, annotated vfs atomic counter
#0 is annotated atomic, the freezing barrier

#1 is a kernfs mutex that, afaics it's per file, but I don't like to see
the lock dependency here. That's a lock we can see now, but it's outside
of btrfs or the vfs. It's a matter of precaution.

Re: [PATCH RFC v3 1/5] Revert "btrfs: add support for processing pending changes" related commits

[Qu Wenruo]

-------- Original Message --------
Subject: Re: [PATCH RFC v3 1/5] Revert "btrfs: add support for 
processing pending changes" related commits
From: David Sterba <dsterba@suse.cz>
To: Qu Wenruo <quwenruo@cn.fujitsu.com>
Date: 2015年01月23日 22:57
> On Fri, Jan 23, 2015 at 05:31:41PM +0800, Qu Wenruo wrote:
>> For mount option change, later patches will introduce copy-n-update
>> method and rwsem protects to keep mount options consistent during
>> transaction.
> That's a better approach, for the mount options.
I'm glad that you like this method.
Although the description in this patch is outdated, it is now 
per-transaction mount option.
Sorry for the confusion.
>
>> For sysfs interface to change label/features, it will keep the same
>> behavior as 'btrfs pro set', so pending changes are also not needed.
> This still leaves the transaction commit inside the syfs handler, that
> was one of the points not to do that.
>
> The callstack looks safe from, eg. the label handler:
>
> [169148.523158] WARNING: CPU: 1 PID: 2044 at fs/btrfs/sysfs.c:394 btrfs_label_store+0x135/0x190 [btrfs]()
> [169148.533925] Modules linked in: btrfs dm_flakey rpcsec_gss_krb5 loop [last unloaded: btrfs]
> [169148.536950] CPU: 1 PID: 2044 Comm: bash Tainted: G        W      3.19.0-rc5-default+ #211
> [169148.536952] Hardware name: Intel Corporation Santa Rosa platform/Matanzas, BIOS TSRSCRB1.86C.0047.B00.0610170821 10/17/06
> [169148.536954]  000000000000018a ffff88007a753dc8 ffffffff81a9898b 000000000000018a
> [169148.536963]  0000000000000000 ffff88007a753e08 ffffffff81077f65 ffff880077fb0100
> [169148.536972]  ffff880075dc0000 ffff880077fbff00 0000000000000009 ffff880075dc06d0
> [169148.536980] Call Trace:
> [169148.536983]  [<ffffffff81a9898b>] dump_stack+0x4f/0x6c
> [169148.536991]  [<ffffffff81077f65>] warn_slowpath_common+0x95/0xe0
> [169148.537000]  [<ffffffff81077fca>] warn_slowpath_null+0x1a/0x20
> [169148.537005]  [<ffffffffa0052b65>] btrfs_label_store+0x135/0x190 [btrfs]
> [169148.537030]  [<ffffffff813ed8b7>] kobj_attr_store+0x17/0x20
> [169148.537037]  [<ffffffff812147ff>] sysfs_kf_write+0x4f/0x70
> [169148.537044]  [<ffffffff81213cc8>] kernfs_fop_write+0x128/0x180
> [169148.537051]  [<ffffffff8119f404>] vfs_write+0xd4/0x1d0
> [169148.537059]  [<ffffffff8119f7b9>] SyS_write+0x59/0xd0
> [169148.537070]  [<ffffffff81a9f9d2>] system_call_fastpath+0x12/0x17
>
> Lockep shows these locks held:
>
> [169148.537296] 4 locks held by bash/2044:
> [169148.537309]  #0:  (sb_writers#5){.+.+.+}, at: [<ffffffff8119f4e0>] vfs_write+0x1b0/0x1d0
> [169148.537319]  #1:  (&of->mutex){+.+.+.}, at: [<ffffffff81213c2e>] kernfs_fop_write+0x8e/0x180
> [169148.537330]  #2:  (s_active#214){.+.+.+}, at: [<ffffffff81213c36>] kernfs_fop_write+0x96/0x180
> [169148.537342]  #3:  (tasklist_lock){.+.+..}, at: [<ffffffff810b9ed4>] debug_show_all_locks+0x44/0x1e0
>
> #3 is from lockdep
> #2 is not really a lock, annotated vfs atomic counter
> #0 is annotated atomic, the freezing barrier
>
> #1 is a kernfs mutex that, afaics it's per file, but I don't like to see
> the lock dependency here. That's a lock we can see now, but it's outside
> of btrfs or the vfs. It's a matter of precaution.
Thanks for pointing out the problem.
It makes sense to delay it.

But we have btrfs-workqueue, why not put it to "worker" workqueue?

If using this method, we can just wrap btrfs_ioctl_set_fslabel() and 
queue it to fs_info->workers.
This can avoid the the lockdep problem, but the behavior is still 
inconsistent with the synchronized
ioctl method.
Although not perfect, it should be good enough and still clean enough.

What do you think about such method?

Thanks,
Qu

Re: [PATCH RFC v3 1/5] Revert "btrfs: add support for processing pending changes" related commits

[Qu Wenruo]

-------- Original Message --------
Subject: Re: [PATCH RFC v3 1/5] Revert "btrfs: add support for 
processing pending changes" related commits
From: Qu Wenruo <quwenruo@cn.fujitsu.com>
To: dsterba@suse.cz, linux-btrfs@vger.kernel.org, miaoxie@huawei.com
Date: 2015年01月26日 08:37
>
> -------- Original Message --------
> Subject: Re: [PATCH RFC v3 1/5] Revert "btrfs: add support for 
> processing pending changes" related commits
> From: David Sterba <dsterba@suse.cz>
> To: Qu Wenruo <quwenruo@cn.fujitsu.com>
> Date: 2015年01月23日 22:57
>> On Fri, Jan 23, 2015 at 05:31:41PM +0800, Qu Wenruo wrote:
>>> For mount option change, later patches will introduce copy-n-update
>>> method and rwsem protects to keep mount options consistent during
>>> transaction.
>> That's a better approach, for the mount options.
> I'm glad that you like this method.
> Although the description in this patch is outdated, it is now 
> per-transaction mount option.
> Sorry for the confusion.
>>
>>> For sysfs interface to change label/features, it will keep the same
>>> behavior as 'btrfs pro set', so pending changes are also not needed.
>> This still leaves the transaction commit inside the syfs handler, that
>> was one of the points not to do that.
>>
>> The callstack looks safe from, eg. the label handler:
>>
>> [169148.523158] WARNING: CPU: 1 PID: 2044 at fs/btrfs/sysfs.c:394 
>> btrfs_label_store+0x135/0x190 [btrfs]()
>> [169148.533925] Modules linked in: btrfs dm_flakey rpcsec_gss_krb5 
>> loop [last unloaded: btrfs]
>> [169148.536950] CPU: 1 PID: 2044 Comm: bash Tainted: G W      
>> 3.19.0-rc5-default+ #211
>> [169148.536952] Hardware name: Intel Corporation Santa Rosa 
>> platform/Matanzas, BIOS TSRSCRB1.86C.0047.B00.0610170821 10/17/06
>> [169148.536954]  000000000000018a ffff88007a753dc8 ffffffff81a9898b 
>> 000000000000018a
>> [169148.536963]  0000000000000000 ffff88007a753e08 ffffffff81077f65 
>> ffff880077fb0100
>> [169148.536972]  ffff880075dc0000 ffff880077fbff00 0000000000000009 
>> ffff880075dc06d0
>> [169148.536980] Call Trace:
>> [169148.536983]  [<ffffffff81a9898b>] dump_stack+0x4f/0x6c
>> [169148.536991]  [<ffffffff81077f65>] warn_slowpath_common+0x95/0xe0
>> [169148.537000]  [<ffffffff81077fca>] warn_slowpath_null+0x1a/0x20
>> [169148.537005]  [<ffffffffa0052b65>] btrfs_label_store+0x135/0x190 
>> [btrfs]
>> [169148.537030]  [<ffffffff813ed8b7>] kobj_attr_store+0x17/0x20
>> [169148.537037]  [<ffffffff812147ff>] sysfs_kf_write+0x4f/0x70
>> [169148.537044]  [<ffffffff81213cc8>] kernfs_fop_write+0x128/0x180
>> [169148.537051]  [<ffffffff8119f404>] vfs_write+0xd4/0x1d0
>> [169148.537059]  [<ffffffff8119f7b9>] SyS_write+0x59/0xd0
>> [169148.537070]  [<ffffffff81a9f9d2>] system_call_fastpath+0x12/0x17
>>
>> Lockep shows these locks held:
>>
>> [169148.537296] 4 locks held by bash/2044:
>> [169148.537309]  #0:  (sb_writers#5){.+.+.+}, at: 
>> [<ffffffff8119f4e0>] vfs_write+0x1b0/0x1d0
>> [169148.537319]  #1:  (&of->mutex){+.+.+.}, at: [<ffffffff81213c2e>] 
>> kernfs_fop_write+0x8e/0x180
>> [169148.537330]  #2:  (s_active#214){.+.+.+}, at: 
>> [<ffffffff81213c36>] kernfs_fop_write+0x96/0x180
>> [169148.537342]  #3:  (tasklist_lock){.+.+..}, at: 
>> [<ffffffff810b9ed4>] debug_show_all_locks+0x44/0x1e0
>>
>> #3 is from lockdep
>> #2 is not really a lock, annotated vfs atomic counter
>> #0 is annotated atomic, the freezing barrier
>>
>> #1 is a kernfs mutex that, afaics it's per file, but I don't like to see
>> the lock dependency here. That's a lock we can see now, but it's outside
>> of btrfs or the vfs. It's a matter of precaution.
> Thanks for pointing out the problem.
> It makes sense to delay it.
>
> But we have btrfs-workqueue, why not put it to "worker" workqueue?
>
> If using this method, we can just wrap btrfs_ioctl_set_fslabel() and 
> queue it to fs_info->workers.
> This can avoid the the lockdep problem, but the behavior is still 
> inconsistent with the synchronized
> ioctl method.
> Although not perfect, it should be good enough and still clean enough.
Wait a second, #1 is a mutex, so I didn't quite understand the problem.
Just because it is not btrfs/vfs mutex so we want to avoid it?
It seems not convincing enough for me...

For readonly/freeze check, I prefer extra vfsmount from sb->s_mounts and 
use mnt_want_write() (handle ro)
and transaction (handle freeze).
So IMHO it just needs some small tweaks on the original implementation.

Thanks,
Qu
>
> What do you think about such method?
>
> Thanks,
> Qu

Re: [PATCH RFC v3 1/5] Revert "btrfs: add support for processing pending changes" related commits

[David Sterba]

On Mon, Jan 26, 2015 at 02:05:18PM +0800, Qu Wenruo wrote:
> 
> -------- Original Message --------
> Subject: Re: [PATCH RFC v3 1/5] Revert "btrfs: add support for 
> processing pending changes" related commits
> From: Qu Wenruo <quwenruo@cn.fujitsu.com>
> To: dsterba@suse.cz, linux-btrfs@vger.kernel.org, miaoxie@huawei.com
> Date: 2015年01月26日 08:37
> >
> > -------- Original Message --------
> > Subject: Re: [PATCH RFC v3 1/5] Revert "btrfs: add support for 
> > processing pending changes" related commits
> > From: David Sterba <dsterba@suse.cz>
> > To: Qu Wenruo <quwenruo@cn.fujitsu.com>
> > Date: 2015年01月23日 22:57
> >> On Fri, Jan 23, 2015 at 05:31:41PM +0800, Qu Wenruo wrote:
> >>> For mount option change, later patches will introduce copy-n-update
> >>> method and rwsem protects to keep mount options consistent during
> >>> transaction.
> >> That's a better approach, for the mount options.
> > I'm glad that you like this method.
> > Although the description in this patch is outdated, it is now 
> > per-transaction mount option.
> > Sorry for the confusion.
> >>
> >>> For sysfs interface to change label/features, it will keep the same
> >>> behavior as 'btrfs pro set', so pending changes are also not needed.
> >> This still leaves the transaction commit inside the syfs handler, that
> >> was one of the points not to do that.
> >>
> >> The callstack looks safe from, eg. the label handler:
> >>
> >> [169148.523158] WARNING: CPU: 1 PID: 2044 at fs/btrfs/sysfs.c:394 
> >> btrfs_label_store+0x135/0x190 [btrfs]()
> >> [169148.533925] Modules linked in: btrfs dm_flakey rpcsec_gss_krb5 
> >> loop [last unloaded: btrfs]
> >> [169148.536950] CPU: 1 PID: 2044 Comm: bash Tainted: G W      
> >> 3.19.0-rc5-default+ #211
> >> [169148.536952] Hardware name: Intel Corporation Santa Rosa 
> >> platform/Matanzas, BIOS TSRSCRB1.86C.0047.B00.0610170821 10/17/06
> >> [169148.536954]  000000000000018a ffff88007a753dc8 ffffffff81a9898b 
> >> 000000000000018a
> >> [169148.536963]  0000000000000000 ffff88007a753e08 ffffffff81077f65 
> >> ffff880077fb0100
> >> [169148.536972]  ffff880075dc0000 ffff880077fbff00 0000000000000009 
> >> ffff880075dc06d0
> >> [169148.536980] Call Trace:
> >> [169148.536983]  [<ffffffff81a9898b>] dump_stack+0x4f/0x6c
> >> [169148.536991]  [<ffffffff81077f65>] warn_slowpath_common+0x95/0xe0
> >> [169148.537000]  [<ffffffff81077fca>] warn_slowpath_null+0x1a/0x20
> >> [169148.537005]  [<ffffffffa0052b65>] btrfs_label_store+0x135/0x190 
> >> [btrfs]
> >> [169148.537030]  [<ffffffff813ed8b7>] kobj_attr_store+0x17/0x20
> >> [169148.537037]  [<ffffffff812147ff>] sysfs_kf_write+0x4f/0x70
> >> [169148.537044]  [<ffffffff81213cc8>] kernfs_fop_write+0x128/0x180
> >> [169148.537051]  [<ffffffff8119f404>] vfs_write+0xd4/0x1d0
> >> [169148.537059]  [<ffffffff8119f7b9>] SyS_write+0x59/0xd0
> >> [169148.537070]  [<ffffffff81a9f9d2>] system_call_fastpath+0x12/0x17
> >>
> >> Lockep shows these locks held:
> >>
> >> [169148.537296] 4 locks held by bash/2044:
> >> [169148.537309]  #0:  (sb_writers#5){.+.+.+}, at: 
> >> [<ffffffff8119f4e0>] vfs_write+0x1b0/0x1d0
> >> [169148.537319]  #1:  (&of->mutex){+.+.+.}, at: [<ffffffff81213c2e>] 
> >> kernfs_fop_write+0x8e/0x180
> >> [169148.537330]  #2:  (s_active#214){.+.+.+}, at: 
> >> [<ffffffff81213c36>] kernfs_fop_write+0x96/0x180
> >> [169148.537342]  #3:  (tasklist_lock){.+.+..}, at: 
> >> [<ffffffff810b9ed4>] debug_show_all_locks+0x44/0x1e0
> >>
> >> #3 is from lockdep
> >> #2 is not really a lock, annotated vfs atomic counter
> >> #0 is annotated atomic, the freezing barrier
> >>
> >> #1 is a kernfs mutex that, afaics it's per file, but I don't like to see
> >> the lock dependency here. That's a lock we can see now, but it's outside
> >> of btrfs or the vfs. It's a matter of precaution.
> > Thanks for pointing out the problem.
> > It makes sense to delay it.
> >
> > But we have btrfs-workqueue, why not put it to "worker" workqueue?

This only differs how the work item is stored until it's processed, it
will have to synchronize with the transaction commit anyway. The pending
changes can act as a special purpose work queue.

> > If using this method, we can just wrap btrfs_ioctl_set_fslabel() and 
> > queue it to fs_info->workers.
> > This can avoid the the lockdep problem,

There's no lockdep problem, the output comes from lockdep but just to
illustrate the lock chain at the time of potential commit time.

> > but the behavior is still 
> > inconsistent with the synchronized
> > ioctl method.
> > Although not perfect, it should be good enough and still clean enough.
> Wait a second, #1 is a mutex, so I didn't quite understand the problem.
> Just because it is not btrfs/vfs mutex so we want to avoid it?
> It seems not convincing enough for me...

It's sysfs/kernfs, isn't that enough? :)

The problem I see is that the whole transaction commit is called from
under that lock. We do some sysfs-related stuff like add or remove
objects (eg. devices), exporting space info etc.

Are we sure that there's no possible deadlock when we eg. change the
label via sysfs in the middle of a balance that removes some of the
files? Or other combination of operations. Can we guarantee that this
will be ok in the long term and not introduced accidentally?

> For readonly/freeze check, I prefer extra vfsmount from sb->s_mounts and 
> use mnt_want_write() (handle ro)
> and transaction (handle freeze).
> So IMHO it just needs some small tweaks on the original implementation.

But how can we do the mnt_want_write call inside sysfs handler?

Although we could drop the write support for label via sysfs in the end,
this whole exercise can be used later to decide what is/not safe to do
via sysfs. So it's good to try find ways right now.

Re: [PATCH RFC v3 1/5] Revert "btrfs: add support for processing pending changes" related commits

[Qu Wenruo]

-------- Original Message --------
Subject: Re: [PATCH RFC v3 1/5] Revert "btrfs: add support for 
processing pending changes" related commits
From: David Sterba <dsterba@suse.cz>
To: Qu Wenruo <quwenruo@cn.fujitsu.com>
Date: 2015年01月28日 21:25
> On Mon, Jan 26, 2015 at 02:05:18PM +0800, Qu Wenruo wrote:
>> -------- Original Message --------
>> Subject: Re: [PATCH RFC v3 1/5] Revert "btrfs: add support for
>> processing pending changes" related commits
>> From: Qu Wenruo <quwenruo@cn.fujitsu.com>
>> To: dsterba@suse.cz, linux-btrfs@vger.kernel.org, miaoxie@huawei.com
>> Date: 2015年01月26日 08:37
>>> [...snipped...]
>>> Thanks for pointing out the problem.
>>> It makes sense to delay it.
>>>
>>> But we have btrfs-workqueue, why not put it to "worker" workqueue?
> This only differs how the work item is stored until it's processed, it
> will have to synchronize with the transaction commit anyway. The pending
> changes can act as a special purpose work queue.
>
>>> If using this method, we can just wrap btrfs_ioctl_set_fslabel() and
>>> queue it to fs_info->workers.
>>> This can avoid the the lockdep problem,
> There's no lockdep problem, the output comes from lockdep but just to
> illustrate the lock chain at the time of potential commit time.
>
>>> but the behavior is still
>>> inconsistent with the synchronized
>>> ioctl method.
>>> Although not perfect, it should be good enough and still clean enough.
>> Wait a second, #1 is a mutex, so I didn't quite understand the problem.
>> Just because it is not btrfs/vfs mutex so we want to avoid it?
>> It seems not convincing enough for me...
> It's sysfs/kernfs, isn't that enough? :)
>
> The problem I see is that the whole transaction commit is called from
> under that lock. We do some sysfs-related stuff like add or remove
> objects (eg. devices), exporting space info etc.
>
> Are we sure that there's no possible deadlock when we eg. change the
> label via sysfs in the middle of a balance that removes some of the
> files? Or other combination of operations. Can we guarantee that this
> will be ok in the long term and not introduced accidentally?
For me, I didn't see the difference between VFS staff and sysfs/kernfs 
staff.
They both have their own locking things which is out of the control of 
btrfs.

But we are still using VFS staffs, right? If we want to use sysfs 
interfaces to do things like change label,
then it's our responsibility to ensure things works fine. If not we 
should either modify btrfs or sysfs to
do it, just like what we were doing with VFS staffs.

To ensure the cooperation works fine, we just need extra testcases, much 
like what we were doing.

So IMHO, I didn't really see the difference between VFS and sysfs staffs 
(except sysfs is not so wided adapted).
We just needs to do all the old style work, modify btrfs or sysfs or 
both and, and add tons of test case.
>
>> For readonly/freeze check, I prefer extra vfsmount from sb->s_mounts and
>> use mnt_want_write() (handle ro)
>> and transaction (handle freeze).
>> So IMHO it just needs some small tweaks on the original implementation.
> But how can we do the mnt_want_write call inside sysfs handler?
>
> Although we could drop the write support for label via sysfs in the end,
> this whole exercise can be used later to decide what is/not safe to do
> via sysfs. So it's good to try find ways right now.
I have already sent the new patchset, but it seems vger.kernel.org 
refused to accept the patchset,
I'll send it again try using my personal gmail account.

In that patchset, I added a new helper function in VFS, 
get_vfsmount_sb() to get a vfsmount from a sb,
so even we are in sysfs handler, we can still use mnt_want_write() to do 
all the protections.

Thanks,
Qu

Re: [PATCH RFC v3 1/5] Revert "btrfs: add support for processing pending changes" related commits

[David Sterba]

On Thu, Jan 29, 2015 at 09:15:12AM +0800, Qu Wenruo wrote:
> > Are we sure that there's no possible deadlock when we eg. change the
> > label via sysfs in the middle of a balance that removes some of the
> > files? Or other combination of operations. Can we guarantee that this
> > will be ok in the long term and not introduced accidentally?
> For me, I didn't see the difference between VFS staff and sysfs/kernfs 
> staff.
> They both have their own locking things which is out of the control of 
> btrfs.

VFS is a close neighbor in the layering, syscalls come through it,
affects filesystem state very often and API changes propagate to all the
filesystems.

Sysfs provides us some API but in a very limited scope compared to VFS.

> But we are still using VFS staffs, right? If we want to use sysfs 
> interfaces to do things like change label,
> then it's our responsibility to ensure things works fine.

I absolutelly agree with that and that's why I'm trying to minimize the
potential traps when the subsystems become interconnected too closely,
eg.  depending on internal locks.

> If not we 
> should either modify btrfs or sysfs to
> do it, just like what we were doing with VFS staffs.

This means changing innternal workings of the two, this seems unlikely
as we're mere users for them. Though we can bring new requests for API
or some such, we can't easily affect their internal logic just because
it's easy for us to throw a transaction commit somewhere and stop
caring.

> To ensure the cooperation works fine, we just need extra testcases, much 
> like what we were doing.
> 
> So IMHO, I didn't really see the difference between VFS and sysfs staffs 
> (except sysfs is not so wided adapted).
> We just needs to do all the old style work, modify btrfs or sysfs or 
> both and, and add tons of test case.

And I see a big difference, if nothing else, sysfs is user of VFS layer.