[PATCH] src/seek_sanity_test: ensure file size is big enough

[PATCH] src/seek_sanity_test: ensure file size is big enough

[Luis Henriques]

Tests test07, test08, and test09 preallocate a file and assume the file
size used is bigger than 10xbufsz (100xbufsz for test09).  This patch
adjusts the file size so this assumption is always true.

As an example, here's test06 output for cephfs, where the allocation size
is set to 4194304, and the output is (4194304 * 10 + 4194304)

  07. Test file with unwritten extents, only have dirty pages
  07.01 SEEK_HOLE expected 0 or 4194304, got 46137344.              FAIL
  07.02 SEEK_HOLE expected 1 or 4194304, got 46137344.              FAIL

(Note: The test will be skipped if an integer overflow occurs.)

Signed-off-by: Luis Henriques <lhenriques@suse.com>
---
 src/seek_sanity_test.c | 21 +++++++++++++++++++++
 1 file changed, 21 insertions(+)

diff --git a/src/seek_sanity_test.c b/src/seek_sanity_test.c
index a6dd48cc257b..41da59801212 100644
--- a/src/seek_sanity_test.c
+++ b/src/seek_sanity_test.c
@@ -282,6 +282,13 @@ static int test09(int fd, int testnum)
 	int bufsz = alloc_size;
 	int filsz = 8 << 20;
 
+	while ((filsz < (bufsz * 100 + bufsz)) && (filsz > 0))
+		filsz <<= 1;
+	if (filsz <= 0) {
+		fprintf(stdout, "Test skipped due to int overflow.\n");
+		return ret;
+	}
+
 	/*
 	 * HOLE - unwritten DATA in dirty page - HOLE -
 	 * unwritten DATA in writeback page
@@ -338,6 +345,13 @@ static int test08(int fd, int testnum)
 	int bufsz = alloc_size;
 	int filsz = 4 << 20;
 
+	while ((filsz < (bufsz * 10 + bufsz)) && (filsz > 0))
+		filsz <<= 1;
+	if (filsz <= 0) {
+		fprintf(stdout, "Test skipped due to int overflow.\n");
+		return ret;
+	}
+
 	/* HOLE - unwritten DATA in writeback page */
 	/* Each unit is bufsz */
 	buf = do_malloc(bufsz);
@@ -387,6 +401,13 @@ static int test07(int fd, int testnum)
 	int bufsz = alloc_size;
 	int filsz = 4 << 20;
 
+	while ((filsz < (bufsz * 10 + bufsz)) && (filsz > 0))
+		filsz <<= 1;
+	if (filsz <= 0) {
+		fprintf(stdout, "Test skipped due to int overflow.\n");
+		return ret;
+	}
+
 	/* HOLE - unwritten DATA in dirty page */
 	/* Each unit is bufsz */
 	buf = do_malloc(bufsz);

Re: [PATCH] src/seek_sanity_test: ensure file size is big enough

[Eryu Guan]

On Fri, May 05, 2017 at 03:57:23PM +0100, Luis Henriques wrote:
> Tests test07, test08, and test09 preallocate a file and assume the file
> size used is bigger than 10xbufsz (100xbufsz for test09).  This patch
> adjusts the file size so this assumption is always true.
> 
> As an example, here's test06 output for cephfs, where the allocation size
                        ^^^^^^ meant test07?
> is set to 4194304, and the output is (4194304 * 10 + 4194304)
> 
>   07. Test file with unwritten extents, only have dirty pages
>   07.01 SEEK_HOLE expected 0 or 4194304, got 46137344.              FAIL
>   07.02 SEEK_HOLE expected 1 or 4194304, got 46137344.              FAIL
> 
> (Note: The test will be skipped if an integer overflow occurs.)
> 
> Signed-off-by: Luis Henriques <lhenriques@suse.com>
> ---
>  src/seek_sanity_test.c | 21 +++++++++++++++++++++
>  1 file changed, 21 insertions(+)
> 
> diff --git a/src/seek_sanity_test.c b/src/seek_sanity_test.c
> index a6dd48cc257b..41da59801212 100644
> --- a/src/seek_sanity_test.c
> +++ b/src/seek_sanity_test.c
> @@ -282,6 +282,13 @@ static int test09(int fd, int testnum)
>  	int bufsz = alloc_size;
>  	int filsz = 8 << 20;
>  
> +	while ((filsz < (bufsz * 100 + bufsz)) && (filsz > 0))
> +		filsz <<= 1;
> +	if (filsz <= 0) {
> +		fprintf(stdout, "Test skipped due to int overflow.\n");
> +		return ret;
> +	}
> +

