[PATCH 1/4] common: Fix file leak in _get_max_file_size

[PATCH 1/4] common: Fix file leak in _get_max_file_size

[Pavel Reichl]

This is obviously mostly problematic for FS lacking support for sparse
files.

Signed-off-by: Pavel Reichl <preichl@redhat.com>
---
 common/rc         | 1 +
 tests/generic/692 | 0
 2 files changed, 1 insertion(+)
 mode change 100644 => 100755 tests/generic/692

diff --git a/common/rc b/common/rc
index 228fcb37..c9078649 100644
--- a/common/rc
+++ b/common/rc
@@ -4637,6 +4637,7 @@ _get_max_file_size()
 			l=$m
 		fi
 	done
+	rm -f $testfile
 	echo $l
 }
 
diff --git a/tests/generic/692 b/tests/generic/692
old mode 100644
new mode 100755
-- 
2.37.3

[PATCH 2/4] g/299: speed up the test

[Pavel Reichl]

Move the _require_xfs_io_command "falloc" above the
potencially 'expensive' function _get_max_file_size

Signed-off-by: Pavel Reichl <preichl@redhat.com>
---
 tests/generic/299 | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tests/generic/299 b/tests/generic/299
index 8e1a112c..1bb371a3 100755
--- a/tests/generic/299
+++ b/tests/generic/299
@@ -24,6 +24,7 @@ _require_scratch
 _require_odirect
 _require_aio
 _require_block_device $SCRATCH_DEV
+_require_xfs_io_command "falloc"
 
 NUM_JOBS=$((4*LOAD_FACTOR))
 BLK_DEV_SIZE=`blockdev --getsz $SCRATCH_DEV`
@@ -96,7 +97,6 @@ filename=buffered-aio-verifier
 EOF
 
 _require_fio $fio_config
-_require_xfs_io_command "falloc"
 
 _scratch_mkfs >> $seqres.full 2>&1
 _scratch_mount
-- 
2.37.3

Re: [PATCH 2/4] g/299: speed up the test

[Zorro Lang]

On Thu, Sep 22, 2022 at 03:48:20PM +0200, Pavel Reichl wrote:
> Move the _require_xfs_io_command "falloc" above the
> potencially 'expensive' function _get_max_file_size
> 
> Signed-off-by: Pavel Reichl <preichl@redhat.com>
> ---

It's not for speeding up this test, I think you're trying to notrun this case
on a fs which doesn't support fallocate and sparse file but support truncate
syscall. Anyway I don't have objection on this patch.

>  tests/generic/299 | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/tests/generic/299 b/tests/generic/299
> index 8e1a112c..1bb371a3 100755
> --- a/tests/generic/299
> +++ b/tests/generic/299
> @@ -24,6 +24,7 @@ _require_scratch
>  _require_odirect
>  _require_aio
>  _require_block_device $SCRATCH_DEV
> +_require_xfs_io_command "falloc"
>  
>  NUM_JOBS=$((4*LOAD_FACTOR))
>  BLK_DEV_SIZE=`blockdev --getsz $SCRATCH_DEV`
> @@ -96,7 +97,6 @@ filename=buffered-aio-verifier
>  EOF
>  
>  _require_fio $fio_config
> -_require_xfs_io_command "falloc"
>  
>  _scratch_mkfs >> $seqres.full 2>&1
>  _scratch_mount
> -- 
> 2.37.3
> 

[PATCH 3/4] generic: cleanup test files

[Pavel Reichl]

There seems to be some seek_sanity_testfile files that are not
cleaned up and take up space.

-rwxr-xr-x. 1 root root 8.0G Sep 22 13:39 seek_sanity_testfile10

Signed-off-by: Pavel Reichl <preichl@redhat.com>
---
 tests/generic/285 | 2 +-
 tests/generic/436 | 2 +-
 tests/generic/445 | 2 +-
 tests/generic/448 | 2 +-
 4 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/tests/generic/285 b/tests/generic/285
