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

[PATCH v2 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.  On ext4, a write+fsync workload showed the same gap: the
write component was charged but the flush component was not.

The cause is that the builtin linear cost model defines coefficients
only for READ and WRITE.  Standalone flushes (the dataless
REQ_OP_WRITE | REQ_PREFLUSH bios from blkdev_issue_flush()), zone
append requests on zoned devices, and the flush component of
data-bearing REQ_OP_WRITE | REQ_PREFLUSH bios (e.g. journal commits)
fall through to a cost of zero.  Zone append completions are also
excluded from the latency statistics, so the vrate feedback loop
cannot respond to latency induced by zone append operations.

 1/4: charge the flush component of any REQ_PREFLUSH bio (standalone
     and pre-flush) as pageless random writes with one-page floor,
     and skip the cursor update for dataless bios
 2/4: charge zone appends as sequential writes; skip cursor update
     (ZA bi_sector is zone start, not the actual write position)
 3/4: count ZA completions in latency stats (vrate feedback)
 4/4: fix stale comment referring to nonexistent aux_iocg

After this series, on the same 1%-weight cgroup:

  - the fsync loop is limited to 24 flushes per 12s (matching the
    expected budget for the hdd profile)
  - 16000 zone appends on a zoned null_blk are charged 533264 usec
  - on ext4, the write+fsync workload is correctly accounted through
    the journal layer (~2.2us per flush, matching the ssd_fast
    profile's page-cost floor)
  - sequential read throughput is unchanged

Both v1 review findings (cursor corruption by dataless flushes,
missing charge for the PREFLUSH component of data-bearing writes)
are addressed in 1/4; see its changelog for details.  Proper
flush-cost modeling with a dedicated coefficient will follow as a
separate RFC.

Tao Cui (4):
  blk-iocost: charge flushes as pageless random writes
  blk-iocost: charge zone appends as page-counted sequential writes
  blk-iocost: account zone append completions in latency stats
  blk-iocost: fix stale comment in ioc_rqos_throttle()

 block/blk-iocost.c | 42 ++++++++++++++++++++++++++++++++-------
 1 file changed, 35 insertions(+), 7 deletions(-)

--
2.43.0

[PATCH v2 1/4] blk-iocost: charge flushes as pageless random writes

[Tao Cui]

From: Tao Cui <cuitao@kylinos.cn>

Standalone flushes issued by blkdev_issue_flush() are represented as
dataless REQ_OP_WRITE | REQ_PREFLUSH bios, which
calc_vtime_cost_builtin() prices at zero.  The flush component of
flush-heavy workloads such as database commits, journal flushes, and
metadata sync is thus neither charged nor throttled: a cgroup at 1% weight
can issue ~510k flushes per 12s, monopolizing the device while iocost
reports zero usage.

The same is true for the flush component of data-bearing
REQ_OP_WRITE | REQ_PREFLUSH bios, e.g. journal commit writes: they are
charged for their data only, and the cache flush the flush machine runs
ahead of it is free.

Charge the flush component of any REQ_PREFLUSH bio on top of its data
cost, priced as a pageless random write (LCOEF_WRANDIO), which provides
an approximation of the device time consumed by a flush.  For profiles
where WRANDIO clamps to zero (ssd_dfl / ssd_fast), use a one-page floor
(LCOEF_WPAGE).  A dataless flush bio falls out of the switch with zero
data cost and picks up the same surcharge, so standalone and pre-flush
forms are priced the same way.  After this patch, the same 1%-weight
cgroup is limited to 24 flushes per 12s; on ext4, write+fsync workloads
are correctly accounted through the journal layer (~2.2us per flush on
the ssd_fast profile).

A standalone flush must also not update iocg->cursor: its bi_sector
(usually 0) is not a data position, so setting the cursor from it
would misclassify the following READ/WRITE bios, and a zero cursor
defeats the !iocg->cursor sentinel in calc_vtime_cost_builtin().
Skip the cursor update for dataless bios.

Fixes: 7caa47151ab2 ("blkcg: implement blk-iocost")
Signed-off-by: Tao Cui <cuitao@kylinos.cn>
---
Changes in v2:
- Skip the iocg->cursor update for dataless flush bios, which would
  otherwise corrupt the seq/rand classification of the following IOs
  (reported in review of v1).
- Charge the flush component of data-bearing REQ_PREFLUSH bios too;
  v1 only priced standalone flushes (reported in review of v1).
---
 block/blk-iocost.c | 20 +++++++++++++++++---
 1 file changed, 17 insertions(+), 3 deletions(-)

diff --git a/block/blk-iocost.c b/block/blk-iocost.c
index 2745bffcd5eef..082f26d6e27b6 100644
--- a/block/blk-iocost.c
+++ b/block/blk-iocost.c
@@ -2532,8 +2532,20 @@ 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;
 
-	/* Can't calculate cost for empty bio */
+	/*
+	 * A WRITE|REQ_PREFLUSH bio carries a flush component: the flush
+	 * machine runs a cache flush for it, either standalone (dataless)
+	 * or ahead of the data.  Charge the flush on top of the data cost,
+	 * priced as a pageless random write with a one-page floor so fast
+	 * profiles still charge something.  Flush bios are never merged.
+	 */
+	if (!is_merge && (bio->bi_opf & REQ_PREFLUSH))
+		flush_cost = max(ioc->params.lcoefs[LCOEF_WRANDIO],
+				 ioc->params.lcoefs[LCOEF_WPAGE]);
+
+	/* Can't calculate data cost for empty bio */
 	if (!bio->bi_iter.bi_size)
 		goto out;
 
@@ -2566,7 +2578,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 +2720,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);
 
-- 
2.43.0

[PATCH v2 2/4] blk-iocost: charge zone appends as page-counted sequential writes

[Tao Cui]

From: Tao Cui <cuitao@kylinos.cn>

Zone append is a primary write operation for zoned devices; zoned
btrfs and f2fs 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.

A zone append advances the zone write pointer and is therefore
sequential from the device's perspective; the actual sector is only
returned after completion, so the cursor-based seq/rand
classification doesn't apply.  Price it as a page-counted sequential
write.  After this patch, 16000 appends from the same cgroup are
charged 533264 usec (33us per append).

Also skip the cursor update for ZA bios: bi_sector is the zone
start, not the actual write position (which is only returned after
completion).  Setting the cursor from ZA would misclassify subsequent
READ/WRITE bios.

Fixes: 7caa47151ab2 ("blkcg: implement blk-iocost")
Signed-off-by: Tao Cui <cuitao@kylinos.cn>
---
 block/blk-iocost.c | 15 +++++++++++++--
 1 file changed, 13 insertions(+), 2 deletions(-)

diff --git a/block/blk-iocost.c b/block/blk-iocost.c
index 082f26d6e27b6..2e4e9ce2c9359 100644
--- a/block/blk-iocost.c
+++ b/block/blk-iocost.c
@@ -2560,6 +2560,17 @@ static void calc_vtime_cost_builtin(struct bio *bio, struct ioc_gq *iocg,
 		coef_randio	= ioc->params.lcoefs[LCOEF_WRANDIO];
 		coef_page	= ioc->params.lcoefs[LCOEF_WPAGE];
 		break;
+	case REQ_OP_ZONE_APPEND:
+		/*
+		 * A zone append advances the zone write pointer and is
+		 * therefore sequential from the device's perspective, so
+		 * the cursor-based classification below doesn't apply.
+		 * Compute the full cost here.
+		 */
+		if (!is_merge)
+			cost += ioc->params.lcoefs[LCOEF_WSEQIO];
+		cost += pages * ioc->params.lcoefs[LCOEF_WPAGE];
+		goto out;
 	default:
 		goto out;
 	}
@@ -2720,8 +2731,8 @@ static void ioc_rqos_throttle(struct rq_qos *rqos, struct bio *bio)
 	if (!iocg_activate(iocg, &now))
 		return;
 
-	/* dataless bios have no meaningful position for seq/rand detection */
-	if (bio->bi_iter.bi_size)
+	/* ZA bi_sector is zone start, dataless bios have no write position */
+	if (bio->bi_iter.bi_size && bio_op(bio) != REQ_OP_ZONE_APPEND)
 		iocg->cursor = bio_end_sector(bio);
 	vtime = atomic64_read(&iocg->vtime);
 	cost = adjust_inuse_and_calc_cost(iocg, vtime, abs_cost, &now);
-- 
2.43.0

[PATCH v2 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.

This is not covered by the patch that prices zone appends: charging
goes through the cost model, while the latency window is the feedback
path the vrate controller watches.

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

Fixes: 7caa47151ab2 ("blkcg: implement blk-iocost")
Signed-off-by: Tao Cui <cuitao@kylinos.cn>
---
 block/blk-iocost.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/block/blk-iocost.c b/block/blk-iocost.c
index 2e4e9ce2c9359..ef1b7e65455e2 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 v2 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>
---
 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 ef1b7e65455e2..0a0821a080043 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