[PATCH 0/8][v2] Rework reserve ticket handling

[PATCH 0/8][v2] Rework reserve ticket handling

[Josef Bacik]

Hello,

Just some minor tweaks that needed to be added to fix issues introduced by the
next series of enospc fixes.

v1->v2:
- added "btrfs: fix may_commit_transaction to deal with no partial filling"
- fixed "btrfs: refactor the ticket wakeup code" to return true if we find a
  smaller ticket than our first ticket in the list.

Updated diffstat for the new series

 fs/btrfs/block-group.c    |   5 +-
 fs/btrfs/block-rsv.c      |   5 --
 fs/btrfs/delalloc-space.c |   4 --
 fs/btrfs/extent-tree.c    |  13 +---
 fs/btrfs/space-info.c     | 176 ++++++++++++++++++----------------------------
 fs/btrfs/space-info.h     |  30 +++++---
 6 files changed, 92 insertions(+), 141 deletions(-)

- Original email -
While cleaning up some things around the global reserve and can_overcommit I
started getting ENOSPC's with plenty of space to make reservations.  The root
cause of the problem has to do with how we satisfy ticket reservations.

Previously we would add any space we were returning to the space info to the
first ticket we found.  The reason we did this was because new reservations just
check the counters to see if they can continue, so we didn't want them to get
reservations when we had waiters already queued up.  So instead of returning the
bytes to the space info, I'd add it to the ticket.  Then if we failed to satisfy
that ticket reservation we'd take any space we found and add it to the next guy
in case it satisfied the next ticket reservation.

This works generally well in practice, but there are several xfstests that run
ENOSPC tests against very small file systems.  These tests uncovered a corner
case when it comes to overcommitting.  If we overcommit the space, and then are
no longer allowed to overcommit, we won't actually give any returned space to
the tickets, because that would be really bad.  Instead we return that space to
the space_info and carry on.

What was biting us in these test cases was the fact that we had very small
metadata area, 8mib, and unlink asks for about 2mib of space.  If we had
overcommitted 8.1mib, we'd give back almost 2mib of space to the space_info,
which could have instead been used for the reservation.  This would result in an
early ENOSPC.

Since we are only doing this partial filling dance to avoid racing with new
reservations we just fix that race by checking if we have pending reservations
on the list, closing that race.  Then we are free to use the normal checks to
see if a ticket can be woken up.  This simplifies the code a bunch, we no longer
have to keep track of how much space the tickets were given and return those
bytes, and I could consolidate the wakeup code into one function instead of two.

The diffstat is as follows, this all passes xfstests, and sets us up nicely for
the upcoming changesets.  Thanks,

Josef

[PATCH 1/8] btrfs: do not allow reservations if we have pending tickets

[Josef Bacik]

If we already have tickets on the list we don't want to steal their
reservations.  This is a preparation patch for upcoming changes,
technically this shouldn't happen today because of the way we add bytes
to tickets before adding them to the space_info in most cases.

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

