Re: [PATCH 02/18] btrfs: fix start transaction qgroup rsv double free

[Josef Bacik <josef@toxicpanda.com>]

On Wed, Jul 05, 2023 at 04:20:39PM -0700, Boris Burkov wrote:
> btrfs_start_transaction reserves metadata space of the PERTRANS type
> before it identifies a transaction to start/join. This allows flushing
> when reserving that space without a deadlock. However, it results in a
> race which temporarily breaks qgroup rsv accounting.
> 
> T1                                              T2
> start_transaction
> do_stuff
>                                             start_transaction
>                                                 qgroup_reserve_meta_pertrans
> commit_transaction
>     qgroup_free_meta_all_pertrans
>                                             hit an error starting txn
>                                             goto reserve_fail
>                                             qgroup_free_meta_pertrans (already freed!)
> 
> The basic issue is that there is nothing preventing another commit from
> committing before start_transaction finishes (in fact sometimes we
> intentionally wait for it..) so any error path that frees the reserve is
> at risk of this race.
> 
> While this exact space was getting freed anyway, and it's not a huge
> deal to double free it (just a warning, the free code catches this), it
> can result in incorrectly freeing some other pertrans reservation in
> this same reservation, which could then lead to spuriously granting
> reservations we might not have the space for. Therefore, I do believe it
> is worth fixing.
> 
> To fix it, use the existing prealloc->pertrans conversion mechanism.
> When we first reserve the space, we reserve prealloc space and only when
> we are sure we have a transaction do we convert it to pertrans. This way
> any racing commits do not blow away our reservation, but we still get a
> pertrans reservation that is freed when _this_ transaction gets committed.
> 
> This issue can be reproduced by running generic/269 with either qgroups
> or squotas enabled via mkfs on the scratch device.
> 
> Signed-off-by: Boris Burkov <boris@bur.io>

Reviewed-by: Josef Bacik <josef@toxicpanda.com>

Thanks,

Josef