[PATCH v3 0/7] Use global rsv stealing for evict and clean things up

[PATCH v3 0/7] Use global rsv stealing for evict and clean things up

[Josef Bacik]

v2->v3:
- Nikolay suggested an additional cleanup, which lead to multiple other cleanups
  and small fixes.

v1->v2:
- Reworked the stealing logic to be inside of the priority metadata loop, since
  that's the part we care about.
- Renamed the helper to see if we can steal to can_steal.
- Added Nikolay's reviewed-by's.

--- Original email ---

Hello,

While trying to remove direct access of fs_info->extent_root I noticed we were
passing root into btrfs_reserve_metadata_bytes() for the sole purpose of
stealing from the global reserve if we were doing orphan cleanup.  This isn't
really necessary anymore, but I needed to clean up a few things

1) We have global reserve stealing logic in the flushing code now that does the
   proper ordering already.  We just hadn't converted evict to this yet, so I've
   done that.
2) Since we already do the global reserve stealing as a part of the reservation
   process we don't need the extra check to steal from the global reserve if we
   fail to make our reservation during orphan cleanup.
3) Since we no longer need this logic we don't need the orphan_cleanup_state bit
   in the root so we can remove that.
4) Finally with all of this removed we don't have a need for root in
   btrfs_reserve_metadata_bytes(), so change it to fs_info and change it's main
   callers as well.

With that we've got more consistent global reserve stealing handling in evict,
and I've cleaned up the reservation path so I no longer have to worry about a
couple of places where we were doing
btrfs_reserve_metadata_bytes(root->extent_root).  Thanks,

Josef Bacik (7):
  btrfs: handle priority ticket failures in their respective helpers
  btrfs: check for priority ticket granting before flushing
  btrfs: check ticket->steal in steal_from_global_block_rsv
  btrfs: make BTRFS_RESERVE_FLUSH_EVICT use the global rsv stealing code
  btrfs: remove global rsv stealing logic for orphan cleanup
  btrfs: get rid of root->orphan_cleanup_state
  btrfs: change root to fs_info for btrfs_reserve_metadata_bytes

 fs/btrfs/block-group.c    |  2 +-
 fs/btrfs/block-rsv.c      | 12 +++---
 fs/btrfs/block-rsv.h      |  4 +-
 fs/btrfs/ctree.h          |  9 +----
 fs/btrfs/delalloc-space.c |  2 +-
 fs/btrfs/delayed-inode.c  |  2 +-
 fs/btrfs/delayed-ref.c    |  4 +-
 fs/btrfs/disk-io.c        |  1 -
 fs/btrfs/inode.c          | 21 ++++------
 fs/btrfs/props.c          |  5 ++-
 fs/btrfs/relocation.c     | 17 ++++----
 fs/btrfs/root-tree.c      |  2 +-
 fs/btrfs/space-info.c     | 84 +++++++++++++++++++++------------------
 fs/btrfs/space-info.h     |  2 +-
 fs/btrfs/transaction.c    |  4 +-
 15 files changed, 86 insertions(+), 85 deletions(-)

-- 
2.26.3

[PATCH v3 1/7] btrfs: handle priority ticket failures in their respective helpers

[Josef Bacik]

Currently the error case for the priority tickets is handled where we
deal with all of the tickets, priority and non-priority.  This is ok in
general, but it makes for some awkward locking.  We take and drop the
space_info->lock back to back because of these different types of
tickets.

Rework the code to handle priority ticket failures in their respective
helpers.  This allows us to be less wonky with our space_info->lock
usage, and means that the main handler simply has to check
ticket->error, as the ticket is guaranteed to be off any list and
completely handled by the time it exits one of the handlers.

Signed-off-by: Josef Bacik <josef@toxicpanda.com>
---
 fs/btrfs/space-info.c | 46 ++++++++++++++++++++-----------------------
 1 file changed, 21 insertions(+), 25 deletions(-)

