Re: [PATCH] btrfs: discard: reduce the block group ref when grabbing from unused block group list

[Marcos Paulo de Souza <marcos@mpdesouza.com>]

On Fri, 2020-07-03 at 15:05 +0800, Qu Wenruo wrote:
> [BUG]
> The following small test script can trigger ASSERT() at unmount time:
> 
>   mkfs.btrfs -f $dev
>   mount $dev $mnt
>   mount -o remount,discard=async $mnt
>   umount $mnt
> 
> The call trace:
>   assertion failed: atomic_read(&block_group->count) == 1, in
> fs/btrfs/block-group.c:3431
>   ------------[ cut here ]------------
>   kernel BUG at fs/btrfs/ctree.h:3204!
>   invalid opcode: 0000 [#1] PREEMPT SMP NOPTI
>   CPU: 4 PID: 10389 Comm: umount Tainted: G           O      5.8.0-
> rc3-custom+ #68
>   Hardware name: QEMU Standard PC (Q35 + ICH9, 2009), BIOS 0.0.0
> 02/06/2015
>   Call Trace:
>    btrfs_free_block_groups.cold+0x22/0x55 [btrfs]
>    close_ctree+0x2cb/0x323 [btrfs]
>    btrfs_put_super+0x15/0x17 [btrfs]
>    generic_shutdown_super+0x72/0x110
>    kill_anon_super+0x18/0x30
>    btrfs_kill_super+0x17/0x30 [btrfs]
>    deactivate_locked_super+0x3b/0xa0
>    deactivate_super+0x40/0x50
>    cleanup_mnt+0x135/0x190
>    __cleanup_mnt+0x12/0x20
>    task_work_run+0x64/0xb0
>    __prepare_exit_to_usermode+0x1bc/0x1c0
>    __syscall_return_slowpath+0x47/0x230
>    do_syscall_64+0x64/0xb0
>    entry_SYSCALL_64_after_hwframe+0x44/0xa9
> 
> The code:
>                 ASSERT(atomic_read(&block_group->count) == 1);
>                 btrfs_put_block_group(block_group);
> 
> [CAUSE]
> Obviously it's some btrfs_get_block_group() call doesn't get its put
> call.
> 
> The offending btrfs_get_block_group() happens here:
> 
>   void btrfs_mark_bg_unused(struct btrfs_block_group *bg)
>   {
>   	if (list_empty(&bg->bg_list)) {
>   		btrfs_get_block_group(bg);
> 		list_add_tail(&bg->bg_list, &fs_info->unused_bgs);
>   	}
>   }
> 
> So every call sites removing the block group from unused_bgs list
> should
> reduce the ref count of that block group.
> 
> However for async discard, it didn't follow the call convention:
> 
>   void btrfs_discard_punt_unused_bgs_list(struct btrfs_fs_info
> *fs_info)
>   {
>   	list_for_each_entry_safe(block_group, next, &fs_info-
> >unused_bgs,
>   				 bg_list) {
>   		list_del_init(&block_group->bg_list);
>   		btrfs_discard_queue_work(&fs_info->discard_ctl,
> block_group);
>   	}
>   }
> 
> And in btrfs_discard_queue_work(), it doesn't call
> btrfs_put_block_group() either.
> 
> [FIX]
> Fix the problem by reducing the reference count when we grab the
> block
> group from unused_bgs list.

xfstests is happy about the change.

Tested-by: Marcos Paulo de Souza <mpdesouza@suse.com>

> 
> Reported-by: Marcos Paulo de Souza <marcos@mpdesouza.com>
> Fixes: 6e80d4f8c422 ("btrfs: handle empty block_group removal for
> async discard")
> Signed-off-by: Qu Wenruo <wqu@suse.com>
> ---
>  fs/btrfs/discard.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/fs/btrfs/discard.c b/fs/btrfs/discard.c
> index 5615320fa659..741c7e19c32f 100644
> --- a/fs/btrfs/discard.c
> +++ b/fs/btrfs/discard.c
> @@ -619,6 +619,7 @@ void btrfs_discard_punt_unused_bgs_list(struct
> btrfs_fs_info *fs_info)
>  	list_for_each_entry_safe(block_group, next, &fs_info-
> >unused_bgs,
>  				 bg_list) {
>  		list_del_init(&block_group->bg_list);
> +		btrfs_put_block_group(block_group);
>  		btrfs_discard_queue_work(&fs_info->discard_ctl,
> block_group);
>  	}
>  	spin_unlock(&fs_info->unused_bgs_lock);