Sage Weil
2008-Aug-02 19:39 UTC
[PATCH] fix ioctl-initiated transactions vs wait_current_trans() deadlock
Hi Chris,
Commit 597:466b27332893 (btrfs_start_transaction: wait for commits in
progress) breaks the transaction start/stop ioctls by making
btrfs_start_transaction conditionally wait for the next transaction to
start. If an application artificially is holding a transaction open,
things deadlock.
This workaround maintains a count of open ioctl-initiated transactions in
fs_info, and avoids wait_current_trans() if any are currently open (in
start_transaction() and btrfs_throttle()). The start transaction ioctl
uses a new btrfs_start_ioctl_transaction() that _does_ call
wait_current_trans(), effectively pushing the join/wait decision to the
outer ioctl-initiated transaction.
This more or less neuters btrfs_throttle() when ioctl-initiated
transactions are in use, but that seems like a pretty fundamental
consequence of wrapping lots of write()''s in a transaction. Btrfs has
no
way to tell if the application considers a given operation as part of
it''s
transaction.
I''m not sure if throttle_on_drops() should also be avoided in that
case?
Obviously, if the transaction start/stop ioctls aren''t being used,
there
is no effect on current behavior.
Signed-off-by: Sage Weil <sage@newdream.net>
---
ctree.h | 1 +
ioctl.c | 12 +++++++++++-
transaction.c | 18 +++++++++++++-----
transaction.h | 2 ++
4 files changed, 27 insertions(+), 6 deletions(-)
diff -r 76a2ce720c36 ctree.h
--- a/ctree.h Fri Aug 01 15:11:20 2008 -0400
+++ b/ctree.h Sat Aug 02 12:06:17 2008 -0700
@@ -518,6 +518,7 @@ struct btrfs_fs_info {
u64 generation;
u64 last_trans_committed;
+ u64 open_ioctl_trans;
unsigned long mount_opt;
u64 max_extent;
u64 max_inline;
diff -r 76a2ce720c36 ioctl.c
--- a/ioctl.c Fri Aug 01 15:11:20 2008 -0400
+++ b/ioctl.c Sat Aug 02 12:06:17 2008 -0700
@@ -715,7 +715,12 @@ long btrfs_ioctl_trans_start(struct file
ret = -EINPROGRESS;
goto out;
}
- trans = btrfs_start_transaction(root, 0);
+
+ mutex_lock(&root->fs_info->trans_mutex);
+ root->fs_info->open_ioctl_trans++;
+ mutex_unlock(&root->fs_info->trans_mutex);
+
+ trans = btrfs_start_ioctl_transaction(root, 0);
if (trans)
file->private_data = trans;
else
@@ -745,6 +750,11 @@ long btrfs_ioctl_trans_end(struct file *
}
btrfs_end_transaction(trans, root);
file->private_data = 0;
+
+ mutex_lock(&root->fs_info->trans_mutex);
+ root->fs_info->open_ioctl_trans--;
+ mutex_unlock(&root->fs_info->trans_mutex);
+
out:
return ret;
}
diff -r 76a2ce720c36 transaction.c
--- a/transaction.c Fri Aug 01 15:11:20 2008 -0400
+++ b/transaction.c Sat Aug 02 12:06:17 2008 -0700
@@ -152,14 +152,14 @@ static void wait_current_trans(struct bt
}
struct btrfs_trans_handle *start_transaction(struct btrfs_root *root,
- int num_blocks, int join)
+ int num_blocks, int wait)
{
struct btrfs_trans_handle *h kmem_cache_alloc(btrfs_trans_handle_cachep,
GFP_NOFS);
int ret;
mutex_lock(&root->fs_info->trans_mutex);
- if (!join)
+ if ((wait == 1 && !root->fs_info->open_ioctl_trans) || wait ==
2)
wait_current_trans(root);
ret = join_transaction(root);
BUG_ON(ret);
@@ -180,13 +180,20 @@ struct btrfs_trans_handle *btrfs_start_t
struct btrfs_trans_handle *btrfs_start_transaction(struct btrfs_root *root,
int num_blocks)
{
- return start_transaction(root, num_blocks, 0);
+ return start_transaction(root, num_blocks, 1);
}
struct btrfs_trans_handle *btrfs_join_transaction(struct btrfs_root *root,
int num_blocks)
{
- return start_transaction(root, num_blocks, 1);
+ return start_transaction(root, num_blocks, 0);
}
+
+struct btrfs_trans_handle *btrfs_start_ioctl_transaction(struct btrfs_root *r,
+ int num_blocks)
+{
+ return start_transaction(r, num_blocks, 2);
+}
+
static noinline int wait_for_commit(struct btrfs_root *root,
struct btrfs_transaction *commit)
@@ -232,7 +239,8 @@ void btrfs_throttle(struct btrfs_root *r
void btrfs_throttle(struct btrfs_root *root)
{
mutex_lock(&root->fs_info->trans_mutex);
- wait_current_trans(root);
+ if (!root->fs_info->open_ioctl_trans)
+ wait_current_trans(root);
mutex_unlock(&root->fs_info->trans_mutex);
throttle_on_drops(root);
diff -r 76a2ce720c36 transaction.h
--- a/transaction.h Fri Aug 01 15:11:20 2008 -0400
+++ b/transaction.h Sat Aug 02 12:06:17 2008 -0700
@@ -83,6 +83,8 @@ struct btrfs_trans_handle *btrfs_start_t
int num_blocks);
struct btrfs_trans_handle *btrfs_join_transaction(struct btrfs_root *root,
int num_blocks);
+struct btrfs_trans_handle *btrfs_start_ioctl_transaction(struct btrfs_root *r,
+ int num_blocks);
int btrfs_write_and_wait_transaction(struct btrfs_trans_handle *trans,
struct btrfs_root *root);
int btrfs_commit_tree_roots(struct btrfs_trans_handle *trans,
--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs"
in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Chris Mason
2008-Aug-04 13:01 UTC
Re: [PATCH] fix ioctl-initiated transactions vs wait_current_trans() deadlock
On Sat, 2008-08-02 at 12:39 -0700, Sage Weil wrote:> Hi Chris, > > Commit 597:466b27332893 (btrfs_start_transaction: wait for commits in > progress) breaks the transaction start/stop ioctls by making > btrfs_start_transaction conditionally wait for the next transaction to > start. If an application artificially is holding a transaction open, > things deadlock. >Right, once I was done tuning the throttle code I was going to email you on this. Your patch looks pretty good, I''ll take it in today. -chris -- To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html