The following changes since commit f5fe7c17ac4e309e47e78f0f9761aebc8d2f2c81:

Merge tag 'pull-tcg-20230823-2' of https://gitlab.com/rth7680/qemu into staging (2023-08-28 16:07:04 -0400)

are available in the Git repository at:

https://gitlab.com/hreitz/qemu.git tags/pull-block-2023-09-01

for you to fetch changes up to 380448464dd89291cf7fd7434be6c225482a334d:

tests/file-io-error: New test (2023-08-29 13:01:24 +0200)

Zhenwei Pi (9):

throttle: introduce enum ThrottleDirection

test-throttle: use enum ThrottleDirection

throttle: support read-only and write-only

test-throttle: test read only and write only

cryptodev: use NULL throttle timer cb for read direction

throttle: use enum ThrottleDirection instead of bool is_write

throttle: use THROTTLE_MAX/ARRAY_SIZE for hard code

fsdev: Use ThrottleDirection instread of bool is_write

block/throttle-groups: Use ThrottleDirection instread of bool is_write

fsdev/qemu-fsdev-throttle.h | 4 +-

include/block/throttle-groups.h | 6 +-

include/qemu/throttle.h | 16 +-

backends/cryptodev.c | 12 +-

block/block-backend.c | 4 +-

block/file-posix.c | 42 +++---

block/throttle-groups.c | 163 +++++++++++----------

block/throttle.c | 8 +-

fsdev/qemu-fsdev-throttle.c | 18 ++-

hw/9pfs/cofile.c | 4 +-

tests/unit/test-throttle.c | 76 +++++++++-

util/throttle.c | 84 +++++++----

tests/qemu-iotests/tests/file-io-error | 119 +++++++++++++++

tests/qemu-iotests/tests/file-io-error.out | 33 +++++

14 files changed, 418 insertions(+), 171 deletions(-)

create mode 100755 tests/qemu-iotests/tests/file-io-error

create mode 100644 tests/qemu-iotests/tests/file-io-error.out

2.41.0

From: zhenwei pi <pizhenwei@bytedance.com>

Use enum ThrottleDirection instead of number index.

Reviewed-by: Alberto Garcia <berto@igalia.com>

Reviewed-by: Hanna Czenczek <hreitz@redhat.com>

Signed-off-by: zhenwei pi <pizhenwei@bytedance.com>

Message-Id: <20230728022006.1098509-2-pizhenwei@bytedance.com>

Signed-off-by: Hanna Czenczek <hreitz@redhat.com>

include/qemu/throttle.h | 11 ++++++++---

util/throttle.c | 16 +++++++++-------

2 files changed, 17 insertions(+), 10 deletions(-)

@@ -XXX,XX +XXX,XX @@ typedef struct ThrottleState {

int64_t previous_leak; /* timestamp of the last leak done */

} ThrottleState;

+typedef enum {

+ THROTTLE_READ = 0,

+ THROTTLE_WRITE,

+ THROTTLE_MAX

+} ThrottleDirection;

+

typedef struct ThrottleTimers {

- QEMUTimer *timers[2]; /* timers used to do the throttling */

+ QEMUTimer *timers[THROTTLE_MAX]; /* timers used to do the throttling */

QEMUClockType clock_type; /* the clock used */

/* Callbacks */

- QEMUTimerCB *read_timer_cb;

- QEMUTimerCB *write_timer_cb;

+ QEMUTimerCB *timer_cb[THROTTLE_MAX];

void *timer_opaque;

} ThrottleTimers;

