On Tue, Oct 15, 2019 at 03:12:17PM +0200, David Sterba wrote:
> 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.
>
> Please put the discard directory under debug/ for now.
>
Just double checking, but you mean to have it be:
/sys/fs/btrfs/<uuid>/debug/discard/*?
> > Signed-off-by: Dennis Zhou <dennis@xxxxxxxxxx>
> > ---
> > 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;
>
> At the end of the series this becomes
>
> 452 atomic_t discard_extents;
> 453 atomic64_t discardable_bytes;
> 454 atomic_t delay;
> 455 atomic_t iops_limit;
> 456 atomic64_t bps_limit;
> 457 atomic64_t discard_extent_bytes;
> 458 atomic64_t discard_bitmap_bytes;
> 459 atomic64_t discard_bytes_saved;
>
> raising many eyebrows. What's the reason to use so many atomics? As this
> is purely for accounting and perhaps not contended, add one spinlock
> protecting all of them.
>
> None of delay, bps_limit and iops_limit use the atomict_t semantics at
> all, it's just _set and _read.
>
> As this seem to cascade to all other patches, I'll postpone my review
> until I see V2.
Yeah... I think the following 3 would be nice to keep as atomics as the
first two are propagated per block group and are protected via the
free_space_ctl's lock. Then multiple allocations can go through
concurrently. discard_bytes_saved is also something that can be
incremented by multiple block groups at once for the same reason, so an
atomic makes life simple.
> 452 atomic_t discard_extents;
> 453 atomic64_t discardable_bytes;
> 459 atomic64_t discard_bytes_saved;
The others I'll flip over to the proper type.
Thanks,
Dennis
