* [PATCH 1/3] btrfs: Fix the bug that fs_info->pending_changes is never cleared.
2015-01-20 18:05 [PULL] [PATCH 0/3] Btrfs, fixes for freezing vs pending changes David Sterba
@ 2015-01-20 18:05 ` David Sterba
2015-01-20 18:05 ` [PATCH 2/3] btrfs: Don't call btrfs_start_transaction() on frozen fs to avoid deadlock David Sterba
` (2 subsequent siblings)
3 siblings, 0 replies; 6+ messages in thread
From: David Sterba @ 2015-01-20 18:05 UTC (permalink / raw)
To: linux-btrfs; +Cc: guihc.fnst, quwenruo, miaoxie, David Sterba
From: Qu Wenruo <quwenruo@cn.fujitsu.com>
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>
Signed-off-by: David Sterba <dsterba@suse.cz>
---
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 a605d4e2f2bc..e88b59d13439 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.1.3
^ permalink raw reply related [flat|nested] 6+ messages in thread* [PATCH 2/3] btrfs: Don't call btrfs_start_transaction() on frozen fs to avoid deadlock.
2015-01-20 18:05 [PULL] [PATCH 0/3] Btrfs, fixes for freezing vs pending changes David Sterba
2015-01-20 18:05 ` [PATCH 1/3] btrfs: Fix the bug that fs_info->pending_changes is never cleared David Sterba
@ 2015-01-20 18:05 ` David Sterba
2015-01-20 18:05 ` [PATCH 3/3] btrfs: remove a no-op unfreeze superbock callback David Sterba
2015-01-23 2:32 ` [PULL] [PATCH 0/3] Btrfs, fixes for freezing vs pending changes Qu Wenruo
3 siblings, 0 replies; 6+ messages in thread
From: David Sterba @ 2015-01-20 18:05 UTC (permalink / raw)
To: linux-btrfs; +Cc: guihc.fnst, quwenruo, miaoxie, David Sterba
From: Qu Wenruo <quwenruo@cn.fujitsu.com>
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.
Cc: David Sterba <dsterba@suse.cz>
Reported-by: Gui Hecheng <guihc.fnst@cn.fujitsu.com>
Signed-off-by: Qu Wenruo <quwenruo@cn.fujitsu.com>
[enhanced comment, changed to SB_FREEZE_WRITE]
Signed-off-by: David Sterba <dsterba@suse.cz>
---
fs/btrfs/super.c | 10 ++++++++++
1 file changed, 10 insertions(+)
diff --git a/fs/btrfs/super.c b/fs/btrfs/super.c
index c86fb5438454..6f49b2872a64 100644
--- a/fs/btrfs/super.c
+++ b/fs/btrfs/super.c
@@ -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
^ permalink raw reply related [flat|nested] 6+ messages in thread* [PATCH 3/3] btrfs: remove a no-op unfreeze superbock callback
2015-01-20 18:05 [PULL] [PATCH 0/3] Btrfs, fixes for freezing vs pending changes David Sterba
2015-01-20 18:05 ` [PATCH 1/3] btrfs: Fix the bug that fs_info->pending_changes is never cleared David Sterba
2015-01-20 18:05 ` [PATCH 2/3] btrfs: Don't call btrfs_start_transaction() on frozen fs to avoid deadlock David Sterba
@ 2015-01-20 18:05 ` David Sterba
2015-01-23 2:32 ` [PULL] [PATCH 0/3] Btrfs, fixes for freezing vs pending changes Qu Wenruo
3 siblings, 0 replies; 6+ messages in thread
From: David Sterba @ 2015-01-20 18:05 UTC (permalink / raw)
To: linux-btrfs; +Cc: guihc.fnst, quwenruo, miaoxie, David Sterba
Signed-off-by: David Sterba <dsterba@suse.cz>
---
fs/btrfs/super.c | 6 ------
1 file changed, 6 deletions(-)
diff --git a/fs/btrfs/super.c b/fs/btrfs/super.c
index 6f49b2872a64..05fef198ff94 100644
--- a/fs/btrfs/super.c
+++ b/fs/btrfs/super.c
@@ -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
^ permalink raw reply related [flat|nested] 6+ messages in thread* Re: [PULL] [PATCH 0/3] Btrfs, fixes for freezing vs pending changes
2015-01-20 18:05 [PULL] [PATCH 0/3] Btrfs, fixes for freezing vs pending changes David Sterba
` (2 preceding siblings ...)
2015-01-20 18:05 ` [PATCH 3/3] btrfs: remove a no-op unfreeze superbock callback David Sterba
@ 2015-01-23 2:32 ` Qu Wenruo
2015-01-23 16:07 ` David Sterba
3 siblings, 1 reply; 6+ messages in thread
From: Qu Wenruo @ 2015-01-23 2:32 UTC (permalink / raw)
To: David Sterba, linux-btrfs; +Cc: guihc.fnst, miaoxie, clm
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 <dsterba@suse.cz>
To: <linux-btrfs@vger.kernel.org>
Date: 2015年01月21日 02:05
> 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(-)
>
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PULL] [PATCH 0/3] Btrfs, fixes for freezing vs pending changes
2015-01-23 2:32 ` [PULL] [PATCH 0/3] Btrfs, fixes for freezing vs pending changes Qu Wenruo
@ 2015-01-23 16:07 ` David Sterba
0 siblings, 0 replies; 6+ messages in thread
From: David Sterba @ 2015-01-23 16:07 UTC (permalink / raw)
To: Qu Wenruo; +Cc: David Sterba, linux-btrfs, guihc.fnst, miaoxie, clm
On Fri, Jan 23, 2015 at 10:32:16AM +0800, Qu Wenruo wrote:
> 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.
The commit is to flush all the data that should see the same state
of the mount option.
> What we need is just to keep the mount option doesn't change during
> transaction.
Yes, that's better.
> 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.
Ok.
> 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.
That's a unrelated fix to the pending changes, but a needed one.
> 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.
The calling context is different from 'btrfs prop set' and direct ioctl
call. Details in the reply to your RFC mail.
> What do you think about this idea?
> Although, I'm afraid this may revert all your pending_changes
> patches.... :-<
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.
^ permalink raw reply [flat|nested] 6+ messages in thread