[LTP] [PATCH 2/2] Avoid zero or negative int results in calculations

[LTP] [PATCH 2/2] Avoid zero or negative int results in calculations

[Jozef Pupava]

Define BUF_SIZE smaller than BLOCKSIZE for possitive offset result

Acked-by: Martin Doucha <mdoucha@suse.cz>
Signed-off-by: Jozef Pupava <jpupava@suse.com>
---
 testcases/kernel/syscalls/fsync/fsync02.c | 17 ++++++++---------
 1 file changed, 8 insertions(+), 9 deletions(-)

diff --git a/testcases/kernel/syscalls/fsync/fsync02.c b/testcases/kernel/syscalls/fsync/fsync02.c
index 9506b4868..42a45d0c5 100644
--- a/testcases/kernel/syscalls/fsync/fsync02.c
+++ b/testcases/kernel/syscalls/fsync/fsync02.c
@@ -22,9 +22,10 @@
 #define BLOCKSIZE 8192
 #define MAXBLKS 262144
 #define TIME_LIMIT 120
+#define BUF_SIZE 2048
 
 char tempfile[40] = "";
-char pbuf[BUFSIZ];
+char pbuf[BUF_SIZE];
 int fd;
 off_t max_blks = MAXBLKS;
 
@@ -47,7 +48,7 @@ static void setup(void) {
 
 #ifdef LARGEFILE
 	SAFE_FCNTL(fd, F_SETFL, O_LARGEFILE);
-	SAFE_WRITE(1, fd, pbuf, BUFSIZ);
+	SAFE_WRITE(1, fd, pbuf, BUF_SIZE);
 #endif
 }
 