index 6c755b5f..d1850021 100755
--- a/tests/generic/285
+++ b/tests/generic/285
@@ -28,7 +28,7 @@ _require_test_program "seek_sanity_test"
 # Override the default cleanup function.
 _cleanup()
 {
-	eval "rm -f $BASE_TEST_FILE.*"
+	rm -f $BASE_TEST_FILE*
 }
 
 _run_seek_sanity_test $BASE_TEST_FILE > $seqres.full 2>&1 ||
diff --git a/tests/generic/436 b/tests/generic/436
index 7fb24b41..d54af436 100755
--- a/tests/generic/436
+++ b/tests/generic/436
@@ -16,7 +16,7 @@ BASE_TEST_FILE=$TEST_DIR/seek_sanity_testfile
 # Override the default cleanup function.
 _cleanup()
 {
-	rm -f $tmp.* $BASE_TEST_FILE.*
+	rm -f $tmp.* $BASE_TEST_FILE*
 }
 
 # Import common functions.
diff --git a/tests/generic/445 b/tests/generic/445
index 05d1c5b3..d9e9f84b 100755
--- a/tests/generic/445
+++ b/tests/generic/445
@@ -16,7 +16,7 @@ BASE_TEST_FILE=$TEST_DIR/seek_sanity_testfile
 # Override the default cleanup function.
 _cleanup()
 {
-	rm -f $tmp.* $BASE_TEST_FILE.*
+	rm -f $tmp.* $BASE_TEST_FILE*
 }
 
 # Import common functions.
diff --git a/tests/generic/448 b/tests/generic/448
index d66967bc..11945549 100755
--- a/tests/generic/448
+++ b/tests/generic/448
@@ -15,7 +15,7 @@ BASE_TEST_FILE=$TEST_DIR/seek_sanity_testfile_$seq
 _cleanup()
 {
 	cd /
-	rm -f $tmp.* $BASE_TEST_FILE
+	rm -f $tmp.* $BASE_TEST_FILE*
 }
 
 # Import common functions.
-- 
2.37.3

Re: [PATCH 3/4] generic: cleanup test files

[Zorro Lang]

On Thu, Sep 22, 2022 at 03:48:21PM +0200, Pavel Reichl wrote:
> There seems to be some seek_sanity_testfile files that are not
> cleaned up and take up space.
> 
> -rwxr-xr-x. 1 root root 8.0G Sep 22 13:39 seek_sanity_testfile10
> 
> Signed-off-by: Pavel Reichl <preichl@redhat.com>
> ---
>  tests/generic/285 | 2 +-
>  tests/generic/436 | 2 +-
>  tests/generic/445 | 2 +-
>  tests/generic/448 | 2 +-
>  4 files changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/tests/generic/285 b/tests/generic/285
> index 6c755b5f..d1850021 100755
> --- a/tests/generic/285
> +++ b/tests/generic/285
> @@ -28,7 +28,7 @@ _require_test_program "seek_sanity_test"
>  # Override the default cleanup function.
>  _cleanup()
>  {
> -	eval "rm -f $BASE_TEST_FILE.*"
> +	rm -f $BASE_TEST_FILE*

OK, this eval looks not reasonable.

>  }
>  
>  _run_seek_sanity_test $BASE_TEST_FILE > $seqres.full 2>&1 ||
> diff --git a/tests/generic/436 b/tests/generic/436
> index 7fb24b41..d54af436 100755
> --- a/tests/generic/436
> +++ b/tests/generic/436
> @@ -16,7 +16,7 @@ BASE_TEST_FILE=$TEST_DIR/seek_sanity_testfile

I'm wondering two questions:
1) If these cases all use $TEST_DIR/seek_sanity_testfile, and don't really
   remove these seek_sanity_testfile* files, will them affect each testing
   result? Can we set BASE_TEST_FILE=$TEST_DIR/seek_sanity_testfile_$seq
   at least, to avoid they use same files?

