On Thu, Oct 10, 2019 at 11:47:19AM -0400, Josef Bacik wrote:
> On Mon, Oct 07, 2019 at 04:17:42PM -0400, Dennis Zhou wrote:
> > Provide an ability to rate limit based on mbps in addition to the iops
> > delay calculated from number of discardable extents.
> >
> > Signed-off-by: Dennis Zhou <dennis@xxxxxxxxxx>
> > ---
> > fs/btrfs/ctree.h | 2 ++
> > fs/btrfs/discard.c | 11 +++++++++++
> > fs/btrfs/sysfs.c | 30 ++++++++++++++++++++++++++++++
> > 3 files changed, 43 insertions(+)
> >
> > diff --git a/fs/btrfs/ctree.h b/fs/btrfs/ctree.h
> > index b0823961d049..e81f699347e0 100644
> > --- a/fs/btrfs/ctree.h
> > +++ b/fs/btrfs/ctree.h
> > @@ -447,10 +447,12 @@ struct btrfs_discard_ctl {
> > spinlock_t lock;
> > struct btrfs_block_group_cache *cache;
> > struct list_head discard_list[BTRFS_NR_DISCARD_LISTS];
> > + u64 prev_discard;
> > atomic_t discard_extents;
> > atomic64_t discardable_bytes;
> > atomic_t delay;
> > atomic_t iops_limit;
> > + atomic64_t bps_limit;
> > };
> >
> > /* delayed seq elem */
> > diff --git a/fs/btrfs/discard.c b/fs/btrfs/discard.c
> > index c7afb5f8240d..072c73f48297 100644
> > --- a/fs/btrfs/discard.c
> > +++ b/fs/btrfs/discard.c
> > @@ -176,6 +176,13 @@ void btrfs_discard_schedule_work(struct btrfs_discard_ctl *discard_ctl,
> > cache = find_next_cache(discard_ctl, now);
> > if (cache) {
> > u64 delay = atomic_read(&discard_ctl->delay);
> > + s64 bps_limit = atomic64_read(&discard_ctl->bps_limit);
> > +
> > + if (bps_limit)
> > + delay = max_t(u64, delay,
> > + msecs_to_jiffies(MSEC_PER_SEC *
> > + discard_ctl->prev_discard /
> > + bps_limit));
>
> I forget, are we allowed to do 0 / some value? I feel like I did this at some
> point with io.latency and it panic'ed and was very confused. Maybe I'm just
> misremembering.
>
I don't remember there being an issue there, but I'd rather not find
out. I added discard_ctl->prev_discard to the if statement.
> And a similar nit, maybe we just do
>
> u64 delay = atomic_read(&discard_ctl->delay);
> u64 bps_delay = atomic64_read(&discard_ctl->bps_limit);
> if (bps_delay)
> bps_delay = msecs_to_jiffies(MSEC_PER_SEC * blah)
>
> delay = max(delay, bps_delay);
>
/*
* A single delayed workqueue item is responsible for
* discarding, so we can manage the bytes rate limit by keeping
* track of the previous discard.
*/
if (bps_limit && discard_ctl->prev_discard) {
u64 bps_delay = (MSEC_PER_SEC *
discard_ctl->prev_discard / bps_limit);
delay = max_t(u64, delay, msecs_to_jiffies(bps_delay));
}
This is what I changed it to. I'm not sure I quite grasped what you're
getting at from above.
Thanks,
Dennis
