* numerical option parsing broken in engine options
@ 2014-02-25 1:51 Castor Fu
2014-02-25 17:07 ` Jens Axboe
0 siblings, 1 reply; 6+ messages in thread
From: Castor Fu @ 2014-02-25 1:51 UTC (permalink / raw)
To: fio
I was debugging some problems with parsing fio jobs, and found
options.c:fio_get_kb_base is broken for external engines... The
parameter
'data' is either 0, td, or td->eo. If it's td->eo it's not really
going to work.
The cleaner fix is probably to unwind passing 'data' so far down the
stack, but it
touches a lot of functions, so I thought I'd send this out now.
unsigned int fio_get_kb_base(void *data)
{
struct thread_options *o = data;
unsigned int kb_base = 0;
if (o)
kb_base = o->kb_base;
if (!kb_base)
kb_base = 1024;
return kb_base;
}
If I replace
if(o)
by
if (o && (data - (void *) threads) % sizeof(threads[0]) == 0)
then it behaves as I expect when engine options use the 'k', etc.
-castor
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: numerical option parsing broken in engine options
2014-02-25 1:51 numerical option parsing broken in engine options Castor Fu
@ 2014-02-25 17:07 ` Jens Axboe
2014-02-25 17:34 ` Castor Fu
0 siblings, 1 reply; 6+ messages in thread
From: Jens Axboe @ 2014-02-25 17:07 UTC (permalink / raw)
To: Castor Fu; +Cc: fio
On 2014-02-24 17:51, Castor Fu wrote:
> I was debugging some problems with parsing fio jobs, and found
> options.c:fio_get_kb_base is broken for external engines... The
> parameter
> 'data' is either 0, td, or td->eo. If it's td->eo it's not really
> going to work.
>
> The cleaner fix is probably to unwind passing 'data' so far down the
> stack, but it
> touches a lot of functions, so I thought I'd send this out now.
>
> unsigned int fio_get_kb_base(void *data)
> {
> struct thread_options *o = data;
> unsigned int kb_base = 0;
>
> if (o)
> kb_base = o->kb_base;
> if (!kb_base)
> kb_base = 1024;
>
> return kb_base;
> }
>
>
> If I replace
> if(o)
> by
> if (o && (data - (void *) threads) % sizeof(threads[0]) == 0)
>
> then it behaves as I expect when engine options use the 'k', etc.
Not sure I follow... Are you calling fio_get_kb_base() from the external
engine?
--
Jens Axboe
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: numerical option parsing broken in engine options
2014-02-25 17:07 ` Jens Axboe
@ 2014-02-25 17:34 ` Castor Fu
2014-02-25 18:52 ` Jens Axboe
0 siblings, 1 reply; 6+ messages in thread
From: Castor Fu @ 2014-02-25 17:34 UTC (permalink / raw)
To: Jens Axboe; +Cc: fio
Indirectly. If the external engine has an option which is a
FIO_OPT_INT, and I give it a value like '4k', then we'll eventually
call fio_get_kb_base while pointing to the engine options area instead
of the options / thread_data area.
On Tue, Feb 25, 2014 at 9:07 AM, Jens Axboe <axboe@kernel.dk> wrote:
> On 2014-02-24 17:51, Castor Fu wrote:
>>
>> I was debugging some problems with parsing fio jobs, and found
>> options.c:fio_get_kb_base is broken for external engines... The
>> parameter
>> 'data' is either 0, td, or td->eo. If it's td->eo it's not really
>> going to work.
>>
>> The cleaner fix is probably to unwind passing 'data' so far down the
>> stack, but it
>> touches a lot of functions, so I thought I'd send this out now.
>>
>> unsigned int fio_get_kb_base(void *data)
>> {
>> struct thread_options *o = data;
>> unsigned int kb_base = 0;
>>
>> if (o)
>> kb_base = o->kb_base;
>> if (!kb_base)
>> kb_base = 1024;
>>
>> return kb_base;
>> }
>>
>>
>> If I replace
>> if(o)
>> by
>> if (o && (data - (void *) threads) % sizeof(threads[0]) == 0)
>>
>> then it behaves as I expect when engine options use the 'k', etc.
>
>
> Not sure I follow... Are you calling fio_get_kb_base() from the external
> engine?
>
> --
> Jens Axboe
>
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: numerical option parsing broken in engine options
2014-02-25 17:34 ` Castor Fu
@ 2014-02-25 18:52 ` Jens Axboe
2014-02-25 19:18 ` Jens Axboe
0 siblings, 1 reply; 6+ messages in thread
From: Jens Axboe @ 2014-02-25 18:52 UTC (permalink / raw)
To: Castor Fu; +Cc: fio
On 2014-02-25 09:34, Castor Fu wrote:
> Indirectly. If the external engine has an option which is a
> FIO_OPT_INT, and I give it a value like '4k', then we'll eventually
> call fio_get_kb_base while pointing to the engine options area instead
> of the options / thread_data area.
Ah I see now, yes that is definitely an issue... Let me think about this
a bit. td->eo as data only works for the engine itself, it's not pretty
that the parser ends up thinking it's td.
--
Jens Axboe
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: numerical option parsing broken in engine options
2014-02-25 18:52 ` Jens Axboe
@ 2014-02-25 19:18 ` Jens Axboe
2014-02-25 19:37 ` Jens Axboe
0 siblings, 1 reply; 6+ messages in thread
From: Jens Axboe @ 2014-02-25 19:18 UTC (permalink / raw)
To: Castor Fu; +Cc: fio
On 2014-02-25 10:52, Jens Axboe wrote:
> On 2014-02-25 09:34, Castor Fu wrote:
>> Indirectly. If the external engine has an option which is a
>> FIO_OPT_INT, and I give it a value like '4k', then we'll eventually
>> call fio_get_kb_base while pointing to the engine options area instead
>> of the options / thread_data area.
>
> Ah I see now, yes that is definitely an issue... Let me think about this
> a bit. td->eo as data only works for the engine itself, it's not pretty
> that the parser ends up thinking it's td.
So the issue is really that a non static function makes assumptions
about what 'data' is at this point. That's just not going to work.
Internal use cases are fine, since they know what *data is, but it wont
work for fio_get_kb_base().
So we definitely need some way of knowing if 'data' is our regular
options or not...
--
Jens Axboe
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: numerical option parsing broken in engine options
2014-02-25 19:18 ` Jens Axboe
@ 2014-02-25 19:37 ` Jens Axboe
0 siblings, 0 replies; 6+ messages in thread
From: Jens Axboe @ 2014-02-25 19:37 UTC (permalink / raw)
To: Castor Fu; +Cc: fio
On 2014-02-25 11:18, Jens Axboe wrote:
> On 2014-02-25 10:52, Jens Axboe wrote:
>> On 2014-02-25 09:34, Castor Fu wrote:
>>> Indirectly. If the external engine has an option which is a
>>> FIO_OPT_INT, and I give it a value like '4k', then we'll eventually
>>> call fio_get_kb_base while pointing to the engine options area instead
>>> of the options / thread_data area.
>>
>> Ah I see now, yes that is definitely an issue... Let me think about this
>> a bit. td->eo as data only works for the engine itself, it's not pretty
>> that the parser ends up thinking it's td.
>
> So the issue is really that a non static function makes assumptions
> about what 'data' is at this point. That's just not going to work.
> Internal use cases are fine, since they know what *data is, but it wont
> work for fio_get_kb_base().
>
> So we definitely need some way of knowing if 'data' is our regular
> options or not...
This is the best short term fix I could come up with...
http://git.kernel.dk/?p=fio.git;a=commit;h=cb1402d674fb7694d1b09d7ef4aeb3d4506f23f0
It'd be nice if kb_base always worked as expected, however. So I'll keep
mulling over this and see if we can't come up with something better. We
might just have to have two sets of private, or a private container so
we can always pass in the right thing.
--
Jens Axboe
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2014-02-25 19:37 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2014-02-25 1:51 numerical option parsing broken in engine options Castor Fu
2014-02-25 17:07 ` Jens Axboe
2014-02-25 17:34 ` Castor Fu
2014-02-25 18:52 ` Jens Axboe
2014-02-25 19:18 ` Jens Axboe
2014-02-25 19:37 ` Jens Axboe
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox