Passing argument to both write_bw_log & write_lat_log leads to segfault

Passing argument to both write_bw_log & write_lat_log leads to segfault

[Erwan Velu]

Using fio 1.23:
[root@max13 tmp]# fio myconfigfile
Segmentation fault (core dumped)


Where myconfigfile is the following:
[global]
bsrange=64k-128k,64k-128k
direct=1
ioengine=libaio
iodepth=2
zonesize=256m
zoneskip=2g

[/dev/exa/g1/v1]
rw=read
write_bw_log read-bw
write_lat_log read-lat

[/dev/exa/g1/v1]
stonewall
rw=write
write_bw_log write-bw
write_lat_log write-lat

Note: removing one or the other parameter to write_*_log reports an error like "Bad option write_lat_log read-lat" but letting both of them segfault.

I did this syntaxe as the man page says :
       write_bw_log
              If given, write bandwidth logs of the jobs in this file.

I thought that "this file" meant a parameter.
Cheers,
-- 
Erwan Velu
Pre-Sales Engineer
Seanodes
http://www.seanodes.com
+33 (0)1 41 22 13 83

Re: Passing argument to both write_bw_log & write_lat_log leads to segfault

[Jens Axboe]

On Wed, Nov 19 2008, Erwan Velu wrote:
> Using fio 1.23:
> [root@max13 tmp]# fio myconfigfile
> Segmentation fault (core dumped)
> 
> 
> Where myconfigfile is the following:
> [global]
> bsrange=64k-128k,64k-128k
> direct=1
> ioengine=libaio
> iodepth=2
> zonesize=256m
> zoneskip=2g
> 
> [/dev/exa/g1/v1]
> rw=read
> write_bw_log read-bw
> write_lat_log read-lat
> 
> [/dev/exa/g1/v1]
> stonewall
> rw=write
> write_bw_log write-bw
> write_lat_log write-lat
> 
> Note: removing one or the other parameter to write_*_log reports an error 
> like "Bad option write_lat_log read-lat" but letting both of them segfault.
> 
> I did this syntaxe as the man page says :
>       write_bw_log
>              If given, write bandwidth logs of the jobs in this file.
> 
> I thought that "this file" meant a parameter.

Wow, you are handy at finding old bugs that nobody has hit :-). The
below should fix it.

Generally, fio always wants a '=' to assign a value to a parameter. But
of course the parser should not crash...

diff --git a/parse.c b/parse.c
index 456e3ba..5f7d033 100644
--- a/parse.c
+++ b/parse.c
@@ -487,7 +487,7 @@ static int opt_cmp(const void *p1, const void *p2)
 {
 	struct fio_option *o1, *o2;
 	char *s1, *s2, *foo;
-	int ret;
+	int prio1, prio2;
 
 	s1 = strdup(*((char **) p1));
 	s2 = strdup(*((char **) p2));
@@ -495,14 +495,15 @@ static int opt_cmp(const void *p1, const void *p2)
 	o1 = get_option(s1, fio_options, &foo);
 	o2 = get_option(s2, fio_options, &foo);
 
-	if ((!o1 && o2) || (o1 && !o2))
-		ret = 0;
-	else
-		ret = o2->prio - o1->prio;
+	prio1 = prio2 = 0;
+	if (o1)
+		prio1 = o1->prio;
+	if (o2)
+		prio2 = o2->prio;
 
 	free(s1);
 	free(s2);
-	return ret;
+	return prio2 - prio1;
 }
 
 void sort_options(char **opts, struct fio_option *options, int num_opts)

-- 
Jens Axboe

Re: Passing argument to both write_bw_log & write_lat_log leads to segfault

[Erwan Velu]

Jens Axboe wrote:

[....]
> Wow, you are handy at finding old bugs that nobody has hit :-). The
> below should fix it.
>   
Hehe :p
> Generally, fio always wants a '=' to assign a value to a parameter. But
> of course the parser should not crash...
>   
Ok. It that configuration correct ?

[global]
bsrange=64k-128k,64k-128k
direct=1
ioengine=libaio
iodepth=64
zonesize=256m
zoneskip=2g

