[PATCH v3 0/4] blk-iocost: charge flushes and zone appends

[PATCH v3 0/4] blk-iocost: charge flushes and zone appends

[Tao Cui]

From: Tao Cui <cuitao@kylinos.cn>

While testing iocost's weight-based throttling under concurrent IO, we
observed that a cgroup limited to 1% weight could issue an unbounded
number of flushes without being throttled: an fsync loop produced ~510k
flushes in 12s with cost.usage staying at zero the entire time.  The
device was monopolized while iocost reported no activity.  Zone appends
were in the same position: the builtin linear cost model defines
coefficients only for READ and WRITE, so REQ_OP_ZONE_APPEND is priced
at zero and excluded from the latency statistics as well.

This series prices both, without changing the behavior of existing
setups:

 1/4: add a flushiops entry to io.cost.model, translated like the
      other iops coefficients (VTIME_PER_SEC / flushiops).  A bio with
      REQ_PREFLUSH is charged one flush on top of its data cost, and a
      bio with REQ_FUA one more flush on devices without native FUA
      support (bdev_fua()), mirroring the pre-flush and post-flush the
      block layer issues for them.  flushiops is zero in the builtin
      profiles with no fallback to the write coefficients, so nothing
      changes until it is configured.  Also documents flushiops in
      cgroup-v2.rst.
 2/4: treat zone appends as writes in the builtin cost model -
      REQ_OP_ZONE_APPEND falls through to the REQ_OP_WRITE
      coefficients.
 3/4: count zone append completions in the latency window so the vrate
      feedback loop can see ZA-induced latency.
 4/4: fix a stale comment in ioc_rqos_throttle().

Measured on virtio-blk with a bio test module issuing bios from a
cgroup (300 bios per phase):

  - plain writes cost 23.41 usec, unchanged
  - with the default model, WRITE|PREFLUSH and standalone flushes cost
    the same as before: the flush component is free until flushiops is
    configured
  - with flushiops=100, WRITE|PREFLUSH and WRITE|FUA are charged
    +10000 usec per bio and WRITE|PREFLUSH|FUA +20000, exactly
    VTIME_PER_SEC / 100 per flush (virtio-blk has no native FUA,
    confirmed via queue/fua)
  - standalone flushes are charged 10000.06 usec each, matching
    VTIME_PER_SEC / 100
  - with flushiops=10, a flush storm from a low-weight cgroup is
    throttled to one flush per 100ms

Changes in v2:
- charge the flush component of data-bearing REQ_PREFLUSH bios too,
  not only standalone flushes (Christoph)
- skip the iocg->cursor update for dataless flush bios

Changes in v3:
- the write-coefficient flush pricing is replaced by the flushiops
  model parameter: pricing a flush off the write coefficients is as
  arbitrary as pricing it at zero and changes what existing setups
  get charged, so flushes are only priced when flushiops is
  configured (Tejun)
- the zone append patch is reduced to the minimal change: ZA falls
  through to the REQ_OP_WRITE coefficients and the cursor is left
  alone (Tejun)
- the Fixes tags on 2/4 and 3/4 point at 0512a75b98f8, which added
  the op without pricing it; 3/4 is otherwise unchanged from v2

Tao Cui (4):
  blk-iocost: add flush cost support with the flushiops model parameter
  blk-iocost: charge zone appends as writes
  blk-iocost: account zone append completions in latency stats
  blk-iocost: fix stale comment in ioc_rqos_throttle()

 Documentation/admin-guide/cgroup-v2.rst |  8 ++++++++
 block/blk-iocost.c                      | 40 ++++++++++++++++++++++---------
 2 files changed, 48 insertions(+), 9 deletions(-)

-- 
2.43.0

[PATCH v3 1/4] blk-iocost: add flush cost support with the flushiops model parameter

[Tao Cui]

From: Tao Cui <cuitao@kylinos.cn>

The builtin linear cost model defines coefficients only for READ and
WRITE, so the flush component of IOs is priced at zero: standalone
flushes (the dataless REQ_OP_WRITE | REQ_PREFLUSH bios from
blkdev_issue_flush()) and the pre-flush the block layer issues ahead of
data-bearing REQ_PREFLUSH bios are both free.  A cgroup limited to 1%
weight could issue an unbounded number of flushes without being
throttled: an fsync loop produced ~510k flushes in 12s with
cost.usage staying at zero, monopolizing the device while iocost
reported no activity.  On ext4, a write+fsync workload showed the same
gap: the write component was charged but the flush component was not.

