[PATCH] btrfs: fix btrfs_calc_reclaim_metadata_size calculation

[PATCH] btrfs: fix btrfs_calc_reclaim_metadata_size calculation

[Josef Bacik]

I noticed while running my snapshot torture test that we were getting a
lot of metadata chunks allocated with very little actually used.
Digging into this we would commit the transaction, still not have enough
space, and then force a chunk allocation.

I noticed that we were barely flushing any delalloc at all, despite the
fact that we had around 13gib of outstanding delalloc reservations.  It
turns out this is because of our btrfs_calc_reclaim_metadata_size()
calculation.  It _only_ takes into account the outstanding ticket sizes,
which isn't the whole story.  In this particular workload we're slowly
filling up the disk, which means our overcommit space will suddenly
become a lot less, and our outstanding reservations will be well more
than what we can handle.  However we are only flushing based on our
ticket size, which is much less than we need to actually reclaim.

So fix btrfs_calc_reclaim_metadata_size() to take into account the
overage in the case that we've gotten less available space suddenly.
This makes it so we attempt to reclaim a lot more delalloc space, which
allows us to make our reservations and we no longer are allocating a
bunch of needless metadata chunks.

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

diff --git a/fs/btrfs/space-info.c b/fs/btrfs/space-info.c
index f216ab72f5fc..26e1c492b9b5 100644
--- a/fs/btrfs/space-info.c
+++ b/fs/btrfs/space-info.c
@@ -306,25 +306,20 @@ static inline u64 calc_global_rsv_need_space(struct btrfs_block_rsv *global)
 	return (global->size << 1);
 }
 
