Re: [PATCH 6/7] btrfs: btrfs_clear_delalloc_extent frees rsv

[Boris Burkov <boris@bur.io>]

On Wed, Mar 27, 2024 at 08:56:20AM +1030, Qu Wenruo wrote:
> 
> 
> 在 2024/3/27 08:09, Boris Burkov 写道:
> > Currently, this callsite only converts the reservation. We are marking
> > it not delalloc, so I don't think it makes sense to keep the rsv around.
> > This is a path where we are not sure to join a transaction, so it leads
> > to incorrect free-ing during umount.
> > 
> > Helps with the pass rate of generic/269 and generic/475
> 
> I guess the problem of all these ENOSPC/hutdown test cases is their
> reproducibility.

Yeah, it is definitely annoying to have to run generic/269 and
generic/475 hundreds of times to hit various different flavors of bugs
and try to drive it to 0. :/ It's hard to be sure that you are actually
successful and which fixes are definitely 100% necessary.

> 
> Unlike regular fsstress which can be very reproducible with its seed, it's
> pretty hard to reproduce a situation where you hit a certain qgroup leak.
> 
> Maybe the qgroup rsv leak detection is a little too strict for aborted
> transactions?

I agree for aborted transactions. It feels like a cheat just to beat the
warning. There are many failure paths that don't end in an aborted
transaction that we probably do actually care about, though.

> 
> Anyway, the patch itself looks fine.

Thanks for all the review on this series, btw!

> 
> Reviewed-by: Qu Wenruo <wqu@suse.com>
> 
> Thanks,
> Qu
> > 
> > Signed-off-by: Boris Burkov <boris@bur.io>
> > ---
> >   fs/btrfs/inode.c | 2 +-
> >   1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
> > index 2587a2e25e44..273adbb6b812 100644
> > --- a/fs/btrfs/inode.c
> > +++ b/fs/btrfs/inode.c
> > @@ -2533,7 +2533,7 @@ void btrfs_clear_delalloc_extent(struct btrfs_inode *inode,
> >   		 */
> >   		if (bits & EXTENT_CLEAR_META_RESV &&
> >   		    root != fs_info->tree_root)
> > -			btrfs_delalloc_release_metadata(inode, len, false);
> > +			btrfs_delalloc_release_metadata(inode, len, true);
> >   		/* For sanity tests. */
> >   		if (btrfs_is_testing(fs_info))