On Fri, 9 Jun 2017, Coly Li wrote:
> On 2017/6/8 下午5:34, tang.junhui@xxxxxxxxxx wrote:
> > From: Tang Junhui <tang.junhui@xxxxxxxxxx>
> >
> > When there is not enough data in writeback cache, let the writeback rate
> > to be 0, and delay 30 seconds in read_dirty().
> >
>
> I feel uncomfortable to the above idea. Is the purpose of this change is
> to make writeback more efficient with more dirty data ? Or some other
> reason that you want to make this change?
>
> When data writing back to the backing device, most of the writes are
> random. If the backing device is a hard disk, the write back rate will
> be very slow. Such a longer delay only makes more data accumulated in
> writeback cache, does not help the writeback rate indeed.
>
> Do you observe performance improvement by this patch ? If yes, I'd like
> to see the performance number.
Its not a performance issue, it is a power saving issue. Laptop users
have reported that zero-rate writeback writes 512-bytes every second and
never lets the HDD spin down and several users on the list have asked for
such a bugfix.
It would be nice if ZERO_RATE_DELAY_NS was configurable via sysfs, as some
might want a longer delay than 30s.
Also, if zero-rate has been hit, why not have a full-writeback of whatever
remains so we only use the ZERO_RATE_DELAY_NS when dirty_data==0? No
sense in leaving it slightly dirty if there is only a block or two still
remaining.
--
Eric Wheeler
>
> Thanks.
>
> Coly
>
>
> > Signed-off-by: Tang Junhui <tang.junhui@xxxxxxxxxx>
> > ---
> > drivers/md/bcache/util.c | 17 ++++++++++++-----
> > drivers/md/bcache/writeback.c | 2 +-
> > 2 files changed, 13 insertions(+), 6 deletions(-)
> >
> > diff --git a/drivers/md/bcache/util.c b/drivers/md/bcache/util.c
> > index dde6172..cf750df 100644
> > --- a/drivers/md/bcache/util.c
> > +++ b/drivers/md/bcache/util.c
> > @@ -15,6 +15,8 @@
> >
> > #include "util.h"
> >
> > +#define ZERO_RATE_DELAY_NS 30*NSEC_PER_SEC
> > +
> > #define simple_strtoint(c, end, base) simple_strtol(c, end, base)
> > #define simple_strtouint(c, end, base) simple_strtoul(c, end, base)
> >
> > @@ -209,13 +211,18 @@ uint64_t bch_next_delay(struct bch_ratelimit *d, uint64_t done)
> > {
> > uint64_t now = local_clock();
> >
> > - d->next += div_u64(done * NSEC_PER_SEC, d->rate);
> > + if (!d->rate) {
> > + d->next = now + ZERO_RATE_DELAY_NS;
> > + }
> > + else {
> > + d->next += div_u64(done * NSEC_PER_SEC, d->rate);
> >
> > - if (time_before64(now + NSEC_PER_SEC, d->next))
> > - d->next = now + NSEC_PER_SEC;
> > + if (time_before64(now + NSEC_PER_SEC, d->next))
> > + d->next = now + NSEC_PER_SEC;
> >
> > - if (time_after64(now - NSEC_PER_SEC * 2, d->next))
> > - d->next = now - NSEC_PER_SEC * 2;
> > + if (time_after64(now - NSEC_PER_SEC * 2, d->next))
> > + d->next = now - NSEC_PER_SEC * 2;
> > + }
> >
> > return time_after64(d->next, now)
> > ? div_u64(d->next - now, NSEC_PER_SEC / HZ)
> > diff --git a/drivers/md/bcache/writeback.c b/drivers/md/bcache/writeback.c
> > index 69e1ae5..8fac280 100644
> > --- a/drivers/md/bcache/writeback.c
> > +++ b/drivers/md/bcache/writeback.c
> > @@ -60,7 +60,7 @@ static void __update_writeback_rate(struct cached_dev *dc)
> >
> > dc->writeback_rate.rate =
> > clamp_t(int64_t, (int64_t) dc->writeback_rate.rate + change,
> > - 1, NSEC_PER_MSEC);
> > + 0, NSEC_PER_MSEC);
> >
> > dc->writeback_rate_proportional = proportional;
> > dc->writeback_rate_derivative = derivative;
> >
> --
> To unsubscribe from this list: send the line "unsubscribe linux-bcache" in
> the body of a message to majordomo@xxxxxxxxxxxxxxx
> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
