Re: [PATCH v3 00/19] Fixes, cleanups and unit test for mballoc

[Kemeng Shi]

Friendly ping...

on 4/17/2023 7:05 PM, Kemeng Shi wrote:
> v2->v3:
> 1. Make patches on new branch head and fix conflic on "ext4: add
> EXT4_MB_HINT_GOAL_ONLY test in ext4_mb_use_preallocated"
> 2. Fix build warnings on "ext4: add some kunit stub for mballoc kunit
> test" and "ext4: add first unit test for ext4_mb_new_blocks_simple in
> mballoc"
> 
> There are three parts in this patchset:
> Part1: Patch 1-7 is v2 of sent series
> v1->v2:
> 1. collect reviewed-by from Ojaswin. Only "ext4: add
> EXT4_MB_HINT_GOAL_ONLY test in ext4_mb_use_preallocated" needs futher
> review. See [1] for previous comments.
> 2. drop "ext4: fix wrong unit use in ext4_mb_new_inode_pa" which is
> already done in [2].
> 
> Part2: Patch 8-17 are more fixes and cleanups to mballoc
> Some patches in this part will be conflict with patches in part1, so
> append new patches in this series instead of creating a new one.
> Patch 8-11 are some random fixes and cleanups, see respective log
> message for detail.
> Patch 12-17 factor out codes to mark bit in group is used or free
> which will update on disk block bitmap and group descriptor. Several
> reasons to do this:
> 1. pair behavior of alloc/free bits. For example,
> ext4_mb_new_blocks_simple will update free_clusters in struct flex_groups
> in ext4_mb_mark_bb while ext4_free_blocks_simple forgets this.
> 2. remove repeat code to read from disk, update and write back to disk.
> 3. reduce future unit test mocks to avoid real IO to update structure
> on disk.
> 
> Part3: Patch 18-19 add one unit test for mballoc
> Patch 18 add mocks to functions which will issue IO to disk.
> Patch 19 add unit test for ext4_mb_new_blocks_simple in mballoc.
> Details can be found in respective log message.
> 
> Before add more unit tests, there are something should be discussed:
> 1. How to test static function in mballoc.c
> Currently, include mballoc-test.c in mballoc.c to test static function
> in mballoc.c from mballoc-test.c which is one way suggested in [3].
> Not sure if there is any more elegant way to test static function without
> touch mballoc.c.
> 2. How to add mocks to function in mballoc.c which may issue IO to disk
> Currently, KUNIT_STATIC_STUB_REDIRECT is added to functions as suggested
> in kunit document [4].
> 3. How to simulate a block bitmap.
> Currently, a fake buffer_head with bitmap data is returned, then no
> futher change is needed.
> If we simulate a block bitmap with an array of data structure like:
> struct test_bitmap {
>        unsigned int	start;
>        unsigned int	len;
> }
> which is suggested by Theodore in [5], then we need to add mocks to
> function which expected bitmap from bitmap_bh->b_data, like
> mb_find_next_bit, mb_find_next_zero_bit and maybe more.
> 
> Would like to hear any suggestion! Thanks!
> 
> [1]
> https://lore.kernel.org/linux-ext4/ZC3MoWn2UO6p+Swp@xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx/
> [2]
> https://lore.kernel.org/linux-ext4/9b35f3955a1d7b66bbd713eca1e63026e01f78c1.1679731817.git.ojaswin@xxxxxxxxxxxxx
> [3]
> https://docs.kernel.org/dev-tools/kunit/usage.html#testing-static-functions
> [4]
> https://docs.kernel.org/dev-tools/kunit/api/functionredirection.html#c.KUNIT_STATIC_STUB_REDIRECT
> [5]
> https://lore.kernel.org/linux-ext4/20230317155047.GB3270589@xxxxxxx/
> 
> By the way, the "xfstest somke" passes. Please let me know if any more
> test is needed.
> Unit test result is as followings:
> # ./tools/testing/kunit/kunit.py run --kunitconfig=fs/ext4/.kunitconfig --raw_output
> [18:44:39] Configuring KUnit Kernel ...
> [18:44:39] Building KUnit Kernel ...
> Populating config with:
> $ make ARCH=um O=.kunit olddefconfig
> Building with:
> $ make ARCH=um O=.kunit --jobs=88
> [18:44:47] Starting KUnit Kernel (1/1)...
> KTAP version 1
> 1..2
>     KTAP version 1
>     # Subtest: ext4_mballoc_test
>     1..1
>     ok 1 test_new_blocks_simple
> ok 1 ext4_mballoc_test
>     KTAP version 1
>     # Subtest: ext4_inode_test
>     1..1
>         KTAP version 1
>         # Subtest: inode_test_xtimestamp_decoding
>         ok 1 1901-12-13 Lower bound of 32bit < 0 timestamp, no extra bits
>         ok 2 1969-12-31 Upper bound of 32bit < 0 timestamp, no extra bits
>         ok 3 1970-01-01 Lower bound of 32bit >=0 timestamp, no extra bits
>         ok 4 2038-01-19 Upper bound of 32bit >=0 timestamp, no extra bits
>         ok 5 2038-01-19 Lower bound of 32bit <0 timestamp, lo extra sec bit on
>         ok 6 2106-02-07 Upper bound of 32bit <0 timestamp, lo extra sec bit on
>         ok 7 2106-02-07 Lower bound of 32bit >=0 timestamp, lo extra sec bit on
>         ok 8 2174-02-25 Upper bound of 32bit >=0 timestamp, lo extra sec bit on
>         ok 9 2174-02-25 Lower bound of 32bit <0 timestamp, hi extra sec bit on
>         ok 10 2242-03-16 Upper bound of 32bit <0 timestamp, hi extra sec bit on
>         ok 11 2242-03-16 Lower bound of 32bit >=0 timestamp, hi extra sec bit on
>         ok 12 2310-04-04 Upper bound of 32bit >=0 timestamp, hi extra sec bit on
>         ok 13 2310-04-04 Upper bound of 32bit>=0 timestamp, hi extra sec bit 1. 1 ns
>         ok 14 2378-04-22 Lower bound of 32bit>= timestamp. Extra sec bits 1. Max ns
>         ok 15 2378-04-22 Lower bound of 32bit >=0 timestamp. All extra sec bits on
>         ok 16 2446-05-10 Upper bound of 32bit >=0 timestamp. All extra sec bits on
>     # inode_test_xtimestamp_decoding: pass:16 fail:0 skip:0 total:16
>     ok 1 inode_test_xtimestamp_decoding
> # Totals: pass:16 fail:0 skip:0 total:16
> ok 2 ext4_inode_test
> [18:44:48] Elapsed time: 8.602s total, 0.001s configuring, 8.483s building, 0.072s running
> 
> 
> Kemeng Shi (19):
>   ext4: fix wrong unit use in ext4_mb_normalize_request
>   ext4: fix unit mismatch in ext4_mb_new_blocks_simple
>   ext4: fix wrong unit use in ext4_mb_find_by_goal
>   ext4: treat stripe in block unit
>   ext4: add EXT4_MB_HINT_GOAL_ONLY test in ext4_mb_use_preallocated
>   ext4: remove ext4_block_group and ext4_block_group_offset declaration
>   ext4: try all groups in ext4_mb_new_blocks_simple
>   ext4: get block from bh before pass it to ext4_free_blocks_simple in
>     ext4_free_blocks
>   ext4: remove unsed parameter and unnecessary forward declaration of
>     ext4_mb_new_blocks_simple
>   ext4: fix wrong unit use in ext4_mb_clear_bb
>   ext4: fix wrong unit use in ext4_mb_new_blocks
>   ext4: factor out codes to update block bitmap and group descriptor on
>     disk from ext4_mb_mark_bb
>   ext4: call ext4_mb_mark_group_bb in ext4_free_blocks_simple
>   ext4: extent ext4_mb_mark_group_bb to support allocation under journal
>   ext4: call ext4_mb_mark_group_bb in ext4_mb_mark_diskspace_used
>   ext4: call ext4_mb_mark_group_bb in ext4_mb_clear_bb
>   ext4: call ext4_mb_mark_group_bb in ext4_group_add_blocks
>   ext4: add some kunit stub for mballoc kunit test
>   ext4: add first unit test for ext4_mb_new_blocks_simple in mballoc
> 
>  fs/ext4/balloc.c       |  16 +
>  fs/ext4/ext4.h         |   4 -
>  fs/ext4/mballoc-test.c | 323 +++++++++++++++++++
>  fs/ext4/mballoc.c      | 704 +++++++++++++++++++----------------------
>  fs/ext4/super.c        |  13 +
>  5 files changed, 671 insertions(+), 389 deletions(-)
>  create mode 100644 fs/ext4/mballoc-test.c
> 

-- 
Best wishes
Kemeng Shi