[/dev/sdb]
rw=read
write_bw_log=read-bw
write_lat_log=read-lat

[/dev/sdb]
stonewall
rw=write
write_bw_log=write-bw
write_lat_log=write-lat

If yes, I have a problem because I only have files called : sdb_clat.log 
& sdb_slat.log

-- 
Erwan Velu
Pre-Sales Engineer
Seanodes
http://www.seanodes.com
+33 (0)1 41 22 13 83

Re: Passing argument to both write_bw_log & write_lat_log leads to segfault

[Jens Axboe]

On Wed, Nov 19 2008, Erwan Velu wrote:
> Jens Axboe wrote:
> 
> [....]
> >Wow, you are handy at finding old bugs that nobody has hit :-). The
> >below should fix it.
> >  
> Hehe :p
> >Generally, fio always wants a '=' to assign a value to a parameter. But
> >of course the parser should not crash...
> >  
> Ok. It that configuration correct ?
> 
> [global]
> bsrange=64k-128k,64k-128k
> direct=1
> ioengine=libaio
> iodepth=64
> zonesize=256m
> zoneskip=2g
> 
> [/dev/sdb]
> rw=read
> write_bw_log=read-bw
> write_lat_log=read-lat
> 
> [/dev/sdb]
> stonewall
> rw=write
> write_bw_log=write-bw
> write_lat_log=write-lat
> 
> If yes, I have a problem because I only have files called : sdb_clat.log 
> & sdb_slat.log

They don't accept a parameter. I can rectify that, I guess sometimes
you'd like that (especially for big jobs). The log should contain
entries from both, with a direction field.

Let me double check tonight and make sure it doesn't truncate the log or
anything like that, I don't think I've ever used the log like this. It
should just append.

-- 
Jens Axboe

Re: Passing argument to both write_bw_log & write_lat_log leads to segfault

[Jens Axboe]

On Wed, Nov 19 2008, Jens Axboe wrote:
> On Wed, Nov 19 2008, Erwan Velu wrote:
> > Jens Axboe wrote:
> > 
> > [....]
> > >Wow, you are handy at finding old bugs that nobody has hit :-). The
> > >below should fix it.
> > >  
> > Hehe :p
> > >Generally, fio always wants a '=' to assign a value to a parameter. But
> > >of course the parser should not crash...
> > >  
> > Ok. It that configuration correct ?
> > 
> > [global]
> > bsrange=64k-128k,64k-128k
> > direct=1
> > ioengine=libaio
> > iodepth=64
> > zonesize=256m
> > zoneskip=2g
> > 
> > [/dev/sdb]
> > rw=read
> > write_bw_log=read-bw
> > write_lat_log=read-lat
> > 
> > [/dev/sdb]
> > stonewall
> > rw=write
> > write_bw_log=write-bw
> > write_lat_log=write-lat
> > 
> > If yes, I have a problem because I only have files called : sdb_clat.log 
> > & sdb_slat.log
> 
> They don't accept a parameter. I can rectify that, I guess sometimes
> you'd like that (especially for big jobs). The log should contain
> entries from both, with a direction field.
> 
> Let me double check tonight and make sure it doesn't truncate the log or
> anything like that, I don't think I've ever used the log like this. It
> should just append.

OK, it truncates. This will help. I'll have it accept file names as well
later today.

diff --git a/log.c b/log.c
index 5c468ad..ab457c7 100644
--- a/log.c
+++ b/log.c
@@ -388,7 +388,7 @@ static int init_iolog_write(struct thread_data *td)
 	FILE *f;
 	unsigned int i;
 
-	f = fopen(td->o.write_iolog_file, "w+");
+	f = fopen(td->o.write_iolog_file, "a");
 	if (!f) {
 		perror("fopen write iolog");
 		return 1;
@@ -455,7 +455,7 @@ void __finish_log(struct io_log *log, const char *name)
 	unsigned int i;
 	FILE *f;
 
-	f = fopen(name, "w");
+	f = fopen(name, "a");
 	if (!f) {
 		perror("fopen log");
 		return;

-- 
Jens Axboe