On Tue, Jul 25, 2017 at 11:20:38AM -0700, Tahsin Erdogan wrote: > Eryu, thanks for the feedback! Thanks for the new test! :) > > >> +# FS QA Test 026 > >> +# > >> +# Test ea_inode feature. > > > > Better to have more information about ea_inode feature and what this > > case tests here too. > > Done. > > >> +_scratch_mkfs_ext4 -O ea_inode -I 256 -b 4096 >/dev/null 2>&1 > > > > Is 4k block size a hard requirement? If not, then we lose test coverage > > for 1k/2k block size ext4, if so, some comments on it would be better. > > (and the 256 inode size too). > > The actual block size is not that important for the purposes of this > test. However, knowing the block size helps simplifying the test > because we can hardcode xattr value sizes in the rest of the script. > I've set block size to 4k in case ext4 default changes in the future. In this case, what we generally do is filtering the output and make it consistent across different block size ext4 filesystems, instead of hardcoding the block size in test, so we have better test coverage. But I modified the test a bit to remove the block size specification and tried with 1k and 2k block size, test still passed. Seems there's no problem running the test with non-4k block size ext4. And as you said in v2 comments, 256 inode size is required to store some xattr in inode body, I think we can check the actual inode size after _scratch_mkfs and _notrun if it's smaller than 256. So we don't restrict the test to a certain test config. But again, I tried with 128B inode size and test passed too, even though it didn't test what we want exactly in this case. But that doesn't matter to me, adding some comments to explain the behavior when inode size is 128 would be OK. So I think a bare "_scratch_mkfs >/dev/null 2>&1" would be good for this test, we just need more comments to clarify the behaviors. Thanks, Eryu -- To unsubscribe from this list: send the line "unsubscribe fstests" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
