| Message ID | ce7db2df5f2f7617ac37f7c715a69e476acc7f1d.1711488980.git.boris@bur.io (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | btrfs: various qg meta rsv leak fixes | expand |
在 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. 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? Anyway, the patch itself looks fine. 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))
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))
在 2024/3/28 03:56, Boris Burkov 写道: > 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. I'm wondering if it's possible to add fsstress workload to inject errors (to specified injection points). IIRC we have error injection points for ENOSPC and ENOMEM, and fsstress is so far the most reliable reproducer. I hope that can greatly improve our reproducibility on the error paths. > >> >> 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. Indeed, despite the aborted transactions, we still have a lot of ENOMEM (less common though) and ENOSPC (much more common). Thanks, Qu > >> >> 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)) >
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))
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 Signed-off-by: Boris Burkov <boris@bur.io> --- fs/btrfs/inode.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)