| Message ID | 20230303172120.3800725-21-shikemeng@huaweicloud.com |
|---|---|
| State | Awaiting Upstream |
| Headers | show |
| Series | Some bugfix and cleanup to mballoc | expand |
On Sat, Mar 04, 2023 at 01:21:20AM +0800, Kemeng Shi wrote: > We try to allocate a block from goal in ext4_mb_new_blocks_simple. We > only need get blkoff in first group with goal and set blkoff to 0 for > the rest groups. > > Signed-off-by: Kemeng Shi <shikemeng@huaweicloud.com> While this patch is OK as a simplification, there's a bigger problem with ext4_mb_new_blocks_simple(), and that is that we start looking for free blocks starting at the goal block, and then if we can't any free blocks by the time we get to the last block group.... we stop, and return ENOSPC. This function is only used in the fast commit replay path, but for a very full file system, this could cause a fast commit replay to fail unnecesarily. What we need to do is to try wrapping back to the beginning of the file system, and stop when we hit the original group (of the goal block) minus one. We can fix this up in a separate patch, since this doesn't make things any worse, but essentially we need to do something like this: maxgroups = ext4_get_groups_count(sb); for (g = 0; g < maxgroups ; g++) { ... group++; if (group > maxgroups) group = 0; } - Ted
on 3/16/2023 1:07 PM, Theodore Ts'o wrote: > On Sat, Mar 04, 2023 at 01:21:20AM +0800, Kemeng Shi wrote: >> We try to allocate a block from goal in ext4_mb_new_blocks_simple. We >> only need get blkoff in first group with goal and set blkoff to 0 for >> the rest groups. >> >> Signed-off-by: Kemeng Shi <shikemeng@huaweicloud.com> > > While this patch is OK as a simplification, there's a bigger problem > with ext4_mb_new_blocks_simple(), and that is that we start looking > for free blocks starting at the goal block, and then if we can't any > free blocks by the time we get to the last block group.... we stop, > and return ENOSPC. > > This function is only used in the fast commit replay path, but for a > very full file system, this could cause a fast commit replay to fail > unnecesarily. What we need to do is to try wrapping back to the > beginning of the file system, and stop when we hit the original group > (of the goal block) minus one. > > We can fix this up in a separate patch, since this doesn't make things > any worse, but essentially we need to do something like this: Hi Theodore, thanks for feedback. I will submit another patchset for mballoc and I would like to include this fix if no one else does. As new patches may be conflicted with old ones I submited, I would submit the new patchset after the old ones are fully reviewed and applied if this fix is not in rush. Thanks! > > maxgroups = ext4_get_groups_count(sb); > for (g = 0; g < maxgroups ; g++) { > > ... > group++; > if (group > maxgroups) > group = 0; > } > > - Ted >
On Thu, Mar 16, 2023 at 06:19:40PM +0800, Kemeng Shi wrote: > Hi Theodore, thanks for feedback. I will submit another patchset for > mballoc and I would like to include this fix if no one else does. As > new patches may be conflicted with old ones I submited, I would submit > the new patchset after the old ones are fully reviewed and applied > if this fix is not in rush. Thanks! Hi, I've already taken the your patches into the dev branch; were there any changes you were intending to make to your patches? If you could submit a separate fix for the bug that I noticed, that would be great. Also, if you are interested in doing some more work in mballoc.c, I was wondering if you would be interested in adding some Kunit tests for mballoc.c. A simple example Kunit test for ext4 can be found in fs/ext4/inode_test.c. (The convention is to place tests for foo.c in foo_test.c.) [1] https://docs.kernel.org/dev-tools/kunit/ In order to add mballoc Kunit tests, we will need to add some "mock"[2] functions to simulate what happens when mballoc.c tries reading a block bitmap. My thinking was to have a test provide an array of some data structure like this: struct test_bitmap { unsigned int start; unsigned int len; }; [2] https://en.wikipedia.org/wiki/Mock_object ... which indicates the starting block, and the length of a run of blocks that are marked as in use, where the list of blocks are sorted by starting block number, and where a starting block of ~0 indicates the end of the list of block extents. We would also need have a set of utility ext4 Kunit functions to create "fake" ext4 superblocks and ext4_sb_info structures. I was originally thinking that obvious starting Kunit tests would be for fs/ext4/hash.c and fs/ext4/extents_status.c, since they require the little or no "mocking" support. However, there are so many changes in fs/ext4/mballoc.c, the urgency in having unit tests for it is getting more urgent --- since if there is a bug in one of these functions, such as the one that I noted in ext4_mb_new_blocks_simple(), since it's harder to exhaustively test some of these smaller sub-functions in integration tests such as those found in xfstests. Unit tests are the best way to make sure we're testing all of the code paths in a complex module such as mballoc.c Cheers, - Ted
on 3/17/2023 11:50 PM, Theodore Ts'o wrote: > On Thu, Mar 16, 2023 at 06:19:40PM +0800, Kemeng Shi wrote: >> Hi Theodore, thanks for feedback. I will submit another patchset for >> mballoc and I would like to include this fix if no one else does. As >> new patches may be conflicted with old ones I submited, I would submit >> the new patchset after the old ones are fully reviewed and applied >> if this fix is not in rush. Thanks! > > Hi, I've already taken the your patches into the dev branch; were > there any changes you were intending to make to your patches? > > If you could submit a separate fix for the bug that I noticed, that > would be great. Hi, I was stuck in some urgent work recently and I will do this ASAP and it should be done in this week. > Also, if you are interested in doing some more work in mballoc.c, I > was wondering if you would be interested in adding some Kunit tests > for mballoc.c. A simple example Kunit test for ext4 can be found in > fs/ext4/inode_test.c. (The convention is to place tests for foo.c in > foo_test.c.) > [1] https://docs.kernel.org/dev-tools/kunit/ > > In order to add mballoc Kunit tests, we will need to add some "mock"[2] > functions to simulate what happens when mballoc.c tries reading a > block bitmap. My thinking was to have a test provide an array of some > data structure like this: > > struct test_bitmap { > unsigned int start; > unsigned int len; > }; > > [2] https://en.wikipedia.org/wiki/Mock_object > > ... which indicates the starting block, and the length of a run of > blocks that are marked as in use, where the list of blocks are sorted > by starting block number, and where a starting block of ~0 indicates > the end of the list of block extents. > We would also need have a set of utility ext4 Kunit functions to > create "fake" ext4 superblocks and ext4_sb_info structures. The Kunit tests thing sounds interesting and I would like to this. But I still need some time to get basic knowledge then I maybe able to discuss detais. Of couse, anyone is also interesting in this and can make this work soon is fine.:) > I was originally thinking that obvious starting Kunit tests would be > for fs/ext4/hash.c and fs/ext4/extents_status.c, since they require > the little or no "mocking" support. However, there are so many > changes in fs/ext4/mballoc.c, the urgency in having unit tests for it > is getting more urgent --- since if there is a bug in one of these > functions, such as the one that I noted in > ext4_mb_new_blocks_simple(), since it's harder to exhaustively test > some of these smaller sub-functions in integration tests such as those > found in xfstests. Unit tests are the best way to make sure we're > testing all of the code paths in a complex module such as mballoc.c Yes, I can't agree more and this may be able to find other exsiting bugs.
diff --git a/fs/ext4/mballoc.c b/fs/ext4/mballoc.c index 1103d35b31cb..85d5e219933f 100644 --- a/fs/ext4/mballoc.c +++ b/fs/ext4/mballoc.c @@ -5772,9 +5772,6 @@ static ext4_fsblk_t ext4_mb_new_blocks_simple(handle_t *handle, return 0; } - ext4_get_group_no_and_offset(sb, - max(ext4_group_first_block_no(sb, group), goal), - NULL, &blkoff); while (1) { i = mb_find_next_zero_bit(bitmap_bh->b_data, max, blkoff); @@ -5789,6 +5786,8 @@ static ext4_fsblk_t ext4_mb_new_blocks_simple(handle_t *handle, brelse(bitmap_bh); if (i < max) break; + + blkoff = 0; } if (group >= ext4_get_groups_count(sb) || i >= max) {
We try to allocate a block from goal in ext4_mb_new_blocks_simple. We only need get blkoff in first group with goal and set blkoff to 0 for the rest groups. Signed-off-by: Kemeng Shi <shikemeng@huaweicloud.com> --- fs/ext4/mballoc.c | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-)