2) If these $BASE_TEST_FILE* files won't take too much space, and won't affect
   later testing, how about keeping them in $TEST_DIR, then remove these
   specific _cleanup(). From my testing, after running g/285, g/436, g/445 and
   g/448, I got [1]. Looks like they only takes ~25M space.

Thanks,
Zorro

[1]
# du -h /mnt/test/
25M     /mnt/test/
# du -h /mnt/test/seek_sanity_testfile*
4.0K    /mnt/test/seek_sanity_testfile
0       /mnt/test/seek_sanity_testfile01
4.0K    /mnt/test/seek_sanity_testfile02
12K     /mnt/test/seek_sanity_testfile03
4.0K    /mnt/test/seek_sanity_testfile04
4.0K    /mnt/test/seek_sanity_testfile05
8.0K    /mnt/test/seek_sanity_testfile06
44K     /mnt/test/seek_sanity_testfile07
44K     /mnt/test/seek_sanity_testfile08
404K    /mnt/test/seek_sanity_testfile09
128K    /mnt/test/seek_sanity_testfile10
128K    /mnt/test/seek_sanity_testfile11
128K    /mnt/test/seek_sanity_testfile12
4.0M    /mnt/test/seek_sanity_testfile13
4.0M    /mnt/test/seek_sanity_testfile14
12M     /mnt/test/seek_sanity_testfile15
4.0M    /mnt/test/seek_sanity_testfile16
0       /mnt/test/seek_sanity_testfile17
0       /mnt/test/seek_sanity_testfile_44818


>  # Override the default cleanup function.
>  _cleanup()
>  {
> -	rm -f $tmp.* $BASE_TEST_FILE.*
> +	rm -f $tmp.* $BASE_TEST_FILE*
>  }
>  
>  # Import common functions.
> diff --git a/tests/generic/445 b/tests/generic/445
> index 05d1c5b3..d9e9f84b 100755
> --- a/tests/generic/445
> +++ b/tests/generic/445
> @@ -16,7 +16,7 @@ BASE_TEST_FILE=$TEST_DIR/seek_sanity_testfile
>  # Override the default cleanup function.
>  _cleanup()
>  {
> -	rm -f $tmp.* $BASE_TEST_FILE.*
> +	rm -f $tmp.* $BASE_TEST_FILE*
>  }
>  
>  # Import common functions.
> diff --git a/tests/generic/448 b/tests/generic/448
> index d66967bc..11945549 100755
> --- a/tests/generic/448
> +++ b/tests/generic/448
> @@ -15,7 +15,7 @@ BASE_TEST_FILE=$TEST_DIR/seek_sanity_testfile_$seq
>  _cleanup()
>  {
>  	cd /
> -	rm -f $tmp.* $BASE_TEST_FILE
> +	rm -f $tmp.* $BASE_TEST_FILE*



>  }
>  
>  # Import common functions.
> -- 
> 2.37.3
> 

Re: [PATCH 3/4] generic: cleanup test files

[Zorro Lang]

On Thu, Sep 22, 2022 at 04:55:17PM +0200, Pavel Reichl wrote:
> 2) If these $BASE_TEST_FILE* files won't take too much space, and won't affect
>    later testing, how about keeping them in $TEST_DIR, then remove these
>    specific _cleanup(). From my testing, after running g/285, g/436, g/445 and
>    g/448, I got [1]. Looks like they only takes ~25M space.
> 
>    Hi,
> 
>    as I mentioned in the commit message - in my case I'm getting:
>    8.0G  seek_sanity_testfile10
>    So I think it's good idea to solve it somehow.

OK, you can combine patch 1,3,4 into one patch, due to they all take too
much space on filesystem which doesn't support sparse file, so we make
sure these files be cleanup.

Re: [PATCH 3/4] generic: cleanup test files

[Pavel Reichl]

