Message ID | 20201021062554.68132-2-wqu@suse.com (mailing list archive) |
---|---|
State | New, archived |
Headers | show |
Series | btrfs: add basic rw support for subpage sector size | expand |
On Wed, Oct 21, 2020 at 02:24:47PM +0800, Qu Wenruo wrote: > In extent-io-test, there are two invalid tests: > - Invalid nodesize for test_eb_bitmaps() > Instead of the sectorsize and nodesize combination passed in, we're > always using hand-crafted nodesize. > Although it has some extra check for 64K page size, we can still hit > a case where PAGE_SIZE == 32K, then we got 128K nodesize which is > larger than max valid node size. > > Thankfully most machines are either 4K or 64K page size, thus we > haven't yet hit such case. > > - Invalid extent buffer bytenr > For 64K page size, the only combination we're going to test is > sectorsize = nodesize = 64K. > In that case, we'll try to create an extent buffer with 32K bytenr, > which is not aligned to sectorsize thus invalid. > > This patch will fix both problems by: > - Honor the sectorsize/nodesize combination > Now we won't bother to hand-craft a strange length and use it as > nodesize. > > - Use sectorsize as the 2nd run extent buffer start > This would test the case where extent buffer is aligned to sectorsize > but not always aligned to nodesize. The code has evolved since it was added in 0f3312295d3ce1d823 ("Btrfs: add extent buffer bitmap sanity tests") and "page * 4" is intentional to provide buffer where the shifted bitmap is tested. The logic has not changed, only the ppc64 case was added. And I remember that tweaking this code tended to break on a real machine so there are a few things that bother me: - the test does something and I'm not sure it's invalid (I think it's not) - test on a real 64k page machine is needed - you reduce the scope of the test to fewer combinations If there are combinations that would make it hard for the subpage then it would be better to add it as an exception but otherwise the main usecase is for 4K page and this allows more combinations to test.
On 2020/10/27 上午7:26, David Sterba wrote: > On Wed, Oct 21, 2020 at 02:24:47PM +0800, Qu Wenruo wrote: >> In extent-io-test, there are two invalid tests: >> - Invalid nodesize for test_eb_bitmaps() >> Instead of the sectorsize and nodesize combination passed in, we're >> always using hand-crafted nodesize. >> Although it has some extra check for 64K page size, we can still hit >> a case where PAGE_SIZE == 32K, then we got 128K nodesize which is >> larger than max valid node size. >> >> Thankfully most machines are either 4K or 64K page size, thus we >> haven't yet hit such case. >> >> - Invalid extent buffer bytenr >> For 64K page size, the only combination we're going to test is >> sectorsize = nodesize = 64K. >> In that case, we'll try to create an extent buffer with 32K bytenr, >> which is not aligned to sectorsize thus invalid. >> >> This patch will fix both problems by: >> - Honor the sectorsize/nodesize combination >> Now we won't bother to hand-craft a strange length and use it as >> nodesize. >> >> - Use sectorsize as the 2nd run extent buffer start >> This would test the case where extent buffer is aligned to sectorsize >> but not always aligned to nodesize. > > The code has evolved since it was added in 0f3312295d3ce1d823 ("Btrfs: > add extent buffer bitmap sanity tests") and "page * 4" is intentional to > provide buffer where the shifted bitmap is tested. The logic has not > changed, only the ppc64 case was added. > > And I remember that tweaking this code tended to break on a real machine > so there are a few things that bother me: > > - the test does something and I'm not sure it's invalid (I think it's > not) Sector is the minimal unit that every tree block/data should follow (the only exception is superblock). Thus a sector starts in half of the sector size is definitely invalid. > - test on a real 64k page machine is needed Every time I inserted the btrfs kernel for my RK3399 board with 64K page size it's tested already. > - you reduce the scope of the test to fewer combinations Well, removing invalid cases would definitely lead to fewer combinations anyway. > > If there are combinations that would make it hard for the subpage then > it would be better to add it as an exception but otherwise the main > usecase is for 4K page and this allows more combinations to test. > No, there isn't anything special related to subpage. Just the things related to "sector" are broken in this test cases. Thanks, Qu
On 2020/10/27 上午8:44, Qu Wenruo wrote: > > > On 2020/10/27 上午7:26, David Sterba wrote: >> On Wed, Oct 21, 2020 at 02:24:47PM +0800, Qu Wenruo wrote: >>> In extent-io-test, there are two invalid tests: >>> - Invalid nodesize for test_eb_bitmaps() >>> Instead of the sectorsize and nodesize combination passed in, we're >>> always using hand-crafted nodesize. >>> Although it has some extra check for 64K page size, we can still hit >>> a case where PAGE_SIZE == 32K, then we got 128K nodesize which is >>> larger than max valid node size. >>> >>> Thankfully most machines are either 4K or 64K page size, thus we >>> haven't yet hit such case. >>> >>> - Invalid extent buffer bytenr >>> For 64K page size, the only combination we're going to test is >>> sectorsize = nodesize = 64K. >>> In that case, we'll try to create an extent buffer with 32K bytenr, >>> which is not aligned to sectorsize thus invalid. >>> >>> This patch will fix both problems by: >>> - Honor the sectorsize/nodesize combination >>> Now we won't bother to hand-craft a strange length and use it as >>> nodesize. >>> >>> - Use sectorsize as the 2nd run extent buffer start >>> This would test the case where extent buffer is aligned to sectorsize >>> but not always aligned to nodesize. >> >> The code has evolved since it was added in 0f3312295d3ce1d823 ("Btrfs: >> add extent buffer bitmap sanity tests") and "page * 4" is intentional to >> provide buffer where the shifted bitmap is tested. The logic has not >> changed, only the ppc64 case was added. >> >> And I remember that tweaking this code tended to break on a real machine >> so there are a few things that bother me: >> >> - the test does something and I'm not sure it's invalid (I think it's >> not) > > Sector is the minimal unit that every tree block/data should follow (the > only exception is superblock). > Thus a sector starts in half of the sector size is definitely invalid. > >> - test on a real 64k page machine is needed > > Every time I inserted the btrfs kernel for my RK3399 board with 64K page > size it's tested already. > >> - you reduce the scope of the test to fewer combinations > > Well, removing invalid cases would definitely lead to fewer combinations > anyway. > >> >> If there are combinations that would make it hard for the subpage then >> it would be better to add it as an exception but otherwise the main >> usecase is for 4K page and this allows more combinations to test. >> > No, there isn't anything special related to subpage. > > Just the things related to "sector" are broken in this test cases. Since all later subpage refactor code will require this patch as the basestone, I just want to re-iterate here again: Any extent buffer whose bytenr is not sector aligned is invalid. This does not apply to subpage, but also regular sector size. E.g. for 4K sector size, 4K node size, an eb starting at bytenr 1M + 2K is definitely corrupted. This is patch essential, especially when later patch "btrfs: extent_io: calculate inline extent buffer page size based on page size" will change extent_buffer::pages[] to bare minimal. Anyway, I will add extra comment to explain the importance of this patch. Thanks, Qu > > Thanks, > Qu >
diff --git a/fs/btrfs/tests/extent-io-tests.c b/fs/btrfs/tests/extent-io-tests.c index df7ce874a74b..73e96d505f4f 100644 --- a/fs/btrfs/tests/extent-io-tests.c +++ b/fs/btrfs/tests/extent-io-tests.c @@ -379,54 +379,50 @@ static int __test_eb_bitmaps(unsigned long *bitmap, struct extent_buffer *eb, static int test_eb_bitmaps(u32 sectorsize, u32 nodesize) { struct btrfs_fs_info *fs_info; - unsigned long len; unsigned long *bitmap = NULL; struct extent_buffer *eb = NULL; int ret; test_msg("running extent buffer bitmap tests"); - /* - * In ppc64, sectorsize can be 64K, thus 4 * 64K will be larger than - * BTRFS_MAX_METADATA_BLOCKSIZE. - */ - len = (sectorsize < BTRFS_MAX_METADATA_BLOCKSIZE) - ? sectorsize * 4 : sectorsize; - - fs_info = btrfs_alloc_dummy_fs_info(len, len); + fs_info = btrfs_alloc_dummy_fs_info(nodesize, sectorsize); if (!fs_info) { test_std_err(TEST_ALLOC_FS_INFO); return -ENOMEM; } - bitmap = kmalloc(len, GFP_KERNEL); + bitmap = kmalloc(nodesize, GFP_KERNEL); if (!bitmap) { test_err("couldn't allocate test bitmap"); ret = -ENOMEM; goto out; } - eb = __alloc_dummy_extent_buffer(fs_info, 0, len); + eb = __alloc_dummy_extent_buffer(fs_info, 0, nodesize); if (!eb) { test_std_err(TEST_ALLOC_ROOT); ret = -ENOMEM; goto out; } - ret = __test_eb_bitmaps(bitmap, eb, len); + ret = __test_eb_bitmaps(bitmap, eb, nodesize); if (ret) goto out; - /* Do it over again with an extent buffer which isn't page-aligned. */ free_extent_buffer(eb); - eb = __alloc_dummy_extent_buffer(fs_info, nodesize / 2, len); + + /* + * Test again for case where the tree block is sectorsize aligned but + * not nodesize aligned. + */ + eb = __alloc_dummy_extent_buffer(fs_info, sectorsize, nodesize); if (!eb) { test_std_err(TEST_ALLOC_ROOT); ret = -ENOMEM; goto out; } - ret = __test_eb_bitmaps(bitmap, eb, len); + ret = __test_eb_bitmaps(bitmap, eb, nodesize); out: free_extent_buffer(eb); kfree(bitmap);
In extent-io-test, there are two invalid tests: - Invalid nodesize for test_eb_bitmaps() Instead of the sectorsize and nodesize combination passed in, we're always using hand-crafted nodesize. Although it has some extra check for 64K page size, we can still hit a case where PAGE_SIZE == 32K, then we got 128K nodesize which is larger than max valid node size. Thankfully most machines are either 4K or 64K page size, thus we haven't yet hit such case. - Invalid extent buffer bytenr For 64K page size, the only combination we're going to test is sectorsize = nodesize = 64K. In that case, we'll try to create an extent buffer with 32K bytenr, which is not aligned to sectorsize thus invalid. This patch will fix both problems by: - Honor the sectorsize/nodesize combination Now we won't bother to hand-craft a strange length and use it as nodesize. - Use sectorsize as the 2nd run extent buffer start This would test the case where extent buffer is aligned to sectorsize but not always aligned to nodesize. Signed-off-by: Qu Wenruo <wqu@suse.com> --- fs/btrfs/tests/extent-io-tests.c | 26 +++++++++++--------------- 1 file changed, 11 insertions(+), 15 deletions(-)