-int btrfs_can_overcommit(struct btrfs_fs_info *fs_info,
-			 struct btrfs_space_info *space_info, u64 bytes,
-			 enum btrfs_reserve_flush_enum flush)
+static inline u64
+calc_available_free_space(struct btrfs_fs_info *fs_info,
+			  struct btrfs_space_info *space_info,
+			  enum btrfs_reserve_flush_enum flush)
 {
 	u64 profile;
 	u64 avail;
-	u64 used;
 	int factor;
 
-	/* Don't overcommit when in mixed mode. */
-	if (space_info->flags & BTRFS_BLOCK_GROUP_DATA)
-		return 0;
-
 	if (space_info->flags & BTRFS_BLOCK_GROUP_SYSTEM)
 		profile = btrfs_system_alloc_profile(fs_info);
 	else
 		profile = btrfs_metadata_alloc_profile(fs_info);
 
-	used = btrfs_space_info_used(space_info, true);
 	avail = atomic64_read(&fs_info->free_chunk_space);
 
 	/*
@@ -345,6 +340,22 @@ int btrfs_can_overcommit(struct btrfs_fs_info *fs_info,
 		avail >>= 3;
 	else
 		avail >>= 1;
+	return avail;
+}
+
+int btrfs_can_overcommit(struct btrfs_fs_info *fs_info,
+			 struct btrfs_space_info *space_info, u64 bytes,
+			 enum btrfs_reserve_flush_enum flush)
+{
+	u64 avail;
+	u64 used;
+
+	/* Don't overcommit when in mixed mode. */
+	if (space_info->flags & BTRFS_BLOCK_GROUP_DATA)
+		return 0;
+
+	used = btrfs_space_info_used(space_info, true);
+	avail = calc_available_free_space(fs_info, space_info, flush);
 
 	if (used + bytes < space_info->total_bytes + avail)
 		return 1;
@@ -735,6 +746,7 @@ btrfs_calc_reclaim_metadata_size(struct btrfs_fs_info *fs_info,
 {
 	struct reserve_ticket *ticket;
 	u64 used;
+	u64 avail;
 	u64 expected;
 	u64 to_reclaim = 0;
 
@@ -742,6 +754,20 @@ btrfs_calc_reclaim_metadata_size(struct btrfs_fs_info *fs_info,
 		to_reclaim += ticket->bytes;
 	list_for_each_entry(ticket, &space_info->priority_tickets, list)
 		to_reclaim += ticket->bytes;
+
+	avail = calc_available_free_space(fs_info, space_info,
+					  BTRFS_RESERVE_FLUSH_ALL);
+	used = btrfs_space_info_used(space_info, true);
+
+	/*
+	 * We may be flushing because suddenly we have less space than we had
+	 * before, and now we're well over-committed based on our current free
+	 * space.  If that's the case add in our overage so we make sure to put
+	 * appropriate pressure on the flushing state machine.
+	 */
+	if (space_info->total_bytes + avail < used)
+		to_reclaim += used - (space_info->total_bytes + avail);
+
 	if (to_reclaim)
 		return to_reclaim;
 
-- 
2.24.1

Re: [PATCH] btrfs: fix btrfs_calc_reclaim_metadata_size calculation

[David Sterba]

On Fri, Feb 21, 2020 at 04:41:10PM -0500, Josef Bacik wrote:
> I noticed while running my snapshot torture test that we were getting a
> lot of metadata chunks allocated with very little actually used.
> Digging into this we would commit the transaction, still not have enough
> space, and then force a chunk allocation.
> 
> I noticed that we were barely flushing any delalloc at all, despite the
> fact that we had around 13gib of outstanding delalloc reservations.  It
> turns out this is because of our btrfs_calc_reclaim_metadata_size()
> calculation.  It _only_ takes into account the outstanding ticket sizes,
> which isn't the whole story.  In this particular workload we're slowly
> filling up the disk, which means our overcommit space will suddenly
> become a lot less, and our outstanding reservations will be well more
> than what we can handle.  However we are only flushing based on our
> ticket size, which is much less than we need to actually reclaim.
> 
> So fix btrfs_calc_reclaim_metadata_size() to take into account the
> overage in the case that we've gotten less available space suddenly.
> This makes it so we attempt to reclaim a lot more delalloc space, which
> allows us to make our reservations and we no longer are allocating a
> bunch of needless metadata chunks.
> 
> Signed-off-by: Josef Bacik <josef@toxicpanda.com>

Added to misc-next, thanks.

> ---
>  fs/btrfs/space-info.c | 44 ++++++++++++++++++++++++++++++++++---------
>  1 file changed, 35 insertions(+), 9 deletions(-)
> 
> diff --git a/fs/btrfs/space-info.c b/fs/btrfs/space-info.c
> index f216ab72f5fc..26e1c492b9b5 100644
> --- a/fs/btrfs/space-info.c
> +++ b/fs/btrfs/space-info.c
> @@ -306,25 +306,20 @@ static inline u64 calc_global_rsv_need_space(struct btrfs_block_rsv *global)
>  	return (global->size << 1);
>  }
>  
> -int btrfs_can_overcommit(struct btrfs_fs_info *fs_info,
> -			 struct btrfs_space_info *space_info, u64 bytes,
> -			 enum btrfs_reserve_flush_enum flush)
> +static inline u64
> +calc_available_free_space(struct btrfs_fs_info *fs_info,
> +			  struct btrfs_space_info *space_info,
> +			  enum btrfs_reserve_flush_enum flush)

It does not seem to be a candidate for 'static inline', it's not in a
header and the function body is beyond what looks suitable for inlining.

Re: [PATCH] btrfs: fix btrfs_calc_reclaim_metadata_size calculation

[David Sterba]

On Fri, Feb 21, 2020 at 04:41:10PM -0500, Josef Bacik wrote:
> I noticed while running my snapshot torture test that we were getting a
> lot of metadata chunks allocated with very little actually used.
> Digging into this we would commit the transaction, still not have enough
> space, and then force a chunk allocation.
> 
> I noticed that we were barely flushing any delalloc at all, despite the
> fact that we had around 13gib of outstanding delalloc reservations.  It
> turns out this is because of our btrfs_calc_reclaim_metadata_size()
> calculation.  It _only_ takes into account the outstanding ticket sizes,
> which isn't the whole story.  In this particular workload we're slowly
> filling up the disk, which means our overcommit space will suddenly
> become a lot less, and our outstanding reservations will be well more
> than what we can handle.  However we are only flushing based on our
> ticket size, which is much less than we need to actually reclaim.
> 
> So fix btrfs_calc_reclaim_metadata_size() to take into account the
> overage in the case that we've gotten less available space suddenly.
> This makes it so we attempt to reclaim a lot more delalloc space, which
> allows us to make our reservations and we no longer are allocating a
> bunch of needless metadata chunks.

This seems to be relevant for stable@ but does not apply due to some
cleanups that are not even on 5.5 and for the reset the code has moved
to other files so this would need manual backport.

I'll add the CC: tag so we have that tracked but none of the patches
will apply.