ioengines.c:346: td_io_queue: Assertion `res == 0' failed

[horshack-dpreview]

Assertion in ioengines.c::td_io_queue() fails for pthread_mutex_unlock() on overlap_check mutex when serialize_overlap=1, io_submit_mode=offload, and verify= are used together.

backend.c::fio_io_sync() invokes td_io_queue(), which expects the caller to have ownership of the overlap_check mutex when serialize_overlap and offloading are configured, as part of the overlap-check interlock with IO_U_F_FLIGHT. The mutex is not acquired for this path because it's not an I/O requiring an overlap check.

The fix is to refine the conditional that triggers td_io_queue() to release the overlap_check mutex. Rather than using broad config options, the conditional now uses a new io_u flag named IO_U_F_OVERLAP_LOCK, which is only set for the offload worker thread path that acquires the mutex.

Link: #1520

Signed-off-by: Adam Horshack (horshack@live.com)

[axboe]

This is causing a crash, and I should've done a better job reviewing it. The rate-submit.c is nonsensical. Did you test this at all? I'm going to revert the change for now.

[horshack-dpreview]

This is causing a crash, and I should've done a better job reviewing it. The rate-submit.c is nonsensical. Did you test this at all? I'm going to revert the change for now.

I tested it against a matrix of configurations, like I do all my changes. I have my own automated test bench to do this. What options caused it to crash and what about the change to rate-submit.c is nonsensical?

[axboe]

Just run:

./fio t/jobs/t0013.fio

and it'll insta-crash.

[horshack-dpreview]

Just run:

./fio t/jobs/t0013.fio

and it'll insta-crash.

Reproduced. I don't know how I added that line to rate-submit.c without seeing the obvious terminating value issue for td. I'm sorry.

I was more perplexed how my test runs didn't catch it. Turns out it only faults when numjobs is an even multiple of 8, which none of my test runs are. Digging into it, for_each_td will only yield an ending td value of NULL when the number of threads allocated is an even multiple of JOBS_PER_SEG (8), since that's the only scenario where the thread_segment[n+1] will have a NULL value for threads:

fio/fio.h

Lines 587 to 593 in 9943adf

	static inline struct thread_data *tnumber_to_td(unsigned int tnumber)
	{
	struct thread_segment *seg;
	seg = &segments[tnumber / JOBS_PER_SEG];
	return &seg->threads[tnumber & (JOBS_PER_SEG - 1)];
	}

To prevent others from making my same careless mistake I'd like to propose changing for_each_td to make it impossible to use the terminating value of td. This can be accomplished by 1) moving the scope of td inside for_each_td or 2) explicitly assigning td to NULL at the end of the loop. The first proposal is cleaner and more efficient but touches a lot of files - the latter has only one touch but is less efficient. Please let me know what you think.

[axboe]

Option 1 seems nicer. Even if efficiency isn't THAT important so we could tolerate just clearing it, it'd still just crash rather than be safe by design.

[horshack-dpreview]

@axboe Thanks. I wanted to give both td and i the same macro-scope treatment but in C/C++ if you declare any type within the for() loop initializer all variables must be of that type. For example, the following is invalid:

#define for_each_td(td, i)  \
    for (int (i) = 0, struct thread_data *(td) = &segments[0].threads[0]; (i) < (int) thread_number; (i)++, (td) = tnumber_to_td((i)))

Even if we wanted to only restrict the scope of td the following is invalid too, since i is not the same type as td declared:

#define for_each_td(td, i)  \
    for (struct thread_data *(td) = &segments[0].threads[0], int (i) = 0; (i) < (int) thread_number; (i)++, (td) = tnumber_to_td((i)))

To work around this i must be initialized outside the for() loop initializer:

#define for_each_td(td, i)  \
    (i) = 0;                \
    for (struct thread_data *(td) = &segments[0].threads[0]; (i) < (int) thread_number; (i)++, (td) = tnumber_to_td((i)))

Which is fine but not beautiful. Next issue: Every snippet which uses for_each_td will get a -Wunused-variable warning because the outer-scope version of td is not used in most of fio's code - it's only declared to use inside for_each_td. The cleanest solution would be to remove all existing local declarations of td intended for use in for_each_td (and the loop index variable too while we're at it). But that means we now have to use a hard-coded variable name for the construct, since C/C++ doesn't provide a way to pass a variable name to a macro and have that macro declare a variable with that name...if the name doesn't already exist (which it does in the outer scope for the current implementation). The solution is to always use the name td within the for_each_td block, which is fine except in 4 places where the code uses td2 instead, since td is used as the function argument name in those cases. For those we'd have to rename that existing td to something else, which seems manageable.

It's doable but comes down to style and whether the juice is worth the squeeze. The alternative is the original option 2, where we assure td ends with a value of NULL so we at least catch the usage via a segfault. To implement that I would modify tnumber_to_td() to range check and return NULL for a thread # that is >= #threads.

[horshack-dpreview]

@axboe, Here is the corrected fix for the original issue. Would you like me to create a new pull request or can you pull it from it from my branch?

Link: horshack-dpreview@aac9c03

[horshack-dpreview]

@axboe, I've implemented the discussed changes to for_each_td():

#1531