Re: [PATCH 08/19] btrfs: track discardable extents for asnyc discard

[Josef Bacik <josef@toxicpanda.com>]

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 <dennis@kernel.org>
> ---
>  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