Message ID | 20230303172120.3800725-21-shikemeng@huaweicloud.com |
---|---|
State | New |
Headers |
Return-Path: <linux-kernel-owner@vger.kernel.org> Delivered-To: ouuuleilei@gmail.com Received: by 2002:a5d:5915:0:0:0:0:0 with SMTP id v21csp310808wrd; Fri, 3 Mar 2023 01:27:32 -0800 (PST) X-Google-Smtp-Source: AK7set/85+3fH2IW5xWQBJXx0h3BDbW9ruBzSvbknevresse1BmM04qllZMrWkJCyhf57zyiF+cC X-Received: by 2002:a17:906:6c84:b0:8b1:32b0:2a24 with SMTP id s4-20020a1709066c8400b008b132b02a24mr1177247ejr.47.1677835652605; Fri, 03 Mar 2023 01:27:32 -0800 (PST) ARC-Seal: i=1; a=rsa-sha256; t=1677835652; cv=none; d=google.com; s=arc-20160816; b=Gcy+LUzT65iHHp/BT+ZCBR8dXI3utqSqGERqVbrnUGSYJocChMs4uQals7wvq25a7A HG5gKtQcGDRQVMqsiUwjOjvmsxQl14HndN6sc1x6XVthvILfMyAWXUhrgTo1E2xrfva/ pqRd03cgakpvh+KsE5b969LN5WIYTVDUVV0Lvc4dzOd95moLps8URHrXA9dZ8SDeU3VY thSoIW9IK4bhiQYXem6Yzy2Wc5d4ic98O7mPm6FSEvq0SmePsHF2VSGtFuzS6VeMiyPj kBvm4c3k6DX0autf2ZvfgLi3i/XAhzie7mVqXCplV6VQw1tEjlpmUokwnyOjxEqQ3CBz htxw== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=list-id:precedence:content-transfer-encoding:mime-version :references:in-reply-to:message-id:date:subject:cc:to:from; bh=k5QTBViQSVtd2G9rela0IyMMR67efsliP5z96b9oLsM=; b=W7whZ7/AZpw1eASQeICJruGiELUhVRpqPLhBwR1af7G+lscM+KVsta7KxXUmvwbTGk Kq6Tl1e/Hjt63CnQ8uuuh2g75lAhYLPFyana+vmPyTlWAI7cH1Sg5ZVoUhGsbtRPJ0pP e9GpUe+n8qAFnEle5RsWx5lw4LrHV8F1nIaZnJgdRxE05FrVkIMRAbJf9ATkk0Y0biYZ 9d2MqWdiiMcb4YiA8Lk6a+PZC4hzjGNYFXyx99LGwBCBguqWrpJlwowiz1XNZ5uiwjOM VqxZY7FAq2UNSB7HZfbDcpmFF7ef0gAGcfqlsAAdcXnItpq54P5JM7ISrLML7xCwb8ZK 9Adg== ARC-Authentication-Results: i=1; mx.google.com; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org Received: from out1.vger.email (out1.vger.email. [2620:137:e000::1:20]) by mx.google.com with ESMTP id b24-20020aa7c6d8000000b004ace582aca8si409499eds.256.2023.03.03.01.27.10; Fri, 03 Mar 2023 01:27:32 -0800 (PST) Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20; Authentication-Results: mx.google.com; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S231145AbjCCJVB (ORCPT <rfc822;davidbtadokoro@gmail.com> + 99 others); Fri, 3 Mar 2023 04:21:01 -0500 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:49910 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S230195AbjCCJT6 (ORCPT <rfc822;linux-kernel@vger.kernel.org>); Fri, 3 Mar 2023 04:19:58 -0500 Received: from dggsgout11.his.huawei.com (dggsgout11.his.huawei.com [45.249.212.51]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id B04A7DBDB; Fri, 3 Mar 2023 01:19:57 -0800 (PST) Received: from mail02.huawei.com (unknown [172.30.67.153]) by dggsgout11.his.huawei.com (SkyGuard) with ESMTP id 4PSj9P2JmNz4f3nVG; Fri, 3 Mar 2023 17:19:53 +0800 (CST) Received: from huaweicloud.com (unknown [10.175.124.27]) by APP4 (Coremail) with SMTP id gCh0CgDnnbGyuwFkFpqfEg--.45687S22; Fri, 03 Mar 2023 17:19:55 +0800 (CST) From: Kemeng Shi <shikemeng@huaweicloud.com> To: tytso@mit.edu, adilger.kernel@dilger.ca, ojaswin@linux.ibm.com, ritesh.list@gmail.com Cc: linux-ext4@vger.kernel.org, linux-kernel@vger.kernel.org, shikemeng@huaweicloud.com Subject: [PATCH v3 20/20] ext4: simplify calculation of blkoff in ext4_mb_new_blocks_simple Date: Sat, 4 Mar 2023 01:21:20 +0800 Message-Id: <20230303172120.3800725-21-shikemeng@huaweicloud.com> X-Mailer: git-send-email 2.30.0 In-Reply-To: <20230303172120.3800725-1-shikemeng@huaweicloud.com> References: <20230303172120.3800725-1-shikemeng@huaweicloud.com> MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-CM-TRANSID: gCh0CgDnnbGyuwFkFpqfEg--.45687S22 X-Coremail-Antispam: 1UD129KBjvdXoWrtFWDZw4xCw4DKr4UCw4fGrg_yoWfKFc_Ka 4xZr48JFWrJr1fuFn5JrZ8tF47ta1kJr1ruFZ2qryfZF1YvFWxuw1DArZ0yFyDuay5Aas8 Cr9xurW5Kr10vjkaLaAFLSUrUUUUUb8apTn2vfkv8UJUUUU8Yxn0WfASr-VFAUDa7-sFnT 9fnUUIcSsGvfJTRUUUbqxFF20E14v26rWj6s0DM7CY07I20VC2zVCF04k26cxKx2IYs7xG 6rWj6s0DM7CIcVAFz4kK6r1j6r18M280x2IEY4vEnII2IxkI6r1a6r45M28IrcIa0xkI8V A2jI8067AKxVWUAVCq3wA2048vs2IY020Ec7CjxVAFwI0_Xr0E3s1l8cAvFVAK0II2c7xJ M28CjxkF64kEwVA0rcxSw2x7M28EF7xvwVC0I7IYx2IY67AKxVWDJVCq3wA2z4x0Y4vE2I x0cI8IcVCY1x0267AKxVW8Jr0_Cr1UM28EF7xvwVC2z280aVAFwI0_GcCE3s1l84ACjcxK 6I8E87Iv6xkF7I0E14v26rxl6s0DM2AIxVAIcxkEcVAq07x20xvEncxIr21l5I8CrVACY4 xI64kE6c02F40Ex7xfMcIj6xIIjxv20xvE14v26r1j6r18McIj6I8E87Iv67AKxVWUJVW8 JwAm72CE4IkC6x0Yz7v_Jr0_Gr1lF7xvr2IYc2Ij64vIr41lF7I21c0EjII2zVCS5cI20V AGYxC7MxAIw28IcxkI7VAKI48JMxC20s026xCaFVCjc4AY6r1j6r4UMI8I3I0E5I8CrVAF wI0_Jr0_Jr4lx2IqxVCjr7xvwVAFwI0_JrI_JrWlx4CE17CEb7AF67AKxVWUAVWUtwCIc4 0Y0x0EwIxGrwCI42IY6xIIjxv20xvE14v26ryj6F1UMIIF0xvE2Ix0cI8IcVCY1x0267AK xVW8Jr0_Cr1UMIIF0xvE42xK8VAvwI8IcIk0rVWUJVWUCwCI42IY6I8E87Iv67AKxVW8JV WxJwCI42IY6I8E87Iv6xkF7I0E14v26r4UJVWxJrUvcSsGvfC2KfnxnUUI43ZEXa7sRiVb yDUUUUU== X-CM-SenderInfo: 5vklyvpphqwq5kxd4v5lfo033gof0z/ X-CFilter-Loop: Reflected X-Spam-Status: No, score=0.0 required=5.0 tests=BAYES_00,DATE_IN_FUTURE_06_12, SPF_HELO_NONE,SPF_NONE autolearn=ham autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on lindbergh.monkeyblade.net Precedence: bulk List-ID: <linux-kernel.vger.kernel.org> X-Mailing-List: linux-kernel@vger.kernel.org X-getmail-retrieved-from-mailbox: =?utf-8?q?INBOX?= X-GMAIL-THRID: =?utf-8?q?1759338197043278083?= X-GMAIL-MSGID: =?utf-8?q?1759338197043278083?= |
Series |
Some bugfix and cleanup to mballoc
|
|
Commit Message
Kemeng Shi
March 3, 2023, 5:21 p.m. UTC
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(-)
Comments
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) {