| Message ID | 2ddc45133bcee20c64699abf10cc24bf2737b606.1727174151.git.fdmanana@suse.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | btrfs: make extent map shrinker more efficient and re-enable it | expand |
在 2024/9/24 20:15, fdmanana@kernel.org 写道: > From: Filipe Manana <fdmanana@suse.com> > > Now that the extent map shrinker can only be run by a single task and runs > asynchronously as a work queue job, enable it as it can no longer cause > stalls on tasks allocating memory and entering the extent map shrinker > through the fs shrinker (implemented by btrfs_free_cached_objects()). > > This is crucial to prevent exhaustion of memory due to unbounded extent > map creation, primarily with direct IO but also for buffered IO on files > with holes. This problem, for the direct IO case, was first reported in > the Link tag below. That report was added to a Link tag of the first patch > that introduced the extent map shrinker, commit 956a17d9d050 ("btrfs: add > a shrinker for extent maps"), however the Link tag disappeared somehow > from the committed patch (but was included in the submitted patch to the > mailing list), so adding it below for future reference. > > Link: https://lore.kernel.org/linux-btrfs/13f94633dcf04d29aaf1f0a43d42c55e@amazon.com/ Forgot to mention, I'd prefer the enablement patch to be merged in a later release cycle, not at the same time inside the series. We need more tests, especially from the original reporters, and that's why we have EXPERIMENTAL flags. Thanks, Qu > Signed-off-by: Filipe Manana <fdmanana@suse.com> > --- > fs/btrfs/super.c | 8 +------- > 1 file changed, 1 insertion(+), 7 deletions(-) > > diff --git a/fs/btrfs/super.c b/fs/btrfs/super.c > index e9e209dd8e05..7e20b5e8386c 100644 > --- a/fs/btrfs/super.c > +++ b/fs/btrfs/super.c > @@ -2401,13 +2401,7 @@ static long btrfs_nr_cached_objects(struct super_block *sb, struct shrink_contro > > trace_btrfs_extent_map_shrinker_count(fs_info, nr); > > - /* > - * Only report the real number for EXPERIMENTAL builds, as there are > - * reports of serious performance degradation caused by too frequent shrinks. > - */ > - if (IS_ENABLED(CONFIG_BTRFS_EXPERIMENTAL)) > - return nr; > - return 0; > + return nr; > } > > static long btrfs_free_cached_objects(struct super_block *sb, struct shrink_control *sc)
On Thu, Sep 26, 2024 at 11:00 AM Qu Wenruo <quwenruo.btrfs@gmx.com> wrote: > > > > 在 2024/9/24 20:15, fdmanana@kernel.org 写道: > > From: Filipe Manana <fdmanana@suse.com> > > > > Now that the extent map shrinker can only be run by a single task and runs > > asynchronously as a work queue job, enable it as it can no longer cause > > stalls on tasks allocating memory and entering the extent map shrinker > > through the fs shrinker (implemented by btrfs_free_cached_objects()). > > > > This is crucial to prevent exhaustion of memory due to unbounded extent > > map creation, primarily with direct IO but also for buffered IO on files > > with holes. This problem, for the direct IO case, was first reported in > > the Link tag below. That report was added to a Link tag of the first patch > > that introduced the extent map shrinker, commit 956a17d9d050 ("btrfs: add > > a shrinker for extent maps"), however the Link tag disappeared somehow > > from the committed patch (but was included in the submitted patch to the > > mailing list), so adding it below for future reference. > > > > Link: https://lore.kernel.org/linux-btrfs/13f94633dcf04d29aaf1f0a43d42c55e@amazon.com/ > > Forgot to mention, I'd prefer the enablement patch to be merged in a > later release cycle, not at the same time inside the series. > > We need more tests, especially from the original reporters, and that's > why we have EXPERIMENTAL flags. Sure I can ping them and see if they have the availability to test and report. But expecting every single user to be able to test may not be possible. But I don't think we ever had to have explicit ack from users to move things out of experimental, especially for things that don't affect disk format changes or incompatibility issues, for things that are fully transparent. > > Thanks, > Qu > > > Signed-off-by: Filipe Manana <fdmanana@suse.com> > > --- > > fs/btrfs/super.c | 8 +------- > > 1 file changed, 1 insertion(+), 7 deletions(-) > > > > diff --git a/fs/btrfs/super.c b/fs/btrfs/super.c > > index e9e209dd8e05..7e20b5e8386c 100644 > > --- a/fs/btrfs/super.c > > +++ b/fs/btrfs/super.c > > @@ -2401,13 +2401,7 @@ static long btrfs_nr_cached_objects(struct super_block *sb, struct shrink_contro > > > > trace_btrfs_extent_map_shrinker_count(fs_info, nr); > > > > - /* > > - * Only report the real number for EXPERIMENTAL builds, as there are > > - * reports of serious performance degradation caused by too frequent shrinks. > > - */ > > - if (IS_ENABLED(CONFIG_BTRFS_EXPERIMENTAL)) > > - return nr; > > - return 0; > > + return nr; > > } > > > > static long btrfs_free_cached_objects(struct super_block *sb, struct shrink_control *sc) >
On Thu, Sep 26, 2024 at 11:52 AM Filipe Manana <fdmanana@kernel.org> wrote: > > On Thu, Sep 26, 2024 at 11:00 AM Qu Wenruo <quwenruo.btrfs@gmx.com> wrote: > > > > > > > > 在 2024/9/24 20:15, fdmanana@kernel.org 写道: > > > From: Filipe Manana <fdmanana@suse.com> > > > > > > Now that the extent map shrinker can only be run by a single task and runs > > > asynchronously as a work queue job, enable it as it can no longer cause > > > stalls on tasks allocating memory and entering the extent map shrinker > > > through the fs shrinker (implemented by btrfs_free_cached_objects()). > > > > > > This is crucial to prevent exhaustion of memory due to unbounded extent > > > map creation, primarily with direct IO but also for buffered IO on files > > > with holes. This problem, for the direct IO case, was first reported in > > > the Link tag below. That report was added to a Link tag of the first patch > > > that introduced the extent map shrinker, commit 956a17d9d050 ("btrfs: add > > > a shrinker for extent maps"), however the Link tag disappeared somehow > > > from the committed patch (but was included in the submitted patch to the > > > mailing list), so adding it below for future reference. > > > > > > Link: https://lore.kernel.org/linux-btrfs/13f94633dcf04d29aaf1f0a43d42c55e@amazon.com/ > > > > Forgot to mention, I'd prefer the enablement patch to be merged in a > > later release cycle, not at the same time inside the series. > > > > We need more tests, especially from the original reporters, and that's > > why we have EXPERIMENTAL flags. > > Sure I can ping them and see if they have the availability to test and report. > But expecting every single user to be able to test may not be possible. > > But I don't think we ever had to have explicit ack from users to move > things out of experimental, especially for things that don't affect > disk format changes or incompatibility issues, for things that are > fully transparent. Further while this is under the experimental flags (previously debug), it's not a feature but a bug fix (functional and security). It just happened that while I was under vacation it was placed in that category just to disable it, instead of commenting out code or other alternatives. > > > > > Thanks, > > Qu > > > > > Signed-off-by: Filipe Manana <fdmanana@suse.com> > > > --- > > > fs/btrfs/super.c | 8 +------- > > > 1 file changed, 1 insertion(+), 7 deletions(-) > > > > > > diff --git a/fs/btrfs/super.c b/fs/btrfs/super.c > > > index e9e209dd8e05..7e20b5e8386c 100644 > > > --- a/fs/btrfs/super.c > > > +++ b/fs/btrfs/super.c > > > @@ -2401,13 +2401,7 @@ static long btrfs_nr_cached_objects(struct super_block *sb, struct shrink_contro > > > > > > trace_btrfs_extent_map_shrinker_count(fs_info, nr); > > > > > > - /* > > > - * Only report the real number for EXPERIMENTAL builds, as there are > > > - * reports of serious performance degradation caused by too frequent shrinks. > > > - */ > > > - if (IS_ENABLED(CONFIG_BTRFS_EXPERIMENTAL)) > > > - return nr; > > > - return 0; > > > + return nr; > > > } > > > > > > static long btrfs_free_cached_objects(struct super_block *sb, struct shrink_control *sc) > >
diff --git a/fs/btrfs/super.c b/fs/btrfs/super.c index e9e209dd8e05..7e20b5e8386c 100644 --- a/fs/btrfs/super.c +++ b/fs/btrfs/super.c @@ -2401,13 +2401,7 @@ static long btrfs_nr_cached_objects(struct super_block *sb, struct shrink_contro trace_btrfs_extent_map_shrinker_count(fs_info, nr); - /* - * Only report the real number for EXPERIMENTAL builds, as there are - * reports of serious performance degradation caused by too frequent shrinks. - */ - if (IS_ENABLED(CONFIG_BTRFS_EXPERIMENTAL)) - return nr; - return 0; + return nr; } static long btrfs_free_cached_objects(struct super_block *sb, struct shrink_control *sc)