A flush is not like a write and the write coefficients say nothing
about what it costs, so instead of pricing it off them, add a flushiops
entry to io.cost.model, following the existing iops parameters: it sets
the rate at which flushes are charged, translated like the other iops
coefficients to LCOEF_FLUSH = VTIME_PER_SEC / flushiops.  The linear
model cannot express how the cost of a flush depends on the preceding
writes, so this is a user-tunable policy knob rather than a hardware
property.  A bio with REQ_PREFLUSH is charged one flush on top of its
data cost, and a bio with REQ_FUA one more flush on devices without
native FUA support, mirroring the
pre-flush and post-flush the block layer issues for them.  Zero (the
default and the builtin profiles) means no charge, so nothing changes
until the parameter is configured.

Also skip the iocg->cursor update for dataless bios: they only reach it
once priced, and their bi_sector is not a data position, so setting
the cursor from it would misclassify the following IOs.

Signed-off-by: Tao Cui <cuitao@kylinos.cn>
---
 Documentation/admin-guide/cgroup-v2.rst |  8 ++++++
 block/blk-iocost.c                      | 37 +++++++++++++++++++++----
 2 files changed, 40 insertions(+), 5 deletions(-)

diff --git a/Documentation/admin-guide/cgroup-v2.rst b/Documentation/admin-guide/cgroup-v2.rst
index 8d2603751c51..5a6ba29b7789 100644
--- a/Documentation/admin-guide/cgroup-v2.rst
+++ b/Documentation/admin-guide/cgroup-v2.rst
@@ -2131,6 +2131,7 @@ IO Interface Files
 	  [r|w]bps	The maximum sequential IO throughput
 	  [r|w]seqiops	The maximum 4k sequential IOs per second
 	  [r|w]randiops	The maximum 4k random IOs per second
+	  flushiops	The rate at which flushes are charged
 	  =============	========================================
 
 	From the above, the builtin linear model determines the base
@@ -2138,6 +2139,13 @@ IO Interface Files
 	for the IO size.  While simple, this model can cover most
 	common device classes acceptably.
 
+	"flushiops" determines the cost of a cache flush: a bio with
+	REQ_PREFLUSH is charged one flush on top of its data cost, and
+	a bio with REQ_FUA is charged one more flush on devices without
+	native FUA support, mirroring the pre-flush and post-flush the
+	block layer issues for them.  It is zero in the builtin
+	profiles, so flushes stay free until it is configured.
+
 	The IO cost model isn't expected to be accurate in absolute
 	sense and is scaled to the device behavior dynamically.
 
diff --git a/block/blk-iocost.c b/block/blk-iocost.c
index 2745bffcd5ee..0a0352554dde 100644
--- a/block/blk-iocost.c
+++ b/block/blk-iocost.c
@@ -353,6 +353,7 @@ enum {
 	I_LCOEF_WBPS,
 	I_LCOEF_WSEQIOPS,
 	I_LCOEF_WRANDIOPS,
+	I_LCOEF_FLUSHIOPS,
 	NR_I_LCOEFS,
 };
 
@@ -363,6 +364,7 @@ enum {
 	LCOEF_WPAGE,
 	LCOEF_WSEQIO,
 	LCOEF_WRANDIO,
+	LCOEF_FLUSH,
 	NR_LCOEFS,
 };
 
@@ -883,6 +885,9 @@ static void ioc_refresh_lcoefs(struct ioc *ioc)
 		    &c[LCOEF_RPAGE], &c[LCOEF_RSEQIO], &c[LCOEF_RRANDIO]);
 	calc_lcoefs(u[I_LCOEF_WBPS], u[I_LCOEF_WSEQIOPS], u[I_LCOEF_WRANDIOPS],
 		    &c[LCOEF_WPAGE], &c[LCOEF_WSEQIO], &c[LCOEF_WRANDIO]);
+
+	c[LCOEF_FLUSH] = u[I_LCOEF_FLUSHIOPS] ?
+		DIV64_U64_ROUND_UP(VTIME_PER_SEC, u[I_LCOEF_FLUSHIOPS]) : 0;
 }
 
 /*
@@ -2532,8 +2537,25 @@ static void calc_vtime_cost_builtin(struct bio *bio, struct ioc_gq *iocg,
 	u64 pages = max_t(u64, bio_sectors(bio) >> IOC_SECT_TO_PAGE_SHIFT, 1);
 	u64 seek_pages = 0;
 	u64 cost = 0;
+	u64 flush_cost = 0;
+
+	/*
+	 * The flush machine runs a cache flush for REQ_PREFLUSH, either
+	 * standalone (dataless) or ahead of the data, and a post-flush for
+	 * REQ_FUA on devices without native FUA support.  Charge each
+	 * component on top of the data cost using the flush coefficient
+	 * (VTIME_PER_SEC / flushiops; zero when flushiops is unset).  Flush
+	 * bios are never merged.
+	 */
+	if (!is_merge) {
+		if (bio->bi_opf & REQ_PREFLUSH)
+			flush_cost += ioc->params.lcoefs[LCOEF_FLUSH];
+		if ((bio->bi_opf & REQ_FUA) &&
+		    !bdev_fua(bio->bi_bdev))
+			flush_cost += ioc->params.lcoefs[LCOEF_FLUSH];
+	}
 