> I'm wondering two questions:
> 1) If these cases all use $TEST_DIR/seek_sanity_testfile, and don't really
>     remove these seek_sanity_testfile* files, will them affect each testing
>     result? Can we set BASE_TEST_FILE=$TEST_DIR/seek_sanity_testfile_$seq
>     at least, to avoid they use same files?
I'm happy to try to implement this idea.

Re: [PATCH 3/4] generic: cleanup test files

[Zorro Lang]

On Thu, Sep 22, 2022 at 07:05:57PM +0200, Pavel Reichl wrote:
> 
> > I'm wondering two questions:
> > 1) If these cases all use $TEST_DIR/seek_sanity_testfile, and don't really
> >     remove these seek_sanity_testfile* files, will them affect each testing
> >     result? Can we set BASE_TEST_FILE=$TEST_DIR/seek_sanity_testfile_$seq
> >     at least, to avoid they use same files?
> I'm happy to try to implement this idea.

If you need to remove them in cleanup, then the name won't conflict :)

> 

[PATCH 4/4] g/394: clean up test files

[Pavel Reichl]

Clean up test files taking space.

-rwxr-xr-x. 1 root root 1.0G Sep 22 14:58 394.1183899
-rwxr-xr-x. 1 root root 1.0G Sep 22 14:58 394.1183899-1
-rwxr-xr-x. 1 root root    0 sep 22 14:58 394.1183899+1

Signed-off-by: Pavel Reichl <preichl@redhat.com>
---
 tests/generic/394 | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tests/generic/394 b/tests/generic/394
index fc03e2f3..cbc2ce13 100755
--- a/tests/generic/394
+++ b/tests/generic/394
@@ -14,7 +14,7 @@ _cleanup()
 {
 	cd /
 	ulimit -f unlimited
-	rm -f $tmp.*
+	rm -f $tmp.* $TEST_DIR/$seq.*
 }
 
 # Import common functions.
-- 
2.37.3

Re: [PATCH 4/4] g/394: clean up test files

[Zorro Lang]

On Thu, Sep 22, 2022 at 03:48:22PM +0200, Pavel Reichl wrote:
> Clean up test files taking space.
> 
> -rwxr-xr-x. 1 root root 1.0G Sep 22 14:58 394.1183899
> -rwxr-xr-x. 1 root root 1.0G Sep 22 14:58 394.1183899-1
> -rwxr-xr-x. 1 root root    0 sep 22 14:58 394.1183899+1
> 
> Signed-off-by: Pavel Reichl <preichl@redhat.com>
> ---
>  tests/generic/394 | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/tests/generic/394 b/tests/generic/394
> index fc03e2f3..cbc2ce13 100755
> --- a/tests/generic/394
> +++ b/tests/generic/394
> @@ -14,7 +14,7 @@ _cleanup()
>  {
>  	cd /
>  	ulimit -f unlimited
> -	rm -f $tmp.*
> +	rm -f $tmp.* $TEST_DIR/$seq.*

I think you can combine this patch with patch 1/4, due to they're for same
purpose, all due to some fs (e.g. exfat) doesn't support sparse file, so
we need to remove those large files created by truncate. If you find same
issue on other places, better to fix them in same patch.

>  }
>  
>  # Import common functions.
> -- 
> 2.37.3
> 

Re: [PATCH 1/4] common: Fix file leak in _get_max_file_size

[Zorro Lang]

On Thu, Sep 22, 2022 at 03:48:19PM +0200, Pavel Reichl wrote:
> This is obviously mostly problematic for FS lacking support for sparse
> files.
> 
> Signed-off-by: Pavel Reichl <preichl@redhat.com>
> ---
>  common/rc         | 1 +
>  tests/generic/692 | 0
>  2 files changed, 1 insertion(+)
>  mode change 100644 => 100755 tests/generic/692
> 
> diff --git a/common/rc b/common/rc
> index 228fcb37..c9078649 100644
> --- a/common/rc
> +++ b/common/rc
> @@ -4637,6 +4637,7 @@ _get_max_file_size()
>  			l=$m
>  		fi
>  	done
> +	rm -f $testfile

