Linux Btrfs filesystem development
 help / color / mirror / Atom feed
* [PATCH] btrfs: qgroup: don't try to wait flushing if we're already holding a transaction
@ 2020-12-04  1:24 Qu Wenruo
  2020-12-04  1:24 ` [PATCH] btrfs: qgroup: don't commit transaction when we already hold the handle Qu Wenruo
                   ` (2 more replies)
  0 siblings, 3 replies; 7+ messages in thread
From: Qu Wenruo @ 2020-12-04  1:24 UTC (permalink / raw)
  To: linux-btrfs

There is a chance of racing for qgroup flushing which may lead to
deadlock:

	Thread A		|	Thread B
   (no trans handler hold)	|  (already hold a trans handler)
--------------------------------+--------------------------------
__btrfs_qgroup_reserve_meta()   | __btrfs_qgroup_reserve_meta()
|- try_flush_qgroup()		| |- try_flushing_qgroup()
   |- QGROUP_FLUSHING bit set   |    |
   |				|    |- test_and_set_bit()
   |				|    |- wait_event()
   |- btrfs_join_transaction()	|
   |- btrfs_commit_transaction()|

			!!! DEAD LOCK !!!

Since thread A want to commit transaction, but thread B is hold a
transaction handler, blocking the commit.
At the same time, thread B is waiting thread A to finish it commit.

This is just a hot fix, and would lead to more EDQUOT when we're near
the qgroup limit.

The root fix would to make all metadata/data reservation to happen
without a transaction handler hold.

Signed-off-by: Qu Wenruo <wqu@suse.com>
---
 fs/btrfs/qgroup.c | 31 +++++++++++++++++++++----------
 1 file changed, 21 insertions(+), 10 deletions(-)

diff --git a/fs/btrfs/qgroup.c b/fs/btrfs/qgroup.c
index fe3046007f52..7785dfa348d2 100644
--- a/fs/btrfs/qgroup.c
+++ b/fs/btrfs/qgroup.c
@@ -3530,16 +3530,6 @@ static int try_flush_qgroup(struct btrfs_root *root)
 	int ret;
 	bool can_commit = true;
 
-	/*
-	 * We don't want to run flush again and again, so if there is a running
-	 * one, we won't try to start a new flush, but exit directly.
-	 */
-	if (test_and_set_bit(BTRFS_ROOT_QGROUP_FLUSHING, &root->state)) {
-		wait_event(root->qgroup_flush_wait,
-			!test_bit(BTRFS_ROOT_QGROUP_FLUSHING, &root->state));
-		return 0;
-	}
-
 	/*
 	 * If current process holds a transaction, we shouldn't flush, as we
 	 * assume all space reservation happens before a transaction handle is
@@ -3554,6 +3544,27 @@ static int try_flush_qgroup(struct btrfs_root *root)
 	    current->journal_info != BTRFS_SEND_TRANS_STUB)
 		can_commit = false;
 
+	/*
+	 * We don't want to run flush again and again, so if there is a running
+	 * one, we won't try to start a new flush, but exit directly.
+	 */
+	if (test_and_set_bit(BTRFS_ROOT_QGROUP_FLUSHING, &root->state)) {
+		/*
+		 * We are already holding a trans, thus we can block other
+		 * threads from flushing.
+		 * So exit right now. This increases the chance of EDQUOT for
+		 * heavy load and near limit cases.
+		 * But we can argue that if we're already near limit, EDQUOT
+		 * is unavoidable anyway.
+		 */
+		if (!can_commit)
+			return 0;
+
+		wait_event(root->qgroup_flush_wait,
+			!test_bit(BTRFS_ROOT_QGROUP_FLUSHING, &root->state));
+		return 0;
+	}
+
 	ret = btrfs_start_delalloc_snapshot(root);
 	if (ret < 0)
 		goto out;
-- 
2.29.2


^ permalink raw reply related	[flat|nested] 7+ messages in thread
end of thread, other threads:[~2020-12-05  2:57 UTC | newest]

Thread overview: 7+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2020-12-04  1:24 [PATCH] btrfs: qgroup: don't try to wait flushing if we're already holding a transaction Qu Wenruo
2020-12-04  1:24 ` [PATCH] btrfs: qgroup: don't commit transaction when we already hold the handle Qu Wenruo
2020-12-04  7:37   ` Nikolay Borisov
2020-12-04  7:46     ` Qu Wenruo
2020-12-04 11:48 ` [PATCH] btrfs: qgroup: don't try to wait flushing if we're already holding a transaction Filipe Manana
2020-12-04 17:28 ` David Sterba
2020-12-05  2:55   ` Qu Wenruo

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox