Re: [PATCH 4.9 083/116] btrfs: free block groups after freeing fs trees

From: Greg Kroah-Hartman
Date: Thu Feb 13 2020 - 18:00:02 EST


On Thu, Feb 13, 2020 at 09:55:33PM +0100, David Sterba wrote:
> On Thu, Feb 13, 2020 at 07:20:27AM -0800, Greg Kroah-Hartman wrote:
> > From: Josef Bacik <josef@xxxxxxxxxxxxxx>
> >
> > [ Upstream commit 4e19443da1941050b346f8fc4c368aa68413bc88 ]
> >
> > 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.
> >
> > CC: stable@xxxxxxxxxxxxxxx # 4.9+
> > Signed-off-by: Josef Bacik <josef@xxxxxxxxxxxxxx>
> > Reviewed-by: David Sterba <dsterba@xxxxxxxx>
> > Signed-off-by: David Sterba <dsterba@xxxxxxxx>
> > Signed-off-by: Sasha Levin <sashal@xxxxxxxxxx>
> > ---
> > fs/btrfs/disk-io.c | 9 +++++++++
> > 1 file changed, 9 insertions(+)
> >
> > diff --git a/fs/btrfs/disk-io.c b/fs/btrfs/disk-io.c
> > index eab5a9065f093..439b5f5dc3274 100644
> > --- a/fs/btrfs/disk-io.c
> > +++ b/fs/btrfs/disk-io.c
> > @@ -3864,6 +3864,15 @@ void close_ctree(struct btrfs_root *root)
> > clear_bit(BTRFS_FS_OPEN, &fs_info->flags);
> > free_root_pointers(fs_info, true);
> >
> > + /*
> > + * 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);
>
> Something's wrong here. The patch 4e19443da1 moves the
> btrfs_free_block_groups() call and the stable backport lacks the "-"
> line. However the patch applies cleanly on 4.9.213.
>
> 3855 btrfs_free_block_groups(fs_info);
> ^^^^
>
> 3856
> 3857 /*
> 3858 * we must make sure there is not any read request to
> 3859 * submit after we stopping all workers.
> 3860 */
> 3861 invalidate_inode_pages2(fs_info->btree_inode->i_mapping);
> 3862 btrfs_stop_all_workers(fs_info);
> 3863
> 3864 clear_bit(BTRFS_FS_OPEN, &fs_info->flags);
> 3865 free_root_pointers(fs_info, 1);
> 3866
> 3867 /*
> 3868 * We must free the block groups after dropping the fs_roots as we could
> 3869 * have had an IO error and have left over tree log blocks that aren't
> 3870 * cleaned up until the fs roots are freed. This makes the block group
> 3871 * accounting appear to be wrong because there's pending reserved bytes,
> 3872 * so make sure we do the block group cleanup afterwards.
> 3873 */
> 3874 btrfs_free_block_groups(fs_info);
>
> The first one should not be there.

Now fixed up by just dropping this patch entirely :)

Sasha, can you fix it up and add it back for the next round?

thanks,

greg k-h