Date:   Thu, 17 Jun 2021 11:53:03 +0200
From:   Michael Stapelberg <stapelberg+linux@...gle.com>
To:     Andrew Morton <akpm@...ux-foundation.org>
Cc:     linux-kernel@...r.kernel.org, linux-mm@...ck.org,
        linux-fsdevel@...r.kernel.org,
        Michael Stapelberg <stapelberg+linux@...gle.com>,
        Tejun Heo <tj@...nel.org>, Dennis Zhou <dennis@...nel.org>,
        Jens Axboe <axboe@...nel.dk>, Roman Gushchin <guro@...com>,
        Johannes Thumshirn <johannes.thumshirn@....com>,
        Jan Kara <jack@...e.cz>, Song Liu <song@...nel.org>,
        David Sterba <dsterba@...e.com>
Subject: [PATCH] backing_dev_info: introduce min_bw/max_bw limits

These new knobs allow e.g. FUSE file systems to guide kernel memory
writeback bandwidth throttling.

Background:

When using mmap(2) to read/write files, the page-writeback code tries to
measure how quick file system backing devices (BDI) are able to write data,
so that it can throttle processes accordingly.

Unfortunately, certain usage patterns, such as linkers (tested with GCC,
but also the Go linker) seem to hit an unfortunate corner case when writing
their large executable output files: the kernel only ever measures
the (non-representative) rising slope of the starting bulk write, but the
whole file write is already over before the kernel could possibly measure
the representative steady-state.

As a consequence, with each program invocation hitting this corner case,
the FUSE write bandwidth steadily sinks in a downward spiral, until it
eventually reaches 0 (!). This results in the kernel heavily throttling
page dirtying in programs trying to write to FUSE, which in turn manifests
itself in slow or even entirely stalled linker processes.

Change:

This commit adds 2 knobs which allow avoiding this situation entirely on a
per-file-system basis by restricting the minimum/maximum bandwidth.

There are no negative effects expected from applying this patch.

At Google, we have been running this patch for about 1 year on many
thousands of developer PCs without observing any issues. Our in-house FUSE
filesystems pin the BDI write rate at its default setting of 100 MB/s,
which successfully prevents the bug described above.

Usage:

To inspect the measured bandwidth, check the BdiWriteBandwidth field in
e.g. /sys/kernel/debug/bdi/0:93/stats.

To pin the measured bandwidth to its default of 100 MB/s, use:

    echo 25600 > /sys/class/bdi/0:42/min_bw
    echo 25600 > /sys/class/bdi/0:42/max_bw

Notes:

For more discussion, including a test program for reproducing the issue,
see the following discussion thread on the Linux Kernel Mailing List:

https://lore.kernel.org/linux-fsdevel/CANnVG6n=ySfe1gOr=0ituQidp56idGARDKHzP0hv=ERedeMrMA@mail.gmail.com/

