When setup_system_zone, flex_bg is not initialzied so it is always 1.
and when meta_bg enabled, group descriptors are located in the first,
second, and the last group of meta group.
And this patch also adds reserved GDT blocks to system zone.
Signed-off-by: Wang Jianjian <[email protected]>
---
fs/ext4/block_validity.c | 10 ++++++----
1 file changed, 6 insertions(+), 4 deletions(-)
diff --git a/fs/ext4/block_validity.c b/fs/ext4/block_validity.c
index 5504f72bbbbe..5df357763975 100644
--- a/fs/ext4/block_validity.c
+++ b/fs/ext4/block_validity.c
@@ -215,7 +215,6 @@ int ext4_setup_system_zone(struct super_block *sb)
struct ext4_system_blocks *system_blks;
struct ext4_group_desc *gdp;
ext4_group_t i;
- int flex_size = ext4_flex_bg_size(sbi);
int ret;
system_blks = kzalloc(sizeof(*system_blks), GFP_KERNEL);
@@ -224,11 +223,14 @@ int ext4_setup_system_zone(struct super_block *sb)
for (i=0; i < ngroups; i++) {
cond_resched();
- if (ext4_bg_has_super(sb, i) &&
- ((i < 5) || ((i % flex_size) == 0))) {
+ unsigned int sb_num = ext4_bg_has_super(sb, i);
+ unsigned long gdb_num = ext4_bg_num_gdb(sb, i);
+ unsigned int rsvd_gdt = le16_to_cpu(sbi->es->s_reserved_gdt_blocks);
+
+ if (sb_num != 0 || gdb_num != 0) {
ret = add_system_zone(system_blks,
ext4_group_first_block_no(sb, i),
- ext4_bg_num_gdb(sb, i) + 1, 0);
+ sb_num + gdb_num + rsvd_gdt, 0);
if (ret)
goto err;
}
--
2.34.3
Hi Wang,
kernel test robot noticed the following build warnings:
[auto build test WARNING on tytso-ext4/dev]
[also build test WARNING on linus/master v6.4-rc4 next-20230602]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]
url: https://github.com/intel-lab-lkp/linux/commits/Wang-Jianjian/ext4-Add-correct-group-descriptors-and-reserved-GDT-blocks-to-system-zone/20230604-003439
base: https://git.kernel.org/pub/scm/linux/kernel/git/tytso/ext4.git dev
patch link: https://lore.kernel.org/r/tencent_4A474CC049B9E77D0F172468991EED5B9105%40qq.com
patch subject: [PATCH] ext4: Add correct group descriptors and reserved GDT blocks to system zone
config: x86_64-defconfig (https://download.01.org/0day-ci/archive/20230604/[email protected]/config)
compiler: gcc-12 (Debian 12.2.0-14) 12.2.0
reproduce (this is a W=1 build):
# https://github.com/intel-lab-lkp/linux/commit/54ea70d6b1189628ed4017129457d77e0bfa7fde
git remote add linux-review https://github.com/intel-lab-lkp/linux
git fetch --no-tags linux-review Wang-Jianjian/ext4-Add-correct-group-descriptors-and-reserved-GDT-blocks-to-system-zone/20230604-003439
git checkout 54ea70d6b1189628ed4017129457d77e0bfa7fde
# save the config file
mkdir build_dir && cp config build_dir/.config
make W=1 O=build_dir ARCH=x86_64 olddefconfig
make W=1 O=build_dir ARCH=x86_64 SHELL=/bin/bash fs/ext4/
If you fix the issue, kindly add following tag where applicable
| Reported-by: kernel test robot <[email protected]>
| Closes: https://lore.kernel.org/oe-kbuild-all/[email protected]/
All warnings (new ones prefixed by >>):
fs/ext4/block_validity.c: In function 'ext4_setup_system_zone':
>> fs/ext4/block_validity.c:226:17: warning: ISO C90 forbids mixed declarations and code [-Wdeclaration-after-statement]
226 | unsigned int sb_num = ext4_bg_has_super(sb, i);
| ^~~~~~~~
In file included from include/linux/byteorder/little_endian.h:5,
from arch/x86/include/uapi/asm/byteorder.h:5,
from arch/x86/include/asm/orc_types.h:49,
from arch/x86/include/asm/unwind_hints.h:6,
from arch/x86/include/asm/nospec-branch.h:13,
from arch/x86/include/asm/paravirt_types.h:27,
from arch/x86/include/asm/ptrace.h:97,
from arch/x86/include/asm/math_emu.h:5,
from arch/x86/include/asm/processor.h:13,
from arch/x86/include/asm/timex.h:5,
from include/linux/timex.h:67,
from include/linux/time32.h:13,
from include/linux/time.h:60,
from fs/ext4/block_validity.c:12:
fs/ext4/block_validity.c:228:58: error: 'struct ext4_sb_info' has no member named 'es'; did you mean 's_es'?
228 | unsigned int rsvd_gdt = le16_to_cpu(sbi->es->s_reserved_gdt_blocks);
| ^~
include/uapi/linux/byteorder/little_endian.h:37:51: note: in definition of macro '__le16_to_cpu'
37 | #define __le16_to_cpu(x) ((__force __u16)(__le16)(x))
| ^
fs/ext4/block_validity.c:228:41: note: in expansion of macro 'le16_to_cpu'
228 | unsigned int rsvd_gdt = le16_to_cpu(sbi->es->s_reserved_gdt_blocks);
| ^~~~~~~~~~~
vim +226 fs/ext4/block_validity.c
201
202 /*
203 * Build system zone rbtree which is used for block validity checking.
204 *
205 * The update of system_blks pointer in this function is protected by
206 * sb->s_umount semaphore. However we have to be careful as we can be
207 * racing with ext4_inode_block_valid() calls reading system_blks rbtree
208 * protected only by RCU. That's why we first build the rbtree and then
209 * swap it in place.
210 */
211 int ext4_setup_system_zone(struct super_block *sb)
212 {
213 ext4_group_t ngroups = ext4_get_groups_count(sb);
214 struct ext4_sb_info *sbi = EXT4_SB(sb);
215 struct ext4_system_blocks *system_blks;
216 struct ext4_group_desc *gdp;
217 ext4_group_t i;
218 int ret;
219
220 system_blks = kzalloc(sizeof(*system_blks), GFP_KERNEL);
221 if (!system_blks)
222 return -ENOMEM;
223
224 for (i=0; i < ngroups; i++) {
225 cond_resched();
> 226 unsigned int sb_num = ext4_bg_has_super(sb, i);
227 unsigned long gdb_num = ext4_bg_num_gdb(sb, i);
228 unsigned int rsvd_gdt = le16_to_cpu(sbi->es->s_reserved_gdt_blocks);
229
230 if (sb_num != 0 || gdb_num != 0) {
231 ret = add_system_zone(system_blks,
232 ext4_group_first_block_no(sb, i),
233 sb_num + gdb_num + rsvd_gdt, 0);
234 if (ret)
235 goto err;
236 }
237 gdp = ext4_get_group_desc(sb, i, NULL);
238 ret = add_system_zone(system_blks,
239 ext4_block_bitmap(sb, gdp), 1, 0);
240 if (ret)
241 goto err;
242 ret = add_system_zone(system_blks,
243 ext4_inode_bitmap(sb, gdp), 1, 0);
244 if (ret)
245 goto err;
246 ret = add_system_zone(system_blks,
247 ext4_inode_table(sb, gdp),
248 sbi->s_itb_per_group, 0);
249 if (ret)
250 goto err;
251 }
252 if (ext4_has_feature_journal(sb) && sbi->s_es->s_journal_inum) {
253 ret = ext4_protect_reserved_inode(sb, system_blks,
254 le32_to_cpu(sbi->s_es->s_journal_inum));
255 if (ret)
256 goto err;
257 }
258
259 /*
260 * System blks rbtree complete, announce it once to prevent racing
261 * with ext4_inode_block_valid() accessing the rbtree at the same
262 * time.
263 */
264 rcu_assign_pointer(sbi->s_system_blks, system_blks);
265
266 if (test_opt(sb, DEBUG))
267 debug_print_tree(sbi);
268 return 0;
269 err:
270 release_system_zone(system_blks);
271 kfree(system_blks);
272 return ret;
273 }
274
--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki
Hi Wang,
kernel test robot noticed the following build errors:
[auto build test ERROR on tytso-ext4/dev]
[also build test ERROR on linus/master v6.4-rc4 next-20230602]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]
url: https://github.com/intel-lab-lkp/linux/commits/Wang-Jianjian/ext4-Add-correct-group-descriptors-and-reserved-GDT-blocks-to-system-zone/20230604-003439
base: https://git.kernel.org/pub/scm/linux/kernel/git/tytso/ext4.git dev
patch link: https://lore.kernel.org/r/tencent_4A474CC049B9E77D0F172468991EED5B9105%40qq.com
patch subject: [PATCH] ext4: Add correct group descriptors and reserved GDT blocks to system zone
config: hexagon-randconfig-r041-20230604 (https://download.01.org/0day-ci/archive/20230604/[email protected]/config)
compiler: clang version 15.0.4 (https://github.com/llvm/llvm-project 5c68a1cb123161b54b72ce90e7975d95a8eaf2a4)
reproduce (this is a W=1 build):
mkdir -p ~/bin
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# https://github.com/intel-lab-lkp/linux/commit/54ea70d6b1189628ed4017129457d77e0bfa7fde
git remote add linux-review https://github.com/intel-lab-lkp/linux
git fetch --no-tags linux-review Wang-Jianjian/ext4-Add-correct-group-descriptors-and-reserved-GDT-blocks-to-system-zone/20230604-003439
git checkout 54ea70d6b1189628ed4017129457d77e0bfa7fde
# save the config file
mkdir build_dir && cp config build_dir/.config
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang ~/bin/make.cross W=1 O=build_dir ARCH=hexagon olddefconfig
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang ~/bin/make.cross W=1 O=build_dir ARCH=hexagon SHELL=/bin/bash fs/ext4/
If you fix the issue, kindly add following tag where applicable
| Reported-by: kernel test robot <[email protected]>
| Closes: https://lore.kernel.org/oe-kbuild-all/[email protected]/
All error/warnings (new ones prefixed by >>):
In file included from fs/ext4/block_validity.c:16:
In file included from include/linux/buffer_head.h:12:
In file included from include/linux/blk_types.h:10:
In file included from include/linux/bvec.h:10:
In file included from include/linux/highmem.h:12:
In file included from include/linux/hardirq.h:11:
In file included from ./arch/hexagon/include/generated/asm/hardirq.h:1:
In file included from include/asm-generic/hardirq.h:17:
In file included from include/linux/irq.h:20:
In file included from include/linux/io.h:13:
In file included from arch/hexagon/include/asm/io.h:334:
include/asm-generic/io.h:547:31: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
val = __raw_readb(PCI_IOBASE + addr);
~~~~~~~~~~ ^
include/asm-generic/io.h:560:61: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
val = __le16_to_cpu((__le16 __force)__raw_readw(PCI_IOBASE + addr));
~~~~~~~~~~ ^
include/uapi/linux/byteorder/little_endian.h:37:51: note: expanded from macro '__le16_to_cpu'
#define __le16_to_cpu(x) ((__force __u16)(__le16)(x))
^
In file included from fs/ext4/block_validity.c:16:
In file included from include/linux/buffer_head.h:12:
In file included from include/linux/blk_types.h:10:
In file included from include/linux/bvec.h:10:
In file included from include/linux/highmem.h:12:
In file included from include/linux/hardirq.h:11:
In file included from ./arch/hexagon/include/generated/asm/hardirq.h:1:
In file included from include/asm-generic/hardirq.h:17:
In file included from include/linux/irq.h:20:
In file included from include/linux/io.h:13:
In file included from arch/hexagon/include/asm/io.h:334:
include/asm-generic/io.h:573:61: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
val = __le32_to_cpu((__le32 __force)__raw_readl(PCI_IOBASE + addr));
~~~~~~~~~~ ^
include/uapi/linux/byteorder/little_endian.h:35:51: note: expanded from macro '__le32_to_cpu'
#define __le32_to_cpu(x) ((__force __u32)(__le32)(x))
^
In file included from fs/ext4/block_validity.c:16:
In file included from include/linux/buffer_head.h:12:
In file included from include/linux/blk_types.h:10:
In file included from include/linux/bvec.h:10:
In file included from include/linux/highmem.h:12:
In file included from include/linux/hardirq.h:11:
In file included from ./arch/hexagon/include/generated/asm/hardirq.h:1:
In file included from include/asm-generic/hardirq.h:17:
In file included from include/linux/irq.h:20:
In file included from include/linux/io.h:13:
In file included from arch/hexagon/include/asm/io.h:334:
include/asm-generic/io.h:584:33: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
__raw_writeb(value, PCI_IOBASE + addr);
~~~~~~~~~~ ^
include/asm-generic/io.h:594:59: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
__raw_writew((u16 __force)cpu_to_le16(value), PCI_IOBASE + addr);
~~~~~~~~~~ ^
include/asm-generic/io.h:604:59: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
__raw_writel((u32 __force)cpu_to_le32(value), PCI_IOBASE + addr);
~~~~~~~~~~ ^
>> fs/ext4/block_validity.c:228:44: error: no member named 'es' in 'struct ext4_sb_info'
unsigned int rsvd_gdt = le16_to_cpu(sbi->es->s_reserved_gdt_blocks);
~~~ ^
include/linux/byteorder/generic.h:91:21: note: expanded from macro 'le16_to_cpu'
#define le16_to_cpu __le16_to_cpu
^
include/uapi/linux/byteorder/little_endian.h:37:51: note: expanded from macro '__le16_to_cpu'
#define __le16_to_cpu(x) ((__force __u16)(__le16)(x))
^
>> fs/ext4/block_validity.c:226:16: warning: mixing declarations and code is incompatible with standards before C99 [-Wdeclaration-after-statement]
unsigned int sb_num = ext4_bg_has_super(sb, i);
^
7 warnings and 1 error generated.
vim +228 fs/ext4/block_validity.c
201
202 /*
203 * Build system zone rbtree which is used for block validity checking.
204 *
205 * The update of system_blks pointer in this function is protected by
206 * sb->s_umount semaphore. However we have to be careful as we can be
207 * racing with ext4_inode_block_valid() calls reading system_blks rbtree
208 * protected only by RCU. That's why we first build the rbtree and then
209 * swap it in place.
210 */
211 int ext4_setup_system_zone(struct super_block *sb)
212 {
213 ext4_group_t ngroups = ext4_get_groups_count(sb);
214 struct ext4_sb_info *sbi = EXT4_SB(sb);
215 struct ext4_system_blocks *system_blks;
216 struct ext4_group_desc *gdp;
217 ext4_group_t i;
218 int ret;
219
220 system_blks = kzalloc(sizeof(*system_blks), GFP_KERNEL);
221 if (!system_blks)
222 return -ENOMEM;
223
224 for (i=0; i < ngroups; i++) {
225 cond_resched();
> 226 unsigned int sb_num = ext4_bg_has_super(sb, i);
227 unsigned long gdb_num = ext4_bg_num_gdb(sb, i);
> 228 unsigned int rsvd_gdt = le16_to_cpu(sbi->es->s_reserved_gdt_blocks);
229
230 if (sb_num != 0 || gdb_num != 0) {
231 ret = add_system_zone(system_blks,
232 ext4_group_first_block_no(sb, i),
233 sb_num + gdb_num + rsvd_gdt, 0);
234 if (ret)
235 goto err;
236 }
237 gdp = ext4_get_group_desc(sb, i, NULL);
238 ret = add_system_zone(system_blks,
239 ext4_block_bitmap(sb, gdp), 1, 0);
240 if (ret)
241 goto err;
242 ret = add_system_zone(system_blks,
243 ext4_inode_bitmap(sb, gdp), 1, 0);
244 if (ret)
245 goto err;
246 ret = add_system_zone(system_blks,
247 ext4_inode_table(sb, gdp),
248 sbi->s_itb_per_group, 0);
249 if (ret)
250 goto err;
251 }
252 if (ext4_has_feature_journal(sb) && sbi->s_es->s_journal_inum) {
253 ret = ext4_protect_reserved_inode(sb, system_blks,
254 le32_to_cpu(sbi->s_es->s_journal_inum));
255 if (ret)
256 goto err;
257 }
258
259 /*
260 * System blks rbtree complete, announce it once to prevent racing
261 * with ext4_inode_block_valid() accessing the rbtree at the same
262 * time.
263 */
264 rcu_assign_pointer(sbi->s_system_blks, system_blks);
265
266 if (test_opt(sb, DEBUG))
267 debug_print_tree(sbi);
268 return 0;
269 err:
270 release_system_zone(system_blks);
271 kfree(system_blks);
272 return ret;
273 }
274
--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki
> diff --git a/fs/ext4/block_validity.c b/fs/ext4/block_validity.c
> index 5504f72bbbbe..5df357763975 100644
> --- a/fs/ext4/block_validity.c
> +++ b/fs/ext4/block_validity.c
> @@ -224,11 +223,14 @@ int ext4_setup_system_zone(struct super_block *sb)
>
> for (i=0; i < ngroups; i++) {
> cond_resched();
> - if (ext4_bg_has_super(sb, i) &&
> - ((i < 5) || ((i % flex_size) == 0))) {
> + unsigned int sb_num = ext4_bg_has_super(sb, i);
> + unsigned long gdb_num = ext4_bg_num_gdb(sb, i);
> + unsigned int rsvd_gdt = le16_to_cpu(sbi->es->s_reserved_gdt_blocks);
> +
> + if (sb_num != 0 || gdb_num != 0) {
> ret = add_system_zone(system_blks,
> ext4_group_first_block_no(sb, i),
> - ext4_bg_num_gdb(sb, i) + 1, 0);
> + sb_num + gdb_num + rsvd_gdt, 0);
> if (ret)
> goto err;
> }
How the reserved GDT blocks should be added to the system zone are not
handled correctly in this patch. It can't be unconditionally added to
all block groups.
See the logic in ext4_num_base_meta_clusters() in fs/ext4/balloc.c ---
without the EXT4_NUM_B2C() at the end of the function, since the
system zone tracking is done at the block level, not the cluster
level.
- Ted
Thanks Ted, I send a new fix. Please help review.
On 6/4/23 11:45, Theodore Ts'o wrote:
>> diff --git a/fs/ext4/block_validity.c b/fs/ext4/block_validity.c
>> index 5504f72bbbbe..5df357763975 100644
>> --- a/fs/ext4/block_validity.c
>> +++ b/fs/ext4/block_validity.c
>> @@ -224,11 +223,14 @@ int ext4_setup_system_zone(struct super_block *sb)
>>
>> for (i=0; i < ngroups; i++) {
>> cond_resched();
>> - if (ext4_bg_has_super(sb, i) &&
>> - ((i < 5) || ((i % flex_size) == 0))) {
>> + unsigned int sb_num = ext4_bg_has_super(sb, i);
>> + unsigned long gdb_num = ext4_bg_num_gdb(sb, i);
>> + unsigned int rsvd_gdt = le16_to_cpu(sbi->es->s_reserved_gdt_blocks);
>> +
>> + if (sb_num != 0 || gdb_num != 0) {
>> ret = add_system_zone(system_blks,
>> ext4_group_first_block_no(sb, i),
>> - ext4_bg_num_gdb(sb, i) + 1, 0);
>> + sb_num + gdb_num + rsvd_gdt, 0);
>> if (ret)
>> goto err;
>> }
>
>
> How the reserved GDT blocks should be added to the system zone are not
> handled correctly in this patch. It can't be unconditionally added to
> all block groups.
>
> See the logic in ext4_num_base_meta_clusters() in fs/ext4/balloc.c ---
> without the EXT4_NUM_B2C() at the end of the function, since the
> system zone tracking is done at the block level, not the cluster
> level.
>
> - Ted
>