-	/* Can't calculate cost for empty bio */
+	/* Can't calculate data cost for empty bio */
 	if (!bio->bi_iter.bi_size)
 		goto out;
 
@@ -2566,7 +2588,7 @@ static void calc_vtime_cost_builtin(struct bio *bio, struct ioc_gq *iocg,
 	}
 	cost += pages * coef_page;
 out:
-	*costp = cost;
+	*costp = cost + flush_cost;
 }
 
 static u64 calc_vtime_cost(struct bio *bio, struct ioc_gq *iocg, bool is_merge)
@@ -2708,7 +2730,9 @@ static void ioc_rqos_throttle(struct rq_qos *rqos, struct bio *bio)
 	if (!iocg_activate(iocg, &now))
 		return;
 
-	iocg->cursor = bio_end_sector(bio);
+	/* dataless bios have no meaningful position for seq/rand detection */
+	if (bio->bi_iter.bi_size)
+		iocg->cursor = bio_end_sector(bio);
 	vtime = atomic64_read(&iocg->vtime);
 	cost = adjust_inuse_and_calc_cost(iocg, vtime, abs_cost, &now);
 
@@ -3440,10 +3464,12 @@ static u64 ioc_cost_model_prfill(struct seq_file *sf,
 	spin_lock_irq(&ioc->lock);
 	seq_printf(sf, "%s ctrl=%s model=linear "
 		   "rbps=%llu rseqiops=%llu rrandiops=%llu "
-		   "wbps=%llu wseqiops=%llu wrandiops=%llu\n",
+		   "wbps=%llu wseqiops=%llu wrandiops=%llu "
+		   "flushiops=%llu\n",
 		   dname, ioc->user_cost_model ? "user" : "auto",
 		   u[I_LCOEF_RBPS], u[I_LCOEF_RSEQIOPS], u[I_LCOEF_RRANDIOPS],
-		   u[I_LCOEF_WBPS], u[I_LCOEF_WSEQIOPS], u[I_LCOEF_WRANDIOPS]);
+		   u[I_LCOEF_WBPS], u[I_LCOEF_WSEQIOPS], u[I_LCOEF_WRANDIOPS],
+		   u[I_LCOEF_FLUSHIOPS]);
 	spin_unlock_irq(&ioc->lock);
 	return 0;
 }
@@ -3470,6 +3496,7 @@ static const match_table_t i_lcoef_tokens = {
 	{ I_LCOEF_WBPS,		"wbps=%u"	},
 	{ I_LCOEF_WSEQIOPS,	"wseqiops=%u"	},
 	{ I_LCOEF_WRANDIOPS,	"wrandiops=%u"	},
+	{ I_LCOEF_FLUSHIOPS,	"flushiops=%u"	},
 	{ NR_I_LCOEFS,		NULL		},
 };
 
-- 
2.43.0

[PATCH v3 2/4] blk-iocost: charge zone appends as writes

[Tao Cui]

From: Tao Cui <cuitao@kylinos.cn>

Zone append is a primary write operation for zoned devices; zoned xfs
and btrfs use it for data writes.  It is priced at zero, so the zone
append portion of zoned workloads runs outside the controller: a
1%-weight cgroup issued 16000 appends at zero cost on a zoned null_blk.

Treat REQ_OP_ZONE_APPEND the same as REQ_OP_WRITE so it goes through
the existing seq/rand classification and charging.

Fixes: 0512a75b98f8 ("block: Introduce REQ_OP_ZONE_APPEND")
Signed-off-by: Tao Cui <cuitao@kylinos.cn>
---
 block/blk-iocost.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/block/blk-iocost.c b/block/blk-iocost.c
