Re: [PATCH RESEND 5/10] xfstest: shared/001: Standard collapse range tests
2014-02-04, Dave Chinner : > On Sun, Feb 02, 2014 at 02:45:58PM +0900, Namjae Jeon wrote: >> From: Namjae Jeon >> >> This testcase(001) tries to test various corner cases >> for fcollapse range functionality over different type of extents. >> >> Signed-off-by: Namjae Jeon >> Signed-off-by: Ashish Sangwan > > Couple of things: > >> -c "$map_cmd -v" $testfile | $filter_cmd >> [ $? -ne 0 ] && die_now >> _md5_checksum $testfile >> @@ -415,10 +425,10 @@ _test_generic_punch() >> if [ "$remove_testfile" ]; then >> rm -f $testfile >> fi >> -$XFS_IO_PROG -f -c "truncate 20k" \ >> --c "$alloc_cmd 0 8k" \ >> --c "pwrite 8k 8k" $sync_cmd \ >> --c "$zero_cmd 4k 8k" \ >> +$XFS_IO_PROG -f -c "truncate $(($multiple * 20))k" \ >> +-c "$alloc_cmd 0 $(($multiple * 8))k" \ >> +-c "pwrite $(($multiple * 8))k $(($multiple * 8))k" >> $sync_cmd \ >> +-c "$zero_cmd $(($multiple * 4))k $(($multiple * 8))k" \ >> -c "$map_cmd -v" $testfile | $filter_cmd > Hi. Dave. > This is unreadable, and therefore I'd consider that these changes > render _test_generic_punch unmaintainable. > > Either it needs tobe factored to be more readable, or we need a more > readable way of representing the offsets and sizes if we want them > to be variable. For example: > > _4k="$((multiple * 4))k" > _8k="$((multiple * 8))k" > _20k="$((multiple * 20))k" > > leads to: > > $XFS_IO_PROG -f -c "truncate $_20k" \ > -c "$alloc_cmd 0 $_8k" \ > -c "pwrite $_8k $_8k" $sync_cmd \ > -c "$zero_cmd $_4k $_8k" \ > -c "$map_cmd -v" $testfile | $filter_cmd > > which is still readable and allows us to arbitrarily scale the > parameters. It even allows us to handle different filesystem block > sizes if we really want to Okay, I will change it as you suggest. > >> -c "$map_cmd -v" $testfile | $filter_cmd >> [ $? -ne 0 ] && die_now >> _md5_checksum $testfile >> >> +# If zero_cmd is fcollpase, don't check unaligned offsets >> +if [ "$zero_cmd" == "fcollapse" ]; then >> +if [ "$remove_testfile" ]; then >> +rm -f $testfile >> +rm -f $testfile.2 >> +fi >> +return >> +fi > > No need to remove the test files here - we remove them at > test startup to ensure we have a known initial state Okay. > >> +0: [0..63]: extent >> +bb7df04e1b0a2570657527a7e108ae23 >> +13. data -> unwritten -> data >> +0: [0..63]: extent >> +0f0151cbed83e4bf6e5bde26e82ab115 >> +14. data -> hole @ EOF >> +fallocate: Invalid argument >> +0: [0..159]: extent > > This error appears in all the golden outputs. If it's correct, then > perhaps it should be filtered out or commented somewhere to explain > why it is expected. Okay, I will add the comments to explain about this. Thanks for your review :) > > Cheers, > > Dave. > -- > Dave Chinner > da...@fromorbit.com > -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH RESEND 5/10] xfstest: shared/001: Standard collapse range tests
On Sun, Feb 02, 2014 at 02:45:58PM +0900, Namjae Jeon wrote: > From: Namjae Jeon > > This testcase(001) tries to test various corner cases > for fcollapse range functionality over different type of extents. > > Signed-off-by: Namjae Jeon > Signed-off-by: Ashish Sangwan Couple of things: > -c "$map_cmd -v" $testfile | $filter_cmd > [ $? -ne 0 ] && die_now > _md5_checksum $testfile > @@ -415,10 +425,10 @@ _test_generic_punch() > if [ "$remove_testfile" ]; then > rm -f $testfile > fi > - $XFS_IO_PROG -f -c "truncate 20k" \ > - -c "$alloc_cmd 0 8k" \ > - -c "pwrite 8k 8k" $sync_cmd \ > - -c "$zero_cmd 4k 8k" \ > + $XFS_IO_PROG -f -c "truncate $(($multiple * 20))k" \ > + -c "$alloc_cmd 0 $(($multiple * 8))k" \ > + -c "pwrite $(($multiple * 8))k $(($multiple * 8))k" > $sync_cmd \ > + -c "$zero_cmd $(($multiple * 4))k $(($multiple * 8))k" \ > -c "$map_cmd -v" $testfile | $filter_cmd This is unreadable, and therefore I'd consider that these changes render _test_generic_punch unmaintainable. Either it needs tobe factored to be more readable, or we need a more readable way of representing the offsets and sizes if we want them to be variable. For example: _4k="$((multiple * 4))k" _8k="$((multiple * 8))k" _20k="$((multiple * 20))k" leads to: $XFS_IO_PROG -f -c "truncate $_20k" \ -c "$alloc_cmd 0 $_8k" \ -c "pwrite $_8k $_8k" $sync_cmd \ -c "$zero_cmd $_4k $_8k" \ -c "$map_cmd -v" $testfile | $filter_cmd which is still readable and allows us to arbitrarily scale the parameters. It even allows us to handle different filesystem block sizes if we really want to > -c "$map_cmd -v" $testfile | $filter_cmd > [ $? -ne 0 ] && die_now > _md5_checksum $testfile > > + # If zero_cmd is fcollpase, don't check unaligned offsets > + if [ "$zero_cmd" == "fcollapse" ]; then > + if [ "$remove_testfile" ]; then > + rm -f $testfile > + rm -f $testfile.2 > + fi > + return > + fi No need to remove the test files here - we remove them at test startup to ensure we have a known initial state > +0: [0..63]: extent > +bb7df04e1b0a2570657527a7e108ae23 > + 13. data -> unwritten -> data > +0: [0..63]: extent > +0f0151cbed83e4bf6e5bde26e82ab115 > + 14. data -> hole @ EOF > +fallocate: Invalid argument > +0: [0..159]: extent This error appears in all the golden outputs. If it's correct, then perhaps it should be filtered out or commented somewhere to explain why it is expected. Cheers, Dave. -- Dave Chinner da...@fromorbit.com -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
[PATCH RESEND 5/10] xfstest: shared/001: Standard collapse range tests
From: Namjae Jeon This testcase(001) tries to test various corner cases for fcollapse range functionality over different type of extents. Signed-off-by: Namjae Jeon Signed-off-by: Ashish Sangwan --- common/punch | 133 -- common/rc| 14 ++ tests/shared/001 | 65 tests/shared/001.out | 53 tests/shared/group |1 + 5 files changed, 209 insertions(+), 57 deletions(-) create mode 100644 tests/shared/001 create mode 100644 tests/shared/001.out diff --git a/common/punch b/common/punch index a49638c..f401455 100644 --- a/common/punch +++ b/common/punch @@ -317,13 +317,23 @@ _test_generic_punch() map_cmd=$4 filter_cmd=$5 testfile=$6 + multiple=1 + + # + # If we are testing collapse range, we increare all the offsets of this + # test by a factor of 4. We do this because unlike punch, collapse + # range also decreases the size of file hence require bigger offsets. + # + if [ "$zero_cmd" == "fcollapse" ]; then + multiple=4 + fi echo " 1. into a hole" if [ "$remove_testfile" ]; then rm -f $testfile fi - $XFS_IO_PROG -f -c "truncate 20k" \ - -c "$zero_cmd 4k 8k" \ + $XFS_IO_PROG -f -c "truncate $(($multiple * 20))k" \ + -c "$zero_cmd $(($multiple * 4))k $(($multiple * 8))k" \ -c "$map_cmd -v" $testfile | $filter_cmd [ $? -ne 0 ] && die_now _md5_checksum $testfile @@ -332,9 +342,9 @@ _test_generic_punch() if [ "$remove_testfile" ]; then rm -f $testfile fi - $XFS_IO_PROG -f -c "truncate 20k" \ - -c "pwrite 0 20k" $sync_cmd \ - -c "$zero_cmd 4k 8k" \ + $XFS_IO_PROG -f -c "truncate $(($multiple * 20))k" \ + -c "pwrite 0 $(($multiple * 20))k" $sync_cmd \ + -c "$zero_cmd $(($multiple * 4))k $(($multiple * 8))k" \ -c "$map_cmd -v" $testfile | $filter_cmd [ $? -ne 0 ] && die_now _md5_checksum $testfile @@ -344,9 +354,9 @@ _test_generic_punch() if [ "$remove_testfile" ]; then rm -f $testfile fi - $XFS_IO_PROG -f -c "truncate 20k" \ - -c "$alloc_cmd 0 20k" \ - -c "$zero_cmd 4k 8k" \ + $XFS_IO_PROG -f -c "truncate $(($multiple * 20))k" \ + -c "$alloc_cmd 0 $(($multiple * 20))k" \ + -c "$zero_cmd $(($multiple * 4))k $(($multiple * 8))k" \ -c "$map_cmd -v" $testfile | $filter_cmd [ $? -ne 0 ] && die_now _md5_checksum $testfile @@ -356,9 +366,9 @@ _test_generic_punch() if [ "$remove_testfile" ]; then rm -f $testfile fi - $XFS_IO_PROG -f -c "truncate 20k" \ - -c "pwrite 8k 8k" $sync_cmd \ - -c "$zero_cmd 4k 8k" \ + $XFS_IO_PROG -f -c "truncate $(($multiple * 20))k" \ + -c "pwrite $(($multiple * 8))k $(($multiple * 8))k" $sync_cmd \ + -c "$zero_cmd $(($multiple * 4))k $(($multiple * 8))k" \ -c "$map_cmd -v" $testfile | $filter_cmd [ $? -ne 0 ] && die_now _md5_checksum $testfile @@ -368,9 +378,9 @@ _test_generic_punch() if [ "$remove_testfile" ]; then rm -f $testfile fi - $XFS_IO_PROG -f -c "truncate 20k" \ - -c "$alloc_cmd 8k 8k" \ - -c "$zero_cmd 4k 8k" \ + $XFS_IO_PROG -f -c "truncate $(($multiple * 20))k" \ + -c "$alloc_cmd $(($multiple * 8))k $(($multiple * 8))k" \ + -c "$zero_cmd $(($multiple * 4))k $(($multiple * 8))k" \ -c "$map_cmd -v" $testfile | $filter_cmd [ $? -ne 0 ] && die_now _md5_checksum $testfile @@ -380,9 +390,9 @@ _test_generic_punch() if [ "$remove_testfile" ]; then rm -f $testfile fi - $XFS_IO_PROG -f -c "truncate 20k" \ - -c "pwrite 0 8k" $sync_cmd \ - -c "$zero_cmd 4k 8k" \ + $XFS_IO_PROG -f -c "truncate $(($multiple * 20))k" \ + -c "pwrite 0 $(($multiple * 8))k" $sync_cmd \ +-c "$zero_cmd $(($multiple * 4))k $(($multiple * 8))k" \ -c "$map_cmd -v" $testfile | $filter_cmd [ $? -ne 0 ] && die_now _md5_checksum $testfile @@ -392,10 +402,10 @@ _test_generic_punch() if [ "$remove_testfile" ]; then rm -f $testfile fi - $XFS_IO_PROG -f -c "truncate 20k" \ - -c "pwrite 0 8k" $sync_cmd \ - -c "$alloc_cmd 8k 8k" \ -
