Re: [PATCH] btrfs: let btrfs_delete_unused_bgs() to clean relocated bgs

[Josef Bacik]

On 09/02/2016 03:46 AM, Naohiro Aota wrote:
> Currently, btrfs_relocate_chunk() is removing relocated BG by itself. But
> the work can be done by btrfs_delete_unused_bgs() (and it's better since it
> trim the BG). Let's dedupe the code.
>
> While btrfs_delete_unused_bgs() is already hitting the relocated BG, it
> skip the BG since the BG has "ro" flag set (to keep balancing BG intact).
> On the other hand, btrfs cannot drop "ro" flag here to prevent additional
> writes. So this patch make use of "removed" flag.
> btrfs_delete_unused_bgs() now detect the flag to distinguish whether a
> read-only BG is relocating or not.

This seems racey to me.  We remove the last part of the block group, it ends up 
on the unused_bgs_list, we process this list, see that removed isn't set and we 
skip it, then later we set removed, but it's too late.  I think the right way is 
to actually do a transaction, set ->removed, manually add it to the 
unused_bgs_list if it's not already, then end the transaction.  This way we are 
guaranteed to have the bg on the list when it is ready to be removed.  This is 
my analysis after looking at it for 10 seconds after being awake for like 30 
minutes so if I'm missing something let me know.  Thanks,

Josef