index 0a0352554dde..7ce68e2c2e1a 100644
--- a/block/blk-iocost.c
+++ b/block/blk-iocost.c
@@ -2565,6 +2565,7 @@ static void calc_vtime_cost_builtin(struct bio *bio, struct ioc_gq *iocg,
 		coef_randio	= ioc->params.lcoefs[LCOEF_RRANDIO];
 		coef_page	= ioc->params.lcoefs[LCOEF_RPAGE];
 		break;
+	case REQ_OP_ZONE_APPEND:
 	case REQ_OP_WRITE:
 		coef_seqio	= ioc->params.lcoefs[LCOEF_WSEQIO];
 		coef_randio	= ioc->params.lcoefs[LCOEF_WRANDIO];
-- 
2.43.0

[PATCH v3 3/4] blk-iocost: account zone append completions in latency stats

[Tao Cui]

From: Tao Cui <cuitao@kylinos.cn>

ioc_rqos_done() only accounts READ and WRITE completions, so zone
append completions are excluded from the latency window: the vrate
feedback loop cannot see ZA-induced latency, leaving it unable to
respond to device saturation caused by zone appends.  Similarly,
calc_size_vtime_cost_builtin() does not classify zone append as a
write operation.

Charging (the cost model) and feedback (the latency window) are
separate mechanisms, so this is not covered by the previous patch
that prices zone appends.

Treat zone append as WRITE for both latency accounting and cost
classification.

Fixes: 0512a75b98f8 ("block: Introduce REQ_OP_ZONE_APPEND")
Signed-off-by: Tao Cui <cuitao@kylinos.cn>
Reviewed-by: Christoph Hellwig <hch@lst.de>
---
 block/blk-iocost.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/block/blk-iocost.c b/block/blk-iocost.c
index 7ce68e2c2e1a..ec10028249fb 100644
--- a/block/blk-iocost.c
+++ b/block/blk-iocost.c
@@ -2609,6 +2609,7 @@ static void calc_size_vtime_cost_builtin(struct request *rq, struct ioc *ioc,
 	case REQ_OP_READ:
 		*costp = pages * ioc->params.lcoefs[LCOEF_RPAGE];
 		break;
+	case REQ_OP_ZONE_APPEND:
 	case REQ_OP_WRITE:
 		*costp = pages * ioc->params.lcoefs[LCOEF_WPAGE];
 		break;
@@ -2879,6 +2880,7 @@ static void ioc_rqos_done(struct rq_qos *rqos, struct request *rq)
 		pidx = QOS_RLAT;
 		rw = READ;
 		break;
+	case REQ_OP_ZONE_APPEND:
 	case REQ_OP_WRITE:
 		pidx = QOS_WLAT;
 		rw = WRITE;
-- 
2.43.0

[PATCH v3 4/4] blk-iocost: fix stale comment in ioc_rqos_throttle()

[Tao Cui]

From: Tao Cui <cuitao@kylinos.cn>

The comment says that priority-inversion IOs are "punted to
@ioc->aux_iocg", but no aux_iocg field ever existed in struct ioc.
The comment was introduced already stale by commit da437b95db83
("blk-iocost: grab ioc->lock for debt handling").  Update it to
describe the current use_debt / iocg->abs_vdebt mechanism.

Signed-off-by: Tao Cui <cuitao@kylinos.cn>
Reviewed-by: Christoph Hellwig <hch@lst.de>
---
 block/blk-iocost.c | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/block/blk-iocost.c b/block/blk-iocost.c
index ec10028249fb..7475a4f79b97 100644
--- a/block/blk-iocost.c
+++ b/block/blk-iocost.c
@@ -2751,10 +2751,11 @@ static void ioc_rqos_throttle(struct rq_qos *rqos, struct bio *bio)
 
 	/*
 	 * We're over budget. This can be handled in two ways. IOs which may
-	 * cause priority inversions are punted to @ioc->aux_iocg and charged as
-	 * debt. Otherwise, the issuer is blocked on @iocg->waitq. Debt handling
-	 * requires @ioc->lock, waitq handling @iocg->waitq.lock. Determine
-	 * whether debt handling is needed and acquire locks accordingly.
+	 * cause priority inversions are issued regardless and charged against
+	 * @iocg->abs_vdebt as debt. Otherwise, the issuer is blocked on
+	 * @iocg->waitq. Debt handling requires @ioc->lock, waitq handling
+	 * @iocg->waitq.lock. Determine whether debt handling is needed and
+	 * acquire locks accordingly.
 	 */
 	use_debt = bio_issue_as_root_blkg(bio) || fatal_signal_pending(current);
 	ioc_locked = use_debt || READ_ONCE(iocg->abs_vdebt);
-- 
2.43.0