[PATCH 0/5] btrfs: some fixes around unused block deletion

[PATCH 0/5] btrfs: some fixes around unused block deletion

[fdmanana]

From: Filipe Manana <fdmanana@suse.com>

These fix a couple issues regarding block group deletion, either deleting
an unused block group when it shouldn't be deleted due to outstanding
reservations relying on the block group, or unused block groups never
getting deleted since they were created due to pessimistic space
reservation and ended up never being used. More details on the change
logs of each patch.

Filipe Manana (5):
  btrfs: add and use helper to check if block group is used
  btrfs: do not delete unused block group if it may be used soon
  btrfs: add new unused block groups to the list of unused block groups
  btrfs: document what the spinlock unused_bgs_lock protects
  btrfs: add comment about list_is_singular() use at btrfs_delete_unused_bgs()

 fs/btrfs/block-group.c | 87 +++++++++++++++++++++++++++++++++++++++++-
 fs/btrfs/block-group.h |  7 ++++
 fs/btrfs/fs.h          |  3 ++
 3 files changed, 95 insertions(+), 2 deletions(-)

-- 
2.40.1

[PATCH 1/5] btrfs: add and use helper to check if block group is used

[fdmanana]

From: Filipe Manana <fdmanana@suse.com>

Add a helper function to determine if a block group is being used and make
use of it at btrfs_delete_unused_bgs(). This helper will also be used in
future code changes.

Signed-off-by: Filipe Manana <fdmanana@suse.com>
---
 fs/btrfs/block-group.c | 3 +--
 fs/btrfs/block-group.h | 7 +++++++
 2 files changed, 8 insertions(+), 2 deletions(-)

diff --git a/fs/btrfs/block-group.c b/fs/btrfs/block-group.c
index a9be9ac99222..9daef18bcbbc 100644
--- a/fs/btrfs/block-group.c
+++ b/fs/btrfs/block-group.c
@@ -1512,8 +1512,7 @@ void btrfs_delete_unused_bgs(struct btrfs_fs_info *fs_info)
 		}
 
 		spin_lock(&block_group->lock);