diff --git a/fs/btrfs/space-info.c b/fs/btrfs/space-info.c
index e9406b2133d1..d671d6476eed 100644
--- a/fs/btrfs/space-info.c
+++ b/fs/btrfs/space-info.c
@@ -938,6 +938,7 @@ static int __reserve_metadata_bytes(struct btrfs_fs_info *fs_info,
 	u64 used;
 	u64 reclaim_bytes = 0;
 	int ret = 0;
+	bool pending_tickets;
 
 	ASSERT(orig_bytes);
 	ASSERT(!current->journal_info || flush != BTRFS_RESERVE_FLUSH_ALL);
@@ -945,14 +946,17 @@ static int __reserve_metadata_bytes(struct btrfs_fs_info *fs_info,
 	spin_lock(&space_info->lock);
 	ret = -ENOSPC;
 	used = btrfs_space_info_used(space_info, true);
+	pending_tickets = !list_empty(&space_info->tickets) ||
+		!list_empty(&space_info->priority_tickets);
 
 	/*
 	 * Carry on if we have enough space (short-circuit) OR call
 	 * can_overcommit() to ensure we can overcommit to continue.
 	 */
-	if ((used + orig_bytes <= space_info->total_bytes) ||
-	    can_overcommit(fs_info, space_info, orig_bytes, flush,
-			   system_chunk)) {
+	if (!pending_tickets &&
+	    ((used + orig_bytes <= space_info->total_bytes) ||
+	     can_overcommit(fs_info, space_info, orig_bytes, flush,
+			   system_chunk))) {
 		btrfs_space_info_update_bytes_may_use(fs_info, space_info,
 						      orig_bytes);
 		trace_btrfs_space_reservation(fs_info, "space_info",
-- 
2.21.0

[PATCH 2/8] btrfs: roll tracepoint into btrfs_space_info_update helper

[Josef Bacik]

We duplicate this tracepoint everywhere we call these helpers, so update
the helper to have the tracepoint as well.

Signed-off-by: Josef Bacik <josef@toxicpanda.com>
---
 fs/btrfs/block-group.c    |  3 ---
 fs/btrfs/block-rsv.c      |  5 -----
 fs/btrfs/delalloc-space.c |  4 ----
 fs/btrfs/extent-tree.c    |  9 ---------
 fs/btrfs/space-info.c     | 10 ----------
 fs/btrfs/space-info.h     | 10 +++++++---
 6 files changed, 7 insertions(+), 34 deletions(-)

diff --git a/fs/btrfs/block-group.c b/fs/btrfs/block-group.c
index 262e62ef52a5..9867c5d98650 100644
--- a/fs/btrfs/block-group.c
+++ b/fs/btrfs/block-group.c
@@ -2696,9 +2696,6 @@ int btrfs_update_block_group(struct btrfs_trans_handle *trans,
 			spin_unlock(&cache->lock);
 			spin_unlock(&cache->space_info->lock);
 
-			trace_btrfs_space_reservation(info, "pinned",
-						      cache->space_info->flags,
-						      num_bytes, 1);
 			percpu_counter_add_batch(
 					&cache->space_info->total_bytes_pinned,
 					num_bytes,
diff --git a/fs/btrfs/block-rsv.c b/fs/btrfs/block-rsv.c
index 698470b9f32d..c64b460a4301 100644
--- a/fs/btrfs/block-rsv.c
+++ b/fs/btrfs/block-rsv.c
@@ -283,16 +283,11 @@ void btrfs_update_global_block_rsv(struct btrfs_fs_info *fs_info)
 			block_rsv->reserved += num_bytes;
 			btrfs_space_info_update_bytes_may_use(fs_info, sinfo,
 							      num_bytes);
-			trace_btrfs_space_reservation(fs_info, "space_info",
-						      sinfo->flags, num_bytes,
-						      1);
 		}
 	} else if (block_rsv->reserved > block_rsv->size) {
 		num_bytes = block_rsv->reserved - block_rsv->size;
 		btrfs_space_info_update_bytes_may_use(fs_info, sinfo,
 						      -num_bytes);
-		trace_btrfs_space_reservation(fs_info, "space_info",
-				      sinfo->flags, num_bytes, 0);
 		block_rsv->reserved = block_rsv->size;
 	}
 
diff --git a/fs/btrfs/delalloc-space.c b/fs/btrfs/delalloc-space.c
index d2dfc201b2e1..1fc6bef3ccdf 100644
--- a/fs/btrfs/delalloc-space.c
+++ b/fs/btrfs/delalloc-space.c
@@ -130,8 +130,6 @@ int btrfs_alloc_data_chunk_ondemand(struct btrfs_inode *inode, u64 bytes)
 		return -ENOSPC;
 	}
 	btrfs_space_info_update_bytes_may_use(fs_info, data_sinfo, bytes);
-	trace_btrfs_space_reservation(fs_info, "space_info",
-				      data_sinfo->flags, bytes, 1);
 	spin_unlock(&data_sinfo->lock);
 
 	return 0;
@@ -183,8 +181,6 @@ void btrfs_free_reserved_data_space_noquota(struct inode *inode, u64 start,
 	data_sinfo = fs_info->data_sinfo;
 	spin_lock(&data_sinfo->lock);
 	btrfs_space_info_update_bytes_may_use(fs_info, data_sinfo, -len);
-	trace_btrfs_space_reservation(fs_info, "space_info",
-				      data_sinfo->flags, len, 0);
 	spin_unlock(&data_sinfo->lock);
 }
 
diff --git a/fs/btrfs/extent-tree.c b/fs/btrfs/extent-tree.c
index cd210550a349..32f9473c8426 100644
--- a/fs/btrfs/extent-tree.c
+++ b/fs/btrfs/extent-tree.c
@@ -2580,8 +2580,6 @@ static int pin_down_extent(struct btrfs_block_group_cache *cache,
 	spin_unlock(&cache->lock);
 	spin_unlock(&cache->space_info->lock);
 
-	trace_btrfs_space_reservation(fs_info, "pinned",
-				      cache->space_info->flags, num_bytes, 1);
 	percpu_counter_add_batch(&cache->space_info->total_bytes_pinned,
 		    num_bytes, BTRFS_TOTAL_BYTES_PINNED_BATCH);
 	set_extent_dirty(fs_info->pinned_extents, bytenr,
@@ -2839,9 +2837,6 @@ static int unpin_extent_range(struct btrfs_fs_info *fs_info,
 		spin_lock(&cache->lock);
 		cache->pinned -= len;
 		btrfs_space_info_update_bytes_pinned(fs_info, space_info, -len);
-
-		trace_btrfs_space_reservation(fs_info, "pinned",
-					      space_info->flags, len, 0);
 		space_info->max_extent_size = 0;
 		percpu_counter_add_batch(&space_info->total_bytes_pinned,
 			    -len, BTRFS_TOTAL_BYTES_PINNED_BATCH);
@@ -2863,10 +2858,6 @@ static int unpin_extent_range(struct btrfs_fs_info *fs_info,
 						space_info, to_add);
 				if (global_rsv->reserved >= global_rsv->size)
 					global_rsv->full = 1;
-				trace_btrfs_space_reservation(fs_info,
-							      "space_info",
-							      space_info->flags,
-							      to_add, 1);
 				len -= to_add;
 			}
 			spin_unlock(&global_rsv->lock);
diff --git a/fs/btrfs/space-info.c b/fs/btrfs/space-info.c
index d671d6476eed..780be5df31b4 100644
--- a/fs/btrfs/space-info.c
+++ b/fs/btrfs/space-info.c
@@ -279,8 +279,6 @@ void btrfs_space_info_add_old_bytes(struct btrfs_fs_info *fs_info,
 		goto again;
 	}
 	btrfs_space_info_update_bytes_may_use(fs_info, space_info, -num_bytes);
-	trace_btrfs_space_reservation(fs_info, "space_info",
-				      space_info->flags, num_bytes, 0);
 	spin_unlock(&space_info->lock);
 }
 
@@ -301,9 +299,6 @@ void btrfs_space_info_add_new_bytes(struct btrfs_fs_info *fs_info,
 		ticket = list_first_entry(head, struct reserve_ticket,
 					  list);
 		if (num_bytes >= ticket->bytes) {
-			trace_btrfs_space_reservation(fs_info, "space_info",
-						      space_info->flags,
-						      ticket->bytes, 1);
 			list_del_init(&ticket->list);
 			num_bytes -= ticket->bytes;
 			btrfs_space_info_update_bytes_may_use(fs_info,
@@ -313,9 +308,6 @@ void btrfs_space_info_add_new_bytes(struct btrfs_fs_info *fs_info,
 			space_info->tickets_id++;
 			wake_up(&ticket->wait);
 		} else {
-			trace_btrfs_space_reservation(fs_info, "space_info",
-						      space_info->flags,
-						      num_bytes, 1);
 			btrfs_space_info_update_bytes_may_use(fs_info,
 							      space_info,
 							      num_bytes);
@@ -959,8 +951,6 @@ static int __reserve_metadata_bytes(struct btrfs_fs_info *fs_info,
 			   system_chunk))) {
 		btrfs_space_info_update_bytes_may_use(fs_info, space_info,
 						      orig_bytes);
-		trace_btrfs_space_reservation(fs_info, "space_info",
-					      space_info->flags, orig_bytes, 1);
 		ret = 0;
 	}
 
diff --git a/fs/btrfs/space-info.h b/fs/btrfs/space-info.h
index c2b54b8e1a14..025f7ce2c9b1 100644
--- a/fs/btrfs/space-info.h
+++ b/fs/btrfs/space-info.h
@@ -87,14 +87,18 @@ static inline bool btrfs_mixed_space_info(struct btrfs_space_info *space_info)
  *
  * Declare a helper function to detect underflow of various space info members
  */
-#define DECLARE_SPACE_INFO_UPDATE(name)					\
+#define DECLARE_SPACE_INFO_UPDATE(name, trace_name)			\
 static inline void							\
 btrfs_space_info_update_##name(struct btrfs_fs_info *fs_info,		\
 			       struct btrfs_space_info *sinfo,		\
 			       s64 bytes)				\
 {									\
+	u64 abs_bytes = (bytes < 0) ? -bytes : bytes;			\
 	lockdep_assert_held(&sinfo->lock);				\
 	trace_update_##name(fs_info, sinfo, sinfo->name, bytes);	\
+	trace_btrfs_space_reservation(fs_info, trace_name,		\
+				      sinfo->flags, abs_bytes,		\
+				      bytes > 0);			\
 	if (bytes < 0 && sinfo->name < -bytes) {			\
 		WARN_ON(1);						\
 		sinfo->name = 0;					\
@@ -103,8 +107,8 @@ btrfs_space_info_update_##name(struct btrfs_fs_info *fs_info,		\
 	sinfo->name += bytes;						\
 }
 
-DECLARE_SPACE_INFO_UPDATE(bytes_may_use);
-DECLARE_SPACE_INFO_UPDATE(bytes_pinned);
+DECLARE_SPACE_INFO_UPDATE(bytes_may_use, "space_info");
+DECLARE_SPACE_INFO_UPDATE(bytes_pinned, "pinned");
 
 void btrfs_space_info_add_new_bytes(struct btrfs_fs_info *fs_info,
 				    struct btrfs_space_info *space_info,
-- 
2.21.0

[PATCH 3/8] btrfs: add space reservation tracepoint for reserved bytes

[Josef Bacik]

I noticed when folding the trace_btrfs_space_reservation() tracepoint
into the btrfs_space_info_update_* helpers that we didn't emit a
tracepoint when doing btrfs_add_reserved_bytes().  I know this is
because we were swapping bytes_may_use for bytes_reserved, so in my mind
there was no reason to have the tracepoint there.  But now there is
because we always emit the unreserve for the bytes_may_use side, and
this would have broken if compression was on anyway.  Add a tracepoint
to cover the bytes_reserved counter so the math still comes out right.

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

diff --git a/fs/btrfs/block-group.c b/fs/btrfs/block-group.c
index 9867c5d98650..afae5c731904 100644
--- a/fs/btrfs/block-group.c
+++ b/fs/btrfs/block-group.c
@@ -2758,6 +2758,8 @@ int btrfs_add_reserved_bytes(struct btrfs_block_group_cache *cache,
 	} else {
 		cache->reserved += num_bytes;
 		space_info->bytes_reserved += num_bytes;
+		trace_btrfs_space_reservation(cache->fs_info, "space_info",
+					      space_info->flags, num_bytes, 1);
 		btrfs_space_info_update_bytes_may_use(cache->fs_info,
 						      space_info, -ram_bytes);
 		if (delalloc)
-- 
2.21.0

[PATCH 4/8] btrfs: rework btrfs_space_info_add_old_bytes

[Josef Bacik]

If there are pending tickets and we are overcommitted we will simply
return free'd reservations to space_info->bytes_may_use if we cannot
overcommit any more.  This is problematic because we assume any free
space would have been added to the tickets, and so if we go from an
overcommitted state to not overcommitted we could have plenty of space
in our space_info but be unable to make progress on our tickets because
we only refill tickets from previous reservations.

Instead of doing this partial filling of tickets dance we need to simply
add our space to the space_info, and then do the normal check to see if
we can satisfy the whole reservation.  If we can then we wake up the
ticket and carry on.

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

diff --git a/fs/btrfs/space-info.c b/fs/btrfs/space-info.c
index 780be5df31b4..5f123b36fdcd 100644
--- a/fs/btrfs/space-info.c
+++ b/fs/btrfs/space-info.c
@@ -233,52 +233,41 @@ void btrfs_space_info_add_old_bytes(struct btrfs_fs_info *fs_info,
 				    struct btrfs_space_info *space_info,
 				    u64 num_bytes)
 {
-	struct reserve_ticket *ticket;
 	struct list_head *head;
-	u64 used;
 	enum btrfs_reserve_flush_enum flush = BTRFS_RESERVE_NO_FLUSH;
-	bool check_overcommit = false;
 
 	spin_lock(&space_info->lock);
 	head = &space_info->priority_tickets;
+	btrfs_space_info_update_bytes_may_use(fs_info, space_info, -num_bytes);
 
-	/*
-	 * If we are over our limit then we need to check and see if we can
-	 * overcommit, and if we can't then we just need to free up our space
-	 * and not satisfy any requests.
-	 */
-	used = btrfs_space_info_used(space_info, true);
-	if (used - num_bytes >= space_info->total_bytes)
-		check_overcommit = true;
 again:
-	while (!list_empty(head) && num_bytes) {
-		ticket = list_first_entry(head, struct reserve_ticket,
-					  list);
-		/*
-		 * We use 0 bytes because this space is already reserved, so
-		 * adding the ticket space would be a double count.
-		 */
-		if (check_overcommit &&
-		    !can_overcommit(fs_info, space_info, 0, flush, false))
-			break;
-		if (num_bytes >= ticket->bytes) {
+	while (!list_empty(head)) {
+		struct reserve_ticket *ticket;
+		u64 used = btrfs_space_info_used(space_info, true);
+
+		ticket = list_first_entry(head, struct reserve_ticket, list);
+
+		/* Check and see if our ticket can be satisified now. */
+		if ((used + ticket->bytes <= space_info->total_bytes) ||
+		    can_overcommit(fs_info, space_info, ticket->bytes, flush,
+				   false)) {
+			btrfs_space_info_update_bytes_may_use(fs_info,
+							      space_info,
+							      ticket->bytes);
 			list_del_init(&ticket->list);
-			num_bytes -= ticket->bytes;
 			ticket->bytes = 0;
 			space_info->tickets_id++;
 			wake_up(&ticket->wait);
 		} else {
-			ticket->bytes -= num_bytes;
-			num_bytes = 0;
+			break;
 		}
 	}
 
-	if (num_bytes && head == &space_info->priority_tickets) {
+	if (head == &space_info->priority_tickets) {
 		head = &space_info->tickets;
 		flush = BTRFS_RESERVE_FLUSH_ALL;
 		goto again;
 	}
-	btrfs_space_info_update_bytes_may_use(fs_info, space_info, -num_bytes);
 	spin_unlock(&space_info->lock);
 }
 
-- 
2.21.0

[PATCH 5/8] btrfs: refactor the ticket wakeup code

[Josef Bacik]

Now that btrfs_space_info_add_old_bytes simply checks if we can make the
reservation and updates bytes_may_use, there's no reason to have both
helpers in place.  Factor out the ticket wakeup logic into it's own
helper, make btrfs_space_info_add_old_bytes() update bytes_may_use and
then call the wakeup helper, and replace all calls to
btrfs_space_info_add_new_bytes() with the wakeup helper.

Signed-off-by: Josef Bacik <josef@toxicpanda.com>
---
 fs/btrfs/extent-tree.c |  4 ++--
 fs/btrfs/space-info.c  | 53 +++---------------------------------------
 fs/btrfs/space-info.h  | 19 ++++++++++-----
 3 files changed, 18 insertions(+), 58 deletions(-)

diff --git a/fs/btrfs/extent-tree.c b/fs/btrfs/extent-tree.c
index 32f9473c8426..08c6fcfc418d 100644
--- a/fs/btrfs/extent-tree.c
+++ b/fs/btrfs/extent-tree.c
@@ -2863,8 +2863,8 @@ static int unpin_extent_range(struct btrfs_fs_info *fs_info,
 			spin_unlock(&global_rsv->lock);
 			/* Add to any tickets we may have */
 			if (len)
-				btrfs_space_info_add_new_bytes(fs_info,
-						space_info, len);
+				btrfs_try_to_wakeup_tickets(fs_info,
+							    space_info);
 		}
 		spin_unlock(&space_info->lock);
 	}
diff --git a/fs/btrfs/space-info.c b/fs/btrfs/space-info.c
index 5f123b36fdcd..8a1c7ada67cb 100644
--- a/fs/btrfs/space-info.c
+++ b/fs/btrfs/space-info.c
@@ -131,9 +131,7 @@ void btrfs_update_space_info(struct btrfs_fs_info *info, u64 flags,
 	found->bytes_readonly += bytes_readonly;
 	if (total_bytes > 0)
 		found->full = 0;
-	btrfs_space_info_add_new_bytes(info, found,
-				       total_bytes - bytes_used -
-				       bytes_readonly);
+	btrfs_try_to_wakeup_tickets(info, found);
 	spin_unlock(&found->lock);
 	*space_info = found;
 }
@@ -229,17 +227,13 @@ static int can_overcommit(struct btrfs_fs_info *fs_info,
  * This is for space we already have accounted in space_info->bytes_may_use, so
  * basically when we're returning space from block_rsv's.
  */
-void btrfs_space_info_add_old_bytes(struct btrfs_fs_info *fs_info,
-				    struct btrfs_space_info *space_info,
-				    u64 num_bytes)
+void btrfs_try_to_wakeup_tickets(struct btrfs_fs_info *fs_info,
+				 struct btrfs_space_info *space_info)
 {
 	struct list_head *head;
 	enum btrfs_reserve_flush_enum flush = BTRFS_RESERVE_NO_FLUSH;
 
-	spin_lock(&space_info->lock);
 	head = &space_info->priority_tickets;
-	btrfs_space_info_update_bytes_may_use(fs_info, space_info, -num_bytes);
-
 again:
 	while (!list_empty(head)) {
 		struct reserve_ticket *ticket;
@@ -268,47 +262,6 @@ void btrfs_space_info_add_old_bytes(struct btrfs_fs_info *fs_info,
 		flush = BTRFS_RESERVE_FLUSH_ALL;
 		goto again;
 	}
-	spin_unlock(&space_info->lock);
-}
-
-/*
- * This is for newly allocated space that isn't accounted in
- * space_info->bytes_may_use yet.  So if we allocate a chunk or unpin an extent
- * we use this helper.
- */
-void btrfs_space_info_add_new_bytes(struct btrfs_fs_info *fs_info,
-				    struct btrfs_space_info *space_info,
-				    u64 num_bytes)
-{
-	struct reserve_ticket *ticket;
-	struct list_head *head = &space_info->priority_tickets;
-
-again:
-	while (!list_empty(head) && num_bytes) {
-		ticket = list_first_entry(head, struct reserve_ticket,
-					  list);
-		if (num_bytes >= ticket->bytes) {
-			list_del_init(&ticket->list);
-			num_bytes -= ticket->bytes;
-			btrfs_space_info_update_bytes_may_use(fs_info,
-							      space_info,
-							      ticket->bytes);
-			ticket->bytes = 0;
-			space_info->tickets_id++;
-			wake_up(&ticket->wait);
-		} else {
-			btrfs_space_info_update_bytes_may_use(fs_info,
-							      space_info,
-							      num_bytes);
-			ticket->bytes -= num_bytes;
-			num_bytes = 0;
-		}
-	}
-
-	if (num_bytes && head == &space_info->priority_tickets) {
-		head = &space_info->tickets;
-		goto again;
-	}
 }
 
 #define DUMP_BLOCK_RSV(fs_info, rsv_name)				\
diff --git a/fs/btrfs/space-info.h b/fs/btrfs/space-info.h
index 025f7ce2c9b1..9ae5cae52fde 100644
--- a/fs/btrfs/space-info.h
+++ b/fs/btrfs/space-info.h
@@ -110,12 +110,6 @@ btrfs_space_info_update_##name(struct btrfs_fs_info *fs_info,		\
 DECLARE_SPACE_INFO_UPDATE(bytes_may_use, "space_info");
 DECLARE_SPACE_INFO_UPDATE(bytes_pinned, "pinned");
 
-void btrfs_space_info_add_new_bytes(struct btrfs_fs_info *fs_info,
-				    struct btrfs_space_info *space_info,
-				    u64 num_bytes);
-void btrfs_space_info_add_old_bytes(struct btrfs_fs_info *fs_info,
-				    struct btrfs_space_info *space_info,
-				    u64 num_bytes);
 int btrfs_init_space_info(struct btrfs_fs_info *fs_info);
 void btrfs_update_space_info(struct btrfs_fs_info *info, u64 flags,
 			     u64 total_bytes, u64 bytes_used,
@@ -133,5 +127,18 @@ int btrfs_reserve_metadata_bytes(struct btrfs_root *root,
 				 struct btrfs_block_rsv *block_rsv,
 				 u64 orig_bytes,
 				 enum btrfs_reserve_flush_enum flush);
+void btrfs_try_to_wakeup_tickets(struct btrfs_fs_info *fs_info,
+				 struct btrfs_space_info *space_info);
+
+static inline void
+btrfs_space_info_add_old_bytes(struct btrfs_fs_info *fs_info,
+			       struct btrfs_space_info *space_info,
+			       u64 num_bytes)
+{
+	spin_lock(&space_info->lock);
+	btrfs_space_info_update_bytes_may_use(fs_info, space_info, -num_bytes);
+	btrfs_try_to_wakeup_tickets(fs_info, space_info);
+	spin_unlock(&space_info->lock);
+}
 
 #endif /* BTRFS_SPACE_INFO_H */
-- 
2.21.0

[PATCH 6/8] btrfs: rework wake_all_tickets

[Josef Bacik]

Now that we no longer partially fill tickets we need to rework
wake_all_tickets to call btrfs_try_to_wakeup_tickets() in order to see
if any subsequent tickets are able to be satisfied.  If our tickets_id
changes we know something happened and we can keep flushing.

Also if we find a ticket that is smaller than the first ticket in our
queue then we want to retry the flushing loop again in case
may_commit_transaction() decides we could satisfy the ticket by
committing the transaction.

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

diff --git a/fs/btrfs/space-info.c b/fs/btrfs/space-info.c
index 8a1c7ada67cb..bd485be783b8 100644
--- a/fs/btrfs/space-info.c
+++ b/fs/btrfs/space-info.c
@@ -676,19 +676,39 @@ static inline int need_do_async_reclaim(struct btrfs_fs_info *fs_info,
 		!test_bit(BTRFS_FS_STATE_REMOUNTING, &fs_info->fs_state));
 }
 
-static bool wake_all_tickets(struct list_head *head)
+static bool wake_all_tickets(struct btrfs_fs_info *fs_info,
+			     struct btrfs_space_info *space_info)
 {
 	struct reserve_ticket *ticket;
+	u64 tickets_id = space_info->tickets_id;
+	u64 first_ticket_bytes = 0;
+
+	while (!list_empty(&space_info->tickets) &&
+	       tickets_id == space_info->tickets_id) {
+		ticket = list_first_entry(&space_info->tickets,
+					  struct reserve_ticket, list);
+
+		/*
+		 * may_commit_transaction will avoid committing the transaction
+		 * if it doesn't feel like the space reclaimed by the commit
+		 * would result in the ticket succeeding.  However if we have a
+		 * smaller ticket in the queue it may be small enough to be
+		 * satisified by committing the transaction, so if any
+		 * subsequent ticket is smaller than the first ticket go ahead
+		 * and send us back for another loop through the enospc flushing
+		 * code.
+		 */
+		if (first_ticket_bytes == 0)
+			first_ticket_bytes = ticket->bytes;
+		else if (first_ticket_bytes > ticket->bytes)
+			return true;
 
-	while (!list_empty(head)) {
-		ticket = list_first_entry(head, struct reserve_ticket, list);
 		list_del_init(&ticket->list);
 		ticket->error = -ENOSPC;
 		wake_up(&ticket->wait);
-		if (ticket->bytes != ticket->orig_bytes)
-			return true;
+		btrfs_try_to_wakeup_tickets(fs_info, space_info);
 	}
-	return false;
+	return (tickets_id != space_info->tickets_id);
 }
 
 /*
@@ -756,7 +776,7 @@ static void btrfs_async_reclaim_metadata_space(struct work_struct *work)
 		if (flush_state > COMMIT_TRANS) {
 			commit_cycles++;
 			if (commit_cycles > 2) {
-				if (wake_all_tickets(&space_info->tickets)) {
+				if (wake_all_tickets(fs_info, space_info)) {
 					flush_state = FLUSH_DELAYED_ITEMS_NR;
 					commit_cycles--;
 				} else {
-- 
2.21.0

[PATCH 7/8] btrfs: fix may_commit_transaction to deal with no partial filling

[Josef Bacik]

Now that we aren't partially filling tickets we may have some slack
space left in the space_info.  We need to account for this in
may_commit_transaction, otherwise we may choose to not commit the
transaction despite it actually having enough space to satisfy our
ticket.

Calculate the free space we have in the space_info, if any.  Then check
to see if its >= the amount of bytes_needed after we've accounted for
the space being used by the delayed refs rsv.  If it's not subtract it
from the bytes_needed before doing the final pinned check.  If we still
don't have enough space then we are truly out of space.

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

diff --git a/fs/btrfs/space-info.c b/fs/btrfs/space-info.c
index bd485be783b8..f79afdc04925 100644
--- a/fs/btrfs/space-info.c
+++ b/fs/btrfs/space-info.c
@@ -471,12 +471,19 @@ static int may_commit_transaction(struct btrfs_fs_info *fs_info,
 	struct btrfs_trans_handle *trans;
 	u64 bytes_needed;
 	u64 reclaim_bytes = 0;
+	u64 cur_free_bytes = 0;
 
 	trans = (struct btrfs_trans_handle *)current->journal_info;
 	if (trans)
 		return -EAGAIN;
 
 	spin_lock(&space_info->lock);
+	cur_free_bytes = btrfs_space_info_used(space_info, true);
+	if (cur_free_bytes < space_info->total_bytes)
+		cur_free_bytes = space_info->total_bytes - cur_free_bytes;
+	else
+		cur_free_bytes = 0;
+
 	if (!list_empty(&space_info->priority_tickets))
 		ticket = list_first_entry(&space_info->priority_tickets,
 					  struct reserve_ticket, list);
@@ -522,6 +529,18 @@ static int may_commit_transaction(struct btrfs_fs_info *fs_info,
 		goto commit;
 	bytes_needed -= reclaim_bytes;
 
+	/*
+	 * We don't partially fill tickets anymore, so we could have some free
+	 * bytes in the space_info already, just not enough to satisfy the
+	 * ticket.  If bytes_needed is already below cur_free_bytes after taking
+	 * away the delayed refs and delayed rsv's then we can commit.
+	 * Otherwise subtract our cur_free_bytes from bytes_needed before we
+	 * check pinned.
+	 */
+	if (bytes_needed <= cur_free_bytes)
+		goto commit;
+	bytes_needed -= cur_free_bytes;
+
 	if (__percpu_counter_compare(&space_info->total_bytes_pinned,
 				   bytes_needed,
 				   BTRFS_TOTAL_BYTES_PINNED_BATCH) < 0)
-- 
2.21.0

[PATCH 8/8] btrfs: remove orig_bytes from reserve_ticket

[Josef Bacik]

Now that we do not do partial filling of tickets simply remove
orig_bytes, it is no longer needed.

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

diff --git a/fs/btrfs/space-info.c b/fs/btrfs/space-info.c
index f79afdc04925..c8867a0d9f5a 100644
--- a/fs/btrfs/space-info.c
+++ b/fs/btrfs/space-info.c
@@ -856,7 +856,6 @@ static int wait_reserve_ticket(struct btrfs_fs_info *fs_info,
 
 {
 	DEFINE_WAIT(wait);
-	u64 reclaim_bytes = 0;
 	int ret = 0;
 
 	spin_lock(&space_info->lock);
@@ -877,13 +876,7 @@ static int wait_reserve_ticket(struct btrfs_fs_info *fs_info,
 		ret = ticket->error;
 	if (!list_empty(&ticket->list))
 		list_del_init(&ticket->list);
-	if (ticket->bytes && ticket->bytes < ticket->orig_bytes)
-		reclaim_bytes = ticket->orig_bytes - ticket->bytes;
 	spin_unlock(&space_info->lock);
-
-	if (reclaim_bytes)
-		btrfs_space_info_add_old_bytes(fs_info, space_info,
-					       reclaim_bytes);
 	return ret;
 }
 
@@ -909,7 +902,6 @@ static int __reserve_metadata_bytes(struct btrfs_fs_info *fs_info,
 {
 	struct reserve_ticket ticket;
 	u64 used;
-	u64 reclaim_bytes = 0;
 	int ret = 0;
 	bool pending_tickets;
 
@@ -943,7 +935,6 @@ static int __reserve_metadata_bytes(struct btrfs_fs_info *fs_info,
 	 * the list and we will do our own flushing further down.
 	 */
 	if (ret && flush != BTRFS_RESERVE_NO_FLUSH) {
-		ticket.orig_bytes = orig_bytes;
 		ticket.bytes = orig_bytes;
 		ticket.error = 0;
 		init_waitqueue_head(&ticket.wait);
@@ -990,16 +981,10 @@ static int __reserve_metadata_bytes(struct btrfs_fs_info *fs_info,
 	priority_reclaim_metadata_space(fs_info, space_info, &ticket);
 	spin_lock(&space_info->lock);
 	if (ticket.bytes) {
-		if (ticket.bytes < orig_bytes)
-			reclaim_bytes = orig_bytes - ticket.bytes;
 		list_del_init(&ticket.list);
 		ret = -ENOSPC;
 	}
 	spin_unlock(&space_info->lock);
-
-	if (reclaim_bytes)
-		btrfs_space_info_add_old_bytes(fs_info, space_info,
-					       reclaim_bytes);
 	ASSERT(list_empty(&ticket.list));
 	return ret;
 }
diff --git a/fs/btrfs/space-info.h b/fs/btrfs/space-info.h
index 9ae5cae52fde..ebc5b407a954 100644
--- a/fs/btrfs/space-info.h
+++ b/fs/btrfs/space-info.h
@@ -70,7 +70,6 @@ struct btrfs_space_info {
 };
 
 struct reserve_ticket {
-	u64 orig_bytes;
 	u64 bytes;
 	int error;
 	struct list_head list;
-- 
2.21.0

Re: [PATCH 1/8] btrfs: do not allow reservations if we have pending tickets

[Nikolay Borisov]

On 16.08.19 г. 17:19 ч., Josef Bacik wrote:
> If we already have tickets on the list we don't want to steal their
> reservations.  This is a preparation patch for upcoming changes,
> technically this shouldn't happen today because of the way we add bytes
> to tickets before adding them to the space_info in most cases.

nit: IMO this changelog should be a bit more explicit since this commit
really makes reservations in FIFO order. Have you also quantified what's
the latency impact as I suspect this will introduce such latencies?


> 
> Signed-off-by: Josef Bacik <josef@toxicpanda.com>
> ---
>  fs/btrfs/space-info.c | 10 +++++++---
>  1 file changed, 7 insertions(+), 3 deletions(-)
> 
> diff --git a/fs/btrfs/space-info.c b/fs/btrfs/space-info.c
> index e9406b2133d1..d671d6476eed 100644
> --- a/fs/btrfs/space-info.c
> +++ b/fs/btrfs/space-info.c
> @@ -938,6 +938,7 @@ static int __reserve_metadata_bytes(struct btrfs_fs_info *fs_info,
>  	u64 used;
>  	u64 reclaim_bytes = 0;
>  	int ret = 0;
> +	bool pending_tickets;
>  
>  	ASSERT(orig_bytes);
>  	ASSERT(!current->journal_info || flush != BTRFS_RESERVE_FLUSH_ALL);
> @@ -945,14 +946,17 @@ static int __reserve_metadata_bytes(struct btrfs_fs_info *fs_info,
>  	spin_lock(&space_info->lock);
>  	ret = -ENOSPC;
>  	used = btrfs_space_info_used(space_info, true);
> +	pending_tickets = !list_empty(&space_info->tickets) ||
> +		!list_empty(&space_info->priority_tickets);
>  
>  	/*
>  	 * Carry on if we have enough space (short-circuit) OR call
>  	 * can_overcommit() to ensure we can overcommit to continue.
>  	 */
> -	if ((used + orig_bytes <= space_info->total_bytes) ||
> -	    can_overcommit(fs_info, space_info, orig_bytes, flush,
> -			   system_chunk)) {
> +	if (!pending_tickets &&
> +	    ((used + orig_bytes <= space_info->total_bytes) ||
> +	     can_overcommit(fs_info, space_info, orig_bytes, flush,
> +			   system_chunk))) {
>  		btrfs_space_info_update_bytes_may_use(fs_info, space_info,
>  						      orig_bytes);
>  		trace_btrfs_space_reservation(fs_info, "space_info",
> 

Re: [PATCH 1/8] btrfs: do not allow reservations if we have pending tickets

[Josef Bacik]

On Mon, Aug 19, 2019 at 03:54:29PM +0300, Nikolay Borisov wrote:
> 
> 
> On 16.08.19 г. 17:19 ч., Josef Bacik wrote:
> > If we already have tickets on the list we don't want to steal their
> > reservations.  This is a preparation patch for upcoming changes,
> > technically this shouldn't happen today because of the way we add bytes
> > to tickets before adding them to the space_info in most cases.
> 
> nit: IMO this changelog should be a bit more explicit since this commit
> really makes reservations in FIFO order. Have you also quantified what's
> the latency impact as I suspect this will introduce such latencies?

Reservations were already in FIFO order because we would add new space to
existing tickets.  This doesn't change the behavior, just makes it so we get the
same behavior without refilling.  Thanks,

Josef

Re: [PATCH 5/8] btrfs: refactor the ticket wakeup code

[Nikolay Borisov]

On 16.08.19 г. 17:19 ч., Josef Bacik wrote:
> Now that btrfs_space_info_add_old_bytes simply checks if we can make the
> reservation and updates bytes_may_use, there's no reason to have both
> helpers in place.  Factor out the ticket wakeup logic into it's own
> helper, make btrfs_space_info_add_old_bytes() update bytes_may_use and
> then call the wakeup helper, and replace all calls to
> btrfs_space_info_add_new_bytes() with the wakeup helper.
> 
> Signed-off-by: Josef Bacik <josef@toxicpanda.com>

Generally looks good and much nicer than before, one small nit below,
when fixed you can add:

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

> ---
>  fs/btrfs/extent-tree.c |  4 ++--
>  fs/btrfs/space-info.c  | 53 +++---------------------------------------
>  fs/btrfs/space-info.h  | 19 ++++++++++-----
>  3 files changed, 18 insertions(+), 58 deletions(-)
> 
> diff --git a/fs/btrfs/extent-tree.c b/fs/btrfs/extent-tree.c
> index 32f9473c8426..08c6fcfc418d 100644
> --- a/fs/btrfs/extent-tree.c
> +++ b/fs/btrfs/extent-tree.c
> @@ -2863,8 +2863,8 @@ static int unpin_extent_range(struct btrfs_fs_info *fs_info,
>  			spin_unlock(&global_rsv->lock);
>  			/* Add to any tickets we may have */
>  			if (len)
> -				btrfs_space_info_add_new_bytes(fs_info,
> -						space_info, len);
> +				btrfs_try_to_wakeup_tickets(fs_info,
> +							    space_info);
>  		}
>  		spin_unlock(&space_info->lock);
>  	}
> diff --git a/fs/btrfs/space-info.c b/fs/btrfs/space-info.c
> index 5f123b36fdcd..8a1c7ada67cb 100644
> --- a/fs/btrfs/space-info.c
> +++ b/fs/btrfs/space-info.c
> @@ -131,9 +131,7 @@ void btrfs_update_space_info(struct btrfs_fs_info *info, u64 flags,
>  	found->bytes_readonly += bytes_readonly;
>  	if (total_bytes > 0)
>  		found->full = 0;
> -	btrfs_space_info_add_new_bytes(info, found,
> -				       total_bytes - bytes_used -
> -				       bytes_readonly);
> +	btrfs_try_to_wakeup_tickets(info, found);
>  	spin_unlock(&found->lock);
>  	*space_info = found;
>  }
> @@ -229,17 +227,13 @@ static int can_overcommit(struct btrfs_fs_info *fs_info,
>   * This is for space we already have accounted in space_info->bytes_may_use, so
>   * basically when we're returning space from block_rsv's.
>   */
> -void btrfs_space_info_add_old_bytes(struct btrfs_fs_info *fs_info,
> -				    struct btrfs_space_info *space_info,
> -				    u64 num_bytes)
> +void btrfs_try_to_wakeup_tickets(struct btrfs_fs_info *fs_info,
> +				 struct btrfs_space_info *space_info)
>  {
>  	struct list_head *head;
>  	enum btrfs_reserve_flush_enum flush = BTRFS_RESERVE_NO_FLUSH;
>  
> -	spin_lock(&space_info->lock);


Add an lockdep_assert_held(&space_info->lock);

<snip>

Re: [PATCH 6/8] btrfs: rework wake_all_tickets

[Nikolay Borisov]

On 16.08.19 г. 17:19 ч., Josef Bacik wrote:
> Now that we no longer partially fill tickets we need to rework
> wake_all_tickets to call btrfs_try_to_wakeup_tickets() in order to see
> if any subsequent tickets are able to be satisfied.  If our tickets_id
> changes we know something happened and we can keep flushing.
> 
> Also if we find a ticket that is smaller than the first ticket in our
> queue then we want to retry the flushing loop again in case
> may_commit_transaction() decides we could satisfy the ticket by
> committing the transaction.
> 
> Signed-off-by: Josef Bacik <josef@toxicpanda.com>
> ---
>  fs/btrfs/space-info.c | 34 +++++++++++++++++++++++++++-------
>  1 file changed, 27 insertions(+), 7 deletions(-)
> 
> diff --git a/fs/btrfs/space-info.c b/fs/btrfs/space-info.c
> index 8a1c7ada67cb..bd485be783b8 100644
> --- a/fs/btrfs/space-info.c
> +++ b/fs/btrfs/space-info.c
> @@ -676,19 +676,39 @@ static inline int need_do_async_reclaim(struct btrfs_fs_info *fs_info,
>  		!test_bit(BTRFS_FS_STATE_REMOUNTING, &fs_info->fs_state));
>  }
>  
> -static bool wake_all_tickets(struct list_head *head)
> +static bool wake_all_tickets(struct btrfs_fs_info *fs_info,
> +			     struct btrfs_space_info *space_info)
>  {
>  	struct reserve_ticket *ticket;
> +	u64 tickets_id = space_info->tickets_id;
> +	u64 first_ticket_bytes = 0;
> +
> +	while (!list_empty(&space_info->tickets) &&
> +	       tickets_id == space_info->tickets_id) {
> +		ticket = list_first_entry(&space_info->tickets,
> +					  struct reserve_ticket, list);
> +
> +		/*
> +		 * may_commit_transaction will avoid committing the transaction
> +		 * if it doesn't feel like the space reclaimed by the commit
> +		 * would result in the ticket succeeding.  However if we have a
> +		 * smaller ticket in the queue it may be small enough to be
> +		 * satisified by committing the transaction, so if any
> +		 * subsequent ticket is smaller than the first ticket go ahead
> +		 * and send us back for another loop through the enospc flushing
> +		 * code.
> +		 */
> +		if (first_ticket_bytes == 0)
> +			first_ticket_bytes = ticket->bytes;
> +		else if (first_ticket_bytes > ticket->bytes)
> +			return true;
>  
> -	while (!list_empty(head)) {
> -		ticket = list_first_entry(head, struct reserve_ticket, list);
>  		list_del_init(&ticket->list);
>  		ticket->error = -ENOSPC;
>  		wake_up(&ticket->wait);
> -		if (ticket->bytes != ticket->orig_bytes)
> -			return true;
> +		btrfs_try_to_wakeup_tickets(fs_info, space_info);

So the change in this logic is directly related to the implementation of
btrfs_try_to_wakeup_tickets. Because when we fail and remove a ticket in
this function we give a chance that the next ticket *could* be
satisfied. But how well does that work in practice, given you fail
normal prio tickets here, whereas btrfs_try_to_wakeup_tickets first
checks the prio ticket. So even if you are failing normal ticket but
there is one unsatifiable prio ticket that won't really change anything.

>  	}
> -	return false;
> +	return (tickets_id != space_info->tickets_id);
>  }
>  
>  /*
> @@ -756,7 +776,7 @@ static void btrfs_async_reclaim_metadata_space(struct work_struct *work)
>  		if (flush_state > COMMIT_TRANS) {
>  			commit_cycles++;
>  			if (commit_cycles > 2) {
> -				if (wake_all_tickets(&space_info->tickets)) {
> +				if (wake_all_tickets(fs_info, space_info)) {
>  					flush_state = FLUSH_DELAYED_ITEMS_NR;
>  					commit_cycles--;
>  				} else {
> 

Re: [PATCH 6/8] btrfs: rework wake_all_tickets

[Josef Bacik]

On Mon, Aug 19, 2019 at 05:49:45PM +0300, Nikolay Borisov wrote:
> 
> 
> On 16.08.19 г. 17:19 ч., Josef Bacik wrote:
> > Now that we no longer partially fill tickets we need to rework
> > wake_all_tickets to call btrfs_try_to_wakeup_tickets() in order to see
> > if any subsequent tickets are able to be satisfied.  If our tickets_id
> > changes we know something happened and we can keep flushing.
> > 
> > Also if we find a ticket that is smaller than the first ticket in our
> > queue then we want to retry the flushing loop again in case
> > may_commit_transaction() decides we could satisfy the ticket by
> > committing the transaction.
> > 
> > Signed-off-by: Josef Bacik <josef@toxicpanda.com>
> > ---
> >  fs/btrfs/space-info.c | 34 +++++++++++++++++++++++++++-------
> >  1 file changed, 27 insertions(+), 7 deletions(-)
> > 
> > diff --git a/fs/btrfs/space-info.c b/fs/btrfs/space-info.c
> > index 8a1c7ada67cb..bd485be783b8 100644
> > --- a/fs/btrfs/space-info.c
> > +++ b/fs/btrfs/space-info.c
> > @@ -676,19 +676,39 @@ static inline int need_do_async_reclaim(struct btrfs_fs_info *fs_info,
> >  		!test_bit(BTRFS_FS_STATE_REMOUNTING, &fs_info->fs_state));
> >  }
> >  
> > -static bool wake_all_tickets(struct list_head *head)
> > +static bool wake_all_tickets(struct btrfs_fs_info *fs_info,
> > +			     struct btrfs_space_info *space_info)
> >  {
> >  	struct reserve_ticket *ticket;
> > +	u64 tickets_id = space_info->tickets_id;
> > +	u64 first_ticket_bytes = 0;
> > +
> > +	while (!list_empty(&space_info->tickets) &&
> > +	       tickets_id == space_info->tickets_id) {
> > +		ticket = list_first_entry(&space_info->tickets,
> > +					  struct reserve_ticket, list);
> > +
> > +		/*
> > +		 * may_commit_transaction will avoid committing the transaction
> > +		 * if it doesn't feel like the space reclaimed by the commit
> > +		 * would result in the ticket succeeding.  However if we have a
> > +		 * smaller ticket in the queue it may be small enough to be
> > +		 * satisified by committing the transaction, so if any
> > +		 * subsequent ticket is smaller than the first ticket go ahead
> > +		 * and send us back for another loop through the enospc flushing
> > +		 * code.
> > +		 */
> > +		if (first_ticket_bytes == 0)
> > +			first_ticket_bytes = ticket->bytes;
> > +		else if (first_ticket_bytes > ticket->bytes)
> > +			return true;
> >  
> > -	while (!list_empty(head)) {
> > -		ticket = list_first_entry(head, struct reserve_ticket, list);
> >  		list_del_init(&ticket->list);
> >  		ticket->error = -ENOSPC;
> >  		wake_up(&ticket->wait);
> > -		if (ticket->bytes != ticket->orig_bytes)
> > -			return true;
> > +		btrfs_try_to_wakeup_tickets(fs_info, space_info);
> 
> So the change in this logic is directly related to the implementation of
> btrfs_try_to_wakeup_tickets. Because when we fail and remove a ticket in
> this function we give a chance that the next ticket *could* be
> satisfied. But how well does that work in practice, given you fail
> normal prio tickets here, whereas btrfs_try_to_wakeup_tickets first
> checks the prio ticket. So even if you are failing normal ticket but
> there is one unsatifiable prio ticket that won't really change anything.

In practice we don't get to this state with high priority tickets on the list.
Anything that would be long-ish term on the priority list is evict, and we wait
for iput()'s in the normal flushing code.  At the point we hit wake_all_tickets
we generally should only have tickets on the normal list.

I suppose we could possibly get into this situation, but again the high priority
tickets are going to be evict, truncate block, and relocate, which all have
significantly lower reservation amounts than things like create or unlink.  If
those things are unable to get reservations then we are truly out of space.
Thanks,

Josef

Re: [PATCH 6/8] btrfs: rework wake_all_tickets

[Nikolay Borisov]

On 19.08.19 г. 18:06 ч., Josef Bacik wrote:
> On Mon, Aug 19, 2019 at 05:49:45PM +0300, Nikolay Borisov wrote:
>>
>>
>> On 16.08.19 г. 17:19 ч., Josef Bacik wrote:
>>> Now that we no longer partially fill tickets we need to rework
>>> wake_all_tickets to call btrfs_try_to_wakeup_tickets() in order to see
>>> if any subsequent tickets are able to be satisfied.  If our tickets_id
>>> changes we know something happened and we can keep flushing.
>>>
>>> Also if we find a ticket that is smaller than the first ticket in our
>>> queue then we want to retry the flushing loop again in case
>>> may_commit_transaction() decides we could satisfy the ticket by
>>> committing the transaction.
>>>
>>> Signed-off-by: Josef Bacik <josef@toxicpanda.com>
>>> ---
>>>  fs/btrfs/space-info.c | 34 +++++++++++++++++++++++++++-------
>>>  1 file changed, 27 insertions(+), 7 deletions(-)
>>>
>>> diff --git a/fs/btrfs/space-info.c b/fs/btrfs/space-info.c
>>> index 8a1c7ada67cb..bd485be783b8 100644
>>> --- a/fs/btrfs/space-info.c
>>> +++ b/fs/btrfs/space-info.c
>>> @@ -676,19 +676,39 @@ static inline int need_do_async_reclaim(struct btrfs_fs_info *fs_info,
>>>  		!test_bit(BTRFS_FS_STATE_REMOUNTING, &fs_info->fs_state));
>>>  }
>>>  
>>> -static bool wake_all_tickets(struct list_head *head)
>>> +static bool wake_all_tickets(struct btrfs_fs_info *fs_info,
>>> +			     struct btrfs_space_info *space_info)
>>>  {
>>>  	struct reserve_ticket *ticket;
>>> +	u64 tickets_id = space_info->tickets_id;
>>> +	u64 first_ticket_bytes = 0;
>>> +
>>> +	while (!list_empty(&space_info->tickets) &&
>>> +	       tickets_id == space_info->tickets_id) {
>>> +		ticket = list_first_entry(&space_info->tickets,
>>> +					  struct reserve_ticket, list);
>>> +
>>> +		/*
>>> +		 * may_commit_transaction will avoid committing the transaction
>>> +		 * if it doesn't feel like the space reclaimed by the commit
>>> +		 * would result in the ticket succeeding.  However if we have a
>>> +		 * smaller ticket in the queue it may be small enough to be
>>> +		 * satisified by committing the transaction, so if any
>>> +		 * subsequent ticket is smaller than the first ticket go ahead
>>> +		 * and send us back for another loop through the enospc flushing
>>> +		 * code.
>>> +		 */
>>> +		if (first_ticket_bytes == 0)
>>> +			first_ticket_bytes = ticket->bytes;
>>> +		else if (first_ticket_bytes > ticket->bytes)
>>> +			return true;
>>>  
>>> -	while (!list_empty(head)) {
>>> -		ticket = list_first_entry(head, struct reserve_ticket, list);
>>>  		list_del_init(&ticket->list);
>>>  		ticket->error = -ENOSPC;
>>>  		wake_up(&ticket->wait);
>>> -		if (ticket->bytes != ticket->orig_bytes)
>>> -			return true;
>>> +		btrfs_try_to_wakeup_tickets(fs_info, space_info);
>>
>> So the change in this logic is directly related to the implementation of
>> btrfs_try_to_wakeup_tickets. Because when we fail and remove a ticket in
>> this function we give a chance that the next ticket *could* be
>> satisfied. But how well does that work in practice, given you fail
>> normal prio tickets here, whereas btrfs_try_to_wakeup_tickets first
>> checks the prio ticket. So even if you are failing normal ticket but
>> there is one unsatifiable prio ticket that won't really change anything.
> 
> In practice we don't get to this state with high priority tickets on the list.
> Anything that would be long-ish term on the priority list is evict, and we wait
> for iput()'s in the normal flushing code.  At the point we hit wake_all_tickets
> we generally should only have tickets on the normal list.

Be that as it may, I think this assumption needs to be codified via an
assert or WARN_ON.

> 
> I suppose we could possibly get into this situation, but again the high priority
> tickets are going to be evict, truncate block, and relocate, which all have
> significantly lower reservation amounts than things like create or unlink.  If
> those things are unable to get reservations then we are truly out of space.
> Thanks,
> 
> Josef
> 

Re: [PATCH 7/8] btrfs: fix may_commit_transaction to deal with no partial filling

[Nikolay Borisov]

On 16.08.19 г. 17:19 ч., Josef Bacik wrote:
> Now that we aren't partially filling tickets we may have some slack
> space left in the space_info.  We need to account for this in
> may_commit_transaction, otherwise we may choose to not commit the
> transaction despite it actually having enough space to satisfy our
> ticket.
> 
> Calculate the free space we have in the space_info, if any.  Then check
> to see if its >= the amount of bytes_needed after we've accounted for
> the space being used by the delayed refs rsv.  If it's not subtract it
> from the bytes_needed before doing the final pinned check.  If we still
> don't have enough space then we are truly out of space.
> 
> Signed-off-by: Josef Bacik <josef@toxicpanda.com>
> ---
>  fs/btrfs/space-info.c | 19 +++++++++++++++++++
>  1 file changed, 19 insertions(+)
> 
> diff --git a/fs/btrfs/space-info.c b/fs/btrfs/space-info.c
> index bd485be783b8..f79afdc04925 100644
> --- a/fs/btrfs/space-info.c
> +++ b/fs/btrfs/space-info.c
> @@ -471,12 +471,19 @@ static int may_commit_transaction(struct btrfs_fs_info *fs_info,
>  	struct btrfs_trans_handle *trans;
>  	u64 bytes_needed;
>  	u64 reclaim_bytes = 0;
> +	u64 cur_free_bytes = 0;
>  
>  	trans = (struct btrfs_trans_handle *)current->journal_info;
>  	if (trans)
>  		return -EAGAIN;
>  
>  	spin_lock(&space_info->lock);
> +	cur_free_bytes = btrfs_space_info_used(space_info, true);
> +	if (cur_free_bytes < space_info->total_bytes)
> +		cur_free_bytes = space_info->total_bytes - cur_free_bytes;
> +	else
> +		cur_free_bytes = 0;
> +

Why don't you subtract  cur_free_bytes from bytes_needed right here,
giving you more chance to hit the:

if (reclaim_bytes >= bytes_needed)
                goto commit;

>  	if (!list_empty(&space_info->priority_tickets))
>  		ticket = list_first_entry(&space_info->priority_tickets,
>  					  struct reserve_ticket, list);
> @@ -522,6 +529,18 @@ static int may_commit_transaction(struct btrfs_fs_info *fs_info,
>  		goto commit;
>  	bytes_needed -= reclaim_bytes;
>  
> +	/*
> +	 * We don't partially fill tickets anymore, so we could have some free
> +	 * bytes in the space_info already, just not enough to satisfy the
> +	 * ticket.  If bytes_needed is already below cur_free_bytes after taking
> +	 * away the delayed refs and delayed rsv's then we can commit.
> +	 * Otherwise subtract our cur_free_bytes from bytes_needed before we
> +	 * check pinned.
> +	 */
> +	if (bytes_needed <= cur_free_bytes)
> +		goto commit;

If you do that you won't need that check because you have already
accounted for the current free space.

> +	bytes_needed -= cur_free_bytes;

And this line will be gone as well.

> +
>  	if (__percpu_counter_compare(&space_info->total_bytes_pinned,
>  				   bytes_needed,
>  				   BTRFS_TOTAL_BYTES_PINNED_BATCH) < 0)
> 

Re: [PATCH 8/8] btrfs: remove orig_bytes from reserve_ticket

[Nikolay Borisov]

On 16.08.19 г. 17:19 ч., Josef Bacik wrote:
> Now that we do not do partial filling of tickets simply remove
> orig_bytes, it is no longer needed.
> 
> Signed-off-by: Josef Bacik <josef@toxicpanda.com>

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

nit: I think this patch should follow directly after 'btrfs: rework
wake_all_tickets' or it could even be squashed in it. But let's see how
David will prefer it.

> ---
>  fs/btrfs/space-info.c | 15 ---------------
>  fs/btrfs/space-info.h |  1 -
>  2 files changed, 16 deletions(-)
> 
> diff --git a/fs/btrfs/space-info.c b/fs/btrfs/space-info.c
> index f79afdc04925..c8867a0d9f5a 100644
> --- a/fs/btrfs/space-info.c
> +++ b/fs/btrfs/space-info.c
> @@ -856,7 +856,6 @@ static int wait_reserve_ticket(struct btrfs_fs_info *fs_info,
>  
>  {
>  	DEFINE_WAIT(wait);
> -	u64 reclaim_bytes = 0;
>  	int ret = 0;
>  
>  	spin_lock(&space_info->lock);
> @@ -877,13 +876,7 @@ static int wait_reserve_ticket(struct btrfs_fs_info *fs_info,
>  		ret = ticket->error;
>  	if (!list_empty(&ticket->list))
>  		list_del_init(&ticket->list);
> -	if (ticket->bytes && ticket->bytes < ticket->orig_bytes)
> -		reclaim_bytes = ticket->orig_bytes - ticket->bytes;
>  	spin_unlock(&space_info->lock);
> -
> -	if (reclaim_bytes)
> -		btrfs_space_info_add_old_bytes(fs_info, space_info,
> -					       reclaim_bytes);
>  	return ret;
>  }
>  
> @@ -909,7 +902,6 @@ static int __reserve_metadata_bytes(struct btrfs_fs_info *fs_info,
>  {
>  	struct reserve_ticket ticket;
>  	u64 used;
> -	u64 reclaim_bytes = 0;
>  	int ret = 0;
>  	bool pending_tickets;
>  
> @@ -943,7 +935,6 @@ static int __reserve_metadata_bytes(struct btrfs_fs_info *fs_info,
>  	 * the list and we will do our own flushing further down.
>  	 */
>  	if (ret && flush != BTRFS_RESERVE_NO_FLUSH) {
> -		ticket.orig_bytes = orig_bytes;
>  		ticket.bytes = orig_bytes;
>  		ticket.error = 0;
>  		init_waitqueue_head(&ticket.wait);
> @@ -990,16 +981,10 @@ static int __reserve_metadata_bytes(struct btrfs_fs_info *fs_info,
>  	priority_reclaim_metadata_space(fs_info, space_info, &ticket);
>  	spin_lock(&space_info->lock);
>  	if (ticket.bytes) {
> -		if (ticket.bytes < orig_bytes)
> -			reclaim_bytes = orig_bytes - ticket.bytes;
>  		list_del_init(&ticket.list);
>  		ret = -ENOSPC;
>  	}
>  	spin_unlock(&space_info->lock);
> -
> -	if (reclaim_bytes)
> -		btrfs_space_info_add_old_bytes(fs_info, space_info,
> -					       reclaim_bytes);
>  	ASSERT(list_empty(&ticket.list));
>  	return ret;
>  }
> diff --git a/fs/btrfs/space-info.h b/fs/btrfs/space-info.h
> index 9ae5cae52fde..ebc5b407a954 100644
> --- a/fs/btrfs/space-info.h
> +++ b/fs/btrfs/space-info.h
> @@ -70,7 +70,6 @@ struct btrfs_space_info {
>  };
>  
>  struct reserve_ticket {
> -	u64 orig_bytes;
>  	u64 bytes;
>  	int error;
>  	struct list_head list;
> 

Re: [PATCH 8/8] btrfs: remove orig_bytes from reserve_ticket

[David Sterba]

On Tue, Aug 20, 2019 at 11:28:54AM +0300, Nikolay Borisov wrote:
> 
> 
> On 16.08.19 г. 17:19 ч., Josef Bacik wrote:
> > Now that we do not do partial filling of tickets simply remove
> > orig_bytes, it is no longer needed.
> > 
> > Signed-off-by: Josef Bacik <josef@toxicpanda.com>
> 
> Reviewed-by: Nikolay Borisov <nborisov@suse.com>
> 
> nit: I think this patch should follow directly after 'btrfs: rework
> wake_all_tickets' or it could even be squashed in it. But let's see how
> David will prefer it.

The patch does not apply on current misc-next + the ticket series, but
looking at the patches as they are now, the separation seems ok. 8
depends on 7 so the unused code cleanup is not in the fix.