[PATCH] verify: Fix latency log for verify commands.

[PATCH] verify: Fix latency log for verify commands.

[Gwendal Grignou]

When commands when requeued for the verify operation,
their start time was not reset, resulting in bogus latency graphs.

Signed-off-by: Gwendal Grignou <gwendal@chromium.org>
---
 backend.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/backend.c b/backend.c
index 7ec8d2a..0f6e425 100644
--- a/backend.c
+++ b/backend.c
@@ -565,6 +565,8 @@ static void do_verify(struct thread_data *td, uint64_t verify_bytes)
 			io_u->end_io = verify_io_u;
 
 		ddir = io_u->ddir;
+		if (!td->o.disable_slat)
+			fio_gettime(&io_u->start_time, NULL);
 
 		ret = td_io_queue(td, io_u);
 		switch (ret) {
-- 
2.2.0.rc0.207.ga3a616c

Re: [PATCH] verify: Fix latency log for verify commands.

[Andrey Kuzmin]

On Mon, Feb 9, 2015 at 6:53 AM, Gwendal Grignou <gwendal@chromium.org> wrote:
> When commands when requeued for the verify operation,
> their start time was not reset, resulting in bogus latency graphs.

Does it really make sense to account for the verification pass in the
latency profile?

Regards,
Andrey

>
> Signed-off-by: Gwendal Grignou <gwendal@chromium.org>
> ---
>  backend.c | 2 ++
>  1 file changed, 2 insertions(+)
>
> diff --git a/backend.c b/backend.c
> index 7ec8d2a..0f6e425 100644
> --- a/backend.c
> +++ b/backend.c
> @@ -565,6 +565,8 @@ static void do_verify(struct thread_data *td, uint64_t verify_bytes)
>                         io_u->end_io = verify_io_u;
>
>                 ddir = io_u->ddir;
> +               if (!td->o.disable_slat)
> +                       fio_gettime(&io_u->start_time, NULL);
>
>                 ret = td_io_queue(td, io_u);
>                 switch (ret) {
> --
> 2.2.0.rc0.207.ga3a616c
>
> --
> To unsubscribe from this list: send the line "unsubscribe fio" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] verify: Fix latency log for verify commands.

[Jens Axboe]

On 02/09/2015 02:39 AM, Andrey Kuzmin wrote:
> On Mon, Feb 9, 2015 at 6:53 AM, Gwendal Grignou <gwendal@chromium.org> wrote:
>> When commands when requeued for the verify operation,
>> their start time was not reset, resulting in bogus latency graphs.
>
> Does it really make sense to account for the verification pass in the
> latency profile?

Why not? If you are doing a full write then verification, you get a full 
set of separate read and write latencies.

-- 
Jens Axboe

Re: [PATCH] verify: Fix latency log for verify commands.

[Andrey Kuzmin]

On Mon, Feb 9, 2015 at 6:17 PM, Jens Axboe <axboe@kernel.dk> wrote:
> On 02/09/2015 02:39 AM, Andrey Kuzmin wrote:
>>
>> On Mon, Feb 9, 2015 at 6:53 AM, Gwendal Grignou <gwendal@chromium.org>
>> wrote:
>>>
>>> When commands when requeued for the verify operation,
>>> their start time was not reset, resulting in bogus latency graphs.
>>
>>
>> Does it really make sense to account for the verification pass in the
>> latency profile?
>
>
> Why not? If you are doing a full write then verification, you get a full set
> of separate read and write latencies.

I'd rather get them on a per-pass basis. Imagine an r/w workload being
done with verification pass (just to be on the safe side): I'd rather
keep read-only verification pass latencies separate from the primary
workload latency profile. YMMW :).

>
> --
> Jens Axboe
>

Re: [PATCH] verify: Fix latency log for verify commands.

[Jens Axboe]