Why introduce these knobs instead of trying to tweak the
throttling/measurement algorithm? The effort required to systematically
analyze, improve and land such an algorithm change exceeds the time budget
I have available. For comparison, check out this quote about the original
patch set from 2011: “Fengguang said he draw more than 10K performance
graphs and read even more in the past year.” (from
http://bardofschool.blogspot.com/2011/). Given that nobody else has stepped
up, despite the problem being known since 2016, my suggestion is to add the
knobs until someone can spend significant time on a revision to the
algorithm.

Signed-off-by: Michael Stapelberg <stapelberg+linux@...gle.com>
---
 include/linux/backing-dev-defs.h |  2 ++
 include/linux/backing-dev.h      |  3 +++
 mm/backing-dev.c                 | 40 ++++++++++++++++++++++++++++++++
 mm/page-writeback.c              | 28 ++++++++++++++++++++++
 4 files changed, 73 insertions(+)

diff --git a/include/linux/backing-dev-defs.h b/include/linux/backing-dev-defs.h
index 1d7edad9914f..e34797bb62a1 100644
--- a/include/linux/backing-dev-defs.h
+++ b/include/linux/backing-dev-defs.h
@@ -175,6 +175,8 @@ struct backing_dev_info {
 	unsigned int capabilities; /* Device capabilities */
 	unsigned int min_ratio;
 	unsigned int max_ratio, max_prop_frac;
+	u64 min_bw;
+	u64 max_bw;
 
 	/*
 	 * Sum of avg_write_bw of wbs with dirty inodes.  > 0 if there are
diff --git a/include/linux/backing-dev.h b/include/linux/backing-dev.h
index 44df4fcef65c..bb812a4df3a1 100644
--- a/include/linux/backing-dev.h
+++ b/include/linux/backing-dev.h
@@ -107,6 +107,9 @@ static inline unsigned long wb_stat_error(void)
 int bdi_set_min_ratio(struct backing_dev_info *bdi, unsigned int min_ratio);
 int bdi_set_max_ratio(struct backing_dev_info *bdi, unsigned int max_ratio);
 
+int bdi_set_min_bw(struct backing_dev_info *bdi, u64 min_bw);
+int bdi_set_max_bw(struct backing_dev_info *bdi, u64 max_bw);
+
 /*
  * Flags in backing_dev_info::capability
  *
diff --git a/mm/backing-dev.c b/mm/backing-dev.c
index 271f2ca862c8..0201345d41f2 100644
--- a/mm/backing-dev.c
+++ b/mm/backing-dev.c
@@ -197,6 +197,44 @@ static ssize_t max_ratio_store(struct device *dev,
 }
 BDI_SHOW(max_ratio, bdi->max_ratio)
 
+static ssize_t min_bw_store(struct device *dev,
+		struct device_attribute *attr, const char *buf, size_t count)
+{
+	struct backing_dev_info *bdi = dev_get_drvdata(dev);
+	unsigned long long limit;
+	ssize_t ret;
+
+	ret = kstrtoull(buf, 10, &limit);
+	if (ret < 0)
+		return ret;
+
+	ret = bdi_set_min_bw(bdi, limit);
+	if (!ret)
+		ret = count;
+
+	return ret;
+}
+BDI_SHOW(min_bw, bdi->min_bw)
+
+static ssize_t max_bw_store(struct device *dev,
+		struct device_attribute *attr, const char *buf, size_t count)
+{
+	struct backing_dev_info *bdi = dev_get_drvdata(dev);
+	unsigned long long limit;
+	ssize_t ret;
+
+	ret = kstrtoull(buf, 10, &limit);
+	if (ret < 0)
+		return ret;
+
+	ret = bdi_set_max_bw(bdi, limit);
+	if (!ret)
+		ret = count;
+
+	return ret;
+}
+BDI_SHOW(max_bw, bdi->max_bw)
+
 static ssize_t stable_pages_required_show(struct device *dev,
 					  struct device_attribute *attr,
 					  char *buf)
@@ -211,6 +249,8 @@ static struct attribute *bdi_dev_attrs[] = {
 	&dev_attr_read_ahead_kb.attr,
 	&dev_attr_min_ratio.attr,
 	&dev_attr_max_ratio.attr,
+	&dev_attr_min_bw.attr,
+	&dev_attr_max_bw.attr,
 	&dev_attr_stable_pages_required.attr,
 	NULL,
 };
diff --git a/mm/page-writeback.c b/mm/page-writeback.c
index 9f63548f247c..1ee9636e6088 100644
--- a/mm/page-writeback.c
+++ b/mm/page-writeback.c
@@ -701,6 +701,22 @@ int bdi_set_max_ratio(struct backing_dev_info *bdi, unsigned max_ratio)
 }
 EXPORT_SYMBOL(bdi_set_max_ratio);
 
+int bdi_set_min_bw(struct backing_dev_info *bdi, u64 min_bw)
+{
+	spin_lock_bh(&bdi_lock);
+	bdi->min_bw = min_bw;
+	spin_unlock_bh(&bdi_lock);
+	return 0;
+}
+
+int bdi_set_max_bw(struct backing_dev_info *bdi, u64 max_bw)
+{
+	spin_lock_bh(&bdi_lock);
+	bdi->max_bw = max_bw;
+	spin_unlock_bh(&bdi_lock);
+	return 0;
+}
+
 static unsigned long dirty_freerun_ceiling(unsigned long thresh,
 					   unsigned long bg_thresh)
 {
@@ -1068,6 +1084,15 @@ static void wb_position_ratio(struct dirty_throttle_control *dtc)
 	dtc->pos_ratio = pos_ratio;
 }
 
+static u64 clamp_bw(struct backing_dev_info *bdi, u64 bw)
+{
+	if (bdi->min_bw > 0 && bw < bdi->min_bw)
+		bw = bdi->min_bw;
+	if (bdi->max_bw > 0 && bw > bdi->max_bw)
+		bw = bdi->max_bw;
+	return bw;
+}
+
 static void wb_update_write_bandwidth(struct bdi_writeback *wb,
 				      unsigned long elapsed,
 				      unsigned long written)
@@ -1091,12 +1116,15 @@ static void wb_update_write_bandwidth(struct bdi_writeback *wb,
 	bw *= HZ;
 	if (unlikely(elapsed > period)) {
 		bw = div64_ul(bw, elapsed);
+		bw = clamp_bw(wb->bdi, bw);
 		avg = bw;
 		goto out;
 	}
 	bw += (u64)wb->write_bandwidth * (period - elapsed);
 	bw >>= ilog2(period);
 
+	bw = clamp_bw(wb->bdi, bw);
+
 	/*
 	 * one more level of smoothing, for filtering out sudden spikes
 	 */
-- 
2.32.0.288.g62a8d224e6-goog

Powered by blists - more mailing lists