@@ -XXX,XX +XXX,XX @@ static bool throttle_compute_timer(ThrottleState *ts,

void throttle_timers_attach_aio_context(ThrottleTimers *tt,

AioContext *new_context)

{

- tt->timers[0] = aio_timer_new(new_context, tt->clock_type, SCALE_NS,

- tt->read_timer_cb, tt->timer_opaque);

- tt->timers[1] = aio_timer_new(new_context, tt->clock_type, SCALE_NS,

- tt->write_timer_cb, tt->timer_opaque);

+ tt->timers[THROTTLE_READ] =

+ aio_timer_new(new_context, tt->clock_type, SCALE_NS,

+ tt->timer_cb[THROTTLE_READ], tt->timer_opaque);

+ tt->timers[THROTTLE_WRITE] =

+ aio_timer_new(new_context, tt->clock_type, SCALE_NS,

+ tt->timer_cb[THROTTLE_WRITE], tt->timer_opaque);

}

/*

@@ -XXX,XX +XXX,XX @@ void throttle_timers_init(ThrottleTimers *tt,

memset(tt, 0, sizeof(ThrottleTimers));

tt->clock_type = clock_type;

- tt->read_timer_cb = read_timer_cb;

- tt->write_timer_cb = write_timer_cb;

+ tt->timer_cb[THROTTLE_READ] = read_timer_cb;

+ tt->timer_cb[THROTTLE_WRITE] = write_timer_cb;

tt->timer_opaque = timer_opaque;

throttle_timers_attach_aio_context(tt, aio_context);

}

@@ -XXX,XX +XXX,XX @@ void throttle_timers_detach_aio_context(ThrottleTimers *tt)

{

int i;

- for (i = 0; i < 2; i++) {

+ for (i = 0; i < THROTTLE_MAX; i++) {

throttle_timer_destroy(&tt->timers[i]);

}

}

2.41.0

From: zhenwei pi <pizhenwei@bytedance.com>

Use enum ThrottleDirection instead in the throttle test codes.

Reviewed-by: Alberto Garcia <berto@igalia.com>

Reviewed-by: Hanna Czenczek <hreitz@redhat.com>

Signed-off-by: zhenwei pi <pizhenwei@bytedance.com>

Message-Id: <20230728022006.1098509-3-pizhenwei@bytedance.com>

Signed-off-by: Hanna Czenczek <hreitz@redhat.com>

tests/unit/test-throttle.c | 6 +++---

1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/tests/unit/test-throttle.c b/tests/unit/test-throttle.c

--- a/tests/unit/test-throttle.c

+++ b/tests/unit/test-throttle.c

@@ -XXX,XX +XXX,XX @@ static void test_init(void)

/* check initialized fields */

g_assert(tt->clock_type == QEMU_CLOCK_VIRTUAL);

- g_assert(tt->timers[0]);

- g_assert(tt->timers[1]);

+ g_assert(tt->timers[THROTTLE_READ]);

+ g_assert(tt->timers[THROTTLE_WRITE]);

/* check other fields where cleared */

g_assert(!ts.previous_leak);

@@ -XXX,XX +XXX,XX @@ static void test_destroy(void)

throttle_timers_init(tt, ctx, QEMU_CLOCK_VIRTUAL,

read_timer_cb, write_timer_cb, &ts);

throttle_timers_destroy(tt);

- for (i = 0; i < 2; i++) {

+ for (i = 0; i < THROTTLE_MAX; i++) {

g_assert(!tt->timers[i]);

}

}

2.41.0

From: zhenwei pi <pizhenwei@bytedance.com>

Only one direction is necessary in several scenarios:

- a read-only disk

- operations on a device are considered as *write* only. For example,

encrypt/decrypt/sign/verify operations on a cryptodev use a single

*write* timer(read timer callback is defined, but never invoked).

Allow a single direction in throttle, this reduces memory, and uplayer

does not need a dummy callback any more.

Reviewed-by: Alberto Garcia <berto@igalia.com>

Reviewed-by: Hanna Czenczek <hreitz@redhat.com>

Signed-off-by: zhenwei pi <pizhenwei@bytedance.com>

Message-Id: <20230728022006.1098509-4-pizhenwei@bytedance.com>

Signed-off-by: Hanna Czenczek <hreitz@redhat.com>

util/throttle.c | 42 ++++++++++++++++++++++++++++--------------

1 file changed, 28 insertions(+), 14 deletions(-)

@@ -XXX,XX +XXX,XX @@ static bool throttle_compute_timer(ThrottleState *ts,

void throttle_timers_attach_aio_context(ThrottleTimers *tt,

AioContext *new_context)

- tt->timers[THROTTLE_READ] =

- aio_timer_new(new_context, tt->clock_type, SCALE_NS,

- tt->timer_cb[THROTTLE_READ], tt->timer_opaque);

- tt->timers[THROTTLE_WRITE] =

- aio_timer_new(new_context, tt->clock_type, SCALE_NS,

- tt->timer_cb[THROTTLE_WRITE], tt->timer_opaque);

+ ThrottleDirection dir;

+

+ for (dir = THROTTLE_READ; dir < THROTTLE_MAX; dir++) {

+ if (tt->timer_cb[dir]) {

+ tt->timers[dir] =

+ aio_timer_new(new_context, tt->clock_type, SCALE_NS,

+ tt->timer_cb[dir], tt->timer_opaque);

+ }

+ }

}

/*

@@ -XXX,XX +XXX,XX @@ void throttle_timers_init(ThrottleTimers *tt,

QEMUTimerCB *write_timer_cb,

void *timer_opaque)

{

+ assert(read_timer_cb || write_timer_cb);

memset(tt, 0, sizeof(ThrottleTimers));

tt->clock_type = clock_type;

@@ -XXX,XX +XXX,XX @@ void throttle_timers_init(ThrottleTimers *tt,

/* destroy a timer */

static void throttle_timer_destroy(QEMUTimer **timer)

{

- assert(*timer != NULL);

+ if (*timer == NULL) {

+ return;

+ }

timer_free(*timer);

*timer = NULL;

@@ -XXX,XX +XXX,XX @@ static void throttle_timer_destroy(QEMUTimer **timer)

/* Remove timers from event loop */

void throttle_timers_detach_aio_context(ThrottleTimers *tt)

{

- int i;

+ ThrottleDirection dir;

- for (i = 0; i < THROTTLE_MAX; i++) {

- throttle_timer_destroy(&tt->timers[i]);

+ for (dir = THROTTLE_READ; dir < THROTTLE_MAX; dir++) {

+ throttle_timer_destroy(&tt->timers[dir]);

}

@@ -XXX,XX +XXX,XX @@ void throttle_timers_destroy(ThrottleTimers *tt)

/* is any throttling timer configured */

bool throttle_timers_are_initialized(ThrottleTimers *tt)

{

- if (tt->timers[0]) {

- return true;

+ ThrottleDirection dir;

+ for (dir = THROTTLE_READ; dir < THROTTLE_MAX; dir++) {

+ if (tt->timers[dir]) {

+ return true;

+ }

return false;

@@ -XXX,XX +XXX,XX @@ bool throttle_schedule_timer(ThrottleState *ts,

{

int64_t now = qemu_clock_get_ns(tt->clock_type);

int64_t next_timestamp;

+ QEMUTimer *timer;

bool must_wait;

+ timer = is_write ? tt->timers[THROTTLE_WRITE] : tt->timers[THROTTLE_READ];

+ assert(timer);

+

must_wait = throttle_compute_timer(ts,

is_write,

now,

@@ -XXX,XX +XXX,XX @@ bool throttle_schedule_timer(ThrottleState *ts,

}

/* request throttled and timer pending -> do nothing */

- if (timer_pending(tt->timers[is_write])) {

+ if (timer_pending(timer)) {

return true;

}

/* request throttled and timer not pending -> arm timer */

- timer_mod(tt->timers[is_write], next_timestamp);

+ timer_mod(timer, next_timestamp);

2.41.0

From: zhenwei pi <pizhenwei@bytedance.com>

Reviewed-by: Alberto Garcia <berto@igalia.com>

Reviewed-by: Hanna Czenczek <hreitz@redhat.com>

Signed-off-by: zhenwei pi <pizhenwei@bytedance.com>

Message-Id: <20230728022006.1098509-5-pizhenwei@bytedance.com>

Signed-off-by: Hanna Czenczek <hreitz@redhat.com>

tests/unit/test-throttle.c | 66 ++++++++++++++++++++++++++++++++++++++

1 file changed, 66 insertions(+)

diff --git a/tests/unit/test-throttle.c b/tests/unit/test-throttle.c

--- a/tests/unit/test-throttle.c

+++ b/tests/unit/test-throttle.c

@@ -XXX,XX +XXX,XX @@ static void test_init(void)

throttle_timers_destroy(tt);

+static void test_init_readonly(void)

+ g_assert(!ts.cfg.buckets[i].avg);

+ g_assert(!ts.cfg.buckets[i].max);

+ g_assert(!ts.cfg.buckets[i].level);

+ }

+ throttle_timers_destroy(tt);

+static void test_init_writeonly(void)

+{

+ int i;

+

+ tt = &tgm.throttle_timers;

+

+ /* fill the structures with crap */

+ memset(&ts, 1, sizeof(ts));

+ memset(tt, 1, sizeof(*tt));

+

+ /* init structures */

+ throttle_init(&ts);

+ throttle_timers_init(tt, ctx, QEMU_CLOCK_VIRTUAL,

+ NULL, write_timer_cb, &ts);

+

+ /* check initialized fields */

+ g_assert(tt->clock_type == QEMU_CLOCK_VIRTUAL);

+ g_assert(!tt->timers[THROTTLE_READ]);

+ g_assert(tt->timers[THROTTLE_WRITE]);

+

+ /* check other fields where cleared */

+ g_assert(!ts.previous_leak);

+ g_assert(!ts.cfg.op_size);

+ for (i = 0; i < BUCKETS_COUNT; i++) {

+ g_assert(!ts.cfg.buckets[i].avg);

+ g_assert(!ts.cfg.buckets[i].max);

+ g_assert(!ts.cfg.buckets[i].level);

+ }

+

+ throttle_timers_destroy(tt);

+}

+

static void test_destroy(void)

g_test_add_func("/throttle/leak_bucket", test_leak_bucket);

g_test_add_func("/throttle/compute_wait", test_compute_wait);

g_test_add_func("/throttle/init", test_init);

+ g_test_add_func("/throttle/init_readonly", test_init_readonly);

+ g_test_add_func("/throttle/init_writeonly", test_init_writeonly);

g_test_add_func("/throttle/destroy", test_destroy);

g_test_add_func("/throttle/have_timer", test_have_timer);

g_test_add_func("/throttle/detach_attach", test_detach_attach);

2.41.0

From: zhenwei pi <pizhenwei@bytedance.com>

Operations on a cryptodev are considered as *write* only, the callback

of read direction is never invoked. Use NULL instead of an unreachable

path(cryptodev_backend_throttle_timer_cb on read direction).

The dummy read timer(never invoked) is already removed here, it means

that the 'FIXME' tag is no longer needed.

Reviewed-by: Alberto Garcia <berto@igalia.com>

Reviewed-by: Hanna Czenczek <hreitz@redhat.com>

Signed-off-by: zhenwei pi <pizhenwei@bytedance.com>

Message-Id: <20230728022006.1098509-6-pizhenwei@bytedance.com>

Signed-off-by: Hanna Czenczek <hreitz@redhat.com>

---

backends/cryptodev.c | 3 +--

1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/backends/cryptodev.c b/backends/cryptodev.c

--- a/backends/cryptodev.c

+++ b/backends/cryptodev.c

@@ -XXX,XX +XXX,XX @@ static void cryptodev_backend_set_throttle(CryptoDevBackend *backend, int field,

if (!enabled) {

throttle_init(&backend->ts);

throttle_timers_init(&backend->tt, qemu_get_aio_context(),

- QEMU_CLOCK_REALTIME,

- cryptodev_backend_throttle_timer_cb, /* FIXME */

+ QEMU_CLOCK_REALTIME, NULL,

cryptodev_backend_throttle_timer_cb, backend);

2.41.0

From: zhenwei pi <pizhenwei@bytedance.com>

enum ThrottleDirection is already there, use ThrottleDirection instead

of 'bool is_write' for throttle API, also modify related codes from

block, fsdev, cryptodev and tests.

Reviewed-by: Hanna Czenczek <hreitz@redhat.com>

Signed-off-by: zhenwei pi <pizhenwei@bytedance.com>

Message-Id: <20230728022006.1098509-7-pizhenwei@bytedance.com>

Signed-off-by: Hanna Czenczek <hreitz@redhat.com>

include/qemu/throttle.h | 5 +++--

backends/cryptodev.c | 9 +++++----

block/throttle-groups.c | 6 ++++--

fsdev/qemu-fsdev-throttle.c | 8 +++++---

tests/unit/test-throttle.c | 4 ++--

util/throttle.c | 31 +++++++++++++++++--------------

6 files changed, 36 insertions(+), 27 deletions(-)

@@ -XXX,XX +XXX,XX @@ void throttle_config_init(ThrottleConfig *cfg);

/* usage */

bool throttle_schedule_timer(ThrottleState *ts,

ThrottleTimers *tt,

- bool is_write);

+ ThrottleDirection direction);

-void throttle_account(ThrottleState *ts, bool is_write, uint64_t size);

+void throttle_account(ThrottleState *ts, ThrottleDirection direction,

+ uint64_t size);

void throttle_limits_to_config(ThrottleLimits *arg, ThrottleConfig *cfg,

Error **errp);

void throttle_config_to_limits(ThrottleConfig *cfg, ThrottleLimits *var);

diff --git a/backends/cryptodev.c b/backends/cryptodev.c

index XXXXXXX..XXXXXXX 100644

--- a/backends/cryptodev.c

+++ b/backends/cryptodev.c

@@ -XXX,XX +XXX,XX @@ static void cryptodev_backend_throttle_timer_cb(void *opaque)

continue;

}

- throttle_account(&backend->ts, true, ret);

+ throttle_account(&backend->ts, THROTTLE_WRITE, ret);

cryptodev_backend_operation(backend, op_info);

if (throttle_enabled(&backend->tc) &&

- throttle_schedule_timer(&backend->ts, &backend->tt, true)) {

+ throttle_schedule_timer(&backend->ts, &backend->tt,

+ THROTTLE_WRITE)) {

break;

}

}

@@ -XXX,XX +XXX,XX @@ int cryptodev_backend_crypto_operation(

goto do_account;

}

- if (throttle_schedule_timer(&backend->ts, &backend->tt, true) ||

+ if (throttle_schedule_timer(&backend->ts, &backend->tt, THROTTLE_WRITE) ||

!QTAILQ_EMPTY(&backend->opinfos)) {

QTAILQ_INSERT_TAIL(&backend->opinfos, op_info, next);

return 0;

@@ -XXX,XX +XXX,XX @@ do_account:

return ret;

}

- throttle_account(&backend->ts, true, ret);

+ throttle_account(&backend->ts, THROTTLE_WRITE, ret);

return cryptodev_backend_operation(backend, op_info);

diff --git a/block/throttle-groups.c b/block/throttle-groups.c

index XXXXXXX..XXXXXXX 100644

--- a/block/throttle-groups.c

+++ b/block/throttle-groups.c

@@ -XXX,XX +XXX,XX @@ static bool throttle_group_schedule_timer(ThrottleGroupMember *tgm,

ThrottleState *ts = tgm->throttle_state;

ThrottleGroup *tg = container_of(ts, ThrottleGroup, ts);

ThrottleTimers *tt = &tgm->throttle_timers;

+ ThrottleDirection direction = is_write ? THROTTLE_WRITE : THROTTLE_READ;

bool must_wait;

if (qatomic_read(&tgm->io_limits_disabled)) {

@@ -XXX,XX +XXX,XX @@ static bool throttle_group_schedule_timer(ThrottleGroupMember *tgm,

return true;

}

- must_wait = throttle_schedule_timer(ts, tt, is_write);

+ must_wait = throttle_schedule_timer(ts, tt, direction);

/* If a timer just got armed, set tgm as the current token */

if (must_wait) {

@@ -XXX,XX +XXX,XX @@ void coroutine_fn throttle_group_co_io_limits_intercept(ThrottleGroupMember *tgm

bool must_wait;

ThrottleGroupMember *token;

ThrottleGroup *tg = container_of(tgm->throttle_state, ThrottleGroup, ts);

+ ThrottleDirection direction = is_write ? THROTTLE_WRITE : THROTTLE_READ;

assert(bytes >= 0);

@@ -XXX,XX +XXX,XX @@ void coroutine_fn throttle_group_co_io_limits_intercept(ThrottleGroupMember *tgm

}

/* The I/O will be executed, so do the accounting */

- throttle_account(tgm->throttle_state, is_write, bytes);

+ throttle_account(tgm->throttle_state, direction, bytes);

/* Schedule the next request */

schedule_next_request(tgm, is_write);

diff --git a/fsdev/qemu-fsdev-throttle.c b/fsdev/qemu-fsdev-throttle.c

index XXXXXXX..XXXXXXX 100644

--- a/fsdev/qemu-fsdev-throttle.c

+++ b/fsdev/qemu-fsdev-throttle.c

@@ -XXX,XX +XXX,XX @@ void fsdev_throttle_init(FsThrottle *fst)

void coroutine_fn fsdev_co_throttle_request(FsThrottle *fst, bool is_write,

struct iovec *iov, int iovcnt)

{

+ ThrottleDirection direction = is_write ? THROTTLE_WRITE : THROTTLE_READ;

+

if (throttle_enabled(&fst->cfg)) {

- if (throttle_schedule_timer(&fst->ts, &fst->tt, is_write) ||

+ if (throttle_schedule_timer(&fst->ts, &fst->tt, direction) ||

!qemu_co_queue_empty(&fst->throttled_reqs[is_write])) {

qemu_co_queue_wait(&fst->throttled_reqs[is_write], NULL);

}

- throttle_account(&fst->ts, is_write, iov_size(iov, iovcnt));

+ throttle_account(&fst->ts, direction, iov_size(iov, iovcnt));

if (!qemu_co_queue_empty(&fst->throttled_reqs[is_write]) &&

- !throttle_schedule_timer(&fst->ts, &fst->tt, is_write)) {

+ !throttle_schedule_timer(&fst->ts, &fst->tt, direction)) {

qemu_co_queue_next(&fst->throttled_reqs[is_write]);

}

}

diff --git a/tests/unit/test-throttle.c b/tests/unit/test-throttle.c

index XXXXXXX..XXXXXXX 100644

--- a/tests/unit/test-throttle.c

+++ b/tests/unit/test-throttle.c

@@ -XXX,XX +XXX,XX @@ static bool do_test_accounting(bool is_ops, /* are we testing bps or ops */

throttle_config(&ts, QEMU_CLOCK_VIRTUAL, &cfg);

/* account a read */

- throttle_account(&ts, false, size);

+ throttle_account(&ts, THROTTLE_READ, size);

/* account a write */

- throttle_account(&ts, true, size);

+ throttle_account(&ts, THROTTLE_WRITE, size);

/* check total result */

index = to_test[is_ops][0];

/* This function compute the time that must be waited while this IO

*

- * @is_write: true if the current IO is a write, false if it's a read

+ * @direction: throttle direction

* @ret: time to wait

*/

static int64_t throttle_compute_wait_for(ThrottleState *ts,

- bool is_write)

+ ThrottleDirection direction)

{

BucketType to_check[2][4] = { {THROTTLE_BPS_TOTAL,

THROTTLE_OPS_TOTAL,

@@ -XXX,XX +XXX,XX @@ static int64_t throttle_compute_wait_for(ThrottleState *ts,

int i;

for (i = 0; i < 4; i++) {

- BucketType index = to_check[is_write][i];

+ BucketType index = to_check[direction][i];

wait = throttle_compute_wait(&ts->cfg.buckets[index]);

if (wait > max_wait) {

max_wait = wait;

@@ -XXX,XX +XXX,XX @@ static int64_t throttle_compute_wait_for(ThrottleState *ts,

/* compute the timer for this type of operation

*

- * @is_write: the type of operation

+ * @direction: throttle direction

* @now: the current clock timestamp

* @next_timestamp: the resulting timer

* @ret: true if a timer must be set

*/

static bool throttle_compute_timer(ThrottleState *ts,

- bool is_write,

+ ThrottleDirection direction,

int64_t now,

int64_t *next_timestamp)

{

@@ -XXX,XX +XXX,XX @@ static bool throttle_compute_timer(ThrottleState *ts,

throttle_do_leak(ts, now);

/* compute the wait time if any */

- wait = throttle_compute_wait_for(ts, is_write);

+ wait = throttle_compute_wait_for(ts, direction);

/* if the code must wait compute when the next timer should fire */

if (wait) {

@@ -XXX,XX +XXX,XX @@ void throttle_get_config(ThrottleState *ts, ThrottleConfig *cfg)

* NOTE: this function is not unit tested due to it's usage of timer_mod

*

* @tt: the timers structure

- * @is_write: the type of operation (read/write)

+ * @direction: throttle direction

* @ret: true if the timer has been scheduled else false

*/

bool throttle_schedule_timer(ThrottleState *ts,

ThrottleTimers *tt,

- bool is_write)

+ ThrottleDirection direction)

{

int64_t now = qemu_clock_get_ns(tt->clock_type);

int64_t next_timestamp;

QEMUTimer *timer;

bool must_wait;

- timer = is_write ? tt->timers[THROTTLE_WRITE] : tt->timers[THROTTLE_READ];

+ assert(direction < THROTTLE_MAX);

+ timer = tt->timers[direction];

assert(timer);

must_wait = throttle_compute_timer(ts,

- is_write,

+ direction,

now,

&next_timestamp);

@@ -XXX,XX +XXX,XX @@ bool throttle_schedule_timer(ThrottleState *ts,

/* do the accounting for this operation

*

- * @is_write: the type of operation (read/write)

+ * @direction: throttle direction

* @size: the size of the operation

*/

-void throttle_account(ThrottleState *ts, bool is_write, uint64_t size)

+void throttle_account(ThrottleState *ts, ThrottleDirection direction,

+ uint64_t size)

{

const BucketType bucket_types_size[2][2] = {

{ THROTTLE_BPS_TOTAL, THROTTLE_BPS_READ },

@@ -XXX,XX +XXX,XX @@ void throttle_account(ThrottleState *ts, bool is_write, uint64_t size)

double units = 1.0;

unsigned i;

+ assert(direction < THROTTLE_MAX);

/* if cfg.op_size is defined and smaller than size we compute unit count */

if (ts->cfg.op_size && size > ts->cfg.op_size) {

units = (double) size / ts->cfg.op_size;

@@ -XXX,XX +XXX,XX @@ void throttle_account(ThrottleState *ts, bool is_write, uint64_t size)

for (i = 0; i < 2; i++) {

LeakyBucket *bkt;

- bkt = &ts->cfg.buckets[bucket_types_size[is_write][i]];

+ bkt = &ts->cfg.buckets[bucket_types_size[direction][i]];

bkt->level += size;

if (bkt->burst_length > 1) {

bkt->burst_level += size;

}

- bkt = &ts->cfg.buckets[bucket_types_units[is_write][i]];

+ bkt = &ts->cfg.buckets[bucket_types_units[direction][i]];

bkt->level += units;

if (bkt->burst_length > 1) {

bkt->burst_level += units;

2.41.0

From: zhenwei pi <pizhenwei@bytedance.com>

The first dimension of both to_check and

bucket_types_size/bucket_types_units is used as throttle direction,

use THROTTLE_MAX instead of hard coded number. Also use ARRAY_SIZE()

to avoid hard coded number for the second dimension.

Hanna noticed that the two array should be static. Yes, turn them

into static variables.

Reviewed-by: Hanna Czenczek <hreitz@redhat.com>

Signed-off-by: zhenwei pi <pizhenwei@bytedance.com>

Message-Id: <20230728022006.1098509-8-pizhenwei@bytedance.com>

Signed-off-by: Hanna Czenczek <hreitz@redhat.com>

util/throttle.c | 11 ++++++-----

1 file changed, 6 insertions(+), 5 deletions(-)

@@ -XXX,XX +XXX,XX @@ int64_t throttle_compute_wait(LeakyBucket *bkt)

static int64_t throttle_compute_wait_for(ThrottleState *ts,

ThrottleDirection direction)

{

- BucketType to_check[2][4] = { {THROTTLE_BPS_TOTAL,

+ static const BucketType to_check[THROTTLE_MAX][4] = {

+ {THROTTLE_BPS_TOTAL,

THROTTLE_OPS_TOTAL,

THROTTLE_BPS_READ,

THROTTLE_OPS_READ},

@@ -XXX,XX +XXX,XX @@ static int64_t throttle_compute_wait_for(ThrottleState *ts,

int64_t wait, max_wait = 0;

int i;

- for (i = 0; i < 4; i++) {

+ for (i = 0; i < ARRAY_SIZE(to_check[THROTTLE_READ]); i++) {

BucketType index = to_check[direction][i];

wait = throttle_compute_wait(&ts->cfg.buckets[index]);

if (wait > max_wait) {

@@ -XXX,XX +XXX,XX @@ bool throttle_schedule_timer(ThrottleState *ts,

void throttle_account(ThrottleState *ts, ThrottleDirection direction,

uint64_t size)

{

- const BucketType bucket_types_size[2][2] = {

+ static const BucketType bucket_types_size[THROTTLE_MAX][2] = {

{ THROTTLE_BPS_TOTAL, THROTTLE_BPS_READ },

{ THROTTLE_BPS_TOTAL, THROTTLE_BPS_WRITE }

};

- const BucketType bucket_types_units[2][2] = {

+ static const BucketType bucket_types_units[THROTTLE_MAX][2] = {

{ THROTTLE_OPS_TOTAL, THROTTLE_OPS_READ },

{ THROTTLE_OPS_TOTAL, THROTTLE_OPS_WRITE }

};

@@ -XXX,XX +XXX,XX @@ void throttle_account(ThrottleState *ts, ThrottleDirection direction,

units = (double) size / ts->cfg.op_size;

- for (i = 0; i < 2; i++) {

+ for (i = 0; i < ARRAY_SIZE(bucket_types_size[THROTTLE_READ]); i++) {

LeakyBucket *bkt;

bkt = &ts->cfg.buckets[bucket_types_size[direction][i]];

2.41.0

From: zhenwei pi <pizhenwei@bytedance.com>

'bool is_write' style is obsolete from throttle framework, adapt

fsdev to the new style.

Cc: Greg Kurz <groug@kaod.org>

Reviewed-by: Hanna Czenczek <hreitz@redhat.com>

Signed-off-by: zhenwei pi <pizhenwei@bytedance.com>

Message-Id: <20230728022006.1098509-9-pizhenwei@bytedance.com>

Reviewed-by: Greg Kurz <groug@kaod.org>

Signed-off-by: Hanna Czenczek <hreitz@redhat.com>

fsdev/qemu-fsdev-throttle.h | 4 ++--

fsdev/qemu-fsdev-throttle.c | 14 +++++++-------

hw/9pfs/cofile.c | 4 ++--

3 files changed, 11 insertions(+), 11 deletions(-)

diff --git a/fsdev/qemu-fsdev-throttle.h b/fsdev/qemu-fsdev-throttle.h

--- a/fsdev/qemu-fsdev-throttle.h

+++ b/fsdev/qemu-fsdev-throttle.h

@@ -XXX,XX +XXX,XX @@ typedef struct FsThrottle {

ThrottleState ts;

ThrottleTimers tt;

ThrottleConfig cfg;

- CoQueue throttled_reqs[2];

+ CoQueue throttled_reqs[THROTTLE_MAX];

} FsThrottle;

int fsdev_throttle_parse_opts(QemuOpts *, FsThrottle *, Error **);

void fsdev_throttle_init(FsThrottle *);

-void coroutine_fn fsdev_co_throttle_request(FsThrottle *, bool ,

+void coroutine_fn fsdev_co_throttle_request(FsThrottle *, ThrottleDirection ,

struct iovec *, int);

void fsdev_throttle_cleanup(FsThrottle *);

diff --git a/fsdev/qemu-fsdev-throttle.c b/fsdev/qemu-fsdev-throttle.c

index XXXXXXX..XXXXXXX 100644

--- a/fsdev/qemu-fsdev-throttle.c

+++ b/fsdev/qemu-fsdev-throttle.c

@@ -XXX,XX +XXX,XX @@ void fsdev_throttle_init(FsThrottle *fst)

}

}

-void coroutine_fn fsdev_co_throttle_request(FsThrottle *fst, bool is_write,

+void coroutine_fn fsdev_co_throttle_request(FsThrottle *fst,

+ ThrottleDirection direction,

struct iovec *iov, int iovcnt)

{

- ThrottleDirection direction = is_write ? THROTTLE_WRITE : THROTTLE_READ;

-

+ assert(direction < THROTTLE_MAX);

if (throttle_enabled(&fst->cfg)) {

if (throttle_schedule_timer(&fst->ts, &fst->tt, direction) ||

- !qemu_co_queue_empty(&fst->throttled_reqs[is_write])) {

- qemu_co_queue_wait(&fst->throttled_reqs[is_write], NULL);

+ !qemu_co_queue_empty(&fst->throttled_reqs[direction])) {

+ qemu_co_queue_wait(&fst->throttled_reqs[direction], NULL);

}

throttle_account(&fst->ts, direction, iov_size(iov, iovcnt));

- if (!qemu_co_queue_empty(&fst->throttled_reqs[is_write]) &&

+ if (!qemu_co_queue_empty(&fst->throttled_reqs[direction]) &&

!throttle_schedule_timer(&fst->ts, &fst->tt, direction)) {

- qemu_co_queue_next(&fst->throttled_reqs[is_write]);

+ qemu_co_queue_next(&fst->throttled_reqs[direction]);

}

}

}

diff --git a/hw/9pfs/cofile.c b/hw/9pfs/cofile.c

index XXXXXXX..XXXXXXX 100644

--- a/hw/9pfs/cofile.c

+++ b/hw/9pfs/cofile.c

@@ -XXX,XX +XXX,XX @@ int coroutine_fn v9fs_co_pwritev(V9fsPDU *pdu, V9fsFidState *fidp,

if (v9fs_request_cancelled(pdu)) {

return -EINTR;

}

- fsdev_co_throttle_request(s->ctx.fst, true, iov, iovcnt);

+ fsdev_co_throttle_request(s->ctx.fst, THROTTLE_WRITE, iov, iovcnt);

v9fs_co_run_in_worker(

{

err = s->ops->pwritev(&s->ctx, &fidp->fs, iov, iovcnt, offset);

@@ -XXX,XX +XXX,XX @@ int coroutine_fn v9fs_co_preadv(V9fsPDU *pdu, V9fsFidState *fidp,

if (v9fs_request_cancelled(pdu)) {

return -EINTR;

}

- fsdev_co_throttle_request(s->ctx.fst, false, iov, iovcnt);

+ fsdev_co_throttle_request(s->ctx.fst, THROTTLE_READ, iov, iovcnt);

v9fs_co_run_in_worker(

{

err = s->ops->preadv(&s->ctx, &fidp->fs, iov, iovcnt, offset);

2.41.0

From: zhenwei pi <pizhenwei@bytedance.com>

'bool is_write' style is obsolete from throttle framework, adapt

block throttle groups to the new style:

- use ThrottleDirection instead of 'bool is_write'. Ex,

schedule_next_request(ThrottleGroupMember *tgm, bool is_write)

-> schedule_next_request(ThrottleGroupMember *tgm, ThrottleDirection direction)

- use THROTTLE_MAX instead of hard code. Ex, ThrottleGroupMember *tokens[2]

-> ThrottleGroupMember *tokens[THROTTLE_MAX]

- use ThrottleDirection instead of hard code on iteration. Ex, (i = 0; i < 2; i++)

-> for (dir = THROTTLE_READ; dir < THROTTLE_MAX; dir++)

Use a simple python script to test the new style:

#!/usr/bin/python3

import subprocess

import random

import time

commands = ['virsh blkdeviotune jammy vda --write-bytes-sec ', \

'virsh blkdeviotune jammy vda --write-iops-sec ', \

'virsh blkdeviotune jammy vda --read-bytes-sec ', \

'virsh blkdeviotune jammy vda --read-iops-sec ']

for loop in range(1, 1000):

time.sleep(random.randrange(3, 5))

command = commands[random.randrange(0, 3)] + str(random.randrange(0, 1000000))

subprocess.run(command, shell=True, check=True)

This works fine.

Signed-off-by: zhenwei pi <pizhenwei@bytedance.com>

Message-Id: <20230728022006.1098509-10-pizhenwei@bytedance.com>