fio.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Vincent Fu <vincent.fu@samsung.com>
To: Nick Neumann <nick@pcpartpicker.com>
Cc: "axboe@kernel.dk" <axboe@kernel.dk>,
	"fio@vger.kernel.org" <fio@vger.kernel.org>
Subject: RE: [PATCH 2/5] ioengines: don't record issue_time if ioengines already do it
Date: Wed, 15 Jun 2022 17:06:55 +0000	[thread overview]
Message-ID: <fbe6eea8b6134e92b52a78af25a1fc94@samsung.com> (raw)
In-Reply-To: <CADqNVTpODU3u5siLtvDT8jCBGUZtprMSVi2eqaM9VRbWJ_DuCw@mail.gmail.com>

> -----Original Message-----
> From: Nick Neumann [mailto:nick@pcpartpicker.com]
> Sent: Wednesday, June 15, 2022 11:27 AM
> To: Vincent Fu <vincent.fu@samsung.com>
> Cc: axboe@kernel.dk; fio@vger.kernel.org
> Subject: Re: [PATCH 2/5] ioengines: don't record issue_time if ioengines
> already do it
> 
> On Tue, Jun 14, 2022 at 10:59 AM Vincent Fu <vincent.fu@samsung.com>
> wrote:
> >
> > io_uring, io_uring_cmd, and libaio record issue_time inside the
> ioengine
> > code when their commit functions are called. So we don't need to
> record
> > issue_time again for these ioengines in td_io_queue.
> >
> > If we do fill issue_time twice, then mean(slat) + mean(clat) !=
> > mean(lat):
> 
> I'm a little bit confused though why not recording issue_time again in
> td_io_queue is considered the right fix. By having some ioengines that
> record issue_time in their commit functions, and others that do not
> (and thus have it recorded in td_io_queue), comparing clat between two
> different engines isn't an apples-to-apples comparison. An ioengine
> that does not record issue_time in its commit gets a "discount" of
> sorts in its measured clat, since its issue_time is not recorded until
> later. Does that make sense, or am I missing something? (Has been a
> few months since I've been in the code in detail, so that's possible.)

You make a good point and I'm glad you brought this up for discussion.

We currently have three latency pathways for async ioengines:

(1) async w/o commit: record issue_time in td_io_queue after queue
(2) async w/commit that *does not* record issue_time: records issue_time in
td_io_queue after queue (slat not reported)
(3) async w/commit that *does* record issue_time: issue_time recorded in
both td_io_queue and commit

I think the right long-term solution is to make the ioengines in (2) conform to
(1) or (3). Then we can have fair comparisons. But what 'queue' and 'commit'
mean for them doesn't completely match what the two mean for libaio and
io_uring.

For libaio and io_uring 'queue' prepares an IO and 'commit' actually sends it
to the OS. So it's appropriate to record issue_time after commit. But, for
example, take a look at the nfs ioengine's commit:

https://github.com/axboe/fio/blob/master/engines/nfs.c#L285

nfs isn't doing what libaio and io_uring are doing with commit.

There are four ioengines that fall into category (2): ime_aio, nfs, null, and
cpp_null. It would take some time to test the changes but probably it would be
straightforward to make nfs conform to (1) and {null, cpp_null} conform to (3)
but I have no idea about ime_aio.

I think the current patch is an improvement over what we currently have.  Plus
it really bothers me that mean(slat) + mean(clat) != mean(lat) and this
resolves that issue.  If someone comes along to fix everything up then we can
get rid of FIO_ASYNCIO_SETS_ISSUE_TIME and instead of td_io_queue checking for
that flag before setting issue_time it can instead check for the presence of a
commit hook.


  reply	other threads:[~2022-06-15 17:07 UTC|newest]

Thread overview: 10+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
     [not found] <CGME20220614155830uscas1p1e1d6595b29fb6960b405d5b295a32ffc@uscas1p1.samsung.com>
2022-06-14 15:58 ` [PATCH 0/5] Latency cleanups Vincent Fu
     [not found]   ` <CGME20220614155830uscas1p15aaa5d3bc8eec3da42c78a48520c6dd9@uscas1p1.samsung.com>
2022-06-14 15:58     ` [PATCH 2/5] ioengines: don't record issue_time if ioengines already do it Vincent Fu
2022-06-15 15:26       ` Nick Neumann
2022-06-15 17:06         ` Vincent Fu [this message]
2022-06-15 17:51           ` Nick Neumann
     [not found]   ` <CGME20220614155831uscas1p2312cb623924e5df60abefb210affa1f4@uscas1p2.samsung.com>
2022-06-14 15:58     ` [PATCH 3/5] HOWTO: improve description of latency measures Vincent Fu
     [not found]   ` <CGME20220614155831uscas1p12c7b262538a47a417dc783a45e8df049@uscas1p1.samsung.com>
2022-06-14 15:58     ` [PATCH 4/5] ioengines: update last_issue if we set issue_time Vincent Fu
     [not found]   ` <CGME20220614155830uscas1p2586c5e612430bdd2ce455a1081ce08e4@uscas1p2.samsung.com>
2022-06-14 15:58     ` [PATCH 1/5] ioengines: add helper for trims with async ioengines Vincent Fu
     [not found]   ` <CGME20220614155831uscas1p16ab7450c13a1587f2058617130f4f122@uscas1p1.samsung.com>
2022-06-14 15:58     ` [PATCH 5/5] ioengines: clean up latency accounting for 3 ioengines Vincent Fu
2022-06-15 21:30   ` [PATCH 0/5] Latency cleanups Jens Axboe

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=fbe6eea8b6134e92b52a78af25a1fc94@samsung.com \
    --to=vincent.fu@samsung.com \
    --cc=axboe@kernel.dk \
    --cc=fio@vger.kernel.org \
    --cc=nick@pcpartpicker.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).