On Mon, Oct 07, 2019 at 04:17:39PM -0400, Dennis Zhou wrote: > The number of discardable extents will serve as the rate limiting metric > for how often we should discard. This keeps track of discardable extents > in the free space caches by maintaining deltas and propagating them to > the global count. > > This also setups up a discard directory in btrfs sysfs and exports the > total discard_extents count. > > Signed-off-by: Dennis Zhou <[email protected]> > --- > fs/btrfs/ctree.h | 2 + > fs/btrfs/discard.c | 2 + > fs/btrfs/discard.h | 19 ++++++++ > fs/btrfs/free-space-cache.c | 93 ++++++++++++++++++++++++++++++++++--- > fs/btrfs/free-space-cache.h | 2 + > fs/btrfs/sysfs.c | 33 +++++++++++++ > 6 files changed, 144 insertions(+), 7 deletions(-) > > diff --git a/fs/btrfs/ctree.h b/fs/btrfs/ctree.h > index c328d2e85e4d..43e515939b9c 100644 > --- a/fs/btrfs/ctree.h > +++ b/fs/btrfs/ctree.h > @@ -447,6 +447,7 @@ struct btrfs_discard_ctl { > spinlock_t lock; > struct btrfs_block_group_cache *cache; > struct list_head discard_list[BTRFS_NR_DISCARD_LISTS]; > + atomic_t discard_extents; > }; > > /* delayed seq elem */ > @@ -831,6 +832,7 @@ struct btrfs_fs_info { > struct btrfs_workqueue *scrub_wr_completion_workers; > struct btrfs_workqueue *scrub_parity_workers; > > + struct kobject *discard_kobj; > struct btrfs_discard_ctl discard_ctl; > > #ifdef CONFIG_BTRFS_FS_CHECK_INTEGRITY > diff --git a/fs/btrfs/discard.c b/fs/btrfs/discard.c > index 26a1e44b4bfa..0544eb6717d4 100644 > --- a/fs/btrfs/discard.c > +++ b/fs/btrfs/discard.c > @@ -298,6 +298,8 @@ void btrfs_discard_init(struct btrfs_fs_info *fs_info) > > for (i = 0; i < BTRFS_NR_DISCARD_LISTS; i++) > INIT_LIST_HEAD(&discard_ctl->discard_list[i]); > + > + atomic_set(&discard_ctl->discard_extents, 0); > } > > void btrfs_discard_cleanup(struct btrfs_fs_info *fs_info) > diff --git a/fs/btrfs/discard.h b/fs/btrfs/discard.h > index 22cfa7e401bb..85939d62521e 100644 > --- a/fs/btrfs/discard.h > +++ b/fs/btrfs/discard.h > @@ -71,4 +71,23 @@ void btrfs_discard_queue_work(struct btrfs_discard_ctl > *discard_ctl, > btrfs_discard_schedule_work(discard_ctl, false); > } > > +static inline > +void btrfs_discard_update_discardable(struct btrfs_block_group_cache *cache, > + struct btrfs_free_space_ctl *ctl) > +{ > + struct btrfs_discard_ctl *discard_ctl; > + s32 extents_delta; > + > + if (!cache || !btrfs_test_opt(cache->fs_info, DISCARD_ASYNC)) > + return; > + > + discard_ctl = &cache->fs_info->discard_ctl; > + > + extents_delta = ctl->discard_extents[0] - ctl->discard_extents[1]; > + if (extents_delta) { > + atomic_add(extents_delta, &discard_ctl->discard_extents); > + ctl->discard_extents[1] = ctl->discard_extents[0]; > + }
What the actual fuck? I assume you did this to avoid checking DISCARD_ASYNC on every update, but man this complexity is not worth it. We might as well update the counter every time to avoid doing stuff like this. If there's a better reason for doing it this way then I'm all ears, but even so this is not the way to do it. Just do atomic_add(ctl->discard_extenst, &discard_ctl->discard_extents); ctl->discard_extents = 0; and avoid the two step thing. And a comment, because it was like 5 minutes between me seeing this and getting to your reasoning, and in between there was a lot of swearing. Thanks, Josef