On 02/09/2015 08:25 AM, Andrey Kuzmin wrote:
> On Mon, Feb 9, 2015 at 6:17 PM, Jens Axboe <axboe@kernel.dk> wrote:
>> On 02/09/2015 02:39 AM, Andrey Kuzmin wrote:
>>>
>>> On Mon, Feb 9, 2015 at 6:53 AM, Gwendal Grignou <gwendal@chromium.org>
>>> wrote:
>>>>
>>>> When commands when requeued for the verify operation,
>>>> their start time was not reset, resulting in bogus latency graphs.
>>>
>>>
>>> Does it really make sense to account for the verification pass in the
>>> latency profile?
>>
>>
>> Why not? If you are doing a full write then verification, you get a full set
>> of separate read and write latencies.
>
> I'd rather get them on a per-pass basis. Imagine an r/w workload being
> done with verification pass (just to be on the safe side): I'd rather
> keep read-only verification pass latencies separate from the primary
> workload latency profile. YMMW :).

Sure, if it's a mixed read/write workload and you verify after the fact, 
then it could be handy to have the two "different" kinds of reads 
separate. But that's really orthogonal to the issue being fixed by 
Gwendals patch...


-- 
Jens Axboe

Re: [PATCH] verify: Fix latency log for verify commands.

[Andrey Kuzmin]

On Mon, Feb 9, 2015 at 6:27 PM, Jens Axboe <axboe@kernel.dk> wrote:
> On 02/09/2015 08:25 AM, Andrey Kuzmin wrote:
>>
>> On Mon, Feb 9, 2015 at 6:17 PM, Jens Axboe <axboe@kernel.dk> wrote:
>>>
>>> On 02/09/2015 02:39 AM, Andrey Kuzmin wrote:
>>>>
>>>>
>>>> On Mon, Feb 9, 2015 at 6:53 AM, Gwendal Grignou <gwendal@chromium.org>
>>>> wrote:
>>>>>
>>>>>
>>>>> When commands when requeued for the verify operation,
>>>>> their start time was not reset, resulting in bogus latency graphs.
>>>>
>>>>
>>>>
>>>> Does it really make sense to account for the verification pass in the
>>>> latency profile?
>>>
>>>
>>>
>>> Why not? If you are doing a full write then verification, you get a full
>>> set
>>> of separate read and write latencies.
>>
>>
>> I'd rather get them on a per-pass basis. Imagine an r/w workload being
>> done with verification pass (just to be on the safe side): I'd rather
>> keep read-only verification pass latencies separate from the primary
>> workload latency profile. YMMW :).
>
>
> Sure, if it's a mixed read/write workload and you verify after the fact,
> then it could be handy to have the two "different" kinds of reads separate.

May be a case for an extra option to control latency profile
aggregation (disable_verify_lat ?)?

> But that's really orthogonal to the issue being fixed by Gwendals patch...

Sure, it has just brought up an earlier point :).


Regards,
Andrey


>
>
> --
> Jens Axboe
>

Re: [PATCH] verify: Fix latency log for verify commands.

[Jens Axboe]

On 02/08/2015 08:53 PM, Gwendal Grignou wrote:
> When commands when requeued for the verify operation,
> their start time was not reset, resulting in bogus latency graphs.
>
> Signed-off-by: Gwendal Grignou <gwendal@chromium.org>
> ---
>   backend.c | 2 ++
>   1 file changed, 2 insertions(+)
>
> diff --git a/backend.c b/backend.c
> index 7ec8d2a..0f6e425 100644
> --- a/backend.c
> +++ b/backend.c
> @@ -565,6 +565,8 @@ static void do_verify(struct thread_data *td, uint64_t verify_bytes)
>   			io_u->end_io = verify_io_u;
>
>   		ddir = io_u->ddir;
> +		if (!td->o.disable_slat)
> +			fio_gettime(&io_u->start_time, NULL);
>
>   		ret = td_io_queue(td, io_u);
>   		switch (ret) {

Thanks, applied.


-- 
Jens Axboe