OK, as "truncate" will allocate real space in exfat and other fs not
support sparse file, I think we'd better to remove it. If you find
more similar situation, please remove them in same one patch :)

>  	echo $l
>  }
>  
> diff --git a/tests/generic/692 b/tests/generic/692
> old mode 100644
> new mode 100755

Hmm... is this another fix? Although I don't know if it's worth to be a single
patch, if it doesn't cause anything wrong?

Thanks,
Zorro

> -- 
> 2.37.3
> 

Re: [PATCH 1/4] common: Fix file leak in _get_max_file_size

[Pavel Reichl]

>>   
>> diff --git a/tests/generic/692 b/tests/generic/692
>> old mode 100644
>> new mode 100755
> Hmm... is this another fix? Although I don't know if it's worth to be a single
> patch, if it doesn't cause anything wrong?
Hmm, sorry about this change, it's not on purpose, I'm not sure what 
happened here. I'll remove it in the next iteration

Re: [PATCH 1/4] common: Fix file leak in _get_max_file_size

[Eric Biggers]

On Thu, Sep 22, 2022 at 03:48:19PM +0200, Pavel Reichl wrote:
> This is obviously mostly problematic for FS lacking support for sparse
> files.
> 
> Signed-off-by: Pavel Reichl <preichl@redhat.com>
> ---
>  common/rc         | 1 +
>  tests/generic/692 | 0
>  2 files changed, 1 insertion(+)
>  mode change 100644 => 100755 tests/generic/692
> 
> diff --git a/common/rc b/common/rc
> index 228fcb37..c9078649 100644
> --- a/common/rc
> +++ b/common/rc
> @@ -4637,6 +4637,7 @@ _get_max_file_size()
>  			l=$m
>  		fi
>  	done
> +	rm -f $testfile
>  	echo $l
>  }

Removing the file is fine, but I didn't have filesystems that lack support for
sparse files in mind when I wrote this function.  Maybe it should look at $FSTYP
first, and only use the generic algorithm as a fallback for unhandled types?

- Eric

Re: [PATCH 1/4] common: Fix file leak in _get_max_file_size

[Zorro Lang]

On Thu, Sep 22, 2022 at 08:31:23PM -0700, Eric Biggers wrote:
> On Thu, Sep 22, 2022 at 03:48:19PM +0200, Pavel Reichl wrote:
> > This is obviously mostly problematic for FS lacking support for sparse
> > files.
> > 
> > Signed-off-by: Pavel Reichl <preichl@redhat.com>
> > ---
> >  common/rc         | 1 +
> >  tests/generic/692 | 0
> >  2 files changed, 1 insertion(+)
> >  mode change 100644 => 100755 tests/generic/692
> > 
> > diff --git a/common/rc b/common/rc
> > index 228fcb37..c9078649 100644
> > --- a/common/rc
> > +++ b/common/rc
> > @@ -4637,6 +4637,7 @@ _get_max_file_size()
> >  			l=$m
> >  		fi
> >  	done
> > +	rm -f $testfile
> >  	echo $l
> >  }
> 
> Removing the file is fine, but I didn't have filesystems that lack support for
> sparse files in mind when I wrote this function.  Maybe it should look at $FSTYP
> first, and only use the generic algorithm as a fallback for unhandled types?

That might be good idea. The current algorithm takes too long time for fs which
doesn't support sparse file.

Anyway, this $testfile should be removed. If Pavel would like to change this
function likes:

_get_max_file_size()
{
	case $FSTYP:
	exfat)
		# calculate the max file size of exfat according to blocksize, then
		echo $max_file_size_of_exfat
		;;
	# Add ext* if extN forks think it's better
	*)
		# By default, do the algorithm this function does originally
		;;
	esac
}

he can split this part as a separated patch, then let's review it. Or we can
have this patch at first, then let Eric change it when he'd like to do that.

Thanks,
Zorro

> 
> - Eric
>