[17/18] block: introduce block_account_one_io

Commit Message

Paolo Bonzini May 11, 2017, 2:42 p.m. UTC

This is the common code to account operations that produced actual I/O.

Signed-off-by: Paolo Bonzini <[e-mail]>
---
        v1->v2: new, to hoist qemu_clock_get_ns out of the
                "if (!stats->account_failed)" case.  Failed operations
                are not a fast path, so getting the current time
                unconditionally is not an issue. [Stefan]

 block/accounting.c | 50 +++++++++++++++++++++-----------------------------
 1 file changed, 21 insertions(+), 29 deletions(-)

Comments

Stefan Hajnoczi May 16, 2017, 3:07 p.m. UTC | #1

On Thu, May 11, 2017 at 04:42:07PM +0200, Paolo Bonzini wrote:
> This is the common code to account operations that produced actual I/O.
> 
> Signed-off-by: Paolo Bonzini <[e-mail]>
> ---
>         v1->v2: new, to hoist qemu_clock_get_ns out of the
>                 "if (!stats->account_failed)" case.  Failed operations
>                 are not a fast path, so getting the current time
>                 unconditionally is not an issue. [Stefan]
> 
>  block/accounting.c | 50 +++++++++++++++++++++-----------------------------
>  1 file changed, 21 insertions(+), 29 deletions(-)

Reviewed-by: Stefan Hajnoczi <[e-mail]>

Alberto Garcia May 18, 2017, 12:09 p.m. UTC | #2

On Thu 11 May 2017 04:42:07 PM CEST, Paolo Bonzini wrote:
> This is the common code to account operations that produced actual I/O.
>
> Signed-off-by: Paolo Bonzini <[e-mail]>

Reviewed-by: Alberto Garcia <[e-mail]>

Berto

Fam Zheng May 25, 2017, 7:28 a.m. UTC | #3

On Thu, 05/11 16:42, Paolo Bonzini wrote:
> -void block_acct_done(BlockAcctStats *stats, BlockAcctCookie *cookie)
> +static void block_account_one_io(BlockAcctStats *stats, BlockAcctCookie *cookie, bool failed)

This line is too long. Can be fixed when applying, though.

Fam

Patch

diff --git a/block/accounting.c b/block/accounting.c
index 3f457c4e73..0429e31c9f 100644
--- a/block/accounting.c
+++ b/block/accounting.c
@@ -86,7 +86,7 @@  void block_acct_start(BlockAcctStats *stats, BlockAcctCookie *cookie,
     cookie->type = type;
 }
 
-void block_acct_done(BlockAcctStats *stats, BlockAcctCookie *cookie)
+static void block_account_one_io(BlockAcctStats *stats, BlockAcctCookie *cookie, bool failed)
 {
     BlockAcctTimedStats *s;
     int64_t time_ns = qemu_clock_get_ns(clock_type);
@@ -98,31 +98,14 @@  void block_acct_done(BlockAcctStats *stats, BlockAcctCookie *cookie)
 
     assert(cookie->type < BLOCK_MAX_IOTYPE);
 
-    stats->nr_bytes[cookie->type] += cookie->bytes;
-    stats->nr_ops[cookie->type]++;
-    stats->total_time_ns[cookie->type] += latency_ns;
-    stats->last_access_time_ns = time_ns;
-
-    QSLIST_FOREACH(s, &stats->intervals, entries) {
-        timed_average_account(&s->latency[cookie->type], latency_ns);
+    if (failed) {
+        stats->failed_ops[cookie->type]++;
+    } else {
+        stats->nr_bytes[cookie->type] += cookie->bytes;
+        stats->nr_ops[cookie->type]++;
     }
-}
-
-void block_acct_failed(BlockAcctStats *stats, BlockAcctCookie *cookie)
-{
-    assert(cookie->type < BLOCK_MAX_IOTYPE);
-
-    stats->failed_ops[cookie->type]++;
-
-    if (stats->account_failed) {
-        BlockAcctTimedStats *s;
-        int64_t time_ns = qemu_clock_get_ns(clock_type);
-        int64_t latency_ns = time_ns - cookie->start_time_ns;
-
-        if (qtest_enabled()) {
-            latency_ns = qtest_latency_ns;
-        }
 
+    if (!failed || stats->account_failed) {
         stats->total_time_ns[cookie->type] += latency_ns;
         stats->last_access_time_ns = time_ns;
 
@@ -132,15 +115,24 @@  void block_acct_failed(BlockAcctStats *stats, BlockAcctCookie *cookie)
     }
 }
 
+void block_acct_done(BlockAcctStats *stats, BlockAcctCookie *cookie)
+{
+    block_account_one_io(stats, cookie, false);
+}
+
+void block_acct_failed(BlockAcctStats *stats, BlockAcctCookie *cookie)
+{
+    block_account_one_io(stats, cookie, true);
+}
+
 void block_acct_invalid(BlockAcctStats *stats, enum BlockAcctType type)
 {
     assert(type < BLOCK_MAX_IOTYPE);
 
-    /* block_acct_done() and block_acct_failed() update
-     * total_time_ns[], but this one does not. The reason is that
-     * invalid requests are accounted during their submission,
-     * therefore there's no actual I/O involved. */
-
+    /* block_account_one_io() updates total_time_ns[], but this one does
+     * not.  The reason is that invalid requests are accounted during their
+     * submission, therefore there's no actual I/O involved.
+     */
     stats->invalid_ops[type]++;
 
     if (stats->account_invalid) {