-		if (block_group->reserved || block_group->pinned ||
-		    block_group->used || block_group->ro ||
+		if (btrfs_is_block_group_used(block_group) || block_group->ro ||
 		    list_is_singular(&block_group->list)) {
 			/*
 			 * We want to bail if we made new allocations or have
diff --git a/fs/btrfs/block-group.h b/fs/btrfs/block-group.h
index c4a1f01cc1c2..962b11983901 100644
--- a/fs/btrfs/block-group.h
+++ b/fs/btrfs/block-group.h
@@ -257,6 +257,13 @@ static inline u64 btrfs_block_group_end(struct btrfs_block_group *block_group)
 	return (block_group->start + block_group->length);
 }
 
+static inline bool btrfs_is_block_group_used(const struct btrfs_block_group *bg)
+{
+	lockdep_assert_held(&bg->lock);
+
+	return (bg->used > 0 || bg->reserved > 0 || bg->pinned > 0);
+}
+
 static inline bool btrfs_is_block_group_data_only(
 					struct btrfs_block_group *block_group)
 {
-- 
2.40.1

[PATCH 2/5] btrfs: do not delete unused block group if it may be used soon

[fdmanana]

From: Filipe Manana <fdmanana@suse.com>

Before deleting a block group that is in the list of unused block groups
(fs_info->unused_bgs), we check if the block group became used before
deleting it, as extents from it may have been allocated after it was added
to the list.

However even if the block group was not yet used, there may be tasks that
have only reserved space and have not yet allocated extents, and they
might be relying on the availability of the unused block group in order
to allocate extents. The reservation works first by increasing the
"bytes_may_use" field of the corresponding space_info object (which may
first require flushing delayed items, allocating a new block group, etc),
and only later a task does the actual allocation of extents.

For metadata we usually don't end up using all reserved space, as we are
pessimistic and typically account for the worst cases (need to COW every
single node in a path of a tree at maximum possible height, etc). For
data we usually reserve the exact amount of space we're going to allocate
later, except when using compression where we always reserve space based
on the uncompressed size, as compression is only triggered when writeback
starts so we don't know in advance how much space we'll actually need, or
if the data is compressible.

So don't delete an unused block group if the total size of its space_info
object minus the block group's size is less then the sum of used space and
space that may be used (space_info->bytes_may_use), as that means we have
tasks that reserved space and may need to allocate extents from the block
group. In this case, besides skipping the deletion, re-add the block group
to the list of unused block groups so that it may be reconsidered later,
in case the tasks that reserved space end up not needing to allocate
extents from it.

Allowing the deletion of the block group while we have reserved space, can
result in tasks failing to allocate metadata extents (-ENOSPC) while under
a transaction handle, resulting in a transaction abort, or failure during
writeback for the case of data extents.

CC: stable@vger.kernel.org # 6.0+
Signed-off-by: Filipe Manana <fdmanana@suse.com>
---
 fs/btrfs/block-group.c | 46 ++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 46 insertions(+)

diff --git a/fs/btrfs/block-group.c b/fs/btrfs/block-group.c
index 9daef18bcbbc..5fe37bc82f11 100644
--- a/fs/btrfs/block-group.c
+++ b/fs/btrfs/block-group.c
@@ -1455,6 +1455,7 @@ static bool clean_pinned_extents(struct btrfs_trans_handle *trans,
  */
 void btrfs_delete_unused_bgs(struct btrfs_fs_info *fs_info)
 {
+	LIST_HEAD(retry_list);
 	struct btrfs_block_group *block_group;
 	struct btrfs_space_info *space_info;
 	struct btrfs_trans_handle *trans;
@@ -1476,6 +1477,7 @@ void btrfs_delete_unused_bgs(struct btrfs_fs_info *fs_info)
 
 	spin_lock(&fs_info->unused_bgs_lock);
 	while (!list_empty(&fs_info->unused_bgs)) {
+		u64 used;
 		int trimming;
 
 		block_group = list_first_entry(&fs_info->unused_bgs,
@@ -1511,6 +1513,7 @@ void btrfs_delete_unused_bgs(struct btrfs_fs_info *fs_info)
 			goto next;
 		}
 
+		spin_lock(&space_info->lock);
 		spin_lock(&block_group->lock);
 		if (btrfs_is_block_group_used(block_group) || block_group->ro ||
 		    list_is_singular(&block_group->list)) {
@@ -1522,10 +1525,49 @@ void btrfs_delete_unused_bgs(struct btrfs_fs_info *fs_info)
 			 */
 			trace_btrfs_skip_unused_block_group(block_group);
 			spin_unlock(&block_group->lock);
+			spin_unlock(&space_info->lock);
+			up_write(&space_info->groups_sem);
+			goto next;
+		}
+
+		/*
+		 * The block group may be unused but there may be space reserved
+		 * accounting with the existence of that block group, that is,
+		 * space_info->bytes_may_use was incremented by a task but no
+		 * space was yet allocated from the block group by the task.
+		 * That space may or may not be allocated, as we are generally
+		 * pessimistic about space reservation for metadata as well as
+		 * for data when using compression (as we reserve space based on
+		 * the worst case, when data can't be compressed, and before
+		 * actually attempting compression, before starting writeback).
+		 *
+		 * So check if the total space of the space_info minus the size
+		 * of this block group is less than the used space of the
+		 * space_info - if that's the case, then it means we have tasks
+		 * that might be relying on the block group in order to allocate
+		 * extents, and add back the block group to the unused list when
+		 * we finish, so that we retry later in case no tasks ended up
+		 * needing to allocate extents from the block group.
+		 */
+		used = btrfs_space_info_used(space_info, true);
+		if (space_info->total_bytes - block_group->length < used) {
+			/*
+			 * Add a reference for the list, compensate for the ref
+			 * drop under the "next" label for the
+			 * fs_info->unused_bgs list.
+			 */
+			btrfs_get_block_group(block_group);
+			list_add_tail(&block_group->bg_list, &retry_list);
+
+			trace_btrfs_skip_unused_block_group(block_group);
+			spin_unlock(&block_group->lock);
+			spin_unlock(&space_info->lock);
 			up_write(&space_info->groups_sem);
 			goto next;
 		}
+
 		spin_unlock(&block_group->lock);
+		spin_unlock(&space_info->lock);
 
 		/* We don't want to force the issue, only flip if it's ok. */
 		ret = inc_block_group_ro(block_group, 0);
@@ -1649,12 +1691,16 @@ void btrfs_delete_unused_bgs(struct btrfs_fs_info *fs_info)
 		btrfs_put_block_group(block_group);
 		spin_lock(&fs_info->unused_bgs_lock);
 	}
+	list_splice_tail(&retry_list, &fs_info->unused_bgs);
 	spin_unlock(&fs_info->unused_bgs_lock);
 	mutex_unlock(&fs_info->reclaim_bgs_lock);
 	return;
 
 flip_async:
 	btrfs_end_transaction(trans);
+	spin_lock(&fs_info->unused_bgs_lock);
+	list_splice_tail(&retry_list, &fs_info->unused_bgs);
+	spin_unlock(&fs_info->unused_bgs_lock);
 	mutex_unlock(&fs_info->reclaim_bgs_lock);
 	btrfs_put_block_group(block_group);
 	btrfs_discard_punt_unused_bgs_list(fs_info);
-- 
2.40.1

[PATCH 3/5] btrfs: add new unused block groups to the list of unused block groups

[fdmanana]

From: Filipe Manana <fdmanana@suse.com>

Space reservations for metadata are, most of the time, pessimistic as we
reserve space for worst possible cases - where tree heights are at the
maximum possible height (8), we need to COW every extent buffer in a tree
path, need to split extent buffers, etc.

For data, we generally reserve the exact amount of space we are going to
allocate. The exception here is when using compression, in which case we
reserve space matching the uncompressed size, as the compression only
happens at writeback time and in the worst possible case we need that
amount of space in case the data is not compressible.

This means that when there's not available space in the corresponding
space_info object, we may need to allocate a new block group, and then
that block group might not be used after all. In this case the block
group is never added to the list of unused block groups and ends up
never being deleted - except if we unmount and mount again the fs, as
when reading block groups from disk we add unused ones to the list of
unused block groups (fs_info->unused_bgs). Otherwise a block group is
only added to the list of unused block groups when we deallocate the
last extent from it, so if no extent is ever allocated, the block group
is kept around forever.

This also means that if we have a bunch of tasks reserving space in
parallel we can end up allocating many block groups that end up never
being used or kept around for too long without being used, which has
the potential to result in ENOSPC failures in case for example we over
allocate too many metadata block groups and then end up in a state
without enough unallocated space to allocate a new data block group.

This is more likely to happen with metadata reservations as of kernel
6.7, namely since commit 28270e25c69a ("btrfs: always reserve space for
delayed refs when starting transaction"), because we started to always
reserve space for delayed references when starting a transaction handle
for a non-zero number of items, and also to try to reserve space to fill
the gap between the delayed block reserve's reserved space and its size.

So to avoid this, when finishing the creation a new block group, add the
block group to the list of unused block groups if it's still unused at
that time. This way the next time the cleaner kthread runs, it will delete
the block group if it's still unused and not needed to satisfy existing
space reservations.

CC: stable@vger.kernel.org # 6.7+
Reported-by: Ivan Shapovalov <intelfx@intelfx.name>
Link: https://lore.kernel.org/linux-btrfs/9cdbf0ca9cdda1b4c84e15e548af7d7f9f926382.camel@intelfx.name/
Signed-off-by: Filipe Manana <fdmanana@suse.com>
---
 fs/btrfs/block-group.c | 31 +++++++++++++++++++++++++++++++
 1 file changed, 31 insertions(+)

diff --git a/fs/btrfs/block-group.c b/fs/btrfs/block-group.c
index 5fe37bc82f11..378d9103a207 100644
--- a/fs/btrfs/block-group.c
+++ b/fs/btrfs/block-group.c
@@ -2729,6 +2729,37 @@ void btrfs_create_pending_block_groups(struct btrfs_trans_handle *trans)
 		btrfs_dec_delayed_refs_rsv_bg_inserts(fs_info);
 		list_del_init(&block_group->bg_list);
 		clear_bit(BLOCK_GROUP_FLAG_NEW, &block_group->runtime_flags);
+
+		/*
+		 * If the block group is still unused, add it to the list of
+		 * unused block groups. The block group may have been created in
+		 * order to satisfy a space reservation, in which case the
+		 * extent allocation only happens later. But often we don't
+		 * actually need to allocate space that we previously reserved,
+		 * so the block group may become unused for a long time. For
+		 * example for metadata we generally reserve space for a worst
+		 * possible scenario, but then don't end up allocating all that
+		 * space or none at all (due to no need to COW, extent buffers
+		 * were already COWed in the current transaction and still
+		 * unwritten, tree heights lower than the maximum possible
+		 * height, etc). For data we generally reserve the axact amount
+		 * of space we are going to allocate later, the exception is
+		 * when using compression, as we must reserve space based on the
+		 * uncompressed data size, because the compression is only done
+		 * when writeback triggered and we don't know how much space we
+		 * are actually going to need, so we reserve the uncompressed
+		 * size because the data may be uncompressible in the worst case.
+		 */
+		if (ret == 0) {
+			bool used;
+
+			spin_lock(&block_group->lock);
+			used = btrfs_is_block_group_used(block_group);
+			spin_unlock(&block_group->lock);
+
+			if (!used)
+				btrfs_mark_bg_unused(block_group);
+		}
 	}
 	btrfs_trans_release_chunk_metadata(trans);
 }
-- 
2.40.1

[PATCH 4/5] btrfs: document what the spinlock unused_bgs_lock protects

[fdmanana]

From: Filipe Manana <fdmanana@suse.com>

Add some comments to struct btrfs_fs_info to explicitly document which
members are protected by the spinlock unused_bgs_lock. It is currently
used to protect two linked lists, the reclaim_bgs and unused_bgs lists.

So add an explicit comment on top of each list to mention its protected
by unused_bgs_lock, as well as comment on top of unused_bgs_lock to
mention the lists it protects.

Signed-off-by: Filipe Manana <fdmanana@suse.com>
---
 fs/btrfs/fs.h | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/fs/btrfs/fs.h b/fs/btrfs/fs.h
index f8bb73d6ab68..b231a90e42cf 100644
--- a/fs/btrfs/fs.h
+++ b/fs/btrfs/fs.h
@@ -732,10 +732,13 @@ struct btrfs_fs_info {
 
 	/* Reclaim partially filled block groups in the background */
 	struct work_struct reclaim_bgs_work;
+	/* Protected by unused_bgs_lock. */
 	struct list_head reclaim_bgs;
 	int bg_reclaim_threshold;
 
+	/* Protects the lists unused_bgs and reclaim_bgs. */
 	spinlock_t unused_bgs_lock;
+	/* Protected by unused_bgs_lock. */
 	struct list_head unused_bgs;
 	struct mutex unused_bg_unpin_mutex;
 	/* Protect block groups that are going to be deleted */
-- 
2.40.1

[PATCH 5/5] btrfs: add comment about list_is_singular() use at btrfs_delete_unused_bgs()

[fdmanana]

From: Filipe Manana <fdmanana@suse.com>

At btrfs_delete_unused_bgs(), the use of the list_is_singular() check on
a block group may not be immediately obvious. It is there to prevent
losing raid profile information for a block group type (data, metadata or
system), as that information is removed from
fs_info->avail_[data|metadata|system]_alloc_bits when the last block group
of a given type is deleted. So deleting the block group would later result
in creating block groups of that type with a single profile (because
fs_info->avail_*_alloc_bits would have a value of 0).

This check was added in commit aefbe9a633b5 ("btrfs: Fix lost-data-profile
caused by auto removing bg").

So add a comment mentioning the need for the check.

Signed-off-by: Filipe Manana <fdmanana@suse.com>
---
 fs/btrfs/block-group.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/fs/btrfs/block-group.c b/fs/btrfs/block-group.c
index 378d9103a207..2dc39e8db995 100644
--- a/fs/btrfs/block-group.c
+++ b/fs/btrfs/block-group.c
@@ -1522,6 +1522,13 @@ void btrfs_delete_unused_bgs(struct btrfs_fs_info *fs_info)
 			 * outstanding allocations in this block group.  We do
 			 * the ro check in case balance is currently acting on
 			 * this block group.
+			 *
+			 * Also bail out if this is the only block group for its
+			 * type, because otherwise we would lose profile
+			 * information from fs_info->avail_*_alloc_bits and the
+			 * next block group of this type would be created with a
+			 * "single" profile (even if we're in a raid fs) because
+			 * fs_info->avail_*_alloc_bits would be 0.
 			 */
 			trace_btrfs_skip_unused_block_group(block_group);
 			spin_unlock(&block_group->lock);
-- 
2.40.1

Re: [PATCH 0/5] btrfs: some fixes around unused block deletion

[Johannes Thumshirn]

On 25.01.24 11:28, fdmanana@kernel.org wrote:
> From: Filipe Manana <fdmanana@suse.com>
> 
> These fix a couple issues regarding block group deletion, either deleting
> an unused block group when it shouldn't be deleted due to outstanding
> reservations relying on the block group, or unused block groups never
> getting deleted since they were created due to pessimistic space
> reservation and ended up never being used. More details on the change
> logs of each patch.
> 
> Filipe Manana (5):
>    btrfs: add and use helper to check if block group is used
>    btrfs: do not delete unused block group if it may be used soon
>    btrfs: add new unused block groups to the list of unused block groups
>    btrfs: document what the spinlock unused_bgs_lock protects
>    btrfs: add comment about list_is_singular() use at btrfs_delete_unused_bgs()
> 
>   fs/btrfs/block-group.c | 87 +++++++++++++++++++++++++++++++++++++++++-
>   fs/btrfs/block-group.h |  7 ++++
>   fs/btrfs/fs.h          |  3 ++
>   3 files changed, 95 insertions(+), 2 deletions(-)
> 

Looks good to me,
Reviewed-by: Johannes Thumshirn <johannes.thumshirn@wdc.com>

Re: [PATCH 0/5] btrfs: some fixes around unused block deletion

[Josef Bacik]

On Thu, Jan 25, 2024 at 10:26:23AM +0000, fdmanana@kernel.org wrote:
> From: Filipe Manana <fdmanana@suse.com>
> 
> These fix a couple issues regarding block group deletion, either deleting
> an unused block group when it shouldn't be deleted due to outstanding
> reservations relying on the block group, or unused block groups never
> getting deleted since they were created due to pessimistic space
> reservation and ended up never being used. More details on the change
> logs of each patch.
> 

Reviewed-by: Josef Bacik <josef@toxicpanda.com>

Thanks,

Josef

Re: [PATCH 0/5] btrfs: some fixes around unused block deletion

[Boris Burkov]

On Thu, Jan 25, 2024 at 10:26:23AM +0000, fdmanana@kernel.org wrote:
> From: Filipe Manana <fdmanana@suse.com>
> 
> These fix a couple issues regarding block group deletion, either deleting
> an unused block group when it shouldn't be deleted due to outstanding
> reservations relying on the block group, or unused block groups never
> getting deleted since they were created due to pessimistic space
> reservation and ended up never being used. More details on the change
> logs of each patch.
> 
> Filipe Manana (5):
>   btrfs: add and use helper to check if block group is used
>   btrfs: do not delete unused block group if it may be used soon
>   btrfs: add new unused block groups to the list of unused block groups
>   btrfs: document what the spinlock unused_bgs_lock protects
>   btrfs: add comment about list_is_singular() use at btrfs_delete_unused_bgs()
> 
>  fs/btrfs/block-group.c | 87 +++++++++++++++++++++++++++++++++++++++++-
>  fs/btrfs/block-group.h |  7 ++++
>  fs/btrfs/fs.h          |  3 ++
>  3 files changed, 95 insertions(+), 2 deletions(-)
> 
> -- 
> 2.40.1
> 

Reviewed-by: Boris Burkov <boris@bur.io>

Re: [PATCH 0/5] btrfs: some fixes around unused block deletion

[Ivan Shapovalov]

[-- Attachment #1: Type: text/plain, Size: 1186 bytes --]

On 2024-01-25 at 10:26 +0000, fdmanana@kernel.org wrote:
> From: Filipe Manana <fdmanana@suse.com>
> 
> These fix a couple issues regarding block group deletion, either
> deleting
> an unused block group when it shouldn't be deleted due to outstanding
> reservations relying on the block group, or unused block groups never
> getting deleted since they were created due to pessimistic space
> reservation and ended up never being used. More details on the change
> logs of each patch.
> 
> Filipe Manana (5):
>   btrfs: add and use helper to check if block group is used
>   btrfs: do not delete unused block group if it may be used soon
>   btrfs: add new unused block groups to the list of unused block
> groups
>   btrfs: document what the spinlock unused_bgs_lock protects
>   btrfs: add comment about list_is_singular() use at
> btrfs_delete_unused_bgs()
> 
>  fs/btrfs/block-group.c | 87
> +++++++++++++++++++++++++++++++++++++++++-
>  fs/btrfs/block-group.h |  7 ++++
>  fs/btrfs/fs.h          |  3 ++
>  3 files changed, 95 insertions(+), 2 deletions(-)
> 

Still broken for me, unfortunately.

-- 
Ivan Shapovalov / intelfx /

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH 0/5] btrfs: some fixes around unused block deletion

[Filipe Manana]

On Sat, Jan 27, 2024 at 9:39 PM Ivan Shapovalov <intelfx@intelfx.name> wrote:
>
> On 2024-01-25 at 10:26 +0000, fdmanana@kernel.org wrote:
> > From: Filipe Manana <fdmanana@suse.com>
> >
> > These fix a couple issues regarding block group deletion, either
> > deleting
> > an unused block group when it shouldn't be deleted due to outstanding
> > reservations relying on the block group, or unused block groups never
> > getting deleted since they were created due to pessimistic space
> > reservation and ended up never being used. More details on the change
> > logs of each patch.
> >
> > Filipe Manana (5):
> >   btrfs: add and use helper to check if block group is used
> >   btrfs: do not delete unused block group if it may be used soon
> >   btrfs: add new unused block groups to the list of unused block
> > groups
> >   btrfs: document what the spinlock unused_bgs_lock protects
> >   btrfs: add comment about list_is_singular() use at
> > btrfs_delete_unused_bgs()
> >
> >  fs/btrfs/block-group.c | 87
> > +++++++++++++++++++++++++++++++++++++++++-
> >  fs/btrfs/block-group.h |  7 ++++
> >  fs/btrfs/fs.h          |  3 ++
> >  3 files changed, 95 insertions(+), 2 deletions(-)
> >
>
> Still broken for me, unfortunately.

I'm curious about your workload. Is it something like continuous,
non-stop deduplication or cloning for example?

Did you actually experience -ENOSPC errors?

Also, if you unmount and then mount again the fs, any unused block
groups should be scheduled for deletion once the cleaner thread runs,
at least if there's not a huge workload for a minute or two.

On top of this patchset, can you try the following patch?

https://pastebin.com/raw/U7b0e03g

If that still doesn't help, try the following the following patch on
top of this patchset:

https://pastebin.com/raw/rKiSmG5w

Thanks.

>
> --
> Ivan Shapovalov / intelfx /

Re: [PATCH 0/5] btrfs: some fixes around unused block deletion

[Ivan Shapovalov]

[-- Attachment #1: Type: text/plain, Size: 3104 bytes --]

On 2024-01-29 at 12:49 +0000, Filipe Manana wrote:
> On Sat, Jan 27, 2024 at 9:39 PM Ivan Shapovalov
> <intelfx@intelfx.name> wrote:
> > 
> > On 2024-01-25 at 10:26 +0000, fdmanana@kernel.org wrote:
> > > From: Filipe Manana <fdmanana@suse.com>
> > > 
> > > These fix a couple issues regarding block group deletion, either
> > > deleting
> > > an unused block group when it shouldn't be deleted due to
> > > outstanding
> > > reservations relying on the block group, or unused block groups
> > > never
> > > getting deleted since they were created due to pessimistic space
> > > reservation and ended up never being used. More details on the
> > > change
> > > logs of each patch.
> > > 
> > > Filipe Manana (5):
> > >   btrfs: add and use helper to check if block group is used
> > >   btrfs: do not delete unused block group if it may be used soon
> > >   btrfs: add new unused block groups to the list of unused block
> > > groups
> > >   btrfs: document what the spinlock unused_bgs_lock protects
> > >   btrfs: add comment about list_is_singular() use at
> > > btrfs_delete_unused_bgs()
> > > 
> > >  fs/btrfs/block-group.c | 87
> > > +++++++++++++++++++++++++++++++++++++++++-
> > >  fs/btrfs/block-group.h |  7 ++++
> > >  fs/btrfs/fs.h          |  3 ++
> > >  3 files changed, 95 insertions(+), 2 deletions(-)
> > > 
> > 
> > Still broken for me, unfortunately.
> 
> I'm curious about your workload. Is it something like continuous,
> non-stop deduplication or cloning for example?
> 
> Did you actually experience -ENOSPC errors?
> 
> Also, if you unmount and then mount again the fs, any unused block
> groups should be scheduled for deletion once the cleaner thread runs,
> at least if there's not a huge workload for a minute or two.

Apologies, I must have clarified the workload.

The easiest way to reproduce the original problem was to run a metadata
balance, e. g. `btrfs balance start -m /` or
`for u in {0..75..5}; do btrfs balance start -musage=$u /`.

I originally spotted this regression by observing an abnormally low
metadata space utilization and trying to run the above balance, only to
discover that it makes overallocation even worse.

So, with the patchset applied, the _metadata balance_ is still broken.
I cannot say anything about normal usage, but any attempt to balance
metadata immediately explodes the metadata allocation. I did not
experience a -ENOSPC per se, but the balance (that I used as a
reproducer) eventually consumed *all* unallocated space on the volume
and stopped making progress. Attempting to cancel that balance RO'ed
the filesystem and I had to reboot.

> 
> On top of this patchset, can you try the following patch?
> 
> https://pastebin.com/raw/U7b0e03g
> 
> If that still doesn't help, try the following the following patch on
> top of this patchset:
> 
> https://pastebin.com/raw/rKiSmG5w

Should I still try these patches to see if they help with metadata
balance, or is that a different problem then?

Thanks,
-- 
Ivan Shapovalov / intelfx /

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH 0/5] btrfs: some fixes around unused block deletion

[Filipe Manana]

On Mon, Jan 29, 2024 at 5:56 PM Ivan Shapovalov <intelfx@intelfx.name> wrote:
>
> On 2024-01-29 at 12:49 +0000, Filipe Manana wrote:
> > On Sat, Jan 27, 2024 at 9:39 PM Ivan Shapovalov
> > <intelfx@intelfx.name> wrote:
> > >
> > > On 2024-01-25 at 10:26 +0000, fdmanana@kernel.org wrote:
> > > > From: Filipe Manana <fdmanana@suse.com>
> > > >
> > > > These fix a couple issues regarding block group deletion, either
> > > > deleting
> > > > an unused block group when it shouldn't be deleted due to
> > > > outstanding
> > > > reservations relying on the block group, or unused block groups
> > > > never
> > > > getting deleted since they were created due to pessimistic space
> > > > reservation and ended up never being used. More details on the
> > > > change
> > > > logs of each patch.
> > > >
> > > > Filipe Manana (5):
> > > >   btrfs: add and use helper to check if block group is used
> > > >   btrfs: do not delete unused block group if it may be used soon
> > > >   btrfs: add new unused block groups to the list of unused block
> > > > groups
> > > >   btrfs: document what the spinlock unused_bgs_lock protects
> > > >   btrfs: add comment about list_is_singular() use at
> > > > btrfs_delete_unused_bgs()
> > > >
> > > >  fs/btrfs/block-group.c | 87
> > > > +++++++++++++++++++++++++++++++++++++++++-
> > > >  fs/btrfs/block-group.h |  7 ++++
> > > >  fs/btrfs/fs.h          |  3 ++
> > > >  3 files changed, 95 insertions(+), 2 deletions(-)
> > > >
> > >
> > > Still broken for me, unfortunately.
> >
> > I'm curious about your workload. Is it something like continuous,
> > non-stop deduplication or cloning for example?
> >
> > Did you actually experience -ENOSPC errors?
> >
> > Also, if you unmount and then mount again the fs, any unused block
> > groups should be scheduled for deletion once the cleaner thread runs,
> > at least if there's not a huge workload for a minute or two.
>
> Apologies, I must have clarified the workload.
>
> The easiest way to reproduce the original problem was to run a metadata
> balance, e. g. `btrfs balance start -m /` or
> `for u in {0..75..5}; do btrfs balance start -musage=$u /`.
>
> I originally spotted this regression by observing an abnormally low
> metadata space utilization and trying to run the above balance, only to
> discover that it makes overallocation even worse.
>
> So, with the patchset applied, the _metadata balance_ is still broken.
> I cannot say anything about normal usage, but any attempt to balance
> metadata immediately explodes the metadata allocation. I did not
> experience a -ENOSPC per se, but the balance (that I used as a
> reproducer) eventually consumed *all* unallocated space on the volume
> and stopped making progress. Attempting to cancel that balance RO'ed
> the filesystem and I had to reboot.

I tried that but I didn't see any problem.
Here's a sample test script:

https://pastebin.com/raw/vc70BqY5

>
> >
> > On top of this patchset, can you try the following patch?
> >
> > https://pastebin.com/raw/U7b0e03g
> >
> > If that still doesn't help, try the following the following patch on
> > top of this patchset:
> >
> > https://pastebin.com/raw/rKiSmG5w
>
> Should I still try these patches to see if they help with metadata
> balance, or is that a different problem then?

Yes. Please try them, given that I can't reproduce your issue with the balance.

I could trigger getting 1 or 2 at most unused metadata block groups
after having a bunch of tasks doing metadata operations, and patch 3/5
is to address that case.

So one thing I'm curious about, and I asked before, is that if you
unmount and mount again the fs, after a few minutes (without running
that balance), you should get space reclaimed,
as any empty block groups are scheduled for deletion at mount time,
and the next time the cleaner kthread runs, it deletes them.


>
> Thanks,
> --
> Ivan Shapovalov / intelfx /

Re: [PATCH 0/5] btrfs: some fixes around unused block deletion

[Ivan Shapovalov]

[-- Attachment #1: Type: text/plain, Size: 5060 bytes --]

On 2024-01-29 at 20:28 +0000, Filipe Manana wrote:
> On Mon, Jan 29, 2024 at 5:56 PM Ivan Shapovalov
> <intelfx@intelfx.name> wrote:
> > 
> > On 2024-01-29 at 12:49 +0000, Filipe Manana wrote:
> > > On Sat, Jan 27, 2024 at 9:39 PM Ivan Shapovalov
> > > <intelfx@intelfx.name> wrote:
> > > > 
> > > > On 2024-01-25 at 10:26 +0000, fdmanana@kernel.org wrote:
> > > > > From: Filipe Manana <fdmanana@suse.com>
> > > > > 
> > > > > These fix a couple issues regarding block group deletion,
> > > > > either
> > > > > deleting
> > > > > an unused block group when it shouldn't be deleted due to
> > > > > outstanding
> > > > > reservations relying on the block group, or unused block
> > > > > groups
> > > > > never
> > > > > getting deleted since they were created due to pessimistic
> > > > > space
> > > > > reservation and ended up never being used. More details on
> > > > > the
> > > > > change
> > > > > logs of each patch.
> > > > > 
> > > > > Filipe Manana (5):
> > > > >   btrfs: add and use helper to check if block group is used
> > > > >   btrfs: do not delete unused block group if it may be used
> > > > > soon
> > > > >   btrfs: add new unused block groups to the list of unused
> > > > > block
> > > > > groups
> > > > >   btrfs: document what the spinlock unused_bgs_lock protects
> > > > >   btrfs: add comment about list_is_singular() use at
> > > > > btrfs_delete_unused_bgs()
> > > > > 
> > > > >  fs/btrfs/block-group.c | 87
> > > > > +++++++++++++++++++++++++++++++++++++++++-
> > > > >  fs/btrfs/block-group.h |  7 ++++
> > > > >  fs/btrfs/fs.h          |  3 ++
> > > > >  3 files changed, 95 insertions(+), 2 deletions(-)
> > > > > 
> > > > 
> > > > Still broken for me, unfortunately.
> > > 
> > > I'm curious about your workload. Is it something like continuous,
> > > non-stop deduplication or cloning for example?
> > > 
> > > Did you actually experience -ENOSPC errors?
> > > 
> > > Also, if you unmount and then mount again the fs, any unused
> > > block
> > > groups should be scheduled for deletion once the cleaner thread
> > > runs,
> > > at least if there's not a huge workload for a minute or two.
> > 
> > Apologies, I must have clarified the workload.
> > 
> > The easiest way to reproduce the original problem was to run a
> > metadata
> > balance, e. g. `btrfs balance start -m /` or
> > `for u in {0..75..5}; do btrfs balance start -musage=$u /`.
> > 
> > I originally spotted this regression by observing an abnormally low
> > metadata space utilization and trying to run the above balance,
> > only to
> > discover that it makes overallocation even worse.
> > 
> > So, with the patchset applied, the _metadata balance_ is still
> > broken.
> > I cannot say anything about normal usage, but any attempt to
> > balance
> > metadata immediately explodes the metadata allocation. I did not
> > experience a -ENOSPC per se, but the balance (that I used as a
> > reproducer) eventually consumed *all* unallocated space on the
> > volume
> > and stopped making progress. Attempting to cancel that balance
> > RO'ed
> > the filesystem and I had to reboot.
> 
> I tried that but I didn't see any problem.
> Here's a sample test script:
> 
> https://pastebin.com/raw/vc70BqY5
> 
> > 
> > > 
> > > On top of this patchset, can you try the following patch?
> > > 
> > > https://pastebin.com/raw/U7b0e03g
> > > 
> > > If that still doesn't help, try the following the following patch
> > > on
> > > top of this patchset:
> > > 
> > > https://pastebin.com/raw/rKiSmG5w
> > 
> > Should I still try these patches to see if they help with metadata
> > balance, or is that a different problem then?
> 
> Yes. Please try them, given that I can't reproduce your issue with
> the balance.

Hi,

The second pastebinned patch (applied on top of the patchset and the
first pastebinned patch) passes my balance smoke test.

As for reproducing the issue: it's a long shot, but could you try on a
4K-native block device (i. e. where `blockdev --getpbsz` tells `4096`)?

> 
> I could trigger getting 1 or 2 at most unused metadata block groups
> after having a bunch of tasks doing metadata operations, and patch
> 3/5
> is to address that case.
> 
> So one thing I'm curious about, and I asked before, is that if you
> unmount and mount again the fs, after a few minutes (without running
> that balance), you should get space reclaimed,
> as any empty block groups are scheduled for deletion at mount time,
> and the next time the cleaner kthread runs, it deletes them.

Yes, that indeed does happen. Cancelling the balance and letting the
system idle for a while reclaims all (or almost all) overallocated
space. However, that only happens _after_ I cancel the balance. If I do
not cancel it, it ends up consuming all unallocated space on the volume
and ceases to make progress. So, basically, at least the balance
operation becomes completely unusable.

-- 
Ivan Shapovalov / intelfx /

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH 0/5] btrfs: some fixes around unused block deletion

[Filipe Manana]

On Fri, Feb 2, 2024 at 12:52 AM Ivan Shapovalov <intelfx@intelfx.name> wrote:
>
> On 2024-01-29 at 20:28 +0000, Filipe Manana wrote:
> > On Mon, Jan 29, 2024 at 5:56 PM Ivan Shapovalov
> > <intelfx@intelfx.name> wrote:
> > >
> > > On 2024-01-29 at 12:49 +0000, Filipe Manana wrote:
> > > > On Sat, Jan 27, 2024 at 9:39 PM Ivan Shapovalov
> > > > <intelfx@intelfx.name> wrote:
> > > > >
> > > > > On 2024-01-25 at 10:26 +0000, fdmanana@kernel.org wrote:
> > > > > > From: Filipe Manana <fdmanana@suse.com>
> > > > > >
> > > > > > These fix a couple issues regarding block group deletion,
> > > > > > either
> > > > > > deleting
> > > > > > an unused block group when it shouldn't be deleted due to
> > > > > > outstanding
> > > > > > reservations relying on the block group, or unused block
> > > > > > groups
> > > > > > never
> > > > > > getting deleted since they were created due to pessimistic
> > > > > > space
> > > > > > reservation and ended up never being used. More details on
> > > > > > the
> > > > > > change
> > > > > > logs of each patch.
> > > > > >
> > > > > > Filipe Manana (5):
> > > > > >   btrfs: add and use helper to check if block group is used
> > > > > >   btrfs: do not delete unused block group if it may be used
> > > > > > soon
> > > > > >   btrfs: add new unused block groups to the list of unused
> > > > > > block
> > > > > > groups
> > > > > >   btrfs: document what the spinlock unused_bgs_lock protects
> > > > > >   btrfs: add comment about list_is_singular() use at
> > > > > > btrfs_delete_unused_bgs()
> > > > > >
> > > > > >  fs/btrfs/block-group.c | 87
> > > > > > +++++++++++++++++++++++++++++++++++++++++-
> > > > > >  fs/btrfs/block-group.h |  7 ++++
> > > > > >  fs/btrfs/fs.h          |  3 ++
> > > > > >  3 files changed, 95 insertions(+), 2 deletions(-)
> > > > > >
> > > > >
> > > > > Still broken for me, unfortunately.
> > > >
> > > > I'm curious about your workload. Is it something like continuous,
> > > > non-stop deduplication or cloning for example?
> > > >
> > > > Did you actually experience -ENOSPC errors?
> > > >
> > > > Also, if you unmount and then mount again the fs, any unused
> > > > block
> > > > groups should be scheduled for deletion once the cleaner thread
> > > > runs,
> > > > at least if there's not a huge workload for a minute or two.
> > >
> > > Apologies, I must have clarified the workload.
> > >
> > > The easiest way to reproduce the original problem was to run a
> > > metadata
> > > balance, e. g. `btrfs balance start -m /` or
> > > `for u in {0..75..5}; do btrfs balance start -musage=$u /`.
> > >
> > > I originally spotted this regression by observing an abnormally low
> > > metadata space utilization and trying to run the above balance,
> > > only to
> > > discover that it makes overallocation even worse.
> > >
> > > So, with the patchset applied, the _metadata balance_ is still
> > > broken.
> > > I cannot say anything about normal usage, but any attempt to
> > > balance
> > > metadata immediately explodes the metadata allocation. I did not
> > > experience a -ENOSPC per se, but the balance (that I used as a
> > > reproducer) eventually consumed *all* unallocated space on the
> > > volume
> > > and stopped making progress. Attempting to cancel that balance
> > > RO'ed
> > > the filesystem and I had to reboot.
> >
> > I tried that but I didn't see any problem.
> > Here's a sample test script:
> >
> > https://pastebin.com/raw/vc70BqY5
> >
> > >
> > > >
> > > > On top of this patchset, can you try the following patch?
> > > >
> > > > https://pastebin.com/raw/U7b0e03g
> > > >
> > > > If that still doesn't help, try the following the following patch
> > > > on
> > > > top of this patchset:
> > > >
> > > > https://pastebin.com/raw/rKiSmG5w
> > >
> > > Should I still try these patches to see if they help with metadata
> > > balance, or is that a different problem then?
> >
> > Yes. Please try them, given that I can't reproduce your issue with
> > the balance.
>
> Hi,
>
> The second pastebinned patch (applied on top of the patchset and the
> first pastebinned patch) passes my balance smoke test.

Ok, thanks, I've just sent it to the list a few minutes ago:

https://lore.kernel.org/linux-btrfs/eba624e8cef9a1e84c9e1ba0c8f32347aa487e63.1706892030.git.fdmanana@suse.com/

>
> As for reproducing the issue: it's a long shot, but could you try on a
> 4K-native block device (i. e. where `blockdev --getpbsz` tells `4096`)?

Well, that's irrelevant. It doesn't change anything on btrfs (node
sizes, sector sizes, etc).

>
> >
> > I could trigger getting 1 or 2 at most unused metadata block groups
> > after having a bunch of tasks doing metadata operations, and patch
> > 3/5
> > is to address that case.
> >
> > So one thing I'm curious about, and I asked before, is that if you
> > unmount and mount again the fs, after a few minutes (without running
> > that balance), you should get space reclaimed,
> > as any empty block groups are scheduled for deletion at mount time,
> > and the next time the cleaner kthread runs, it deletes them.
>
> Yes, that indeed does happen. Cancelling the balance and letting the
> system idle for a while reclaims all (or almost all) overallocated
> space. However, that only happens _after_ I cancel the balance. If I do
> not cancel it, it ends up consuming all unallocated space on the volume
> and ceases to make progress. So, basically, at least the balance
> operation becomes completely unusable.

Ok, I see. I think relocation of a block group creates another block
group when reserving
space somewhere, and that block group gets caught next for relocation,
which generates
another one, and so forth.

Thanks for the testing and report.

>
> --
> Ivan Shapovalov / intelfx /