Hmm, why not just set filsz to (bufsz * 100 + bufsz)? This works for me
on XFS/ext4/btrfs and NFS, I guess it should work for cepthfs too?

Thanks,
Eryu

>  	/*
>  	 * HOLE - unwritten DATA in dirty page - HOLE -
>  	 * unwritten DATA in writeback page
> @@ -338,6 +345,13 @@ static int test08(int fd, int testnum)
>  	int bufsz = alloc_size;
>  	int filsz = 4 << 20;
>  
> +	while ((filsz < (bufsz * 10 + bufsz)) && (filsz > 0))
> +		filsz <<= 1;
> +	if (filsz <= 0) {
> +		fprintf(stdout, "Test skipped due to int overflow.\n");
> +		return ret;
> +	}
> +
>  	/* HOLE - unwritten DATA in writeback page */
>  	/* Each unit is bufsz */
>  	buf = do_malloc(bufsz);
> @@ -387,6 +401,13 @@ static int test07(int fd, int testnum)
>  	int bufsz = alloc_size;
>  	int filsz = 4 << 20;
>  
> +	while ((filsz < (bufsz * 10 + bufsz)) && (filsz > 0))
> +		filsz <<= 1;
> +	if (filsz <= 0) {
> +		fprintf(stdout, "Test skipped due to int overflow.\n");
> +		return ret;
> +	}
> +
>  	/* HOLE - unwritten DATA in dirty page */
>  	/* Each unit is bufsz */
>  	buf = do_malloc(bufsz);
> --
> To unsubscribe from this list: send the line "unsubscribe fstests" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] src/seek_sanity_test: ensure file size is big enough

[Luis Henriques]

On Mon, May 08, 2017 at 04:52:34PM +0800, Eryu Guan wrote:
> On Fri, May 05, 2017 at 03:57:23PM +0100, Luis Henriques wrote:
> > Tests test07, test08, and test09 preallocate a file and assume the file
> > size used is bigger than 10xbufsz (100xbufsz for test09).  This patch
> > adjusts the file size so this assumption is always true.
> > 
> > As an example, here's test06 output for cephfs, where the allocation size
>                         ^^^^^^ meant test07?

ups, yeah I did.

> > is set to 4194304, and the output is (4194304 * 10 + 4194304)
> > 
> >   07. Test file with unwritten extents, only have dirty pages
> >   07.01 SEEK_HOLE expected 0 or 4194304, got 46137344.              FAIL
> >   07.02 SEEK_HOLE expected 1 or 4194304, got 46137344.              FAIL
> > 
> > (Note: The test will be skipped if an integer overflow occurs.)
> > 
> > Signed-off-by: Luis Henriques <lhenriques@suse.com>
> > ---
> >  src/seek_sanity_test.c | 21 +++++++++++++++++++++
> >  1 file changed, 21 insertions(+)
> > 
> > diff --git a/src/seek_sanity_test.c b/src/seek_sanity_test.c
> > index a6dd48cc257b..41da59801212 100644
> > --- a/src/seek_sanity_test.c
> > +++ b/src/seek_sanity_test.c
> > @@ -282,6 +282,13 @@ static int test09(int fd, int testnum)
> >  	int bufsz = alloc_size;
> >  	int filsz = 8 << 20;
> >  
> > +	while ((filsz < (bufsz * 100 + bufsz)) && (filsz > 0))
> > +		filsz <<= 1;
> > +	if (filsz <= 0) {
> > +		fprintf(stdout, "Test skipped due to int overflow.\n");
> > +		return ret;
> > +	}
> > +
> 
> Hmm, why not just set filsz to (bufsz * 100 + bufsz)? This works for me
> on XFS/ext4/btrfs and NFS, I guess it should work for cepthfs too?

Right, that should work too.  I've decided to keep it as a power of 2 as I
assumed the original author had a reason for using that value initially
(4Mb).  But yeah if no one objects, I'll submit v2 with your suggestion.

Cheers,
--
Luís