diff --git a/fs/btrfs/space-info.c b/fs/btrfs/space-info.c
index 48d77f360a24..9d6048f54097 100644
--- a/fs/btrfs/space-info.c
+++ b/fs/btrfs/space-info.c
@@ -1260,7 +1260,7 @@ static void priority_reclaim_metadata_space(struct btrfs_fs_info *fs_info,
 				int states_nr)
 {
 	u64 to_reclaim;
-	int flush_state;
+	int flush_state = 0;
 
 	spin_lock(&space_info->lock);
 	to_reclaim = btrfs_calc_reclaim_metadata_size(fs_info, space_info);
@@ -1268,10 +1268,9 @@ static void priority_reclaim_metadata_space(struct btrfs_fs_info *fs_info,
 		spin_unlock(&space_info->lock);
 		return;
 	}
-	spin_unlock(&space_info->lock);
 
-	flush_state = 0;
-	do {
+	while (flush_state < states_nr) {
+		spin_unlock(&space_info->lock);
 		flush_space(fs_info, space_info, to_reclaim, states[flush_state],
 			    false);
 		flush_state++;
@@ -1280,23 +1279,38 @@ static void priority_reclaim_metadata_space(struct btrfs_fs_info *fs_info,
 			spin_unlock(&space_info->lock);
 			return;
 		}
-		spin_unlock(&space_info->lock);
-	} while (flush_state < states_nr);
+	}
+
+	/*
+	 * We must run try_granting_tickets here because we could be a large
+	 * ticket in front of a smaller ticket that can now be satisfied with
+	 * the available space.
+	 */
+	ticket->error = -ENOSPC;
+	remove_ticket(space_info, ticket);
+	btrfs_try_granting_tickets(fs_info, space_info);
+	spin_unlock(&space_info->lock);
 }
 
 static void priority_reclaim_data_space(struct btrfs_fs_info *fs_info,
 					struct btrfs_space_info *space_info,
 					struct reserve_ticket *ticket)
 {
+	spin_lock(&space_info->lock);
 	while (!space_info->full) {
+		spin_unlock(&space_info->lock);
 		flush_space(fs_info, space_info, U64_MAX, ALLOC_CHUNK_FORCE, false);
 		spin_lock(&space_info->lock);
 		if (ticket->bytes == 0) {
 			spin_unlock(&space_info->lock);
 			return;
 		}
-		spin_unlock(&space_info->lock);
 	}
+
+	ticket->error = -ENOSPC;
+	remove_ticket(space_info, ticket);
+	btrfs_try_granting_tickets(fs_info, space_info);
+	spin_unlock(&space_info->lock);
 }
 
 static void wait_reserve_ticket(struct btrfs_fs_info *fs_info,
@@ -1378,25 +1392,7 @@ static int handle_reserve_ticket(struct btrfs_fs_info *fs_info,
 		break;
 	}
 
-	spin_lock(&space_info->lock);
 	ret = ticket->error;
-	if (ticket->bytes || ticket->error) {
-		/*
-		 * We were a priority ticket, so we need to delete ourselves
-		 * from the list.  Because we could have other priority tickets
-		 * behind us that require less space, run
-		 * btrfs_try_granting_tickets() to see if their reservations can
-		 * now be made.
-		 */
-		if (!list_empty(&ticket->list)) {
-			remove_ticket(space_info, ticket);
-			btrfs_try_granting_tickets(fs_info, space_info);
-		}
-
-		if (!ret)
-			ret = -ENOSPC;
-	}
-	spin_unlock(&space_info->lock);
 	ASSERT(list_empty(&ticket->list));
 	/*
 	 * Check that we can't have an error set if the reservation succeeded,
-- 
2.26.3

[PATCH v3 2/7] btrfs: check for priority ticket granting before flushing

[Josef Bacik]

Since we're dropping locks before we enter the priority flushing loops
we could have had our ticket granted before we got the space_info->lock.
So add this check to avoid doing some extra flushing in the priority
flushing cases.

Signed-off-by: Josef Bacik <josef@toxicpanda.com>
---
 fs/btrfs/space-info.c | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/fs/btrfs/space-info.c b/fs/btrfs/space-info.c
index 9d6048f54097..9a362f3a6df4 100644
--- a/fs/btrfs/space-info.c
+++ b/fs/btrfs/space-info.c
@@ -1264,7 +1264,7 @@ static void priority_reclaim_metadata_space(struct btrfs_fs_info *fs_info,
 
 	spin_lock(&space_info->lock);
 	to_reclaim = btrfs_calc_reclaim_metadata_size(fs_info, space_info);
-	if (!to_reclaim) {
+	if (!to_reclaim || ticket->bytes == 0) {
 		spin_unlock(&space_info->lock);
 		return;
 	}
@@ -1297,6 +1297,13 @@ static void priority_reclaim_data_space(struct btrfs_fs_info *fs_info,
 					struct reserve_ticket *ticket)
 {
 	spin_lock(&space_info->lock);
+
+	/* We could have been granted before we got here. */
+	if (ticket->bytes == 0) {
+		spin_unlock(&space_info->lock);
+		return;
+	}
+
 	while (!space_info->full) {
 		spin_unlock(&space_info->lock);
 		flush_space(fs_info, space_info, U64_MAX, ALLOC_CHUNK_FORCE, false);
-- 
2.26.3

[PATCH v3 3/7] btrfs: check ticket->steal in steal_from_global_block_rsv

[Josef Bacik]

We're going to use this helper in the priority flushing loop, move this
check into the helper to simplify the logic.

Signed-off-by: Josef Bacik <josef@toxicpanda.com>
---
 fs/btrfs/space-info.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/fs/btrfs/space-info.c b/fs/btrfs/space-info.c
index 9a362f3a6df4..6498e79d4c9b 100644
--- a/fs/btrfs/space-info.c
+++ b/fs/btrfs/space-info.c
@@ -844,6 +844,9 @@ static bool steal_from_global_rsv(struct btrfs_fs_info *fs_info,
 	struct btrfs_block_rsv *global_rsv = &fs_info->global_block_rsv;
 	u64 min_bytes;
 
+	if (!ticket->steal)
+		return false;
+
 	if (global_rsv->space_info != space_info)
 		return false;
 
@@ -899,8 +902,8 @@ static bool maybe_fail_all_tickets(struct btrfs_fs_info *fs_info,
 		ticket = list_first_entry(&space_info->tickets,
 					  struct reserve_ticket, list);
 
-		if (!aborted && ticket->steal &&
-		    steal_from_global_rsv(fs_info, space_info, ticket))
+		if (!aborted && steal_from_global_rsv(fs_info, space_info,
+						      ticket))
 			return true;
 
 		if (!aborted && btrfs_test_opt(fs_info, ENOSPC_DEBUG))
-- 
2.26.3

[PATCH v3 4/7] btrfs: make BTRFS_RESERVE_FLUSH_EVICT use the global rsv stealing code

[Josef Bacik]

I forgot to convert this over when I introduced the global reserve
stealing code to the space flushing code.  Evict was simply trying to
make its reservation and then if it failed it would steal from the
global rsv, which is racey because it's outside of the normal ticketing
code.

Fix this by setting ticket->steal if we are BTRFS_RESERVE_FLUSH_EVICT,
and then make the priority flushing path do the steal for us.

Signed-off-by: Josef Bacik <josef@toxicpanda.com>
---
 fs/btrfs/inode.c      | 15 ++++++---------
 fs/btrfs/space-info.c | 16 +++++++++++++---
 2 files changed, 19 insertions(+), 12 deletions(-)

diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
index 904b7bb8e2b1..67c7107a79a5 100644
--- a/fs/btrfs/inode.c
+++ b/fs/btrfs/inode.c
@@ -5521,7 +5521,6 @@ static struct btrfs_trans_handle *evict_refill_and_join(struct btrfs_root *root,
 							struct btrfs_block_rsv *rsv)
 {
 	struct btrfs_fs_info *fs_info = root->fs_info;
-	struct btrfs_block_rsv *global_rsv = &fs_info->global_block_rsv;
 	struct btrfs_trans_handle *trans;
 	u64 delayed_refs_extra = btrfs_calc_insert_metadata_size(fs_info, 1);
 	int ret;
@@ -5536,18 +5535,16 @@ static struct btrfs_trans_handle *evict_refill_and_join(struct btrfs_root *root,
 	 * above.  We reserve our extra bit here because we generate a ton of
 	 * delayed refs activity by truncating.
 	 *
-	 * If we cannot make our reservation we'll attempt to steal from the
-	 * global reserve, because we really want to be able to free up space.
+	 * BTRFS_RESERVE_FLUSH_EVICT will steal from the global_rsv if it can,
+	 * if we fail to make this reservation we can re-try without the
+	 * delayed_refs_extra so we can make some forward progress.
 	 */
 	ret = btrfs_block_rsv_refill(root, rsv, rsv->size + delayed_refs_extra,
 				     BTRFS_RESERVE_FLUSH_EVICT);
 	if (ret) {
-		/*
-		 * Try to steal from the global reserve if there is space for
-		 * it.
-		 */
-		if (btrfs_check_space_for_delayed_refs(fs_info) ||
-		    btrfs_block_rsv_migrate(global_rsv, rsv, rsv->size, 0)) {
+		ret = btrfs_block_rsv_refill(root, rsv, rsv->size,
+					     BTRFS_RESERVE_FLUSH_EVICT);
+		if (ret) {
 			btrfs_warn(fs_info,
 				   "could not allocate space for delete; will truncate on mount");
 			return ERR_PTR(-ENOSPC);
diff --git a/fs/btrfs/space-info.c b/fs/btrfs/space-info.c
index 6498e79d4c9b..f72d70051f5b 100644
--- a/fs/btrfs/space-info.c
+++ b/fs/btrfs/space-info.c
@@ -1284,13 +1284,17 @@ static void priority_reclaim_metadata_space(struct btrfs_fs_info *fs_info,
 		}
 	}
 
+	/* Attempt to steal from the global rsv if we can. */
+	if (!steal_from_global_rsv(fs_info, space_info, ticket)) {
+		ticket->error = -ENOSPC;
+		remove_ticket(space_info, ticket);
+	}
+
 	/*
 	 * We must run try_granting_tickets here because we could be a large
 	 * ticket in front of a smaller ticket that can now be satisfied with
 	 * the available space.
 	 */
-	ticket->error = -ENOSPC;
-	remove_ticket(space_info, ticket);
 	btrfs_try_granting_tickets(fs_info, space_info);
 	spin_unlock(&space_info->lock);
 }
@@ -1444,6 +1448,12 @@ static inline void maybe_clamp_preempt(struct btrfs_fs_info *fs_info,
 		space_info->clamp = min(space_info->clamp + 1, 8);
 }
 
+static inline bool can_steal(enum btrfs_reserve_flush_enum flush)
+{
+	return (flush == BTRFS_RESERVE_FLUSH_ALL_STEAL ||
+		flush == BTRFS_RESERVE_FLUSH_EVICT);
+}
+
 /**
  * Try to reserve bytes from the block_rsv's space
  *
@@ -1517,7 +1527,7 @@ static int __reserve_bytes(struct btrfs_fs_info *fs_info,
 		ticket.error = 0;
 		space_info->reclaim_size += ticket.bytes;
 		init_waitqueue_head(&ticket.wait);
-		ticket.steal = (flush == BTRFS_RESERVE_FLUSH_ALL_STEAL);
+		ticket.steal = can_steal(flush);
 		if (trace_btrfs_reserve_ticket_enabled())
 			start_ns = ktime_get_ns();
 
-- 
2.26.3

[PATCH v3 5/7] btrfs: remove global rsv stealing logic for orphan cleanup

[Josef Bacik]

This is very old code before we were stealing from the global reserve
during evict.  We have proper ways to steal from the global reserve
while we're evicting, so rip out this code as it's no longer necessary.

Reviewed-by: Nikolay Borisov <nborisov@suse.com>
Signed-off-by: Josef Bacik <josef@toxicpanda.com>
---
 fs/btrfs/space-info.c | 7 -------
 1 file changed, 7 deletions(-)

diff --git a/fs/btrfs/space-info.c b/fs/btrfs/space-info.c
index f72d70051f5b..6b65baec33d4 100644
--- a/fs/btrfs/space-info.c
+++ b/fs/btrfs/space-info.c
@@ -1601,16 +1601,9 @@ int btrfs_reserve_metadata_bytes(struct btrfs_root *root,
 				 enum btrfs_reserve_flush_enum flush)
 {
 	struct btrfs_fs_info *fs_info = root->fs_info;
-	struct btrfs_block_rsv *global_rsv = &fs_info->global_block_rsv;
 	int ret;
 
 	ret = __reserve_bytes(fs_info, block_rsv->space_info, orig_bytes, flush);
-	if (ret == -ENOSPC &&
-	    unlikely(root->orphan_cleanup_state == ORPHAN_CLEANUP_STARTED)) {
-		if (block_rsv != global_rsv &&
-		    !btrfs_block_rsv_use_bytes(global_rsv, orig_bytes))
-			ret = 0;
-	}
 	if (ret == -ENOSPC) {
 		trace_btrfs_space_reservation(fs_info, "space_info:enospc",
 					      block_rsv->space_info->flags,
-- 
2.26.3

[PATCH v3 6/7] btrfs: get rid of root->orphan_cleanup_state

[Josef Bacik]

Now that we don't care about the stage of the orphan_cleanup_state,
simply replace it with a bit on ->state to make sure we don't call the
orphan cleanup every time we wander into this root.

Reviewed-by: Nikolay Borisov <nborisov@suse.com>
Signed-off-by: Josef Bacik <josef@toxicpanda.com>
---
 fs/btrfs/ctree.h   | 9 ++-------
 fs/btrfs/disk-io.c | 1 -
 fs/btrfs/inode.c   | 4 +---
 3 files changed, 3 insertions(+), 11 deletions(-)

diff --git a/fs/btrfs/ctree.h b/fs/btrfs/ctree.h
index 5e29f3fc527d..f1dd2486dcb3 100644
--- a/fs/btrfs/ctree.h
+++ b/fs/btrfs/ctree.h
@@ -511,11 +511,6 @@ struct btrfs_discard_ctl {
 	atomic64_t discard_bytes_saved;
 };
 
-enum btrfs_orphan_cleanup_state {
-	ORPHAN_CLEANUP_STARTED	= 1,
-	ORPHAN_CLEANUP_DONE	= 2,
-};
-
 void btrfs_init_async_reclaim_work(struct btrfs_fs_info *fs_info);
 
 /* fs_info */
@@ -1110,6 +1105,8 @@ enum {
 	BTRFS_ROOT_HAS_LOG_TREE,
 	/* Qgroup flushing is in progress */
 	BTRFS_ROOT_QGROUP_FLUSHING,
+	/* We started the orphan cleanup for this root. */
+	BTRFS_ROOT_ORPHAN_CLEANUP,
 };
 
 /*
@@ -1178,8 +1175,6 @@ struct btrfs_root {
 	spinlock_t log_extents_lock[2];
 	struct list_head logged_list[2];
 
-	int orphan_cleanup_state;
-
 	spinlock_t inode_lock;
 	/* red-black tree that keeps track of in-memory inodes */
 	struct rb_root inode_tree;
diff --git a/fs/btrfs/disk-io.c b/fs/btrfs/disk-io.c
index 847aabb30676..2fbf29b36926 100644
--- a/fs/btrfs/disk-io.c
+++ b/fs/btrfs/disk-io.c
@@ -1144,7 +1144,6 @@ static void __setup_root(struct btrfs_root *root, struct btrfs_fs_info *fs_info,
 	root->node = NULL;
 	root->commit_root = NULL;
 	root->state = 0;
-	root->orphan_cleanup_state = 0;
 
 	root->last_trans = 0;
 	root->free_objectid = 0;
diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
index 67c7107a79a5..ac0b55ca3e78 100644
--- a/fs/btrfs/inode.c
+++ b/fs/btrfs/inode.c
@@ -3473,7 +3473,7 @@ int btrfs_orphan_cleanup(struct btrfs_root *root)
 	u64 last_objectid = 0;
 	int ret = 0, nr_unlink = 0;
 
-	if (cmpxchg(&root->orphan_cleanup_state, 0, ORPHAN_CLEANUP_STARTED))
+	if (test_and_set_bit(BTRFS_ROOT_ORPHAN_CLEANUP, &root->state))
 		return 0;
 
 	path = btrfs_alloc_path();
@@ -3631,8 +3631,6 @@ int btrfs_orphan_cleanup(struct btrfs_root *root)
 	/* release the path since we're done with it */
 	btrfs_release_path(path);
 
-	root->orphan_cleanup_state = ORPHAN_CLEANUP_DONE;
-
 	if (test_bit(BTRFS_ROOT_ORPHAN_ITEM_INSERTED, &root->state)) {
 		trans = btrfs_join_transaction(root);
 		if (!IS_ERR(trans))
-- 
2.26.3

[PATCH v3 7/7] btrfs: change root to fs_info for btrfs_reserve_metadata_bytes

[Josef Bacik]

We used to need the root for btrfs_reserve_metadata_bytes to check the
orphan cleanup state, but we no longer need that, we simply need the
fs_info.  Change btrfs_reserve_metadata_bytes() to use the fs_info, and
change both btrfs_block_rsv_refill() and btrfs_block_rsv_add() to do the
same as they simply call btrfs_reserve_metadata_bytes() and then
manipulate the block_rsv that is being used.

Reviewed-by: Nikolay Borisov <nborisov@suse.com>
Signed-off-by: Josef Bacik <josef@toxicpanda.com>
---
 fs/btrfs/block-group.c    |  2 +-
 fs/btrfs/block-rsv.c      | 12 +++++++-----
 fs/btrfs/block-rsv.h      |  4 ++--
 fs/btrfs/delalloc-space.c |  2 +-
 fs/btrfs/delayed-inode.c  |  2 +-
 fs/btrfs/delayed-ref.c    |  4 ++--
 fs/btrfs/inode.c          |  4 ++--
 fs/btrfs/props.c          |  5 +++--
 fs/btrfs/relocation.c     | 17 +++++++++--------
 fs/btrfs/root-tree.c      |  2 +-
 fs/btrfs/space-info.c     |  3 +--
 fs/btrfs/space-info.h     |  2 +-
 fs/btrfs/transaction.c    |  4 ++--
 13 files changed, 33 insertions(+), 30 deletions(-)

diff --git a/fs/btrfs/block-group.c b/fs/btrfs/block-group.c
index 6ab864655090..d56fc1b8bb99 100644
--- a/fs/btrfs/block-group.c
+++ b/fs/btrfs/block-group.c
@@ -3790,7 +3790,7 @@ static void reserve_chunk_space(struct btrfs_trans_handle *trans,
 	}
 
 	if (!ret) {
-		ret = btrfs_block_rsv_add(fs_info->chunk_root,
+		ret = btrfs_block_rsv_add(fs_info,
 					  &fs_info->chunk_block_rsv,
 					  bytes, BTRFS_RESERVE_NO_FLUSH);
 		if (!ret)
diff --git a/fs/btrfs/block-rsv.c b/fs/btrfs/block-rsv.c
index 04a6226e0388..ac6fbe75cace 100644
--- a/fs/btrfs/block-rsv.c
+++ b/fs/btrfs/block-rsv.c
@@ -208,7 +208,7 @@ void btrfs_free_block_rsv(struct btrfs_fs_info *fs_info,
 	kfree(rsv);
 }
 
-int btrfs_block_rsv_add(struct btrfs_root *root,
+int btrfs_block_rsv_add(struct btrfs_fs_info *fs_info,
 			struct btrfs_block_rsv *block_rsv, u64 num_bytes,
 			enum btrfs_reserve_flush_enum flush)
 {
@@ -217,7 +217,8 @@ int btrfs_block_rsv_add(struct btrfs_root *root,
 	if (num_bytes == 0)
 		return 0;
 
-	ret = btrfs_reserve_metadata_bytes(root, block_rsv, num_bytes, flush);
+	ret = btrfs_reserve_metadata_bytes(fs_info, block_rsv, num_bytes,
+					   flush);
 	if (!ret)
 		btrfs_block_rsv_add_bytes(block_rsv, num_bytes, true);
 
@@ -241,7 +242,7 @@ int btrfs_block_rsv_check(struct btrfs_block_rsv *block_rsv, int min_factor)
 	return ret;
 }
 
-int btrfs_block_rsv_refill(struct btrfs_root *root,
+int btrfs_block_rsv_refill(struct btrfs_fs_info *fs_info,
 			   struct btrfs_block_rsv *block_rsv, u64 min_reserved,
 			   enum btrfs_reserve_flush_enum flush)
 {
@@ -262,7 +263,8 @@ int btrfs_block_rsv_refill(struct btrfs_root *root,
 	if (!ret)
 		return 0;
 
-	ret = btrfs_reserve_metadata_bytes(root, block_rsv, num_bytes, flush);
+	ret = btrfs_reserve_metadata_bytes(fs_info, block_rsv, num_bytes,
+					   flush);
 	if (!ret) {
 		btrfs_block_rsv_add_bytes(block_rsv, num_bytes, false);
 		return 0;
@@ -523,7 +525,7 @@ struct btrfs_block_rsv *btrfs_use_block_rsv(struct btrfs_trans_handle *trans,
 				block_rsv->type, ret);
 	}
 try_reserve:
-	ret = btrfs_reserve_metadata_bytes(root, block_rsv, blocksize,
+	ret = btrfs_reserve_metadata_bytes(fs_info, block_rsv, blocksize,
 					   BTRFS_RESERVE_NO_FLUSH);
 	if (!ret)
 		return block_rsv;
diff --git a/fs/btrfs/block-rsv.h b/fs/btrfs/block-rsv.h
index 0b6ae5302837..07d61c2c5d28 100644
--- a/fs/btrfs/block-rsv.h
+++ b/fs/btrfs/block-rsv.h
@@ -57,11 +57,11 @@ void btrfs_init_metadata_block_rsv(struct btrfs_fs_info *fs_info,
 				   unsigned short type);
 void btrfs_free_block_rsv(struct btrfs_fs_info *fs_info,
 			  struct btrfs_block_rsv *rsv);
-int btrfs_block_rsv_add(struct btrfs_root *root,
+int btrfs_block_rsv_add(struct btrfs_fs_info *fs_info,
 			struct btrfs_block_rsv *block_rsv, u64 num_bytes,
 			enum btrfs_reserve_flush_enum flush);
 int btrfs_block_rsv_check(struct btrfs_block_rsv *block_rsv, int min_factor);
-int btrfs_block_rsv_refill(struct btrfs_root *root,
+int btrfs_block_rsv_refill(struct btrfs_fs_info *fs_info,
 			   struct btrfs_block_rsv *block_rsv, u64 min_reserved,
 			   enum btrfs_reserve_flush_enum flush);
 int btrfs_block_rsv_migrate(struct btrfs_block_rsv *src_rsv,
diff --git a/fs/btrfs/delalloc-space.c b/fs/btrfs/delalloc-space.c
index 2059d1504149..bca438c7c972 100644
--- a/fs/btrfs/delalloc-space.c
+++ b/fs/btrfs/delalloc-space.c
@@ -331,7 +331,7 @@ int btrfs_delalloc_reserve_metadata(struct btrfs_inode *inode, u64 num_bytes)
 	ret = btrfs_qgroup_reserve_meta_prealloc(root, qgroup_reserve, true);
 	if (ret)
 		return ret;
-	ret = btrfs_reserve_metadata_bytes(root, block_rsv, meta_reserve, flush);
+	ret = btrfs_reserve_metadata_bytes(fs_info, block_rsv, meta_reserve, flush);
 	if (ret) {
 		btrfs_qgroup_free_meta_prealloc(root, qgroup_reserve);
 		return ret;
diff --git a/fs/btrfs/delayed-inode.c b/fs/btrfs/delayed-inode.c
index e164766dcc38..6f134f2c5e68 100644
--- a/fs/btrfs/delayed-inode.c
+++ b/fs/btrfs/delayed-inode.c
@@ -629,7 +629,7 @@ static int btrfs_delayed_inode_reserve_metadata(
 					  BTRFS_QGROUP_RSV_META_PREALLOC, true);
 		if (ret < 0)
 			return ret;
-		ret = btrfs_block_rsv_add(root, dst_rsv, num_bytes,
+		ret = btrfs_block_rsv_add(fs_info, dst_rsv, num_bytes,
 					  BTRFS_RESERVE_NO_FLUSH);
 		/* NO_FLUSH could only fail with -ENOSPC */
 		ASSERT(ret == 0 || ret == -ENOSPC);
diff --git a/fs/btrfs/delayed-ref.c b/fs/btrfs/delayed-ref.c
index cca7e85e32dd..80950a409395 100644
--- a/fs/btrfs/delayed-ref.c
+++ b/fs/btrfs/delayed-ref.c
@@ -191,8 +191,8 @@ int btrfs_delayed_refs_rsv_refill(struct btrfs_fs_info *fs_info,
 	if (!num_bytes)
 		return 0;
 
-	ret = btrfs_reserve_metadata_bytes(fs_info->extent_root, block_rsv,
-					   num_bytes, flush);
+	ret = btrfs_reserve_metadata_bytes(fs_info, block_rsv, num_bytes,
+					   flush);
 	if (ret)
 		return ret;
 	btrfs_block_rsv_add_bytes(block_rsv, num_bytes, 0);
diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
index ac0b55ca3e78..041fcb752a90 100644
--- a/fs/btrfs/inode.c
+++ b/fs/btrfs/inode.c
@@ -5537,10 +5537,10 @@ static struct btrfs_trans_handle *evict_refill_and_join(struct btrfs_root *root,
 	 * if we fail to make this reservation we can re-try without the
 	 * delayed_refs_extra so we can make some forward progress.
 	 */
-	ret = btrfs_block_rsv_refill(root, rsv, rsv->size + delayed_refs_extra,
+	ret = btrfs_block_rsv_refill(fs_info, rsv, rsv->size + delayed_refs_extra,
 				     BTRFS_RESERVE_FLUSH_EVICT);
 	if (ret) {
-		ret = btrfs_block_rsv_refill(root, rsv, rsv->size,
+		ret = btrfs_block_rsv_refill(fs_info, rsv, rsv->size,
 					     BTRFS_RESERVE_FLUSH_EVICT);
 		if (ret) {
 			btrfs_warn(fs_info,
diff --git a/fs/btrfs/props.c b/fs/btrfs/props.c
index a978676aa627..1a6d2d5b4b33 100644
--- a/fs/btrfs/props.c
+++ b/fs/btrfs/props.c
@@ -377,8 +377,9 @@ static int inherit_props(struct btrfs_trans_handle *trans,
 		 */
 		if (need_reserve) {
 			num_bytes = btrfs_calc_insert_metadata_size(fs_info, 1);
-			ret = btrfs_block_rsv_add(root, trans->block_rsv,
-					num_bytes, BTRFS_RESERVE_NO_FLUSH);
+			ret = btrfs_block_rsv_add(fs_info, trans->block_rsv,
+						  num_bytes,
+						  BTRFS_RESERVE_NO_FLUSH);
 			if (ret)
 				return ret;
 		}
diff --git a/fs/btrfs/relocation.c b/fs/btrfs/relocation.c
index ee0a0efc7efd..a455a1ead0d6 100644
--- a/fs/btrfs/relocation.c
+++ b/fs/btrfs/relocation.c
@@ -1736,7 +1736,8 @@ static noinline_for_stack int merge_reloc_root(struct reloc_control *rc,
 	memset(&next_key, 0, sizeof(next_key));
 
 	while (1) {
-		ret = btrfs_block_rsv_refill(root, rc->block_rsv, min_reserved,
+		ret = btrfs_block_rsv_refill(fs_info, rc->block_rsv,
+					     min_reserved,
 					     BTRFS_RESERVE_FLUSH_LIMIT);
 		if (ret)
 			goto out;
@@ -1855,7 +1856,7 @@ int prepare_to_merge(struct reloc_control *rc, int err)
 again:
 	if (!err) {
 		num_bytes = rc->merging_rsv_size;
-		ret = btrfs_block_rsv_add(root, rc->block_rsv, num_bytes,
+		ret = btrfs_block_rsv_add(fs_info, rc->block_rsv, num_bytes,
 					  BTRFS_RESERVE_FLUSH_ALL);
 		if (ret)
 			err = ret;
@@ -2323,8 +2324,8 @@ static int reserve_metadata_space(struct btrfs_trans_handle *trans,
 	 * If we get an enospc just kick back -EAGAIN so we know to drop the
 	 * transaction and try to refill when we can flush all the things.
 	 */
-	ret = btrfs_block_rsv_refill(root, rc->block_rsv, num_bytes,
-				BTRFS_RESERVE_FLUSH_LIMIT);
+	ret = btrfs_block_rsv_refill(fs_info, rc->block_rsv, num_bytes,
+				     BTRFS_RESERVE_FLUSH_LIMIT);
 	if (ret) {
 		tmp = fs_info->nodesize * RELOCATION_RESERVED_NODES;
 		while (tmp <= rc->reserved_bytes)
@@ -3550,7 +3551,7 @@ int prepare_to_relocate(struct reloc_control *rc)
 	rc->reserved_bytes = 0;
 	rc->block_rsv->size = rc->extent_root->fs_info->nodesize *
 			      RELOCATION_RESERVED_NODES;
-	ret = btrfs_block_rsv_refill(rc->extent_root,
+	ret = btrfs_block_rsv_refill(rc->extent_root->fs_info,
 				     rc->block_rsv, rc->block_rsv->size,
 				     BTRFS_RESERVE_FLUSH_ALL);
 	if (ret)
@@ -3598,9 +3599,9 @@ static noinline_for_stack int relocate_block_group(struct reloc_control *rc)
 
 	while (1) {
 		rc->reserved_bytes = 0;
-		ret = btrfs_block_rsv_refill(rc->extent_root,
-					rc->block_rsv, rc->block_rsv->size,
-					BTRFS_RESERVE_FLUSH_ALL);
+		ret = btrfs_block_rsv_refill(fs_info, rc->block_rsv,
+					     rc->block_rsv->size,
+					     BTRFS_RESERVE_FLUSH_ALL);
 		if (ret) {
 			err = ret;
 			break;
diff --git a/fs/btrfs/root-tree.c b/fs/btrfs/root-tree.c
index 458aa74a37ce..f7ff7c71c9ce 100644
--- a/fs/btrfs/root-tree.c
+++ b/fs/btrfs/root-tree.c
@@ -503,7 +503,7 @@ int btrfs_subvolume_reserve_metadata(struct btrfs_root *root,
 	num_bytes = btrfs_calc_insert_metadata_size(fs_info, items);
 	rsv->space_info = btrfs_find_space_info(fs_info,
 					    BTRFS_BLOCK_GROUP_METADATA);
-	ret = btrfs_block_rsv_add(root, rsv, num_bytes,
+	ret = btrfs_block_rsv_add(fs_info, rsv, num_bytes,
 				  BTRFS_RESERVE_FLUSH_ALL);
 
 	if (ret == -ENOSPC && use_global_rsv)
diff --git a/fs/btrfs/space-info.c b/fs/btrfs/space-info.c
index 6b65baec33d4..dbf8bfb8fcb3 100644
--- a/fs/btrfs/space-info.c
+++ b/fs/btrfs/space-info.c
@@ -1595,12 +1595,11 @@ static int __reserve_bytes(struct btrfs_fs_info *fs_info,
  * regain reservations will be made and this will fail if there is not enough
  * space already.
  */
-int btrfs_reserve_metadata_bytes(struct btrfs_root *root,
+int btrfs_reserve_metadata_bytes(struct btrfs_fs_info *fs_info,
 				 struct btrfs_block_rsv *block_rsv,
 				 u64 orig_bytes,
 				 enum btrfs_reserve_flush_enum flush)
 {
-	struct btrfs_fs_info *fs_info = root->fs_info;
 	int ret;
 
 	ret = __reserve_bytes(fs_info, block_rsv->space_info, orig_bytes, flush);
diff --git a/fs/btrfs/space-info.h b/fs/btrfs/space-info.h
index cb5056472e79..d841fed73492 100644
--- a/fs/btrfs/space-info.h
+++ b/fs/btrfs/space-info.h
@@ -123,7 +123,7 @@ void btrfs_clear_space_info_full(struct btrfs_fs_info *info);
 void btrfs_dump_space_info(struct btrfs_fs_info *fs_info,
 			   struct btrfs_space_info *info, u64 bytes,
 			   int dump_block_groups);
-int btrfs_reserve_metadata_bytes(struct btrfs_root *root,
+int btrfs_reserve_metadata_bytes(struct btrfs_fs_info *fs_info,
 				 struct btrfs_block_rsv *block_rsv,
 				 u64 orig_bytes,
 				 enum btrfs_reserve_flush_enum flush);
diff --git a/fs/btrfs/transaction.c b/fs/btrfs/transaction.c
index 1c3a1189c0bd..b958e0fdfe10 100644
--- a/fs/btrfs/transaction.c
+++ b/fs/btrfs/transaction.c
@@ -628,7 +628,7 @@ start_transaction(struct btrfs_root *root, unsigned int num_items,
 			reloc_reserved = true;
 		}
 
-		ret = btrfs_block_rsv_add(root, rsv, num_bytes, flush);
+		ret = btrfs_block_rsv_add(fs_info, rsv, num_bytes, flush);
 		if (ret)
 			goto reserve_fail;
 		if (delayed_refs_bytes) {
@@ -1578,7 +1578,7 @@ static noinline int create_pending_snapshot(struct btrfs_trans_handle *trans,
 	btrfs_reloc_pre_snapshot(pending, &to_reserve);
 
 	if (to_reserve > 0) {
-		pending->error = btrfs_block_rsv_add(root,
+		pending->error = btrfs_block_rsv_add(fs_info,
 						     &pending->block_rsv,
 						     to_reserve,
 						     BTRFS_RESERVE_NO_FLUSH);
-- 
2.26.3

Re: [PATCH v3 1/7] btrfs: handle priority ticket failures in their respective helpers

[Nikolay Borisov]

On 9.11.21 г. 17:12, Josef Bacik wrote:
> Currently the error case for the priority tickets is handled where we
> deal with all of the tickets, priority and non-priority.  This is ok in
> general, but it makes for some awkward locking.  We take and drop the
> space_info->lock back to back because of these different types of
> tickets.
> 
> Rework the code to handle priority ticket failures in their respective
> helpers.  This allows us to be less wonky with our space_info->lock
> usage, and means that the main handler simply has to check
> ticket->error, as the ticket is guaranteed to be off any list and
> completely handled by the time it exits one of the handlers.
> 
> Signed-off-by: Josef Bacik <josef@toxicpanda.com>

Reviewed-by: Nikolay Borisov <nborisov@suse.com>

Re: [PATCH v3 2/7] btrfs: check for priority ticket granting before flushing

[Nikolay Borisov]

On 9.11.21 г. 17:12, Josef Bacik wrote:
> Since we're dropping locks before we enter the priority flushing loops
> we could have had our ticket granted before we got the space_info->lock.
> So add this check to avoid doing some extra flushing in the priority
> flushing cases.
> 
> Signed-off-by: Josef Bacik <josef@toxicpanda.com>

Reviewed-by: Nikolay Borisov <nborisov@suse.com>

> --->  fs/btrfs/space-info.c | 9 ++++++++-
>  1 file changed, 8 insertions(+), 1 deletion(-)
> 
> diff --git a/fs/btrfs/space-info.c b/fs/btrfs/space-info.c
> index 9d6048f54097..9a362f3a6df4 100644
> --- a/fs/btrfs/space-info.c
> +++ b/fs/btrfs/space-info.c
> @@ -1264,7 +1264,7 @@ static void priority_reclaim_metadata_space(struct btrfs_fs_info *fs_info,
>  
>  	spin_lock(&space_info->lock);
>  	to_reclaim = btrfs_calc_reclaim_metadata_size(fs_info, space_info);
> -	if (!to_reclaim) {
> +	if (!to_reclaim || ticket->bytes == 0) {

nit: This is purely an optimization, handling the case where a prio
ticket N is being added to the list, but at the same time we might have
had ticket N-1 just satisfied (or failed) and having called
try_granting_ticket might have satisfied concurrently added ticket N,
right? And this is a completely independent change of the other cleanups
being done here?

>  		spin_unlock(&space_info->lock);
>  		return;
>  	}
> @@ -1297,6 +1297,13 @@ static void priority_reclaim_data_space(struct btrfs_fs_info *fs_info,
>  					struct reserve_ticket *ticket)
>  {
>  	spin_lock(&space_info->lock);
> +
> +	/* We could have been granted before we got here. */
> +	if (ticket->bytes == 0) {
> +		spin_unlock(&space_info->lock);
> +		return;
> +	}
> +
>  	while (!space_info->full) {
>  		spin_unlock(&space_info->lock);
>  		flush_space(fs_info, space_info, U64_MAX, ALLOC_CHUNK_FORCE, false);
> 

Re: [PATCH v3 3/7] btrfs: check ticket->steal in steal_from_global_block_rsv

[Nikolay Borisov]

On 9.11.21 г. 17:12, Josef Bacik wrote:
> We're going to use this helper in the priority flushing loop, move this
> check into the helper to simplify the logic.
> 
> Signed-off-by: Josef Bacik <josef@toxicpanda.com>

Reviewed-by: Nikolay Borisov <nborisov@suse.com>

> ---
>  fs/btrfs/space-info.c | 7 +++++--
>  1 file changed, 5 insertions(+), 2 deletions(-)
> 
> diff --git a/fs/btrfs/space-info.c b/fs/btrfs/space-info.c
> index 9a362f3a6df4..6498e79d4c9b 100644
> --- a/fs/btrfs/space-info.c
> +++ b/fs/btrfs/space-info.c
> @@ -844,6 +844,9 @@ static bool steal_from_global_rsv(struct btrfs_fs_info *fs_info,
>  	struct btrfs_block_rsv *global_rsv = &fs_info->global_block_rsv;
>  	u64 min_bytes;
>  
> +	if (!ticket->steal)
> +		return false;
> +
>  	if (global_rsv->space_info != space_info)
>  		return false;
>  
> @@ -899,8 +902,8 @@ static bool maybe_fail_all_tickets(struct btrfs_fs_info *fs_info,
>  		ticket = list_first_entry(&space_info->tickets,
>  					  struct reserve_ticket, list);
>  
> -		if (!aborted && ticket->steal &&
> -		    steal_from_global_rsv(fs_info, space_info, ticket))
> +		if (!aborted && steal_from_global_rsv(fs_info, space_info,
> +						      ticket))
>  			return true;
>  
>  		if (!aborted && btrfs_test_opt(fs_info, ENOSPC_DEBUG))
> 

Re: [PATCH v3 2/7] btrfs: check for priority ticket granting before flushing

[Josef Bacik]

On Thu, Nov 11, 2021 at 03:14:20PM +0200, Nikolay Borisov wrote:
> 
> 
> On 9.11.21 г. 17:12, Josef Bacik wrote:
> > Since we're dropping locks before we enter the priority flushing loops
> > we could have had our ticket granted before we got the space_info->lock.
> > So add this check to avoid doing some extra flushing in the priority
> > flushing cases.
> > 
> > Signed-off-by: Josef Bacik <josef@toxicpanda.com>
> 
> Reviewed-by: Nikolay Borisov <nborisov@suse.com>
> 
> > --->  fs/btrfs/space-info.c | 9 ++++++++-
> >  1 file changed, 8 insertions(+), 1 deletion(-)
> > 
> > diff --git a/fs/btrfs/space-info.c b/fs/btrfs/space-info.c
> > index 9d6048f54097..9a362f3a6df4 100644
> > --- a/fs/btrfs/space-info.c
> > +++ b/fs/btrfs/space-info.c
> > @@ -1264,7 +1264,7 @@ static void priority_reclaim_metadata_space(struct btrfs_fs_info *fs_info,
> >  
> >  	spin_lock(&space_info->lock);
> >  	to_reclaim = btrfs_calc_reclaim_metadata_size(fs_info, space_info);
> > -	if (!to_reclaim) {
> > +	if (!to_reclaim || ticket->bytes == 0) {
> 
> nit: This is purely an optimization, handling the case where a prio
> ticket N is being added to the list, but at the same time we might have
> had ticket N-1 just satisfied (or failed) and having called
> try_granting_ticket might have satisfied concurrently added ticket N,
> right? And this is a completely independent change of the other cleanups
> being done here?
> 

It's definitely just an optimization, but it can be less specific than this.
Think we came in to reserve, we didn't have the space, we added our ticket to
the list.  But at the same time somebody was waiting on the space_info lock to
add space and do btrfs_try_granting_ticket(), so we drop the lock, get
satisfied, come in to do our loop, and we have been satisified.

This is the priority reclaim path, so to_reclaim could be !0 still because we
may have only satisified the priority tickets and still left non priority
tickets on the list.  We would then have to_reclaim but ->bytes == 0.

Clearly not a huge deal, I just noticied it when I was redoing the locking for
the cleanups and it annoyed me.  Thanks,

Josef

Re: [PATCH v3 4/7] btrfs: make BTRFS_RESERVE_FLUSH_EVICT use the global rsv stealing code

[Nikolay Borisov]

On 9.11.21 г. 17:12, Josef Bacik wrote:
> I forgot to convert this over when I introduced the global reserve
> stealing code to the space flushing code.  Evict was simply trying to
> make its reservation and then if it failed it would steal from the
> global rsv, which is racey because it's outside of the normal ticketing
> code.
> 
> Fix this by setting ticket->steal if we are BTRFS_RESERVE_FLUSH_EVICT,
> and then make the priority flushing path do the steal for us.
> 
> Signed-off-by: Josef Bacik <josef@toxicpanda.com>

Reviewed-by: Nikolay Borisov <nborisov@suse.com>

Re: [PATCH v3 2/7] btrfs: check for priority ticket granting before flushing

[Nikolay Borisov]

On 11.11.21 г. 16:13, Josef Bacik wrote:
> On Thu, Nov 11, 2021 at 03:14:20PM +0200, Nikolay Borisov wrote:
>>
>>
>> On 9.11.21 г. 17:12, Josef Bacik wrote:
>>> Since we're dropping locks before we enter the priority flushing loops
>>> we could have had our ticket granted before we got the space_info->lock.
>>> So add this check to avoid doing some extra flushing in the priority
>>> flushing cases.
>>>
>>> Signed-off-by: Josef Bacik <josef@toxicpanda.com>
>>
>> Reviewed-by: Nikolay Borisov <nborisov@suse.com>
>>
>>> --->  fs/btrfs/space-info.c | 9 ++++++++-
>>>  1 file changed, 8 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/fs/btrfs/space-info.c b/fs/btrfs/space-info.c
>>> index 9d6048f54097..9a362f3a6df4 100644
>>> --- a/fs/btrfs/space-info.c
>>> +++ b/fs/btrfs/space-info.c
>>> @@ -1264,7 +1264,7 @@ static void priority_reclaim_metadata_space(struct btrfs_fs_info *fs_info,
>>>  
>>>  	spin_lock(&space_info->lock);
>>>  	to_reclaim = btrfs_calc_reclaim_metadata_size(fs_info, space_info);
>>> -	if (!to_reclaim) {
>>> +	if (!to_reclaim || ticket->bytes == 0) {
>>
>> nit: This is purely an optimization, handling the case where a prio
>> ticket N is being added to the list, but at the same time we might have
>> had ticket N-1 just satisfied (or failed) and having called
>> try_granting_ticket might have satisfied concurrently added ticket N,
>> right? And this is a completely independent change of the other cleanups
>> being done here?
>>
> 
> It's definitely just an optimization, but it can be less specific than this.
> Think we came in to reserve, we didn't have the space, we added our ticket to
> the list.  But at the same time somebody was waiting on the space_info lock to
> add space and do btrfs_try_granting_ticket(), so we drop the lock, get
> satisfied, come in to do our loop, and we have been satisified.
> 
> This is the priority reclaim path, so to_reclaim could be !0 still because we
> may have only satisified the priority tickets and still left non priority
> tickets on the list.  We would then have to_reclaim but ->bytes == 0.
> 
> Clearly not a huge deal, I just noticied it when I was redoing the locking for
> the cleanups and it annoyed me.  Thanks,

IMO such scenario description should be put in the changelog.

> 
> Josef
> 

Re: [PATCH v3 2/7] btrfs: check for priority ticket granting before flushing

[David Sterba]

On Thu, Nov 11, 2021 at 04:50:48PM +0200, Nikolay Borisov wrote:
> 
> 
> On 11.11.21 г. 16:13, Josef Bacik wrote:
> > On Thu, Nov 11, 2021 at 03:14:20PM +0200, Nikolay Borisov wrote:
> >>
> >>
> >> On 9.11.21 г. 17:12, Josef Bacik wrote:
> >>> Since we're dropping locks before we enter the priority flushing loops
> >>> we could have had our ticket granted before we got the space_info->lock.
> >>> So add this check to avoid doing some extra flushing in the priority
> >>> flushing cases.
> >>>
> >>> Signed-off-by: Josef Bacik <josef@toxicpanda.com>
> >>
> >> Reviewed-by: Nikolay Borisov <nborisov@suse.com>
> >>
> >>> --->  fs/btrfs/space-info.c | 9 ++++++++-
> >>>  1 file changed, 8 insertions(+), 1 deletion(-)
> >>>
> >>> diff --git a/fs/btrfs/space-info.c b/fs/btrfs/space-info.c
> >>> index 9d6048f54097..9a362f3a6df4 100644
> >>> --- a/fs/btrfs/space-info.c
> >>> +++ b/fs/btrfs/space-info.c
> >>> @@ -1264,7 +1264,7 @@ static void priority_reclaim_metadata_space(struct btrfs_fs_info *fs_info,
> >>>  
> >>>  	spin_lock(&space_info->lock);
> >>>  	to_reclaim = btrfs_calc_reclaim_metadata_size(fs_info, space_info);
> >>> -	if (!to_reclaim) {
> >>> +	if (!to_reclaim || ticket->bytes == 0) {
> >>
> >> nit: This is purely an optimization, handling the case where a prio
> >> ticket N is being added to the list, but at the same time we might have
> >> had ticket N-1 just satisfied (or failed) and having called
> >> try_granting_ticket might have satisfied concurrently added ticket N,
> >> right? And this is a completely independent change of the other cleanups
> >> being done here?
> >>
> > 
> > It's definitely just an optimization, but it can be less specific than this.
> > Think we came in to reserve, we didn't have the space, we added our ticket to
> > the list.  But at the same time somebody was waiting on the space_info lock to
> > add space and do btrfs_try_granting_ticket(), so we drop the lock, get
> > satisfied, come in to do our loop, and we have been satisified.
> > 
> > This is the priority reclaim path, so to_reclaim could be !0 still because we
> > may have only satisified the priority tickets and still left non priority
> > tickets on the list.  We would then have to_reclaim but ->bytes == 0.
> > 
> > Clearly not a huge deal, I just noticied it when I was redoing the locking for
> > the cleanups and it annoyed me.  Thanks,
> 
> IMO such scenario description should be put in the changelog.

Agreed. I'll paste and edit it from this answer, no need to resend the
whole patch but I'd appreciate proofreading once it's in misc-next.

Re: [PATCH v3 0/7] Use global rsv stealing for evict and clean things up

[David Sterba]

On Tue, Nov 09, 2021 at 10:12:00AM -0500, Josef Bacik wrote:
> v2->v3:
> - Nikolay suggested an additional cleanup, which lead to multiple other cleanups
>   and small fixes.
> 
> v1->v2:
> - Reworked the stealing logic to be inside of the priority metadata loop, since
>   that's the part we care about.
> - Renamed the helper to see if we can steal to can_steal.
> - Added Nikolay's reviewed-by's.
> 
> --- Original email ---
> 
> Hello,
> 
> While trying to remove direct access of fs_info->extent_root I noticed we were
> passing root into btrfs_reserve_metadata_bytes() for the sole purpose of
> stealing from the global reserve if we were doing orphan cleanup.  This isn't
> really necessary anymore, but I needed to clean up a few things
> 
> 1) We have global reserve stealing logic in the flushing code now that does the
>    proper ordering already.  We just hadn't converted evict to this yet, so I've
>    done that.
> 2) Since we already do the global reserve stealing as a part of the reservation
>    process we don't need the extra check to steal from the global reserve if we
>    fail to make our reservation during orphan cleanup.
> 3) Since we no longer need this logic we don't need the orphan_cleanup_state bit
>    in the root so we can remove that.
> 4) Finally with all of this removed we don't have a need for root in
>    btrfs_reserve_metadata_bytes(), so change it to fs_info and change it's main
>    callers as well.
> 
> With that we've got more consistent global reserve stealing handling in evict,
> and I've cleaned up the reservation path so I no longer have to worry about a
> couple of places where we were doing
> btrfs_reserve_metadata_bytes(root->extent_root).  Thanks,
> 
> Josef Bacik (7):
>   btrfs: handle priority ticket failures in their respective helpers
>   btrfs: check for priority ticket granting before flushing
>   btrfs: check ticket->steal in steal_from_global_block_rsv
>   btrfs: make BTRFS_RESERVE_FLUSH_EVICT use the global rsv stealing code
>   btrfs: remove global rsv stealing logic for orphan cleanup
>   btrfs: get rid of root->orphan_cleanup_state
>   btrfs: change root to fs_info for btrfs_reserve_metadata_bytes

Moved from topic branch to misc-next, thanks.

Re: [PATCH v3 2/7] btrfs: check for priority ticket granting before flushing

[Nikolay Borisov]

On 9.11.21 г. 17:12, Josef Bacik wrote:
> Since we're dropping locks before we enter the priority flushing loops
> we could have had our ticket granted before we got the space_info->lock.
> So add this check to avoid doing some extra flushing in the priority
> flushing cases.
> 
> Signed-off-by: Josef Bacik <josef@toxicpanda.com>
> ---
>  fs/btrfs/space-info.c | 9 ++++++++-
>  1 file changed, 8 insertions(+), 1 deletion(-)
> 
> diff --git a/fs/btrfs/space-info.c b/fs/btrfs/space-info.c
> index 9d6048f54097..9a362f3a6df4 100644
> --- a/fs/btrfs/space-info.c
> +++ b/fs/btrfs/space-info.c
> @@ -1264,7 +1264,7 @@ static void priority_reclaim_metadata_space(struct btrfs_fs_info *fs_info,
>  
>  	spin_lock(&space_info->lock);
>  	to_reclaim = btrfs_calc_reclaim_metadata_size(fs_info, space_info);
> -	if (!to_reclaim) {
> +	if (!to_reclaim || ticket->bytes == 0) {
David,

After speaking with josef it would seem the !to_reclaim condition is
gratuitous so can be removed, care to squash this in the patch?

<snip>

Re: [PATCH v3 2/7] btrfs: check for priority ticket granting before flushing

[David Sterba]

On Wed, Nov 17, 2021 at 07:04:25PM +0200, Nikolay Borisov wrote:
> 
> 
> On 9.11.21 г. 17:12, Josef Bacik wrote:
> > Since we're dropping locks before we enter the priority flushing loops
> > we could have had our ticket granted before we got the space_info->lock.
> > So add this check to avoid doing some extra flushing in the priority
> > flushing cases.
> > 
> > Signed-off-by: Josef Bacik <josef@toxicpanda.com>
> > ---
> >  fs/btrfs/space-info.c | 9 ++++++++-
> >  1 file changed, 8 insertions(+), 1 deletion(-)
> > 
> > diff --git a/fs/btrfs/space-info.c b/fs/btrfs/space-info.c
> > index 9d6048f54097..9a362f3a6df4 100644
> > --- a/fs/btrfs/space-info.c
> > +++ b/fs/btrfs/space-info.c
> > @@ -1264,7 +1264,7 @@ static void priority_reclaim_metadata_space(struct btrfs_fs_info *fs_info,
> >  
> >  	spin_lock(&space_info->lock);
> >  	to_reclaim = btrfs_calc_reclaim_metadata_size(fs_info, space_info);
> > -	if (!to_reclaim) {
> > +	if (!to_reclaim || ticket->bytes == 0) {
> David,
> 
> After speaking with josef it would seem the !to_reclaim condition is
> gratuitous so can be removed, care to squash this in the patch?

Sure, commit updated, thanks.