| Message ID | cover.1727174151.git.fdmanana@suse.com (mailing list archive) |
|---|---|
| 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> > > This makes the extent map shrinker run by a single task and asynchronously > in a work queue, re-enables it by default and some cleanups. More details > in the changelogs of each patch. > > Filipe Manana (5): > btrfs: add and use helper to remove extent map from its inode's tree > btrfs: make the extent map shrinker run asynchronously as a work queue job I only have a small concern on whether we should introduce a delay to the reclaim work. The remaining patches all look good to me. Thanks, Qu > btrfs: simplify tracking progress for the extent map shrinker > btrfs: rename extent map shrinker members from struct btrfs_fs_info > btrfs: re-enable the extent map shrinker > > fs/btrfs/disk-io.c | 4 +- > fs/btrfs/extent_map.c | 122 +++++++++++++++++------------------ > fs/btrfs/extent_map.h | 3 +- > fs/btrfs/fs.h | 7 +- > fs/btrfs/super.c | 21 ++---- > include/trace/events/btrfs.h | 21 +++--- > 6 files changed, 83 insertions(+), 95 deletions(-) >
On Tue, Sep 24, 2024 at 11:45:40AM +0100, fdmanana@kernel.org wrote: > From: Filipe Manana <fdmanana@suse.com> > > This makes the extent map shrinker run by a single task and asynchronously > in a work queue, re-enables it by default and some cleanups. More details > in the changelogs of each patch. > > Filipe Manana (5): > btrfs: add and use helper to remove extent map from its inode's tree > btrfs: make the extent map shrinker run asynchronously as a work queue job > btrfs: simplify tracking progress for the extent map shrinker > btrfs: rename extent map shrinker members from struct btrfs_fs_info > btrfs: re-enable the extent map shrinker > I think as a first step this is good. I am concerned that async shrinking under heavy memory pressure will lead to spurious OOM's because we're never waiting for the async shrinker to do work. I think that a next step would be to investigate that possibility, and possibly use the nr_to_scan or some other mechanism to figure out that we're getting called multiple times and then wait for the shrinker to make progress. This will keep the VM from deciding we aren't making progress and OOM'ing. That being said this series looks good to me, you can add Reviewed-by: Josef Bacik <josef@toxicpanda.com> to the whole thing. Thanks, Josef
On Thu, Oct 10, 2024 at 3:48 PM Josef Bacik <josef@toxicpanda.com> wrote: > > On Tue, Sep 24, 2024 at 11:45:40AM +0100, fdmanana@kernel.org wrote: > > From: Filipe Manana <fdmanana@suse.com> > > > > This makes the extent map shrinker run by a single task and asynchronously > > in a work queue, re-enables it by default and some cleanups. More details > > in the changelogs of each patch. > > > > Filipe Manana (5): > > btrfs: add and use helper to remove extent map from its inode's tree > > btrfs: make the extent map shrinker run asynchronously as a work queue job > > btrfs: simplify tracking progress for the extent map shrinker > > btrfs: rename extent map shrinker members from struct btrfs_fs_info > > btrfs: re-enable the extent map shrinker > > > > I think as a first step this is good. > > I am concerned that async shrinking under heavy memory pressure will > lead to spurious OOM's because we're never waiting for the async > shrinker to do work. I think that a next step would be to investigate > that possibility, and possibly use the nr_to_scan or some other > mechanism to figure out that we're getting called multiple times and > then wait for the shrinker to make progress. This will keep the VM from > deciding we aren't making progress and OOM'ing. Right. The problem is how to decide when to wait and if the waiting will introduce latencies - maybe wait only if the current task is a kswapd task. If you have concrete ideias, I'm all for it. Thanks. > > That being said this series looks good to me, you can add > > Reviewed-by: Josef Bacik <josef@toxicpanda.com> > > to the whole thing. Thanks, > > Josef
From: Filipe Manana <fdmanana@suse.com> This makes the extent map shrinker run by a single task and asynchronously in a work queue, re-enables it by default and some cleanups. More details in the changelogs of each patch. Filipe Manana (5): btrfs: add and use helper to remove extent map from its inode's tree btrfs: make the extent map shrinker run asynchronously as a work queue job btrfs: simplify tracking progress for the extent map shrinker btrfs: rename extent map shrinker members from struct btrfs_fs_info btrfs: re-enable the extent map shrinker fs/btrfs/disk-io.c | 4 +- fs/btrfs/extent_map.c | 122 +++++++++++++++++------------------ fs/btrfs/extent_map.h | 3 +- fs/btrfs/fs.h | 7 +- fs/btrfs/super.c | 21 ++---- include/trace/events/btrfs.h | 21 +++--- 6 files changed, 83 insertions(+), 95 deletions(-)