[PATCH RFC v2 0/5] Fix bugs introduced by the new pending changes

[PATCH RFC v2 0/5] Fix bugs introduced by the new pending changes

[Qu Wenruo]

Commit 572d9ab7845ea0e0 ("btrfs: add support for processing pending
changes") introduced several bugs which will eventually cause a
deadlock.

The deadlock can be triggered by fstests/generic/068 with inode_cache
mount option.

The deadlock happens in the following flow:
moutn with inode_cache -> freeze fs -> sync -> unfreeze.

Sync_fs stack:				Thaw_fs stack:
(Holding s_umount)			(Waiting s_umount)
|- btrfs_sync_fs()
   |- start_transaction()
      (Waiting thaw_fs to unfreeze the fs)

The problem has several causes:
1) Cmpxchg in btrfs_apply_pending_changes() doesn't work
Cmpxchg in btrfs_apply_pending_changes() will only clear
fs_info->pending_changes if it is already 0.
So fs_info->pending_changes will never cleared, and every sync_fs() on
frozen btrfs will try to start a new transaction and deadlock.

So patch 1 fixes it by using xchg() other than cmpxchg().

2) btrfs_freeze() doesn't handle and clear pending_changes
If btrfs start a transaction if there are pending changes but no running
transaction, sync should never start a transaction on frozen fs.
(Except the following sysfs case)

So patch 2~3 fixes it by adding pending changes handler in
btrfs_freeze().

3) Changes through sysfs interface can create pending changes without
   waiting for unfreezing.
Since sysfs changes doesn't go through normal open routine,
which will initiate sb_start_*write() and waiting for unfreezing,
changes through sysfs like changing label will set
fs_info->pending_changes to non-zero, and later sync_fs will deadlock
again.

So patch 4~5 reverts the commits relating patches to fix such problem.

Qu Wenruo (5):
  btrfs: Fix the bug that fs_info->pending_changes is never cleared.
  btrfs: Add btrfs_start_transaction_freeze() to start transaction in   
     btrfs_freeze()
  btrfs: Handle pending changes in btrfs_freeze().
  Revert "btrfs: move commit out of sysfs when changing label"
  Revert "btrfs: move commit out of sysfs when changing features"

 fs/btrfs/super.c       | 52 ++++++++++++++++++++++++--------------------------
 fs/btrfs/sysfs.c       | 34 ++++++++++++++++++++-------------
 fs/btrfs/transaction.c | 10 +++++++++-
 fs/btrfs/transaction.h |  2 ++
 4 files changed, 57 insertions(+), 41 deletions(-)

-- 
2.2.2

[PATCH RFC v2 1/5] btrfs: Fix the bug that fs_info->pending_changes is never cleared.

[Qu Wenruo]

Fs_info->pending_changes is never cleared since the original code uses
cmpxchg(&fs_info->pending_changes, 0, 0), which will only clear it if
pending_changes is already 0.

This will cause a lot of problem when mount it with inode_cache mount
option.
If the btrfs is mounted as inode_cache, pending_changes will always be
1, even when the fs is frozen.

Signed-off-by: Qu Wenruo <quwenruo@cn.fujitsu.com>
---
 fs/btrfs/transaction.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/fs/btrfs/transaction.c b/fs/btrfs/transaction.c
index a605d4e..e88b59d 100644
--- a/fs/btrfs/transaction.c
+++ b/fs/btrfs/transaction.c
@@ -2118,7 +2118,7 @@ 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);
+	prev = xchg(&fs_info->pending_changes, 0);
 	if (!prev)
 		return;
 
-- 
2.2.2

[PATCH RFC v2 2/5] btrfs: Add btrfs_start_transaction_freeze() to start transaction in btrfs_freeze()

[Qu Wenruo]

Add btrfs_start_transaction_freeze() function.

This is needed for btrfs_freeze() to handle the pending changes.
If there is no running transaction when btrfs_freeze() is called,
btrfs_freeze() needs to start a transaction to handle the pending
changes, and it can't initial a sb_start_intwrite() call.

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

diff --git a/fs/btrfs/transaction.c b/fs/btrfs/transaction.c
index e88b59d..13a2b67 100644
--- a/fs/btrfs/transaction.c
+++ b/fs/btrfs/transaction.c
@@ -570,6 +570,14 @@ struct btrfs_trans_handle *btrfs_start_transaction_lflush(
 				 BTRFS_RESERVE_FLUSH_LIMIT);
 }
 
