On 14/04/2021 13:17, Filipe Manana wrote:
> Yep, that's what puzzled me, why the need to do it for non-zoned file
> systems when using -o discard=sync.
> I assumed you ran into a case where discard was not happening due to
> some bug bug in the extent pinning/unpinning mechanism.
>
>> So the correct fix would
>> be to get the block group into the 'trans->transaction->deleted_bgs' list
>> after relocation, which would work if we wouldn't check for block_group->ro
>> in
>> btrfs_delete_unused_bgs(), but I suppose this check is there for a reason.
>
> Actually the check for ->ro does not make sense anymore since I
> introduced the delete_unused_bgs_mutex in commit
> 67c5e7d464bc466471b05e027abe8a6b29687ebd.
>
> When the ->ro check was added
> (47ab2a6c689913db23ccae38349714edf8365e0a), it was meant to prevent
> the cleaner kthread and relocation tasks from calling
> btrfs_remove_chunk() concurrently, but checking for ->ro only was
> buggy, hence the addition of delete_unused_bgs_mutex later.
>
I'll have a look at these commits.
>>
>> How about changing the patch to the following:
>
> Looks good.
> However would just removing the ->ro check by enough as well?
>From how I understand the code, yes. It's a quick test, so let's just do it
and see what breaks.
I'd prefer to just drop the ->ro check, it's less special casing for zoned
btrfs that we have to keep in mind when changing things.
Thanks for helping me with this,
Johannes