> 
> Thanks,
> Eryu
> 
> >  	/*
> >  	 * HOLE - unwritten DATA in dirty page - HOLE -
> >  	 * unwritten DATA in writeback page
> > @@ -338,6 +345,13 @@ static int test08(int fd, int testnum)
> >  	int bufsz = alloc_size;
> >  	int filsz = 4 << 20;
> >  
> > +	while ((filsz < (bufsz * 10 + bufsz)) && (filsz > 0))
> > +		filsz <<= 1;
> > +	if (filsz <= 0) {
> > +		fprintf(stdout, "Test skipped due to int overflow.\n");
> > +		return ret;
> > +	}
> > +
> >  	/* HOLE - unwritten DATA in writeback page */
> >  	/* Each unit is bufsz */
> >  	buf = do_malloc(bufsz);
> > @@ -387,6 +401,13 @@ static int test07(int fd, int testnum)
> >  	int bufsz = alloc_size;
> >  	int filsz = 4 << 20;
> >  
> > +	while ((filsz < (bufsz * 10 + bufsz)) && (filsz > 0))
> > +		filsz <<= 1;
> > +	if (filsz <= 0) {
> > +		fprintf(stdout, "Test skipped due to int overflow.\n");
> > +		return ret;
> > +	}
> > +
> >  	/* HOLE - unwritten DATA in dirty page */
> >  	/* Each unit is bufsz */
> >  	buf = do_malloc(bufsz);
> > --
> > To unsubscribe from this list: send the line "unsubscribe fstests" in
> > the body of a message to majordomo@vger.kernel.org
> > More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] src/seek_sanity_test: ensure file size is big enough

[Luis Henriques]

On Mon, May 08, 2017 at 10:37:23AM +0100, Luis Henriques wrote:
> On Mon, May 08, 2017 at 04:52:34PM +0800, Eryu Guan wrote:
> > On Fri, May 05, 2017 at 03:57:23PM +0100, Luis Henriques wrote:
> > > Tests test07, test08, and test09 preallocate a file and assume the file
> > > size used is bigger than 10xbufsz (100xbufsz for test09).  This patch
> > > adjusts the file size so this assumption is always true.
> > > 
> > > As an example, here's test06 output for cephfs, where the allocation size
> >                         ^^^^^^ meant test07?
> 
> ups, yeah I did.
> 
> > > is set to 4194304, and the output is (4194304 * 10 + 4194304)
> > > 
> > >   07. Test file with unwritten extents, only have dirty pages
> > >   07.01 SEEK_HOLE expected 0 or 4194304, got 46137344.              FAIL
> > >   07.02 SEEK_HOLE expected 1 or 4194304, got 46137344.              FAIL
> > > 
> > > (Note: The test will be skipped if an integer overflow occurs.)
> > > 
> > > Signed-off-by: Luis Henriques <lhenriques@suse.com>
> > > ---
> > >  src/seek_sanity_test.c | 21 +++++++++++++++++++++
> > >  1 file changed, 21 insertions(+)
> > > 
> > > diff --git a/src/seek_sanity_test.c b/src/seek_sanity_test.c
> > > index a6dd48cc257b..41da59801212 100644
> > > --- a/src/seek_sanity_test.c
> > > +++ b/src/seek_sanity_test.c
> > > @@ -282,6 +282,13 @@ static int test09(int fd, int testnum)
> > >  	int bufsz = alloc_size;
> > >  	int filsz = 8 << 20;
> > >  
> > > +	while ((filsz < (bufsz * 100 + bufsz)) && (filsz > 0))
> > > +		filsz <<= 1;
> > > +	if (filsz <= 0) {
> > > +		fprintf(stdout, "Test skipped due to int overflow.\n");
> > > +		return ret;
> > > +	}
> > > +
> > 
> > Hmm, why not just set filsz to (bufsz * 100 + bufsz)? This works for me
> > on XFS/ext4/btrfs and NFS, I guess it should work for cepthfs too?
> 
> Right, that should work too.  I've decided to keep it as a power of 2 as I
> assumed the original author had a reason for using that value initially
> (4Mb).  But yeah if no one objects, I'll submit v2 with your suggestion.
> 

BTW, do you think it's worth keeping that int overflow test with this
change?

Cheers,
--
Luís

Re: [PATCH] src/seek_sanity_test: ensure file size is big enough

[Eryu Guan]