+/* Only used in btrfs_freeze() */
+struct btrfs_trans_handle *btrfs_start_transaction_freeze(
+					struct btrfs_root *root, int num_items)
+{
+	return start_transaction(root, num_items, __TRANS_START,
+				 BTRFS_RESERVE_FLUSH_ALL);
+}
+
 struct btrfs_trans_handle *btrfs_join_transaction(struct btrfs_root *root)
 {
 	return start_transaction(root, 0, TRANS_JOIN, 0);
diff --git a/fs/btrfs/transaction.h b/fs/btrfs/transaction.h
index 00ed29c..49bd7eb 100644
--- a/fs/btrfs/transaction.h
+++ b/fs/btrfs/transaction.h
@@ -140,6 +140,8 @@ struct btrfs_trans_handle *btrfs_start_transaction(struct btrfs_root *root,
 						   int num_items);
 struct btrfs_trans_handle *btrfs_start_transaction_lflush(
 					struct btrfs_root *root, int num_items);
+struct btrfs_trans_handle *btrfs_start_transaction_freeze(
+					struct btrfs_root *root, int num_items);
 struct btrfs_trans_handle *btrfs_join_transaction(struct btrfs_root *root);
 struct btrfs_trans_handle *btrfs_join_transaction_nolock(struct btrfs_root *root);
 struct btrfs_trans_handle *btrfs_attach_transaction(struct btrfs_root *root);
-- 
2.2.2

[PATCH RFC v2 3/5] btrfs: Handle pending changes in btrfs_freeze().

[Qu Wenruo]

Before the patch, btrfs_freeze() does not start a transaction if there
is pending changes but no transaction is running.

This will lead to deadlock if the fs is frozen but there are pending
changes, since sync_fs will start a transaction with s_umount mutex,
blocked by frozen fs. And later unfreeze will wait sync_fs to release
the s_umount mutex.

This patch will ensure the frozen fs from having pending changes, except
some one use sysfs interfaces to create pending changes again.

Signed-off-by: Qu Wenruo <quwenruo@cn.fujitsu.com>
---
 fs/btrfs/super.c | 52 +++++++++++++++++++++++++---------------------------
 1 file changed, 25 insertions(+), 27 deletions(-)

diff --git a/fs/btrfs/super.c b/fs/btrfs/super.c
index 60f7cbe..5229786 100644
--- a/fs/btrfs/super.c
+++ b/fs/btrfs/super.c
@@ -975,32 +975,22 @@ fail_close:
 	return err;
 }
 
-int btrfs_sync_fs(struct super_block *sb, int wait)
+static int commit_current_or_new_trans(struct btrfs_fs_info *fs_info,
+				       int freezing)
 {
 	struct btrfs_trans_handle *trans;
-	struct btrfs_fs_info *fs_info = btrfs_sb(sb);
 	struct btrfs_root *root = fs_info->tree_root;
 
-	trace_btrfs_sync_fs(wait);
-
-	if (!wait) {
-		filemap_flush(fs_info->btree_inode->i_mapping);
-		return 0;
-	}
-
-	btrfs_wait_ordered_roots(fs_info, -1);
-
 	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
-			 */
+			/* No pending changes, don't bother*/
 			if (fs_info->pending_changes == 0)
 				return 0;
-			trans = btrfs_start_transaction(root, 0);
+			if (freezing)
+				trans = btrfs_start_transaction_freeze(root, 0);
+			else
+				trans = btrfs_start_transaction(root, 0);
 		} else {
 			return PTR_ERR(trans);
 		}
@@ -1008,6 +998,22 @@ int btrfs_sync_fs(struct super_block *sb, int wait)
 	return btrfs_commit_transaction(trans, root);
 }
 
