ext4: mballoc/extents: Code cleanup and debug improvements
Subsystems (2)
cover ext4: mballoc/extents: Code cleanup and debug improvements
Hello All,
This series does some code refactoring/cleanups and debug logs improvements around mb_debug() and ext_debug(). These were found when working over improving mballoc ENOSPC handling in ext4.
These should be small and stright forward patches for reviewing.
Ritesh Harjani (16):
ext4: mballoc: Do print bb_free info even when it is 0
ext4: mballoc: Refactor ext4_mb_show_ac()
ext4: mballoc: Add more mb_debug() msgs
ext4: mballoc: Correct the mb_debug() format specifier for pa_len var
ext4: mballoc: Fix few other format specifier in mb_debug()
ext4: mballoc: Simplify error handling in ext4_init_mballoc()
ext4: mballoc: Make ext4_mb_use_preallocated() return type as bool
ext4: mballoc: Refactor code inside DOUBLE_CHECK into separate function
ext4: mballoc: Fix possible NULL ptr & remove BUG_ONs from DOUBLE_CHECK
ext4: balloc: Use task_pid_nr() helper
ext4: Use BIT() macro for BH_** state bits
ext4: Improve ext_debug() msg in case of block allocation failure
ext4: Replace EXT_DEBUG with __maybe_unused in ext4_ext_handle_unwritten_extents()
ext4: mballoc: Make mb_debug() implementation to use pr_debug()
ext4: Make ext_debug() implementation to use pr_debug()
ext4: Add process name and pid in ext4_msg()
fs/ext4/Kconfig | 3 +- fs/ext4/balloc.c | 5 +- fs/ext4/ext4.h | 26 +++-- fs/ext4/extents.c | 150 +++++++++++++------------- fs/ext4/inode.c | 15 +-- fs/ext4/mballoc.c | 265 ++++++++++++++++++++++++++-------------------- fs/ext4/mballoc.h | 16 ++- fs/ext4/super.c | 3 +- 8 files changed, 261 insertions(+), 222 deletions(-)
Signature · 1 line
2.21.0
RFC 1/16 ext4: mballoc: Do print bb_free info even when it is 0
Improve the debugging msg by also printing even if bb_free is 0.
--- fs/ext4/mballoc.c | 2 -- 1 file changed, 2 deletions(-)
fs/ext4/mballoc.c +0 -2
@@ -4170,8 +4170,6 @@ static void ext4_mb_show_ac(struct ext4_allocation_context *ac) }ext4_unlock_group (sb, i); -if (grp->bb_free ==0 ) -continue ;printk (KERN_ERR "%u: %d/%d \n" , i, grp->bb_free, grp->bb_fragments); }
Signature · 1 line
2.21.0
RFC 2/16 ext4: mballoc: Refactor ext4_mb_show_ac()
In reply to Ritesh HarjaniThis factors out ext4_mb_show_pa() function to show all the group's preallocation info. This could be useful info to be added in later patches.
There should be no functionality change in this patch.
--- fs/ext4/mballoc.c | 65 +++++++++++++++++++++++++++++------------------ 1 file changed, 40 insertions(+), 25 deletions(-)
fs/ext4/mballoc.c +40 -25
@@ -4120,38 +4120,16 @@ void ext4_discard_preallocations(struct inode *inode) }#ifdef CONFIG_EXT4_DEBUG -static void ext4_mb_show_ac (struct ext4_allocation_context *ac) +static inline void ext4_mb_show_pa (struct super_block *sb) { -struct super_block *sb = ac->ac_sb; -ext4_group_t ngroups, i; +ext4_group_t i, ngroups;if (!ext4_mballoc_debug || (EXT4_SB (sb)->s_mount_flags &EXT4_MF_FS_ABORTED ))return ; -ext4_msg (ac->ac_sb,KERN_ERR ,"Can't allocate:" -" Allocation context details:" ); -ext4_msg (ac->ac_sb,KERN_ERR ,"status %d flags %d" , - ac->ac_status, ac->ac_flags); -ext4_msg (ac->ac_sb,KERN_ERR ,"orig %lu/%lu/%lu@%lu, " -"goal %lu/%lu/%lu@%lu, " -"best %lu/%lu/%lu@%lu cr %d" , - (unsigned long )ac->ac_o_ex.fe_group, - (unsigned long )ac->ac_o_ex.fe_start, - (unsigned long )ac->ac_o_ex.fe_len, - (unsigned long )ac->ac_o_ex.fe_logical, - (unsigned long )ac->ac_g_ex.fe_group, - (unsigned long )ac->ac_g_ex.fe_start, - (unsigned long )ac->ac_g_ex.fe_len, - (unsigned long )ac->ac_g_ex.fe_logical, - (unsigned long )ac->ac_b_ex.fe_group, - (unsigned long )ac->ac_b_ex.fe_start, - (unsigned long )ac->ac_b_ex.fe_len, - (unsigned long )ac->ac_b_ex.fe_logical, - (int )ac->ac_criteria); -ext4_msg (ac->ac_sb,KERN_ERR ,"%d found" , ac->ac_found); -ext4_msg (ac->ac_sb,KERN_ERR ,"groups: " ); ngroups =ext4_get_groups_count (sb); +ext4_msg (sb,KERN_ERR ,"groups: " );for (i =0 ; i < ngroups; i++) {struct ext4_group_info *grp =ext4_get_group_info (sb, i);struct ext4_prealloc_space *pa;
@@ -4175,9 +4153,46 @@ static void ext4_mb_show_ac(struct ext4_allocation_context *ac) }printk (KERN_ERR "\n" ); } + +static void ext4_mb_show_ac (struct ext4_allocation_context *ac) +{ +struct super_block *sb = ac->ac_sb; + +if (!ext4_mballoc_debug || + (EXT4_SB (sb)->s_mount_flags &EXT4_MF_FS_ABORTED )) +return ; + +ext4_msg (sb,KERN_ERR ,"Can't allocate:" +" Allocation context details:" ); +ext4_msg (sb,KERN_ERR ,"status %d flags %d" , + ac->ac_status, ac->ac_flags); +ext4_msg (sb,KERN_ERR ,"orig %lu/%lu/%lu@%lu, " +"goal %lu/%lu/%lu@%lu, " +"best %lu/%lu/%lu@%lu cr %d" , + (unsigned long )ac->ac_o_ex.fe_group, + (unsigned long )ac->ac_o_ex.fe_start, + (unsigned long )ac->ac_o_ex.fe_len, + (unsigned long )ac->ac_o_ex.fe_logical, + (unsigned long )ac->ac_g_ex.fe_group, + (unsigned long )ac->ac_g_ex.fe_start, + (unsigned long )ac->ac_g_ex.fe_len, + (unsigned long )ac->ac_g_ex.fe_logical, + (unsigned long )ac->ac_b_ex.fe_group, + (unsigned long )ac->ac_b_ex.fe_start, + (unsigned long )ac->ac_b_ex.fe_len, + (unsigned long )ac->ac_b_ex.fe_logical, + (int )ac->ac_criteria); +ext4_msg (sb,KERN_ERR ,"%d found" , ac->ac_found); +ext4_mb_show_pa (sb); +}#else +static inline void ext4_mb_show_pa (struct super_block *sb) +{ +return ; +}static inline void ext4_mb_show_ac (struct ext4_allocation_context *ac) { +ext4_mb_show_pa (ac->ac_sb);return ; }#endif
Signature · 1 line
2.21.0
RFC 3/16 ext4: mballoc: Add more mb_debug() msgs
In reply to Ritesh HarjaniThis patch adds some more debugging mb_debug() msgs to help improve mballoc code debugging.
Other than adding more mb_debug() msgs at few more places, there should be no other functionality change in this patch.
--- fs/ext4/mballoc.c | 18 ++++++++++++++---- 1 file changed, 14 insertions(+), 4 deletions(-)
fs/ext4/mballoc.c +14 -4
@@ -2108,7 +2108,7 @@ static noinline_for_stack int ext4_mb_regular_allocator(struct ext4_allocation_context *ac) {ext4_group_t ngroups, group, i; -int cr; +int cr =-1 ;int err =0 , first_err =0 ;struct ext4_sb_info *sbi;struct super_block *sb;
@@ -2260,6 +2260,10 @@ ext4_mb_regular_allocator(struct ext4_allocation_context *ac) out:if (!err && ac->ac_status !=AC_STATUS_FOUND && first_err) err = first_err; + +mb_debug (1 ,"Best len %d, origin len %d, ac_status %u, ac_flags 0x%x, cr %d ret %d\n" , + ac->ac_b_ex.fe_len, ac->ac_o_ex.fe_len, ac->ac_status, + ac->ac_flags, cr, err);return err; }
@@ -3918,7 +3922,7 @@ ext4_mb_discard_group_preallocations(struct super_block *sb,mb_debug (1 ,"discard preallocation for group %u\n" , group);if (list_empty (&grp->bb_prealloc_list)) -return 0 ; + goto out_dbg; bitmap_bh =ext4_read_block_bitmap (sb, group);if (IS_ERR (bitmap_bh)) {
@@ -3926,7 +3930,7 @@ ext4_mb_discard_group_preallocations(struct super_block *sb,ext4_error_err (sb, -err,"Error %d reading block bitmap for %u" , err, group); -return 0 ; + goto out_dbg; } err =ext4_mb_load_buddy (sb, group, &e4b);
@@ -3934,7 +3938,7 @@ ext4_mb_discard_group_preallocations(struct super_block *sb,ext4_warning (sb,"Error %d loading buddy information for %u" , err, group);put_bh (bitmap_bh); -return 0 ; + goto out_dbg; }if (needed ==0 )
@@ -3979,6 +3983,8 @@ ext4_mb_discard_group_preallocations(struct super_block *sb,/* found anything to free? */ if (list_empty (&list)) {BUG_ON (free !=0 ); +mb_debug (1 ,"Someone else may have freed PA for this group %u\n" , + group); goto out; }
@@ -4003,6 +4009,9 @@ ext4_mb_discard_group_preallocations(struct super_block *sb,ext4_unlock_group (sb, group);ext4_mb_unload_buddy (&e4b);put_bh (bitmap_bh); +out_dbg: +mb_debug (1 ,"discarded (%d) blocks preallocated for group %u bb_free (%d)\n" , + free, group, grp->bb_free);return free; }
@@ -4538,6 +4547,7 @@ ext4_fsblk_t ext4_mb_new_blocks(handle_t *handle, ar->len = ar->len >>1 ; }if (!ar->len) { +ext4_mb_show_pa (sb); *errp = -ENOSPC ;return 0 ; }
Signature · 1 line
2.21.0
RFC 4/16 ext4: mballoc: Correct the mb_debug() format specifier for pa_len var
In reply to Ritesh Harjanipa->pa_len is an integer. Fix all of the format specifier used in mb_debug() for pa_len to %d instead of %u.
As such no functionality change in this patch.
--- fs/ext4/mballoc.c | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-)
fs/ext4/mballoc.c +5 -5
@@ -3720,7 +3720,7 @@ ext4_mb_new_inode_pa(struct ext4_allocation_context *ac) pa->pa_deleted =0 ; pa->pa_type =MB_INODE_PA ; -mb_debug (1 ,"new inode pa %p: %llu/%u for %u\n" , pa, +mb_debug (1 ,"new inode pa %p: %llu/%d for %u\n" , pa, pa->pa_pstart, pa->pa_len, pa->pa_lstart);trace_ext4_mb_new_inode_pa (ac, pa);
@@ -3780,7 +3780,7 @@ ext4_mb_new_group_pa(struct ext4_allocation_context *ac) pa->pa_deleted =0 ; pa->pa_type =MB_GROUP_PA ; -mb_debug (1 ,"new group pa %p: %llu/%u for %u\n" , pa, +mb_debug (1 ,"new group pa %p: %llu/%d for %u\n" , pa, pa->pa_pstart, pa->pa_len, pa->pa_lstart);trace_ext4_mb_new_group_pa (ac, pa);
@@ -3862,10 +3862,10 @@ ext4_mb_release_inode_pa(struct ext4_buddy *e4b, struct buffer_head *bitmap_bh, }if (free != pa->pa_free) {ext4_msg (e4b->bd_sb,KERN_CRIT , -"pa %p: logic %lu, phys. %lu, len %lu" , +"pa %p: logic %lu, phys. %lu, len %d" , pa, (unsigned long ) pa->pa_lstart, (unsigned long ) pa->pa_pstart, - (unsigned long ) pa->pa_len); + pa->pa_len);ext4_grp_locked_error (sb, group,0 ,0 ,"free %u, pa_free %u" , free, pa->pa_free);/*
@@ -4152,7 +4152,7 @@ static inline void ext4_mb_show_pa(struct super_block *sb)ext4_get_group_no_and_offset (sb, pa->pa_pstart,NULL , &start);spin_unlock (&pa->pa_lock); -printk (KERN_ERR "PA:%u:%d:%u \n" , i, +printk (KERN_ERR "PA:%u:%d:%d \n" , i, start, pa->pa_len); }ext4_unlock_group (sb, i);
Signature · 1 line
2.21.0
RFC 5/16 ext4: mballoc: Fix few other format specifier in mb_debug()
In reply to Ritesh HarjaniFix few other format specifiers in mb_debug() msgs. As such no other functionality change in this patch.
--- fs/ext4/mballoc.c | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-)
fs/ext4/mballoc.c +6 -6
@@ -3280,8 +3280,8 @@ ext4_mb_normalize_request(struct ext4_allocation_context *ac, ac->ac_flags |=EXT4_MB_HINT_TRY_GOAL ; } -mb_debug (1 ,"goal: %u(was %u) blocks at %u\n" , (unsigned ) size, - (unsigned ) orig_size, (unsigned ) start); +mb_debug (1 ,"goal: %lld(was %lld) blocks at %u\n" , size, orig_size, + start); }static void ext4_mb_collect_stats (struct ext4_allocation_context *ac)
@@ -3370,7 +3370,7 @@ static void ext4_mb_use_inode_pa(struct ext4_allocation_context *ac,BUG_ON (pa->pa_free < len); pa->pa_free -= len; -mb_debug (1 ,"use %llu/%u from inode pa %p\n" , start, len, pa); +mb_debug (1 ,"use %llu/%d from inode pa %p\n" , start, len, pa); }/*
@@ -3577,7 +3577,7 @@ void ext4_mb_generate_from_pa(struct super_block *sb, void *bitmap,ext4_set_bits (bitmap, start, len); preallocated += len; } -mb_debug (1 ,"preallocated %u for group %u\n" , preallocated, group); +mb_debug (1 ,"preallocated %d for group %u\n" , preallocated, group); }static void ext4_mb_pa_callback (struct rcu_head *head)
@@ -4173,7 +4173,7 @@ static void ext4_mb_show_ac(struct ext4_allocation_context *ac)ext4_msg (sb,KERN_ERR ,"Can't allocate:" " Allocation context details:" ); -ext4_msg (sb,KERN_ERR ,"status %d flags %d" , +ext4_msg (sb,KERN_ERR ,"status %u flags 0x%x" , ac->ac_status, ac->ac_flags);ext4_msg (sb,KERN_ERR ,"orig %lu/%lu/%lu@%lu, " "goal %lu/%lu/%lu@%lu, "
@@ -4191,7 +4191,7 @@ static void ext4_mb_show_ac(struct ext4_allocation_context *ac) (unsigned long )ac->ac_b_ex.fe_len, (unsigned long )ac->ac_b_ex.fe_logical, (int)ac->ac_criteria); -ext4_msg (sb,KERN_ERR ,"%d found" , ac->ac_found); +ext4_msg (sb,KERN_ERR ,"%u found" , ac->ac_found); ext4_mb_show_pa(sb); }#else
Signature · 1 line
2.21.0
RFC 6/16 ext4: mballoc: Simplify error handling in ext4_init_mballoc()
In reply to Ritesh HarjaniThis patch simplifies error handling logic in ext4_init_mballoc(), by adding all the cleanups at one place at the end of that function.
There should be no functionality change in this patch.
--- fs/ext4/mballoc.c | 23 +++++++++++++---------- 1 file changed, 13 insertions(+), 10 deletions(-)
fs/ext4/mballoc.c +13 -10
@@ -2913,23 +2913,26 @@ int __init ext4_init_mballoc(void) ext4_pspace_cachep =KMEM_CACHE (ext4_prealloc_space,SLAB_RECLAIM_ACCOUNT );if (ext4_pspace_cachep ==NULL ) -return -ENOMEM ; + goto out; ext4_ac_cachep =KMEM_CACHE (ext4_allocation_context,SLAB_RECLAIM_ACCOUNT ); -if (ext4_ac_cachep ==NULL ) { -kmem_cache_destroy (ext4_pspace_cachep); -return -ENOMEM ; - } +if (ext4_ac_cachep ==NULL ) + goto out_pa_free; ext4_free_data_cachep =KMEM_CACHE (ext4_free_data,SLAB_RECLAIM_ACCOUNT ); -if (ext4_free_data_cachep ==NULL ) { -kmem_cache_destroy (ext4_pspace_cachep); -kmem_cache_destroy (ext4_ac_cachep); -return -ENOMEM ; - } +if (ext4_free_data_cachep ==NULL ) + goto out_ac_free; +return 0 ; + +out_ac_free: +kmem_cache_destroy (ext4_ac_cachep); +out_pa_free: +kmem_cache_destroy (ext4_pspace_cachep); +out: +return -ENOMEM ; }void ext4_exit_mballoc (void )
Signature · 1 line
2.21.0
RFC 7/16 ext4: mballoc: Make ext4_mb_use_preallocated() return type as bool
In reply to Ritesh HarjaniChange return type of function ext4_mb_use_preallocated() to bool to better reflect what this function can return.
There should be no functionality change in this patch.
--- fs/ext4/mballoc.c | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-)
fs/ext4/mballoc.c +7 -7
@@ -3432,7 +3432,7 @@ ext4_mb_check_group_pa(ext4_fsblk_t goal_block,/* * search goal blocks in preallocated space */ -static noinline_for_stack int +static noinline_for_stack boolext4_mb_use_preallocated (struct ext4_allocation_context *ac) {struct ext4_sb_info *sbi =EXT4_SB (ac->ac_sb);
@@ -3444,7 +3444,7 @@ ext4_mb_use_preallocated(struct ext4_allocation_context *ac)/* only data can be preallocated */ if (!(ac->ac_flags &EXT4_MB_HINT_DATA )) -return 0 ; +return false;/* first, try per-file preallocation */ rcu_read_lock ();
@@ -3471,7 +3471,7 @@ ext4_mb_use_preallocated(struct ext4_allocation_context *ac)spin_unlock (&pa->pa_lock); ac->ac_criteria =10 ;rcu_read_unlock (); -return 1 ; +return true; }spin_unlock (&pa->pa_lock); }
@@ -3479,12 +3479,12 @@ ext4_mb_use_preallocated(struct ext4_allocation_context *ac)/* can we use group allocation? */ if (!(ac->ac_flags &EXT4_MB_HINT_GROUP_ALLOC )) -return 0 ; +return false;/* inode may have no locality group for some reason */ lg = ac->ac_lg;if (lg ==NULL ) -return 0 ; +return false; order =fls (ac->ac_o_ex.fe_len) -1 ;if (order >PREALLOC_TB_SIZE -1 )/* The max size of hash table is PREALLOC_TB_SIZE */
@@ -3513,9 +3513,9 @@ ext4_mb_use_preallocated(struct ext4_allocation_context *ac)if (cpa) {ext4_mb_use_group_pa (ac, cpa); ac->ac_criteria =20 ; -return 1 ; +return true; } -return 0 ; +return false; }/*
Signature · 1 line
2.21.0
RFC 8/16 ext4: mballoc: Refactor code inside DOUBLE_CHECK into separate function
In reply to Ritesh HarjaniThis patch implemets mb_group_bb_bitmap_alloc() and mb_group_bb_bitmap_free() function to remove #ifdef DOUBLE_CHECK macro and it's related code from inside ext4_mb_add_groupinfo()/ext4_mb_release().
There should be no functionality change in this patch.
--- fs/ext4/mballoc.c | 50 +++++++++++++++++++++++++++++++---------------- 1 file changed, 33 insertions(+), 17 deletions(-)
fs/ext4/mballoc.c +33 -17
@@ -511,6 +511,26 @@ static void mb_cmp_bitmaps(struct ext4_buddy *e4b, void *bitmap) } } +static void mb_group_bb_bitmap_alloc (struct super_block *sb, +struct ext4_group_info *grp,ext4_group_t group) +{ +struct buffer_head *bh; + + grp->bb_bitmap =kmalloc (sb->s_blocksize,GFP_NOFS ); +BUG_ON (grp->bb_bitmap ==NULL ); + + bh =ext4_read_block_bitmap (sb, group); +BUG_ON (IS_ERR_OR_NULL (bh)); + +memcpy (grp->bb_bitmap, bh->b_data, sb->s_blocksize); +put_bh (bh); +} + +static void mb_group_bb_bitmap_free (struct ext4_group_info *grp) +{ +kfree (grp->bb_bitmap); +} +#else static inline void mb_free_blocks_double (struct inode *inode,struct ext4_buddy *e4b,int first,int count)
@@ -526,6 +546,17 @@ static inline void mb_cmp_bitmaps(struct ext4_buddy *e4b, void *bitmap) {return ; } + +static inline void mb_group_bb_bitmap_alloc (struct super_block *sb, +struct ext4_group_info *grp,ext4_group_t group) +{ +return ; +} + +static inline void mb_group_bb_bitmap_free (struct ext4_group_info *grp) +{ +return ; +}#endif #ifdef AGGRESSIVE_CHECK
@@ -2456,20 +2487,7 @@ int ext4_mb_add_groupinfo(struct super_block *sb, ext4_group_t group, meta_group_info[i]->bb_free_root =RB_ROOT ; meta_group_info[i]->bb_largest_free_order =-1 ;/* uninit */ -#ifdef DOUBLE_CHECK - { -struct buffer_head *bh; - meta_group_info[i]->bb_bitmap = -kmalloc (sb->s_blocksize,GFP_NOFS ); -BUG_ON (meta_group_info[i]->bb_bitmap ==NULL ); - bh =ext4_read_block_bitmap (sb, group); -BUG_ON (IS_ERR_OR_NULL (bh)); -memcpy (meta_group_info[i]->bb_bitmap, bh->b_data, - sb->s_blocksize); -put_bh (bh); - } -#endif - +mb_group_bb_bitmap_alloc (sb, meta_group_info[i], group);return 0 ; exit_group_info:
@@ -2736,9 +2754,7 @@ int ext4_mb_release(struct super_block *sb)for (i =0 ; i < ngroups; i++) {cond_resched (); grinfo =ext4_get_group_info (sb, i); -#ifdef DOUBLE_CHECK -kfree (grinfo->bb_bitmap); -#endif +mb_group_bb_bitmap_free (grinfo);ext4_lock_group (sb, i);ext4_mb_cleanup_pa (grinfo);ext4_unlock_group (sb, i);
Signature · 1 line
2.21.0
RFC 9/16 ext4: mballoc: Fix possible NULL ptr & remove BUG_ONs from DOUBLE_CHECK
In reply to Ritesh HarjaniMake sure to check for e4b->bd_info->bb_bitmap == NULL, in mb_cmp_bitmaps() and return if NULL, to avoid possible NULL ptr dereference. Similar to how we do this in other ifdef DOUBLE_CHECK functions.
Also remove the BUG_ON() logic if kmalloc() or ext4_read_block_bitmap() fails. We should simply mark grp->bb_bitmap as NULL if above happens. In fact ext4_read_block_bitmap() may even return an error in case of resize ioctl. Hence remove this BUG_ON logic (fstests ext4/032 may trigger this).
---
--- fs/ext4/mballoc.c | 11 +++++++++-- 1 file changed, 9 insertions(+), 2 deletions(-)
fs/ext4/mballoc.c +9 -2
@@ -493,6 +493,8 @@ static void mb_mark_used_double(struct ext4_buddy *e4b, int first, int count)static void mb_cmp_bitmaps (struct ext4_buddy *e4b,void *bitmap) { +if (unlikely (e4b->bd_info->bb_bitmap ==NULL )) +return ;if (memcmp (e4b->bd_info->bb_bitmap, bitmap, e4b->bd_sb->s_blocksize)) {unsigned char *b1, *b2;int i;
@@ -517,10 +519,15 @@ static void mb_group_bb_bitmap_alloc(struct super_block *sb,struct buffer_head *bh; grp->bb_bitmap =kmalloc (sb->s_blocksize,GFP_NOFS ); -BUG_ON (grp->bb_bitmap ==NULL ); +if (!grp->bb_bitmap) +return ; bh =ext4_read_block_bitmap (sb, group); -BUG_ON (IS_ERR_OR_NULL (bh)); +if (IS_ERR_OR_NULL (bh)) { +kfree (grp->bb_bitmap); + grp->bb_bitmap =NULL ; +return ; + }memcpy (grp->bb_bitmap, bh->b_data, sb->s_blocksize);put_bh (bh);
Signature · 1 line
2.21.0
RFC 10/16 ext4: balloc: Use task_pid_nr() helper
In reply to Ritesh HarjaniUse task_pid_nr() function instead of current->pid. There should be no functionality change in this patch.
--- fs/ext4/balloc.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-)
fs/ext4/balloc.c +3 -2
@@ -903,10 +903,11 @@ ext4_fsblk_t ext4_inode_to_goal_block(struct inode *inode)return bg_start;if (bg_start +EXT4_BLOCKS_PER_GROUP (inode->i_sb) <= last_block) - colour = (current->pid %16 ) * + colour = (task_pid_nr (current) %16 ) * (EXT4_BLOCKS_PER_GROUP (inode->i_sb) /16 );else - colour = (current->pid %16 ) * ((last_block - bg_start) /16 ); + colour = (task_pid_nr (current) %16 ) * + ((last_block - bg_start) /16 );return bg_start + colour; }
Signature · 1 line
2.21.0
RFC 11/16 ext4: Use BIT() macro for BH_** state bits
In reply to Ritesh HarjaniSimply use BIT() macro for all BH_** state bits instead of open coding it.
There should be no functionality change in this patch.
--- fs/ext4/ext4.h | 8 ++++---- fs/ext4/inode.c | 4 ++-- 2 files changed, 6 insertions(+), 6 deletions(-)
fs/ext4/ext4.h +4 -4
@@ -171,10 +171,10 @@ struct ext4_allocation_request {* well as to store the information returned by ext4_map_blocks(). It * takes less room on the stack than a struct buffer_head. */ -#define EXT4_MAP_NEW (1 << BH_New) -#define EXT4_MAP_MAPPED (1 << BH_Mapped) -#define EXT4_MAP_UNWRITTEN (1 << BH_Unwritten) -#define EXT4_MAP_BOUNDARY (1 << BH_Boundary) +#define EXT4_MAP_NEW BIT(BH_New) +#define EXT4_MAP_MAPPED BIT(BH_Mapped) +#define EXT4_MAP_UNWRITTEN BIT(BH_Unwritten) +#define EXT4_MAP_BOUNDARY BIT(BH_Boundary)#define EXT4_MAP_FLAGS (EXT4_MAP_NEW | EXT4_MAP_MAPPED |\ EXT4_MAP_UNWRITTEN | EXT4_MAP_BOUNDARY)
fs/ext4/inode.c +2 -2
@@ -2078,7 +2078,7 @@ static int mpage_submit_page(struct mpage_da_data *mpd, struct page *page)return err; } -#define BH_FLAGS ((1 << BH_Unwritten) | (1 << BH_Delay)) +#define BH_FLAGS (BIT(BH_Unwritten) | BIT(BH_Delay))/* * mballoc gives us at most this number of blocks...
@@ -2358,7 +2358,7 @@ static int mpage_map_one_extent(handle_t *handle, struct mpage_da_data *mpd) dioread_nolock =ext4_should_dioread_nolock (inode);if (dioread_nolock) get_blocks_flags |=EXT4_GET_BLOCKS_IO_CREATE_EXT ; -if (map->m_flags & (1 << BH_Delay)) +if (map->m_flags &BIT (BH_Delay)) get_blocks_flags |=EXT4_GET_BLOCKS_DELALLOC_RESERVE ; err =ext4_map_blocks (handle, inode, map, get_blocks_flags);
Signature · 1 line
2.21.0
RFC 12/16 ext4: Improve ext_debug() msg in case of block allocation failure
In reply to Ritesh Harjaniext4_map_blocks() has ext_debug msg early at the start of function. We also get ext_debug msg if we could allocate a block from ext4_ext_map_blocks(). But there is no ext_debug() msg in case of block allocation failure. So add one along with error code.
Also add more info in ext_debug() msg like how many blocks were allocated v/s how many were requested in ext4_ext_map_blocks().
--- fs/ext4/extents.c | 4 ++-- fs/ext4/inode.c | 4 ++++ 2 files changed, 6 insertions(+), 2 deletions(-)
fs/ext4/extents.c +2 -2
@@ -4218,10 +4218,10 @@ int ext4_ext_map_blocks(handle_t *handle, struct inode *inode, newblock =ext4_mb_new_blocks (handle, &ar, &err);if (!newblock) goto out2; -ext_debug ("allocate new block: goal %llu, found %llu/%u\n" , - ar.goal, newblock, allocated); allocated_clusters = ar.len; ar.len =EXT4_C2B (sbi, ar.len) - offset; +ext_debug ("allocate new block: goal %llu, found %llu/%u, requested %u\n" , + ar.goal, newblock, ar.len, allocated);if (ar.len > allocated) ar.len = allocated;
fs/ext4/inode.c +4 -0
@@ -726,6 +726,10 @@ int ext4_map_blocks(handle_t *handle, struct inode *inode,return ret; } } + +if (retval <0 ) +ext_debug ("failed for inode %lu with err %d\n" , + inode->i_ino, retval);return retval; }
Signature · 1 line
2.21.0
RFC 13/16 ext4: Replace EXT_DEBUG with __maybe_unused in ext4_ext_handle_unwritten_extents()
In reply to Ritesh HarjaniReplace EXT_DEBUG with __maybe_unused from insideext4_ext_handle_unwritten_extents() function.
There should be no functionality change in this patch.
--- fs/ext4/extents.c | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-)
fs/ext4/extents.c +1 -3
@@ -3794,9 +3794,7 @@ ext4_ext_handle_unwritten_extents(handle_t *handle, struct inode *inode, struct ext4_ext_path **ppath, int flags, unsigned int allocated, ext4_fsblk_t newblock) { -#ifdef EXT_DEBUG - struct ext4_ext_path *path = *ppath; -#endif + struct ext4_ext_path __maybe_unused *path = *ppath;int ret =0 ;int err =0 ;
Signature · 1 line
2.21.0
RFC 14/16 ext4: mballoc: Make mb_debug() implementation to use pr_debug()
In reply to Ritesh Harjanimb_debug() msg had only 1 control level for all type of msgs. And if we enable mballoc_debug then all of those msgs would be enabled. Instead of adding multiple debug levels for mb_debug() msgs, use pr_debug() with which we could have finer control to print msgs at all of different levels (i.e. at file, func, line no.).
Also add process name/pid, superblk id, and other info in mb_debug() msg. This also kills the mballoc_debug module parameter, since it is not needed any more.
--- fs/ext4/Kconfig | 3 +- fs/ext4/mballoc.c | 104 +++++++++++++++++++++------------------------- fs/ext4/mballoc.h | 16 +++---- 3 files changed, 55 insertions(+), 68 deletions(-)
fs/ext4/Kconfig +1 -2
@@ -99,8 +99,7 @@ config EXT4_DEBUG Enables run-time debugging support for the ext4 filesystem. If you select Y here, then you will be able to turn on debugging - with a command such as: - echo 1 > /sys/module/ext4/parameters/mballoc_debug + using dynamic debug control for mb_debug() msgs. config EXT4_KUNIT_TESTS tristate "KUnit tests for ext4"
fs/ext4/mballoc.c +48 -56
312 lines not on this page · raw message
fs/ext4/mballoc.h +6 -10
@@ -24,19 +24,15 @@#include "ext4.h" /* +* mb_debug() dynamic printk msgs could be used to debug mballoc code. */ #ifdef CONFIG_EXT4_DEBUG -extern ushort ext4_mballoc_debug; - -#define mb_debug (n, fmt, ...) \ -do { \ - if ((n) <= ext4_mballoc_debug) { \ - printk(KERN_DEBUG "(%s, %d): %s: " fmt, \ - __FILE__, __LINE__, __func__, ##__VA_ARGS__); \ - } \ -} while (0) +#define mb_debug (sb, fmt, ...) \ + pr_debug("[%s/%d] EXT4-fs (%s): (%s, %d): %s: " fmt, \ + current->comm, task_pid_nr(current), sb->s_id, \ + __FILE__, __LINE__, __func__, ##__VA_ARGS__)#else -#define mb_debug (n, fmt, ...) no_printk(fmt, ##__VA_ARGS__) +#define mb_debug (sb, fmt, ...) no_printk(fmt, ##__VA_ARGS__)#endif #define EXT4_MB_HISTORY_ALLOC 1/* allocation */
Signature · 1 line
2.21.0
RFC 15/16 ext4: Make ext_debug() implementation to use pr_debug()
In reply to Ritesh Harjaniext_debug() msgs could be helpful, provided those could be enabled without recompiling kernel and also if we could selectively enable only required prints for case by case debugging.
So make ext_debug() implementation use pr_debug(). Also change ext_debug() to be defined with CONFIG_EXT4_DEBUG. So EXT_DEBUG macro now mostly remain for below 3 functions. ext4_ext_show_path/leaf/move() (whose print msgs use ext_debug() which again could be dynamically enabled using pr_debug())
This also changes the ext_debug() to take inode as a parameter to add inode no. in all of it's msgs.
Prints additional info like process name / pid, superblock id etc. This also removes any explicit function names passed in ext_debug(). Since ext_debug() on it's own prints file, func and line no.
--- fs/ext4/Kconfig | 2 +- fs/ext4/ext4.h | 18 ++++-- fs/ext4/extents.c | 144 ++++++++++++++++++++++------------------------ fs/ext4/inode.c | 11 ++-- 4 files changed, 87 insertions(+), 88 deletions(-)
fs/ext4/Kconfig +1 -1
@@ -99,7 +99,7 @@ config EXT4_DEBUG Enables run-time debugging support for the ext4 filesystem. If you select Y here, then you will be able to turn on debugging - using dynamic debug control for mb_debug() msgs. + using dynamic debug control for mb_debug() / ext_debug() msgs. config EXT4_KUNIT_TESTS tristate "KUnit tests for ext4"
fs/ext4/ext4.h +13 -5
@@ -80,14 +80,22 @@#define ext4_debug (fmt, ...) no_printk(fmt, ##__VA_ARGS__)#endif +/* +* Turn on EXT_DEBUG to enable ext4_ext_show_path/leaf/move in extents.c +*/ +#define EXT_DEBUG__ +/* -* Turn on EXT_DEBUG to get lots of info about extents operations. +* Dynamic printk for controlled extents debugging. */ -#define EXT_DEBUG__ -#ifdef EXT_DEBUG -#define ext_debug (fmt, ...) printk(fmt, ##__VA_ARGS__) +#ifdef CONFIG_EXT4_DEBUG +#define ext_debug (ino, fmt, ...) \ + pr_debug("[%s/%d] EXT4-fs (%s): ino %lu: (%s, %d): %s:" fmt, \ + current->comm, task_pid_nr(current), \ + ino->i_sb->s_id, ino->i_ino, __FILE__, __LINE__, \ + __func__, ##__VA_ARGS__)#else -#define ext_debug (fmt, ...) no_printk(fmt, ##__VA_ARGS__) +#define ext_debug (ino, fmt, ...) no_printk(fmt, ##__VA_ARGS__)#endif /* data type for block offset of block group */
fs/ext4/extents.c +69 -75
452 lines not on this page · raw message
fs/ext4/inode.c +4 -7
@@ -493,9 +493,8 @@ int ext4_map_blocks(handle_t *handle, struct inode *inode,#endif map->m_flags =0 ; -ext_debug ("ext4_map_blocks(): inode %lu, flag %d, max_blocks %u," -"logical block %lu\n" , inode->i_ino, flags, map->m_len, - (unsigned long ) map->m_lblk); +ext_debug (inode,"flag 0x%x, max_blocks %u, logical block %lu\n" , + flags, map->m_len, (unsigned long ) map->m_lblk);/* * ext4_map_blocks returns an int, and m_len is an unsigned int
@@ -728,8 +727,7 @@ int ext4_map_blocks(handle_t *handle, struct inode *inode, }if (retval <0 ) -ext_debug ("failed for inode %lu with err %d\n" , - inode->i_ino, retval); +ext_debug (inode,"failed with err %d\n" , retval);return retval; }
@@ -1685,8 +1683,7 @@ static int ext4_da_map_blocks(struct inode *inode, sector_t iblock, invalid_block = ~0 ; map->m_flags =0 ; -ext_debug ("ext4_da_map_blocks(): inode %lu, max_blocks %u," -"logical block %lu\n" , inode->i_ino, map->m_len, +ext_debug (inode,"max_blocks %u, logical block %lu\n" , map->m_len, (unsigned long ) map->m_lblk);/* Lookup extent status tree firstly */
Signature · 1 line
2.21.0
RFC 16/16 ext4: Add process name and pid in ext4_msg()
In reply to Ritesh HarjaniThis adds process name and pid for ext4_msg().
I found this to be useful. For e.g. below print gives more info about process name and pid.
[ 7671.131912] [mount/12543] EXT4-fs (dm-0): mounted filesystem with ordered data mode. Opts: acl,user_xattr
--- fs/ext4/super.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)
fs/ext4/super.c +2 -1
@@ -756,7 +756,8 @@ void __ext4_msg(struct super_block *sb,va_start (args, fmt); vaf.fmt = fmt; vaf.va = &args; -printk ("%sEXT4-fs (%s): %pV\n" , prefix, sb->s_id, &vaf); +printk ("%s [%s/%d] EXT4-fs (%s): %pV\n" , prefix, current->comm, +task_pid_nr (current), sb->s_id, &vaf);va_end (args); }
Signature · 1 line
2.21.0
Theodore Y. Ts'o
maintainerRe: [RFC 16/16] ext4: Add process name and pid in ext4_msg()
Quoted 5 lines · Ritesh Harjani
This adds process name and pid for
ext4_msg().
I found this to be useful. For e.g. below print gives more info about process name and pid.[ 7671.131912] [mount/12543] EXT4-fs (dm-0): mounted filesystem with ordered data mode. Opts: acl,user_xattr
I'm not entirely sure about adding the command/pid at the beginning of the message. The way we do this in ext4_warning and ext4_err is to print that information like this:
printk(KERN_CRIT "EXT4-fs error (device %s): %s:%d: comm %s: %pV\n", sb->s_id, function, line, current->comm, &vaf);
... and I wonder if it would make more sense to add something like to ext4_msg(), just out of consistency's sake. Which of the debugging messages were you finding this to be most helpful?
- Ted
Re: [RFC 16/16] ext4: Add process name and pid in ext4_msg()
Hello Ted,
Sorry about the long delay. Had taken some time off and was tinkering around with another hobby project.
Quoted 11 lines · Theodore Y. Ts'o
Quoted 5 lines · Ritesh Harjani
This adds process name and pid for
ext4_msg().
I found this to be useful. For e.g. below print gives more info about process name and pid.[ 7671.131912] [mount/12543] EXT4-fs (dm-0): mounted filesystem with ordered data mode. Opts: acl,user_xattrI'm not entirely sure about adding the command/pid at the beginning of the message. The way we do this in
ext4_warningandext4_erris to print that information like this:
Quoted 7 lines
printk(KERN_CRIT "EXT4-fs error (device %s): %s:%d: comm %s: %pV\n", sb->s_id, function, line, current->comm, &vaf);... and I wonder if it would make more sense to add something like to
ext4_msg(), just out of consistency's sake. Which of the debugging messages were you finding this to be most helpful?
Well earlier ext4_mb_show_ac() was using ext4_msg() function. But I changed that to use mb_debug() msg in patch-14 of this series, since mb_debug() is meant for those debug msgs.
So I am completely ok if we think this patch is unnecessary, that's also why I kept this patch at the end of the series to check opinion of others.
FWIW, the mballoc issue which I was seeing was mostly due to a multi- threaded application. And without name/pid of the process/threads, it was difficult to identify which debug msgs belonged to which threads. For this reason I thought such info in ext4_msg() would also help in future.
Thanks for taking both patch series!
-ritesh
Theodore Y. Ts'o
maintainerQuoted 6 lines · Ritesh Harjani
Hello All,
This series does some code refactoring/cleanups and debug logs improvements around
mb_debug()andext_debug(). These were found when working over improving mballoc ENOSPC handling in ext4.
These should be small and stright forward patches for reviewing.
I've applied all but the last patch ("Add process namd and pid in ext4_msg"), which I think needs a bit more discussion.
- Ted