On Mon, May 08, 2017 at 10:45:45AM +0100, Luis Henriques wrote:
> On Mon, May 08, 2017 at 10:37:23AM +0100, Luis Henriques wrote:
> > On Mon, May 08, 2017 at 04:52:34PM +0800, Eryu Guan wrote:
> > > On Fri, May 05, 2017 at 03:57:23PM +0100, Luis Henriques wrote:
> > > > Tests test07, test08, and test09 preallocate a file and assume the file
> > > > size used is bigger than 10xbufsz (100xbufsz for test09).  This patch
> > > > adjusts the file size so this assumption is always true.
> > > > 
> > > > As an example, here's test06 output for cephfs, where the allocation size
> > >                         ^^^^^^ meant test07?
> > 
> > ups, yeah I did.
> > 
> > > > is set to 4194304, and the output is (4194304 * 10 + 4194304)
> > > > 
> > > >   07. Test file with unwritten extents, only have dirty pages
> > > >   07.01 SEEK_HOLE expected 0 or 4194304, got 46137344.              FAIL
> > > >   07.02 SEEK_HOLE expected 1 or 4194304, got 46137344.              FAIL
> > > > 
> > > > (Note: The test will be skipped if an integer overflow occurs.)
> > > > 
> > > > Signed-off-by: Luis Henriques <lhenriques@suse.com>
> > > > ---
> > > >  src/seek_sanity_test.c | 21 +++++++++++++++++++++
> > > >  1 file changed, 21 insertions(+)
> > > > 
> > > > diff --git a/src/seek_sanity_test.c b/src/seek_sanity_test.c
> > > > index a6dd48cc257b..41da59801212 100644
> > > > --- a/src/seek_sanity_test.c
> > > > +++ b/src/seek_sanity_test.c
> > > > @@ -282,6 +282,13 @@ static int test09(int fd, int testnum)
> > > >  	int bufsz = alloc_size;
> > > >  	int filsz = 8 << 20;
> > > >  
> > > > +	while ((filsz < (bufsz * 100 + bufsz)) && (filsz > 0))
> > > > +		filsz <<= 1;
> > > > +	if (filsz <= 0) {
> > > > +		fprintf(stdout, "Test skipped due to int overflow.\n");
> > > > +		return ret;
> > > > +	}
> > > > +
> > > 
> > > Hmm, why not just set filsz to (bufsz * 100 + bufsz)? This works for me
> > > on XFS/ext4/btrfs and NFS, I guess it should work for cepthfs too?
> > 
> > Right, that should work too.  I've decided to keep it as a power of 2 as I
> > assumed the original author had a reason for using that value initially
> > (4Mb).  But yeah if no one objects, I'll submit v2 with your suggestion.
> > 
> 
> BTW, do you think it's worth keeping that int overflow test with this
> change?

I have no strong preference on this, but seems if filsz overflows int,
there might be something goes wrong, letting it fail is better than
skipping it sliently.

Thanks,
Eryu

[PATCH v2] src/seek_sanity_test: ensure file size is big enough

[Luis Henriques]

Tests test07, test08, and test09 preallocate a file and assume the file
size used is bigger than 10xbufsz (100xbufsz for test09).  This patch
adjusts the file size so this assumption is always true.

As an example, here's test07 output for cephfs, where the allocation size
is set to 4194304, and the output is (4194304 * 10 + 4194304)

  07. Test file with unwritten extents, only have dirty pages
  07.01 SEEK_HOLE expected 0 or 4194304, got 46137344.              FAIL
  07.02 SEEK_HOLE expected 1 or 4194304, got 46137344.              FAIL

Signed-off-by: Luis Henriques <lhenriques@suse.com>
---
changes since v1:
 - Adjust max file size directly instead of looping
 - Drop integer overflow test
 - Updated changelog

 src/seek_sanity_test.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/src/seek_sanity_test.c b/src/seek_sanity_test.c
index a6dd48cc257b..0f5c22e7f450 100644
--- a/src/seek_sanity_test.c
+++ b/src/seek_sanity_test.c
@@ -280,7 +280,7 @@ static int test09(int fd, int testnum)
 	int ret = 0;
 	char *buf = NULL;
 	int bufsz = alloc_size;
-	int filsz = 8 << 20;
+	int filsz = bufsz * 100 + bufsz;
 
 	/*
 	 * HOLE - unwritten DATA in dirty page - HOLE -
@@ -336,7 +336,7 @@ static int test08(int fd, int testnum)
 	int ret = 0;
 	char *buf = NULL;
 	int bufsz = alloc_size;
-	int filsz = 4 << 20;
+	int filsz = bufsz * 10 + bufsz;
 
 	/* HOLE - unwritten DATA in writeback page */
 	/* Each unit is bufsz */
@@ -385,7 +385,7 @@ static int test07(int fd, int testnum)
 	int ret = 0;
 	char *buf = NULL;
 	int bufsz = alloc_size;
-	int filsz = 4 << 20;
+	int filsz = bufsz * 10 + bufsz;
 
 	/* HOLE - unwritten DATA in dirty page */
 	/* Each unit is bufsz */