@@ -60,17 +61,15 @@ static void run(void) {
 	double time_delta;
 	long int random_number;
 
-	while (max_block <= data_blocks) {
-		random_number = rand();
-		max_block = random_number % max_blks;
-		data_blocks = random_number % 1000 + 1;
-	}
+	random_number = rand();
+	max_block = random_number % max_blks + 1;
+	data_blocks = random_number % max_block;
 
 	for (i = 1; i <= data_blocks; i++) {
 		offset = i * ((BLOCKSIZE * max_block) / data_blocks);
-		offset -= BUFSIZ;
+		offset -= BUF_SIZE;
 		SAFE_LSEEK(fd, offset, SEEK_SET);
-		SAFE_WRITE(1, fd, pbuf, BUFSIZ);
+		SAFE_WRITE(1, fd, pbuf, BUF_SIZE);
 	}
 	time_start = time(0);
 
-- 
2.16.4

[LTP] [PATCH 2/2] Avoid zero or negative int results in calculations

[Petr Vorel]

Hi Jozef,

> Define BUF_SIZE smaller than BLOCKSIZE for possitive offset result

Reviewed-by: Petr Vorel <pvorel@suse.cz>

BTW it significantly prolongs the run, but IMHO ok.

Thanks for your fix.

Kind regards,
Petr

[LTP] [PATCH 2/2] Avoid zero or negative int results in calculations

[Cyril Hrubis]

Hi!
> > Define BUF_SIZE smaller than BLOCKSIZE for possitive offset result
> 
> Reviewed-by: Petr Vorel <pvorel@suse.cz>
> 
> BTW it significantly prolongs the run, but IMHO ok.

Longer testruns should be avoided unless there is a good reason for it.

-- 
Cyril Hrubis
chrubis@suse.cz

[LTP] [PATCH 2/2] Avoid zero or negative int results in calculations

[Petr Vorel]

Hi,

> > > Define BUF_SIZE smaller than BLOCKSIZE for possitive offset result

> > Reviewed-by: Petr Vorel <pvorel@suse.cz>

> > BTW it significantly prolongs the run, but IMHO ok.

> Longer testruns should be avoided unless there is a good reason for it.
+1. Although the commit message does not describe what it fixes, it obviously
fixes something. It'd be nice to explain it a bit.

Before:
real	0m0,013s
user	0m0,004s
sys	0m0,009s

After (slowed by second commit, not by the rewrite):
real	0m0,402s
user	0m0,042s
sys	0m0,360s

Looks like the smaller buffer the faster it is.
I haven't tested other Martin's fix reported later.

Kind regards,
Petr

[LTP] [PATCH 2/2] Avoid zero or negative int results in calculations

[Petr Vorel]

Hi,

> Before:
> real	0m0,013s

> After (slowed by second commit, not by the rewrite):
> real	0m0,402s

Although the slowdown it's ~30 times, it's obviously fast enough,
so I wouldn't consider it as a problem. I was just surprised by it.

My concern is about brief explanation where/how is zero or negative result
appears. But maybe it's obvious and I just don't see it.

Kind regards,
Petr

[LTP] [PATCH 2/2] Avoid zero or negative int results in calculations

[Jozef Pupava]

Hi Petr,

I will try to decrease, ideally remove the slowdown.

Jozef

On Fri, 27 Mar 2020 10:57:15 +0100
Petr Vorel <pvorel@suse.cz> wrote:

> Hi,
> 
> > Before:
> > real	0m0,013s  
> 
> > After (slowed by second commit, not by the rewrite):
> > real	0m0,402s  
> 
> Although the slowdown it's ~30 times, it's obviously fast enough,
> so I wouldn't consider it as a problem. I was just surprised by it.
> 
> My concern is about brief explanation where/how is zero or negative
> result appears. But maybe it's obvious and I just don't see it.
> 
> Kind regards,
> Petr

[LTP] [PATCH 2/2] Avoid zero or negative int results in calculations

[Martin Doucha]

On 27. 03. 20 10:57, Petr Vorel wrote:
> Hi,
> 
>> Before:
>> real	0m0,013s
> 
>> After (slowed by second commit, not by the rewrite):
>> real	0m0,402s
> 
> Although the slowdown it's ~30 times, it's obviously fast enough,
> so I wouldn't consider it as a problem. I was just surprised by it.

Like I said in the previous e-mail, that slowdown is caused by the test
intentionally randomizing the number of disk writes, not the patch. Run
the test several times.

> My concern is about brief explanation where/how is zero or negative result
> appears. But maybe it's obvious and I just don't see it.

Let me explain.

> @@ -60,17 +61,15 @@ static void run(void) {

>  	double time_delta;

>  	long int random_number;

>  

> -	while (max_block <= data_blocks) {

> -		random_number = rand();

> -		max_block = random_number % max_blks;

> -		data_blocks = random_number % 1000 + 1;

> -	}

> +	random_number = rand();

> +	max_block = random_number % max_blks + 1;

> +	data_blocks = random_number % max_block;


This fixes a potential infinite loop if max_blks == 1000. This
calculation is also the reason why the test has random run length.

>  

>  	for (i = 1; i <= data_blocks; i++) {

>  		offset = i * ((BLOCKSIZE * max_block) / data_blocks);

> -		offset -= BUFSIZ;

> +		offset -= BUF_SIZE;


Here the old calculation could produce negative offset if
BUFSIZ > BLOCKSIZE and (float)max_block/data_blocks is close to 1.
BUFSIZ is defined in libc headers so the actual value can be different
on different systems.

-- 
Martin Doucha   mdoucha@suse.cz
QA Engineer for Software Maintenance
SUSE LINUX, s.r.o.
CORSO IIa
Krizikova 148/34
186 00 Prague 8
Czech Republic

[LTP] [PATCH 2/2] Avoid zero or negative int results in calculations

[Petr Vorel]

Hi Martin,

> Like I said in the previous e-mail, that slowdown is caused by the test
> intentionally randomizing the number of disk writes, not the patch. Run
> the test several times.

> > My concern is about brief explanation where/how is zero or negative result
> > appears. But maybe it's obvious and I just don't see it.

> Let me explain.

> > @@ -60,17 +61,15 @@ static void run(void) {

> >  	double time_delta;

> >  	long int random_number;



> > -	while (max_block <= data_blocks) {

> > -		random_number = rand();

> > -		max_block = random_number % max_blks;

> > -		data_blocks = random_number % 1000 + 1;

> > -	}

> > +	random_number = rand();

> > +	max_block = random_number % max_blks + 1;

> > +	data_blocks = random_number % max_block;


> This fixes a potential infinite loop if max_blks == 1000. This
> calculation is also the reason why the test has random run length.

Thanks for an explanation!

> >  	for (i = 1; i <= data_blocks; i++) {

> >  		offset = i * ((BLOCKSIZE * max_block) / data_blocks);

> > -		offset -= BUFSIZ;

> > +		offset -= BUF_SIZE;


> Here the old calculation could produce negative offset if
> BUFSIZ > BLOCKSIZE and (float)max_block/data_blocks is close to 1.
> BUFSIZ is defined in libc headers so the actual value can be different
> on different systems.
BTW BUFSIZ == BLOCKSIZE for glibc (BUFSIZ is 8192; long time ago was 1024. And
it's 1024 for musl and bionic).


Kind regards,
Petr