2014-02-04, Dave Chinner <da...@fromorbit.com>: > On Sun, Feb 02, 2014 at 02:45:58PM +0900, Namjae Jeon wrote: >> From: Namjae Jeon <namjae.j...@samsung.com> >> >> This testcase(001) tries to test various corner cases >> for fcollapse range functionality over different type of extents. >> >> Signed-off-by: Namjae Jeon <namjae.j...@samsung.com> >> Signed-off-by: Ashish Sangwan <a.sang...@samsung.com> > > 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/
