[PATCH][v2] btrfs: free block groups after free'ing fs trees

[PATCH][v2] btrfs: free block groups after free'ing fs trees

[Josef Bacik]

Sometimes when running generic/475 we would trip the
WARN_ON(cache->reserved) check when free'ing the block groups on umount.
This is because sometimes we don't commit the transaction because of IO
errors and thus do not cleanup the tree logs until at umount time.
These blocks are still reserved until they are cleaned up, but they
aren't cleaned up until _after_ we do the free block groups work.  Fix
this by moving the free after free'ing the fs roots, that way all of the
tree logs are cleaned up and we have a properly cleaned fs.  A bunch of
loops of generic/475 confirmed this fixes the problem.

Signed-off-by: Josef Bacik <josef@toxicpanda.com>
---
v1->v2:
- Add a comment to make sure we don't re-order the block group freeing.

 fs/btrfs/disk-io.c | 11 +++++++++--
 1 file changed, 9 insertions(+), 2 deletions(-)

diff --git a/fs/btrfs/disk-io.c b/fs/btrfs/disk-io.c
index d453bdc74e91..56d0a24aec74 100644
--- a/fs/btrfs/disk-io.c
+++ b/fs/btrfs/disk-io.c
@@ -4056,12 +4056,19 @@ void __cold close_ctree(struct btrfs_fs_info *fs_info)
 	invalidate_inode_pages2(fs_info->btree_inode->i_mapping);
 	btrfs_stop_all_workers(fs_info);
 
-	btrfs_free_block_groups(fs_info);
-
 	clear_bit(BTRFS_FS_OPEN, &fs_info->flags);
 	free_root_pointers(fs_info, true);
 	btrfs_free_fs_roots(fs_info);
 
+	/*
+	 * We must free the block groups after dropping the fs_roots as we could
+	 * have had an IO error and have left over tree log blocks that aren't
+	 * cleaned up until the fs roots are freed.  This makes the block group
+	 * accounting appear to be wrong because there's pending reserved bytes,
+	 * so make sure we do the block group cleanup afterwards.
+	 */
+	btrfs_free_block_groups(fs_info);
+
 	iput(fs_info->btree_inode);
 
 #ifdef CONFIG_BTRFS_FS_CHECK_INTEGRITY
-- 
2.24.1

Re: [PATCH][v2] btrfs: free block groups after free'ing fs trees

[David Sterba]

On Tue, Jan 21, 2020 at 09:17:06AM -0500, Josef Bacik wrote:
> Sometimes when running generic/475 we would trip the
> WARN_ON(cache->reserved) check when free'ing the block groups on umount.
> This is because sometimes we don't commit the transaction because of IO
> errors and thus do not cleanup the tree logs until at umount time.
> These blocks are still reserved until they are cleaned up, but they
> aren't cleaned up until _after_ we do the free block groups work.  Fix
> this by moving the free after free'ing the fs roots, that way all of the
> tree logs are cleaned up and we have a properly cleaned fs.  A bunch of
> loops of generic/475 confirmed this fixes the problem.
> 
> Signed-off-by: Josef Bacik <josef@toxicpanda.com>
> ---
> v1->v2:
> - Add a comment to make sure we don't re-order the block group freeing.

Thanks, added to misc-next.