+int btrfs_sync_fs(struct super_block *sb, int wait)
+{
+	struct btrfs_fs_info *fs_info = btrfs_sb(sb);
+
+	trace_btrfs_sync_fs(wait);
+
+	if (!wait) {
+		filemap_flush(fs_info->btree_inode->i_mapping);
+		return 0;
+	}
+
+	btrfs_wait_ordered_roots(fs_info, -1);
+
+	return commit_current_or_new_trans(fs_info, 0);
+}
+
 static int btrfs_show_options(struct seq_file *seq, struct dentry *dentry)
 {
 	struct btrfs_fs_info *info = btrfs_sb(dentry->d_sb);
@@ -1935,17 +1941,9 @@ static long btrfs_control_ioctl(struct file *file, unsigned int cmd,
 
 static int btrfs_freeze(struct super_block *sb)
 {
-	struct btrfs_trans_handle *trans;
-	struct btrfs_root *root = btrfs_sb(sb)->tree_root;
+	struct btrfs_fs_info *fs_info = btrfs_sb(sb);
 
-	trans = btrfs_attach_transaction_barrier(root);
-	if (IS_ERR(trans)) {
-		/* no transaction, don't bother */
-		if (PTR_ERR(trans) == -ENOENT)
-			return 0;
-		return PTR_ERR(trans);
-	}
-	return btrfs_commit_transaction(trans, root);
+	return commit_current_or_new_trans(fs_info, 1);
 }
 
 static int btrfs_unfreeze(struct super_block *sb)
-- 
2.2.2

[PATCH RFC v2 4/5] Revert "btrfs: move commit out of sysfs when changing label"

[Qu Wenruo]

This reverts commit a6f69dc8018d ("btrfs: move commit out of sysfs when
changing label").
Unlike most btrfs file operations, sysfs don't go through the normal
open/read/write routine which will initiate a sb_start_*write/read.

This is very dangerous on frozen btrfs, if using the pending changes
method, sysfs change can make fs_info->pending_changes to 1, and later
sync_fs() call will cause deadlock.

So revert the patch to deal the deadlock caused by the following
workload:
freeze() -> change lable -> sync() -> unfreeze()

Signed-off-by: Qu Wenruo <quwenruo@cn.fujitsu.com>
---
 fs/btrfs/sysfs.c | 21 +++++++++++++--------
 1 file changed, 13 insertions(+), 8 deletions(-)

diff --git a/fs/btrfs/sysfs.c b/fs/btrfs/sysfs.c
index 92db3f6..226f726 100644
--- a/fs/btrfs/sysfs.c
+++ b/fs/btrfs/sysfs.c
@@ -369,6 +369,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 +386,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);
 
-- 
2.2.2

[PATCH RFC v2 5/5] Revert "btrfs: move commit out of sysfs when changing features"

[Qu Wenruo]

This reverts commit 0eae2747ec1dd ("btrfs: move commit out of sysfs
when changing features").

Unlike most btrfs file operations, sysfs don't go through the normal
open/read/write routine which will initiate a sb_start_*write/read.

This is very dangerous on frozen btrfs, if using the pending changes
method, sysfs change can make fs_info->pending_changes to 1, and later
sync_fs() call will cause deadlock.

So revert the patch to deal the deadlock caused by the following workload:
freeze() -> change features -> sync() -> unfreeze()

Signed-off-by: Qu Wenruo <quwenruo@cn.fujitsu.com>
---
 fs/btrfs/sysfs.c | 13 ++++++++-----
 1 file changed, 8 insertions(+), 5 deletions(-)

diff --git a/fs/btrfs/sysfs.c b/fs/btrfs/sysfs.c
index 226f726..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;
 }
-- 
2.2.2

Re: [PATCH RFC v2 0/5] Fix bugs introduced by the new pending changes

[David Sterba]

On Tue, Jan 20, 2015 at 05:05:32PM +0800, Qu Wenruo wrote:
> Commit 572d9ab7845ea0e0 ("btrfs: add support for processing pending
> changes") introduced several bugs which will eventually cause a
> deadlock.
> 
> The deadlock can be triggered by fstests/generic/068 with inode_cache
> mount option.
> 
> The deadlock happens in the following flow:
> moutn with inode_cache -> freeze fs -> sync -> unfreeze.
> 
> Sync_fs stack:				Thaw_fs stack:
> (Holding s_umount)			(Waiting s_umount)
> |- btrfs_sync_fs()
>    |- start_transaction()
>       (Waiting thaw_fs to unfreeze the fs)
> 
> The problem has several causes:
> 1) Cmpxchg in btrfs_apply_pending_changes() doesn't work
> Cmpxchg in btrfs_apply_pending_changes() will only clear
> fs_info->pending_changes if it is already 0.
> So fs_info->pending_changes will never cleared, and every sync_fs() on
> frozen btrfs will try to start a new transaction and deadlock.
> 
> So patch 1 fixes it by using xchg() other than cmpxchg().

Right, that's a silly mistake.

> 2) btrfs_freeze() doesn't handle and clear pending_changes

I don't se a major problem to delay the pending changes, ie. do not
start a new transaction (like the IOC_SYNC does). This would be handled
after thawing and the next full transaction commit.

We could flush the pending changes from the freeze callback, this adds
not much extra work but could mean that some user-triggered change hits
the disk before the filesystem becomes frozen.

> If btrfs start a transaction if there are pending changes but no running
> transaction,

> sync should never start a transaction on frozen fs.

I agree with that and this is the actual bug I see here.

> (Except the following sysfs case)
> 
> So patch 2~3 fixes it by adding pending changes handler in
> btrfs_freeze().
>
> 3) Changes through sysfs interface can create pending changes without
>    waiting for unfreezing.

The sysfs callbacks were changed so they do not trigger any writes, just
make a note for later. There's no difference regarding freezing.

> Since sysfs changes doesn't go through normal open routine,
> which will initiate sb_start_*write() and waiting for unfreezing,
> changes through sysfs like changing label will set
> fs_info->pending_changes to non-zero, and later sync_fs will deadlock
> again.
> 
> So patch 4~5 reverts the commits relating patches to fix such problem.

I don't agree with the reverts for obvious reasons, that's just changing
one buggy behaviour with another one.

Re: [PATCH RFC v2 1/5] btrfs: Fix the bug that fs_info->pending_changes is never cleared.

[David Sterba]

On Tue, Jan 20, 2015 at 05:05:33PM +0800, Qu Wenruo wrote:
> Fs_info->pending_changes is never cleared since the original code uses
> cmpxchg(&fs_info->pending_changes, 0, 0), which will only clear it if
> pending_changes is already 0.
> 
> This will cause a lot of problem when mount it with inode_cache mount
> option.
> If the btrfs is mounted as inode_cache, pending_changes will always be
> 1, even when the fs is frozen.
> 
> Signed-off-by: Qu Wenruo <quwenruo@cn.fujitsu.com>

Reviewed-by: David Sterba <dsterba@suse.cz>

Chris, please add this to 3.19-rc, introduced in this dev cycle.