PATCH: Don't lose pending completions on exit in time- or size-based job with asynchronous I/O engine

PATCH: Don't lose pending completions on exit in time- or size-based job with asynchronous I/O engine

[Andrey Kuzmin]

Probably worth adding to do_verify() as well.

Regards,
Andrey

diff --git a/backend.c b/backend.c
index 9ac94ed..c782fde 100644
--- a/backend.c
+++ b/backend.c
@@ -803,7 +803,8 @@ static uint64_t do_io(struct thread_data *td)
                total_bytes += td->total_io_size;

        while ((td->o.read_iolog_file && !flist_empty(&td->io_log_list)) ||
-               (!flist_empty(&td->trim_list)) ||
!io_issue_bytes_exceeded(td) ||
+               (!flist_empty(&td->trim_list)) ||
+               !(io_issue_bytes_exceeded(td) && !td->cur_depth) ||
                td->o.time_based) {
                struct timeval comp_time;
                struct io_u *io_u;
@@ -819,7 +820,7 @@ static uint64_t do_io(struct thread_data *td)

                if (runtime_exceeded(td, &td->tv_cache)) {
                        __update_tv_cache(td);
-                       if (runtime_exceeded(td, &td->tv_cache)) {
+                       if (runtime_exceeded(td, &td->tv_cache) &&
!td->cur_depth) {
                                fio_mark_td_terminate(td);
                                break;
                        }
@@ -828,7 +829,7 @@ static uint64_t do_io(struct thread_data *td)
                if (flow_threshold_exceeded(td))
                        continue;

-               if (bytes_issued >= total_bytes)
+               if (bytes_issued >= total_bytes && !td->cur_depth)
                        break;

                io_u = get_io_u(td);

Re: PATCH: Don't lose pending completions on exit in time- or size-based job with asynchronous I/O engine

[Jens Axboe]

On 07/17/2015 08:30 AM, Andrey Kuzmin wrote:
> Probably worth adding to do_verify() as well.

Might be better to ensure that they are reaped when we break out of the 
loop instead?

-- 
Jens Axboe

Re: PATCH: Don't lose pending completions on exit in time- or size-based job with asynchronous I/O engine

[Andrey Kuzmin]

[-- Attachment #1: Type: text/plain, Size: 335 bytes --]

On Jul 17, 2015 5:36 PM, "Jens Axboe" <axboe@kernel.dk> wrote:
>
> On 07/17/2015 08:30 AM, Andrey Kuzmin wrote:
>>
>> Probably worth adding to do_verify() as well.
>
>
> Might be better to ensure that they are reaped when we break out of the
loop instead?
>

That's exactly what happens with the patch, doesn't it?
> --
> Jens Axboe
>

[-- Attachment #2: Type: text/html, Size: 523 bytes --]

Re: PATCH: Don't lose pending completions on exit in time- or size-based job with asynchronous I/O engine

[Jens Axboe]

On 07/17/2015 08:53 AM, Andrey Kuzmin wrote:
>
> On Jul 17, 2015 5:36 PM, "Jens Axboe" <axboe@kernel.dk
> <mailto:axboe@kernel.dk>> wrote:
>  >
>  > On 07/17/2015 08:30 AM, Andrey Kuzmin wrote:
>  >>
>  >> Probably worth adding to do_verify() as well.
>  >
>  >
>  > Might be better to ensure that they are reaped when we break out of
> the loop instead?
>  >
>
> That's exactly what happens with the patch, doesn't it?

It might be... It's not very clear why a !td->cur_depth should force us 
to stay in the loop?

So lets back this up a bit. What problems were observed? Test case?

-- 
Jens Axboe

Re: PATCH: Don't lose pending completions on exit in time- or size-based job with asynchronous I/O engine

[Andrey Kuzmin]

On Fri, Jul 17, 2015 at 5:58 PM, Jens Axboe <axboe@kernel.dk> wrote:
> On 07/17/2015 08:53 AM, Andrey Kuzmin wrote:
>>
>>
>> On Jul 17, 2015 5:36 PM, "Jens Axboe" <axboe@kernel.dk
>> <mailto:axboe@kernel.dk>> wrote:
>>  >
>>  > On 07/17/2015 08:30 AM, Andrey Kuzmin wrote:
>>  >>
>>  >> Probably worth adding to do_verify() as well.
>>  >
>>  >
>>  > Might be better to ensure that they are reaped when we break out of
>> the loop instead?
>>  >
>>
>> That's exactly what happens with the patch, doesn't it?
>
>
> It might be... It's not very clear why a !td->cur_depth should force us to
> stay in the loop?

Because to me breaking out of the loop on time- or size-based limit
exceeded condition with a non-zero td->cur_depth means loosing
completions.

Regards,
Andrey

>
> So lets back this up a bit. What problems were observed? Test case?
>
> --
> Jens Axboe
>

Re: PATCH: Don't lose pending completions on exit in time- or size-based job with asynchronous I/O engine

[Jens Axboe]

On 07/17/2015 09:02 AM, Andrey Kuzmin wrote:
> On Fri, Jul 17, 2015 at 5:58 PM, Jens Axboe <axboe@kernel.dk> wrote:
>> On 07/17/2015 08:53 AM, Andrey Kuzmin wrote:
>>>
>>>
>>> On Jul 17, 2015 5:36 PM, "Jens Axboe" <axboe@kernel.dk
>>> <mailto:axboe@kernel.dk>> wrote:
>>>   >
>>>   > On 07/17/2015 08:30 AM, Andrey Kuzmin wrote:
>>>   >>
>>>   >> Probably worth adding to do_verify() as well.
>>>   >
>>>   >
>>>   > Might be better to ensure that they are reaped when we break out of
>>> the loop instead?
>>>   >
>>>
>>> That's exactly what happens with the patch, doesn't it?
>>
>>
>> It might be... It's not very clear why a !td->cur_depth should force us to
>> stay in the loop?
>
> Because to me breaking out of the loop on time- or size-based limit
> exceeded condition with a non-zero td->cur_depth means loosing
> completions.

That's what I thought. Hence my suggestion would be that we reap any 
potentially inflight IO _outside_ of the loop, that would be a lot cleaner.

-- 
Jens Axboe

Re: PATCH: Don't lose pending completions on exit in time- or size-based job with asynchronous I/O engine

[Andrey Kuzmin]

On Fri, Jul 17, 2015 at 6:23 PM, Andrey Kuzmin
<andrey.v.kuzmin@gmail.com> wrote:
>
> On Jul 17, 2015 6:11 PM, "Jens Axboe" <axboe@kernel.dk> wrote:
>>
>> On 07/17/2015 09:02 AM, Andrey Kuzmin wrote:
>>>
>>> On Fri, Jul 17, 2015 at 5:58 PM, Jens Axboe <axboe@kernel.dk> wrote:
>>>>
>>>> On 07/17/2015 08:53 AM, Andrey Kuzmin wrote:
>>>>>
>>>>>
>>>>>
>>>>> On Jul 17, 2015 5:36 PM, "Jens Axboe" <axboe@kernel.dk
>>>>> <mailto:axboe@kernel.dk>> wrote:
>>>>>   >
>>>>>   > On 07/17/2015 08:30 AM, Andrey Kuzmin wrote:
>>>>>   >>
>>>>>   >> Probably worth adding to do_verify() as well.
>>>>>   >
>>>>>   >
>>>>>   > Might be better to ensure that they are reaped when we break out of
>>>>> the loop instead?
>>>>>   >
>>>>>
>>>>> That's exactly what happens with the patch, doesn't it?
>>>>
>>>>
>>>>
>>>> It might be... It's not very clear why a !td->cur_depth should force us
>>>> to
>>>> stay in the loop?
>>>
>>>
>>> Because to me breaking out of the loop on time- or size-based limit
>>> exceeded condition with a non-zero td->cur_depth means loosing
>>> completions.
>>
>>
>> That's what I thought. Hence my suggestion would be that we reap any
>> potentially inflight IO _outside_ of the loop, that would be a lot cleaner.

Turned out the extant completion reaping code that follows the break
out from the main loop is perfectly sufficient, provided the engine
properly appreciates the minimum number of completions required (which
mine didn't). Withdrawing the original patch request.

Regards,
Andrey

>>
> Agreed, will look into the reaping code.
>
> Regards,
> A.
>> --
>> Jens Axboe
>>