| Message ID | 20230911200617.777985-1-aalbersh@redhat.com (mailing list archive) |
|---|---|
| State | Accepted |
| Headers | show |
| Series | common/rc: make _get_max_file_size find file size on mount point | expand |
On Mon, Sep 11, 2023 at 10:06:17PM +0200, Andrey Albershteyn wrote: > Currently, _get_max_file_size finds max file size on $TEST_DIR. > The tests/generic/692 uses this function to detect file size and > then tries to create a file on $SCRATCH_MNT. > > This works fine when test and scratch filesystems have the same > block size. However, it will fail if they differ, for example, TEST > is 4k and SCRATCH is 1k (file will be too big). I thought we generally use same mkfs options on TEST_DEV and SCRATCH_DEV. If you'd like to test 1k blocksize ext4, shouldn't you make 1k blocksize ext4 on TEST_DEV before testing. Thanks, Zorro > > Found this by running generic/692 on ext4 with -b 1024. > > Make _get_max_file_size accept mount point on which to detect max > file size. > > Signed-off-by: Andrey Albershteyn <aalbersh@redhat.com> > --- > common/rc | 8 +++++++- > tests/generic/299 | 2 +- > tests/generic/485 | 2 +- > tests/generic/692 | 2 +- > 4 files changed, 10 insertions(+), 4 deletions(-) > > diff --git a/common/rc b/common/rc > index 68d2ad04..d17ec73a 100644 > --- a/common/rc > +++ b/common/rc > @@ -4809,7 +4809,13 @@ _require_scratch_feature() > # be UINT32_MAX * block_size, but other filesystems may allow up to LLONG_MAX. > _get_max_file_size() > { > - local testfile=$TEST_DIR/maxfilesize.$seq > + if [ -z $1 ] || [ ! -d $1 ]; then > + echo "Missing mount point argument for _get_max_file_size" > + exit 1 > + fi > + > + local mnt=$1 > + local testfile=$mnt/maxfilesize.$seq > local l=0 > local r=9223372036854775807 # LLONG_MAX > > diff --git a/tests/generic/299 b/tests/generic/299 > index d8ecff53..0cd12202 100755 > --- a/tests/generic/299 > +++ b/tests/generic/299 > @@ -30,7 +30,7 @@ NUM_JOBS=$((4*LOAD_FACTOR)) > BLK_DEV_SIZE=`blockdev --getsz $SCRATCH_DEV` > FILE_SIZE=$((BLK_DEV_SIZE * 512)) > > -max_file_size=$(_get_max_file_size) > +max_file_size=$(_get_max_file_size $TEST_DIR) > if [ $max_file_size -lt $FILE_SIZE ]; then > FILE_SIZE=$max_file_size > fi > diff --git a/tests/generic/485 b/tests/generic/485 > index 3f7749ff..8bab450b 100755 > --- a/tests/generic/485 > +++ b/tests/generic/485 > @@ -30,7 +30,7 @@ _require_xfs_io_command "finsert" > _require_xfs_io_command "truncate" > > block_size=$(_get_file_block_size $TEST_DIR) > -max_file_size=$(_get_max_file_size) > +max_file_size=$(_get_max_file_size $TEST_DIR) > max_blocks=$((max_file_size / block_size)) > testfile=$TEST_DIR/testfile.$seq > > diff --git a/tests/generic/692 b/tests/generic/692 > index 95f6ec04..3fb8ac01 100755 > --- a/tests/generic/692 > +++ b/tests/generic/692 > @@ -40,7 +40,7 @@ _scratch_mount > > fsv_file=$SCRATCH_MNT/file.fsv > > -max_sz=$(_get_max_file_size) > +max_sz=$(_get_max_file_size $SCRATCH_MNT) > _fsv_scratch_begin_subtest "way too big: fail on first merkle block" > truncate -s $max_sz $fsv_file > _fsv_enable $fsv_file |& _filter_scratch > -- > 2.40.1 >
On Tue, Sep 12, 2023 at 08:42:13PM +0800, Zorro Lang wrote: > On Mon, Sep 11, 2023 at 10:06:17PM +0200, Andrey Albershteyn wrote: > > Currently, _get_max_file_size finds max file size on $TEST_DIR. > > The tests/generic/692 uses this function to detect file size and > > then tries to create a file on $SCRATCH_MNT. > > > > This works fine when test and scratch filesystems have the same > > block size. However, it will fail if they differ, for example, TEST > > is 4k and SCRATCH is 1k (file will be too big). > > I thought we generally use same mkfs options on TEST_DEV and SCRATCH_DEV. > If you'd like to test 1k blocksize ext4, shouldn't you make 1k blocksize > ext4 on TEST_DEV before testing. Agreed, TEST_DEV (AFAIK) is supposed to have the same options as SCRATCH_MNT. That said, I support adding the flexibility to supply a mount point, since that would be generally useful given the number of tests that add things to MKFS_OPTIONS... > Thanks, > Zorro > > > > > Found this by running generic/692 on ext4 with -b 1024. > > > > Make _get_max_file_size accept mount point on which to detect max > > file size. > > > > Signed-off-by: Andrey Albershteyn <aalbersh@redhat.com> > > --- > > common/rc | 8 +++++++- > > tests/generic/299 | 2 +- > > tests/generic/485 | 2 +- > > tests/generic/692 | 2 +- > > 4 files changed, 10 insertions(+), 4 deletions(-) > > > > diff --git a/common/rc b/common/rc > > index 68d2ad04..d17ec73a 100644 > > --- a/common/rc > > +++ b/common/rc > > @@ -4809,7 +4809,13 @@ _require_scratch_feature() > > # be UINT32_MAX * block_size, but other filesystems may allow up to LLONG_MAX. > > _get_max_file_size() > > { > > - local testfile=$TEST_DIR/maxfilesize.$seq > > + if [ -z $1 ] || [ ! -d $1 ]; then > > + echo "Missing mount point argument for _get_max_file_size" > > + exit 1 > > + fi > > + > > + local mnt=$1 > > + local testfile=$mnt/maxfilesize.$seq > > local l=0 > > local r=9223372036854775807 # LLONG_MAX > > > > diff --git a/tests/generic/299 b/tests/generic/299 > > index d8ecff53..0cd12202 100755 > > --- a/tests/generic/299 > > +++ b/tests/generic/299 > > @@ -30,7 +30,7 @@ NUM_JOBS=$((4*LOAD_FACTOR)) > > BLK_DEV_SIZE=`blockdev --getsz $SCRATCH_DEV` > > FILE_SIZE=$((BLK_DEV_SIZE * 512)) > > > > -max_file_size=$(_get_max_file_size) > > +max_file_size=$(_get_max_file_size $TEST_DIR) > > if [ $max_file_size -lt $FILE_SIZE ]; then > > FILE_SIZE=$max_file_size > > fi > > diff --git a/tests/generic/485 b/tests/generic/485 > > index 3f7749ff..8bab450b 100755 > > --- a/tests/generic/485 > > +++ b/tests/generic/485 > > @@ -30,7 +30,7 @@ _require_xfs_io_command "finsert" > > _require_xfs_io_command "truncate" > > > > block_size=$(_get_file_block_size $TEST_DIR) > > -max_file_size=$(_get_max_file_size) > > +max_file_size=$(_get_max_file_size $TEST_DIR) > > max_blocks=$((max_file_size / block_size)) > > testfile=$TEST_DIR/testfile.$seq > > > > diff --git a/tests/generic/692 b/tests/generic/692 > > index 95f6ec04..3fb8ac01 100755 > > --- a/tests/generic/692 > > +++ b/tests/generic/692 > > @@ -40,7 +40,7 @@ _scratch_mount > > > > fsv_file=$SCRATCH_MNT/file.fsv > > > > -max_sz=$(_get_max_file_size) > > +max_sz=$(_get_max_file_size $SCRATCH_MNT) ...LGTM, so Reviewed-by: Darrick J. Wong <djwong@kernel.org> --D > > _fsv_scratch_begin_subtest "way too big: fail on first merkle block" > > truncate -s $max_sz $fsv_file > > _fsv_enable $fsv_file |& _filter_scratch > > -- > > 2.40.1 > > >
On 2023-09-12 07:56:25, Darrick J. Wong wrote: > On Tue, Sep 12, 2023 at 08:42:13PM +0800, Zorro Lang wrote: > > On Mon, Sep 11, 2023 at 10:06:17PM +0200, Andrey Albershteyn wrote: > > > Currently, _get_max_file_size finds max file size on $TEST_DIR. > > > The tests/generic/692 uses this function to detect file size and > > > then tries to create a file on $SCRATCH_MNT. > > > > > > This works fine when test and scratch filesystems have the same > > > block size. However, it will fail if they differ, for example, TEST > > > is 4k and SCRATCH is 1k (file will be too big). > > > > I thought we generally use same mkfs options on TEST_DEV and SCRATCH_DEV. > > If you'd like to test 1k blocksize ext4, shouldn't you make 1k blocksize > > ext4 on TEST_DEV before testing. > > Agreed, TEST_DEV (AFAIK) is supposed to have the same options as > SCRATCH_MNT. Make sense but I didn't know that this is required, would it make sense to add a check for at least block size? I was running -s ext4_1k -s ext4_4k without recreating the TEST_DEV and that wasn't obvious where the problem is as test is using SCRATCH. > > That said, I support adding the flexibility to supply a mount point, > since that would be generally useful given the number of tests that add > things to MKFS_OPTIONS... > > > Thanks, > > Zorro > > > > > > > > Found this by running generic/692 on ext4 with -b 1024. > > > > > > Make _get_max_file_size accept mount point on which to detect max > > > file size. > > > > > > Signed-off-by: Andrey Albershteyn <aalbersh@redhat.com> > > > --- > > > common/rc | 8 +++++++- > > > tests/generic/299 | 2 +- > > > tests/generic/485 | 2 +- > > > tests/generic/692 | 2 +- > > > 4 files changed, 10 insertions(+), 4 deletions(-) > > > > > > diff --git a/common/rc b/common/rc > > > index 68d2ad04..d17ec73a 100644 > > > --- a/common/rc > > > +++ b/common/rc > > > @@ -4809,7 +4809,13 @@ _require_scratch_feature() > > > # be UINT32_MAX * block_size, but other filesystems may allow up to LLONG_MAX. > > > _get_max_file_size() > > > { > > > - local testfile=$TEST_DIR/maxfilesize.$seq > > > + if [ -z $1 ] || [ ! -d $1 ]; then > > > + echo "Missing mount point argument for _get_max_file_size" > > > + exit 1 > > > + fi > > > + > > > + local mnt=$1 > > > + local testfile=$mnt/maxfilesize.$seq > > > local l=0 > > > local r=9223372036854775807 # LLONG_MAX > > > > > > diff --git a/tests/generic/299 b/tests/generic/299 > > > index d8ecff53..0cd12202 100755 > > > --- a/tests/generic/299 > > > +++ b/tests/generic/299 > > > @@ -30,7 +30,7 @@ NUM_JOBS=$((4*LOAD_FACTOR)) > > > BLK_DEV_SIZE=`blockdev --getsz $SCRATCH_DEV` > > > FILE_SIZE=$((BLK_DEV_SIZE * 512)) > > > > > > -max_file_size=$(_get_max_file_size) > > > +max_file_size=$(_get_max_file_size $TEST_DIR) > > > if [ $max_file_size -lt $FILE_SIZE ]; then > > > FILE_SIZE=$max_file_size > > > fi > > > diff --git a/tests/generic/485 b/tests/generic/485 > > > index 3f7749ff..8bab450b 100755 > > > --- a/tests/generic/485 > > > +++ b/tests/generic/485 > > > @@ -30,7 +30,7 @@ _require_xfs_io_command "finsert" > > > _require_xfs_io_command "truncate" > > > > > > block_size=$(_get_file_block_size $TEST_DIR) > > > -max_file_size=$(_get_max_file_size) > > > +max_file_size=$(_get_max_file_size $TEST_DIR) > > > max_blocks=$((max_file_size / block_size)) > > > testfile=$TEST_DIR/testfile.$seq > > > > > > diff --git a/tests/generic/692 b/tests/generic/692 > > > index 95f6ec04..3fb8ac01 100755 > > > --- a/tests/generic/692 > > > +++ b/tests/generic/692 > > > @@ -40,7 +40,7 @@ _scratch_mount > > > > > > fsv_file=$SCRATCH_MNT/file.fsv > > > > > > -max_sz=$(_get_max_file_size) > > > +max_sz=$(_get_max_file_size $SCRATCH_MNT) > > ...LGTM, so > Reviewed-by: Darrick J. Wong <djwong@kernel.org> > Thanks! > --D > > > > _fsv_scratch_begin_subtest "way too big: fail on first merkle block" > > > truncate -s $max_sz $fsv_file > > > _fsv_enable $fsv_file |& _filter_scratch > > > -- > > > 2.40.1 > > > > > >
On Wed, Sep 13, 2023 at 10:44:50AM +0200, Andrey Albershteyn wrote: > On 2023-09-12 07:56:25, Darrick J. Wong wrote: > > On Tue, Sep 12, 2023 at 08:42:13PM +0800, Zorro Lang wrote: > > > On Mon, Sep 11, 2023 at 10:06:17PM +0200, Andrey Albershteyn wrote: > > > > Currently, _get_max_file_size finds max file size on $TEST_DIR. > > > > The tests/generic/692 uses this function to detect file size and > > > > then tries to create a file on $SCRATCH_MNT. > > > > > > > > This works fine when test and scratch filesystems have the same > > > > block size. However, it will fail if they differ, for example, TEST > > > > is 4k and SCRATCH is 1k (file will be too big). > > > > > > I thought we generally use same mkfs options on TEST_DEV and SCRATCH_DEV. > > > If you'd like to test 1k blocksize ext4, shouldn't you make 1k blocksize > > > ext4 on TEST_DEV before testing. > > > > Agreed, TEST_DEV (AFAIK) is supposed to have the same options as > > SCRATCH_MNT. > > Make sense but I didn't know that this is required, would it make > sense to add a check for at least block size? I was running -s > ext4_1k -s ext4_4k without recreating the TEST_DEV and that wasn't > obvious where the problem is as test is using SCRATCH. I remembered there's a global parameter named "RECREATE_TEST_DEV", if it's "true", the TEST_DEV will be recreated after a section testing done. You can check if it works :) > > > > > That said, I support adding the flexibility to supply a mount point, > > since that would be generally useful given the number of tests that add > > things to MKFS_OPTIONS... > > > > > Thanks, > > > Zorro > > > > > > > > > > > Found this by running generic/692 on ext4 with -b 1024. > > > > > > > > Make _get_max_file_size accept mount point on which to detect max > > > > file size. > > > > > > > > Signed-off-by: Andrey Albershteyn <aalbersh@redhat.com> > > > > --- > > > > common/rc | 8 +++++++- > > > > tests/generic/299 | 2 +- > > > > tests/generic/485 | 2 +- > > > > tests/generic/692 | 2 +- > > > > 4 files changed, 10 insertions(+), 4 deletions(-) > > > > > > > > diff --git a/common/rc b/common/rc > > > > index 68d2ad04..d17ec73a 100644 > > > > --- a/common/rc > > > > +++ b/common/rc > > > > @@ -4809,7 +4809,13 @@ _require_scratch_feature() > > > > # be UINT32_MAX * block_size, but other filesystems may allow up to LLONG_MAX. > > > > _get_max_file_size() > > > > { > > > > - local testfile=$TEST_DIR/maxfilesize.$seq > > > > + if [ -z $1 ] || [ ! -d $1 ]; then > > > > + echo "Missing mount point argument for _get_max_file_size" > > > > + exit 1 > > > > + fi > > > > + > > > > + local mnt=$1 > > > > + local testfile=$mnt/maxfilesize.$seq > > > > local l=0 > > > > local r=9223372036854775807 # LLONG_MAX > > > > > > > > diff --git a/tests/generic/299 b/tests/generic/299 > > > > index d8ecff53..0cd12202 100755 > > > > --- a/tests/generic/299 > > > > +++ b/tests/generic/299 > > > > @@ -30,7 +30,7 @@ NUM_JOBS=$((4*LOAD_FACTOR)) > > > > BLK_DEV_SIZE=`blockdev --getsz $SCRATCH_DEV` > > > > FILE_SIZE=$((BLK_DEV_SIZE * 512)) > > > > > > > > -max_file_size=$(_get_max_file_size) > > > > +max_file_size=$(_get_max_file_size $TEST_DIR) > > > > if [ $max_file_size -lt $FILE_SIZE ]; then > > > > FILE_SIZE=$max_file_size > > > > fi > > > > diff --git a/tests/generic/485 b/tests/generic/485 > > > > index 3f7749ff..8bab450b 100755 > > > > --- a/tests/generic/485 > > > > +++ b/tests/generic/485 > > > > @@ -30,7 +30,7 @@ _require_xfs_io_command "finsert" > > > > _require_xfs_io_command "truncate" > > > > > > > > block_size=$(_get_file_block_size $TEST_DIR) > > > > -max_file_size=$(_get_max_file_size) > > > > +max_file_size=$(_get_max_file_size $TEST_DIR) > > > > max_blocks=$((max_file_size / block_size)) > > > > testfile=$TEST_DIR/testfile.$seq > > > > > > > > diff --git a/tests/generic/692 b/tests/generic/692 > > > > index 95f6ec04..3fb8ac01 100755 > > > > --- a/tests/generic/692 > > > > +++ b/tests/generic/692 > > > > @@ -40,7 +40,7 @@ _scratch_mount > > > > > > > > fsv_file=$SCRATCH_MNT/file.fsv > > > > > > > > -max_sz=$(_get_max_file_size) > > > > +max_sz=$(_get_max_file_size $SCRATCH_MNT) > > > > ...LGTM, so > > Reviewed-by: Darrick J. Wong <djwong@kernel.org> > > > > Thanks! > > > --D > > > > > > _fsv_scratch_begin_subtest "way too big: fail on first merkle block" > > > > truncate -s $max_sz $fsv_file > > > > _fsv_enable $fsv_file |& _filter_scratch > > > > -- > > > > 2.40.1 > > > > > > > > > > > -- > - Andrey >
diff --git a/common/rc b/common/rc index 68d2ad04..d17ec73a 100644 --- a/common/rc +++ b/common/rc @@ -4809,7 +4809,13 @@ _require_scratch_feature() # be UINT32_MAX * block_size, but other filesystems may allow up to LLONG_MAX. _get_max_file_size() { - local testfile=$TEST_DIR/maxfilesize.$seq + if [ -z $1 ] || [ ! -d $1 ]; then + echo "Missing mount point argument for _get_max_file_size" + exit 1 + fi + + local mnt=$1 + local testfile=$mnt/maxfilesize.$seq local l=0 local r=9223372036854775807 # LLONG_MAX diff --git a/tests/generic/299 b/tests/generic/299 index d8ecff53..0cd12202 100755 --- a/tests/generic/299 +++ b/tests/generic/299 @@ -30,7 +30,7 @@ NUM_JOBS=$((4*LOAD_FACTOR)) BLK_DEV_SIZE=`blockdev --getsz $SCRATCH_DEV` FILE_SIZE=$((BLK_DEV_SIZE * 512)) -max_file_size=$(_get_max_file_size) +max_file_size=$(_get_max_file_size $TEST_DIR) if [ $max_file_size -lt $FILE_SIZE ]; then FILE_SIZE=$max_file_size fi diff --git a/tests/generic/485 b/tests/generic/485 index 3f7749ff..8bab450b 100755 --- a/tests/generic/485 +++ b/tests/generic/485 @@ -30,7 +30,7 @@ _require_xfs_io_command "finsert" _require_xfs_io_command "truncate" block_size=$(_get_file_block_size $TEST_DIR) -max_file_size=$(_get_max_file_size) +max_file_size=$(_get_max_file_size $TEST_DIR) max_blocks=$((max_file_size / block_size)) testfile=$TEST_DIR/testfile.$seq diff --git a/tests/generic/692 b/tests/generic/692 index 95f6ec04..3fb8ac01 100755 --- a/tests/generic/692 +++ b/tests/generic/692 @@ -40,7 +40,7 @@ _scratch_mount fsv_file=$SCRATCH_MNT/file.fsv -max_sz=$(_get_max_file_size) +max_sz=$(_get_max_file_size $SCRATCH_MNT) _fsv_scratch_begin_subtest "way too big: fail on first merkle block" truncate -s $max_sz $fsv_file _fsv_enable $fsv_file |& _filter_scratch
Currently, _get_max_file_size finds max file size on $TEST_DIR. The tests/generic/692 uses this function to detect file size and then tries to create a file on $SCRATCH_MNT. This works fine when test and scratch filesystems have the same block size. However, it will fail if they differ, for example, TEST is 4k and SCRATCH is 1k (file will be too big). Found this by running generic/692 on ext4 with -b 1024. Make _get_max_file_size accept mount point on which to detect max file size. Signed-off-by: Andrey Albershteyn <aalbersh@redhat.com> --- common/rc | 8 +++++++- tests/generic/299 | 2 +- tests/generic/485 | 2 +- tests/generic/692 | 2 +- 4 files changed, 10 insertions(+), 4 deletions(-)