Btrfs, fixes for freezing vs pending changes

There was some churn regarding the patches to $SUBJ, here's what I think is enough to fix it for 3.19.

It's based on top of my other patch "btrfs: sync ioctl, handle errors after transaction start" that might land in Chris' for-linus already so it's not included here.

The patches are for review by the involved people, also available in this branch:

git://git.kernel.org/pub/scm/linux/kernel/git/kdave/linux.git

David Sterba (1):
btrfs: remove a no-op unfreeze superbock callback

Qu Wenruo (2):
btrfs: Fix the bug that fs_info->pending_changes is never cleared.
btrfs: Don't call btrfs_start_transaction() on frozen fs to avoid
deadlock.

 fs/btrfs/super.c       | 16 ++++++++++------
 fs/btrfs/transaction.c |  2 +-
 2 files changed, 11 insertions(+), 7 deletions(-)

2.1.3

From: Qu Wenruo <[email protected]>

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.

---
 fs/btrfs/transaction.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

index a605d4e2f2bc..e88b59d13439 100644
@@ -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.1.3

From: Qu Wenruo <[email protected]>

Commit 6b5fe46dfa52 (btrfs: do commit in sync_fs if there are pending changes) will call btrfs_start_transaction() in sync_fs(), to handle some operations needed to be done in next transaction.

However this can cause deadlock if the filesystem is frozen, with the following sys_r+w output:

[  143.255932] Call Trace:
[  143.255936]  [<ffffffff816c0e09>] schedule+0x29/0x70
[  143.255939]  [<ffffffff811cb7f3>] __sb_start_write+0xb3/0x100
[  143.255971]  [<ffffffffa040ec06>] start_transaction+0x2e6/0x5a0
[btrfs]
[  143.255992]  [<ffffffffa040f1eb>] btrfs_start_transaction+0x1b/0x20
[btrfs]
[  143.256003]  [<ffffffffa03dc0ba>] btrfs_sync_fs+0xca/0xd0 [btrfs]
[  143.256007]  [<ffffffff811f7be0>] sync_fs_one_sb+0x20/0x30
[  143.256011]  [<ffffffff811cbd01>] iterate_supers+0xe1/0xf0
[  143.256014]  [<ffffffff811f7d75>] sys_sync+0x55/0x90
[  143.256017]  [<ffffffff816c49d2>] system_call_fastpath+0x12/0x17
[  143.256111] Call Trace:
[  143.256114]  [<ffffffff816c0e09>] schedule+0x29/0x70
[  143.256119]  [<ffffffff816c3405>] rwsem_down_write_failed+0x1c5/0x2d0
[  143.256123]  [<ffffffff8133f013>] call_rwsem_down_write_failed+0x13/0x20
[  143.256131]  [<ffffffff811caae8>] thaw_super+0x28/0xc0
[  143.256135]  [<ffffffff811db3e5>] do_vfs_ioctl+0x3f5/0x540
[  143.256187]  [<ffffffff811db5c1>] SyS_ioctl+0x91/0xb0
[  143.256213]  [<ffffffff816c49d2>] system_call_fastpath+0x12/0x17

The reason is like the following:
(Holding s_umount)
VFS sync_fs staff:
|- btrfs_sync_fs()
|- btrfs_start_transaction()
|- sb_start_intwrite()
(Waiting thaw_fs to unfreeze)
VFS thaw_fs staff:
thaw_fs() (Waiting sync_fs to release
s_umount)

So deadlock happens.
This can be easily triggered by fstest/generic/068 with inode_cache mount option.

The fix is to check if the fs is frozen, if the fs is frozen, just return and waiting for the next transaction.

---
 fs/btrfs/super.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

index c86fb5438454..6f49b2872a64 100644
@@ -1000,6 +1000,16 @@ int btrfs_sync_fs(struct super_block *sb, int wait)
 			 */
 			if (fs_info->pending_changes == 0)
 				return 0;
+			/*
+			 * A non-blocking test if the fs is frozen. We must not
+			 * start a new transaction here otherwise a deadlock
+			 * happens. The pending operations are delayed to the
+			 * next commit after thawing.
+			 */
+			if (__sb_start_write(sb, SB_FREEZE_WRITE, false))
+				__sb_end_write(sb, SB_FREEZE_WRITE);
+			else
+				return 0;
 			trans = btrfs_start_transaction(root, 0);
 		}
 		if (IS_ERR(trans))

2.1.3

---
 fs/btrfs/super.c | 6 ------
 1 file changed, 6 deletions(-)

index 6f49b2872a64..05fef198ff94 100644
@@ -1958,11 +1958,6 @@ static int btrfs_freeze(struct super_block *sb)
 	return btrfs_commit_transaction(trans, root);
 }
 
-static int btrfs_unfreeze(struct super_block *sb)
-{
-	return 0;
-}
-
 static int btrfs_show_devname(struct seq_file *m, struct dentry *root)
 {
 	struct btrfs_fs_info *fs_info = btrfs_sb(root->d_sb);

@@ -2011,7 +2006,6 @@ static const struct super_operations btrfs_super_ops = {
 	.statfs		= btrfs_statfs,
 	.remount_fs	= btrfs_remount,
 	.freeze_fs	= btrfs_freeze,
-	.unfreeze_fs	= btrfs_unfreeze,
 };
 
 static const struct file_operations btrfs_ctl_fops = {

2.1.3

Hi David,

Would you please delay this patchset to be merged into next rc?

In fact, I think there is a better solution for such problem.

1) mount option change problem.
In fact, there is no need to start a transaction to change mount option, since it doesn't change anything on-disk.
What we need is just to keep the mount option doesn't change during transaction. (Although several exceptions still exists, but should be OK)

So I prefer to add a rwsem to protect mount_opt, each btrfs_transaction will hold the read lock on it and upon btrfs_put_transaction(), read unlock it.
btrfs_parse_option() should wait for write lock to change it.

BTW, current btrfs_parse_options() is not atomic, and for nospace_cache mount option, SPACE_CACHE bit is always first set and later cleared, which created a window btrfs_commit_transaction() can create space cache. I'll solve it by using copy-n-update method.

2) Sysfs label/feature change problem
For this problem, I agree with Miao to keep the behavior the same as "btrfs pro set" command, since it will write something on disk.
And since btrfs_ioctl_set_fslabel() is synchronized, I didn't see the necessity to change it to async using sysfs.

What do you think about this idea?
Although, I'm afraid this may revert all your pending_changes patches.... :-< (In fact, I'm already dealing the mount option change problem)

Thanks,
Qu

-------- Original Message --------
Subject: [PULL] [PATCH 0/3] Btrfs, fixes for freezing vs pending changes
From: David Sterba <[email protected]>
To: <[email protected]>
Date: 2015年01月21日 02:05

The commit is to flush all the data that should see the same state of the mount option.

Yes, that's better.

Ok.

That's a unrelated fix to the pending changes, but a needed one.

The calling context is different from 'btrfs prop set' and direct ioctl call. Details in the reply to your RFC mail.

Given the timeframe of dev cycle, I think the minimal fix to avoid the sync deadlock will probably go in, as is in the for-linus branch right now.

Next, the lock-protected mount options would replace current way of handling the inode_cache and we can see if we find different way how to get rid of the async commit.