f2fs: merge f2fs_show_injection_info() into time_to_inject()

  There is no need to additionally use f2fs_show_injection_info()
to output information. Concatenate time_to_inject() and
__time_to_inject() via a macro. In the new __time_to_inject() function,
pass in the caller function name.

In this way, we no longer need the f2fs_show_injection_info() function,
and let's remove it.

Signed-off-by: Yangtao Li <frank.li@vivo.com>
---
 fs/f2fs/checkpoint.c |  5 +----
 fs/f2fs/data.c       |  8 ++------
 fs/f2fs/dir.c        |  4 +---
 fs/f2fs/f2fs.h       | 43 +++++++++++++------------------------------
 fs/f2fs/file.c       |  4 +---
 fs/f2fs/gc.c         |  4 +---
 fs/f2fs/inode.c      |  4 +---
 fs/f2fs/node.c       |  4 +---
 fs/f2fs/segment.c    |  5 +----
 fs/f2fs/super.c      |  8 ++------
 10 files changed, 24 insertions(+), 65 deletions(-)
  

On 2022/12/15 20:20, Yangtao Li wrote:
> There is no need to additionally use f2fs_show_injection_info()
> to output information. Concatenate time_to_inject() and
> __time_to_inject() via a macro. In the new __time_to_inject() function,
> pass in the caller function name.
> 
> In this way, we no longer need the f2fs_show_injection_info() function,
> and let's remove it.
> 
> Signed-off-by: Yangtao Li <frank.li@vivo.com>
> ---
>   fs/f2fs/checkpoint.c |  5 +----
>   fs/f2fs/data.c       |  8 ++------
>   fs/f2fs/dir.c        |  4 +---
>   fs/f2fs/f2fs.h       | 43 +++++++++++++------------------------------
>   fs/f2fs/file.c       |  4 +---
>   fs/f2fs/gc.c         |  4 +---
>   fs/f2fs/inode.c      |  4 +---
>   fs/f2fs/node.c       |  4 +---
>   fs/f2fs/segment.c    |  5 +----
>   fs/f2fs/super.c      |  8 ++------
>   10 files changed, 24 insertions(+), 65 deletions(-)
> 
> diff --git a/fs/f2fs/checkpoint.c b/fs/f2fs/checkpoint.c
> index 56f7d0d6a8b2..d68b3c991888 100644
> --- a/fs/f2fs/checkpoint.c
> +++ b/fs/f2fs/checkpoint.c
> @@ -171,10 +171,8 @@ static bool __is_bitmap_valid(struct f2fs_sb_info *sbi, block_t blkaddr,
>   bool f2fs_is_valid_blkaddr(struct f2fs_sb_info *sbi,
>   					block_t blkaddr, int type)
>   {
> -	if (time_to_inject(sbi, FAULT_BLKADDR)) {
> -		f2fs_show_injection_info(sbi, FAULT_BLKADDR);
> +	if (time_to_inject(sbi, FAULT_BLKADDR))
>   		return false;
> -	}
>   
>   	switch (type) {
>   	case META_NAT:
> @@ -622,7 +620,6 @@ int f2fs_acquire_orphan_inode(struct f2fs_sb_info *sbi)
>   
>   	if (time_to_inject(sbi, FAULT_ORPHAN)) {
>   		spin_unlock(&im->ino_lock);
> -		f2fs_show_injection_info(sbi, FAULT_ORPHAN);
>   		return -ENOSPC;
>   	}
>   
> diff --git a/fs/f2fs/data.c b/fs/f2fs/data.c
> index c77442a42f89..873908ed20f7 100644
> --- a/fs/f2fs/data.c
> +++ b/fs/f2fs/data.c
> @@ -295,10 +295,8 @@ static void f2fs_read_end_io(struct bio *bio)
>   	iostat_update_and_unbind_ctx(bio, 0);
>   	ctx = bio->bi_private;
>   
> -	if (time_to_inject(sbi, FAULT_READ_IO)) {
> -		f2fs_show_injection_info(sbi, FAULT_READ_IO);
> +	if (time_to_inject(sbi, FAULT_READ_IO))
>   		bio->bi_status = BLK_STS_IOERR;
> -	}
>   
>   	if (bio->bi_status) {
>   		f2fs_finish_read_bio(bio, intask);
> @@ -335,10 +333,8 @@ static void f2fs_write_end_io(struct bio *bio)
>   	iostat_update_and_unbind_ctx(bio, 1);
>   	sbi = bio->bi_private;
>   
> -	if (time_to_inject(sbi, FAULT_WRITE_IO)) {
> -		f2fs_show_injection_info(sbi, FAULT_WRITE_IO);
> +	if (time_to_inject(sbi, FAULT_WRITE_IO))
>   		bio->bi_status = BLK_STS_IOERR;
> -	}
>   
>   	bio_for_each_segment_all(bvec, bio, iter_all) {
>   		struct page *page = bvec->bv_page;
> diff --git a/fs/f2fs/dir.c b/fs/f2fs/dir.c
> index 8e025157f35c..9ccdbe120425 100644
> --- a/fs/f2fs/dir.c
> +++ b/fs/f2fs/dir.c
> @@ -732,10 +732,8 @@ int f2fs_add_regular_entry(struct inode *dir, const struct f2fs_filename *fname,
>   	}
>   
>   start:
> -	if (time_to_inject(F2FS_I_SB(dir), FAULT_DIR_DEPTH)) {
> -		f2fs_show_injection_info(F2FS_I_SB(dir), FAULT_DIR_DEPTH);
> +	if (time_to_inject(F2FS_I_SB(dir), FAULT_DIR_DEPTH))
>   		return -ENOSPC;
> -	}
>   
>   	if (unlikely(current_depth == MAX_DIR_HASH_DEPTH))
>   		return -ENOSPC;
> diff --git a/fs/f2fs/f2fs.h b/fs/f2fs/f2fs.h
> index 4035dab1f570..d01817b11f61 100644
> --- a/fs/f2fs/f2fs.h
> +++ b/fs/f2fs/f2fs.h
> @@ -1875,12 +1875,9 @@ struct f2fs_sb_info {
>   };
>   
>   #ifdef CONFIG_F2FS_FAULT_INJECTION
> -#define f2fs_show_injection_info(sbi, type)					\
> -	printk_ratelimited("%sF2FS-fs (%s) : inject %s in %s of %pS\n",	\
> -		KERN_INFO, sbi->sb->s_id,				\
> -		f2fs_fault_name[type],					\
> -		__func__, __builtin_return_address(0))
> -static inline bool time_to_inject(struct f2fs_sb_info *sbi, int type)
> +#define time_to_inject(sbi, type) __time_to_inject(sbi, type, __func__)
> +static inline bool __time_to_inject(struct f2fs_sb_info *sbi, int type,
> +								    const char *func_name)
>   {
>   	struct f2fs_fault_info *ffi = &F2FS_OPTION(sbi).fault_info;
>   
> @@ -1893,12 +1890,14 @@ static inline bool time_to_inject(struct f2fs_sb_info *sbi, int type)
>   	atomic_inc(&ffi->inject_ops);
>   	if (atomic_read(&ffi->inject_ops) >= ffi->inject_rate) {
>   		atomic_set(&ffi->inject_ops, 0);
> +		printk_ratelimited("%sF2FS-fs (%s) : inject %s in %s of %pS\n",
> +			KERN_INFO, sbi->sb->s_id, f2fs_fault_name[type],
> +			func_name, __builtin_return_address(0));

After moving f2fs_show_injection_info() core functionality into time_to_inject(),
__builtin_return_address(0) result changes from return address of caller of
f2fs_show_injection_info() to return address of time_to_inject().

Thanks,

>   		return true;
>   	}
>   	return false;
>   }
>   #else
> -#define f2fs_show_injection_info(sbi, type) do { } while (0)
>   static inline bool time_to_inject(struct f2fs_sb_info *sbi, int type)
>   {
>   	return false;
> @@ -2231,10 +2230,8 @@ static inline void f2fs_lock_op(struct f2fs_sb_info *sbi)
>   
>   static inline int f2fs_trylock_op(struct f2fs_sb_info *sbi)
>   {
> -	if (time_to_inject(sbi, FAULT_LOCK_OP)) {
> -		f2fs_show_injection_info(sbi, FAULT_LOCK_OP);
> +	if (time_to_inject(sbi, FAULT_LOCK_OP))
>   		return 0;
> -	}
>   	return f2fs_down_read_trylock(&sbi->cp_rwsem);
>   }
>   
> @@ -2322,7 +2319,6 @@ static inline int inc_valid_block_count(struct f2fs_sb_info *sbi,
>   		return ret;
>   
>   	if (time_to_inject(sbi, FAULT_BLOCK)) {
> -		f2fs_show_injection_info(sbi, FAULT_BLOCK);
>   		release = *count;
>   		goto release_quota;
>   	}
> @@ -2602,10 +2598,8 @@ static inline int inc_valid_node_count(struct f2fs_sb_info *sbi,
>   			return err;
>   	}
>   
> -	if (time_to_inject(sbi, FAULT_BLOCK)) {
> -		f2fs_show_injection_info(sbi, FAULT_BLOCK);
> +	if (time_to_inject(sbi, FAULT_BLOCK))
>   		goto enospc;
> -	}
>   
>   	spin_lock(&sbi->stat_lock);
>   
> @@ -2729,11 +2723,8 @@ static inline struct page *f2fs_grab_cache_page(struct address_space *mapping,
>   		if (page)
>   			return page;
>   
> -		if (time_to_inject(F2FS_M_SB(mapping), FAULT_PAGE_ALLOC)) {
> -			f2fs_show_injection_info(F2FS_M_SB(mapping),
> -							FAULT_PAGE_ALLOC);
> +		if (time_to_inject(F2FS_M_SB(mapping), FAULT_PAGE_ALLOC))
>   			return NULL;
> -		}
>   	}
>   
>   	if (!for_write)
> @@ -2750,10 +2741,8 @@ static inline struct page *f2fs_pagecache_get_page(
>   				struct address_space *mapping, pgoff_t index,
>   				int fgp_flags, gfp_t gfp_mask)
>   {
> -	if (time_to_inject(F2FS_M_SB(mapping), FAULT_PAGE_GET)) {
> -		f2fs_show_injection_info(F2FS_M_SB(mapping), FAULT_PAGE_GET);
> +	if (time_to_inject(F2FS_M_SB(mapping), FAULT_PAGE_GET))
>   		return NULL;
> -	}
>   
>   	return pagecache_get_page(mapping, index, fgp_flags, gfp_mask);
>   }
> @@ -2803,10 +2792,8 @@ static inline void *f2fs_kmem_cache_alloc(struct kmem_cache *cachep,
>   	if (nofail)
>   		return f2fs_kmem_cache_alloc_nofail(cachep, flags);
>   
> -	if (time_to_inject(sbi, FAULT_SLAB_ALLOC)) {
> -		f2fs_show_injection_info(sbi, FAULT_SLAB_ALLOC);
> +	if (time_to_inject(sbi, FAULT_SLAB_ALLOC))
>   		return NULL;
> -	}
>   
>   	return kmem_cache_alloc(cachep, flags);
>   }
> @@ -3380,10 +3367,8 @@ static inline bool is_dot_dotdot(const u8 *name, size_t len)
>   static inline void *f2fs_kmalloc(struct f2fs_sb_info *sbi,
>   					size_t size, gfp_t flags)
>   {
> -	if (time_to_inject(sbi, FAULT_KMALLOC)) {
> -		f2fs_show_injection_info(sbi, FAULT_KMALLOC);
> +	if (time_to_inject(sbi, FAULT_KMALLOC))
>   		return NULL;
> -	}
>   
>   	return kmalloc(size, flags);
>   }
> @@ -3397,10 +3382,8 @@ static inline void *f2fs_kzalloc(struct f2fs_sb_info *sbi,
>   static inline void *f2fs_kvmalloc(struct f2fs_sb_info *sbi,
>   					size_t size, gfp_t flags)
>   {
> -	if (time_to_inject(sbi, FAULT_KVMALLOC)) {
> -		f2fs_show_injection_info(sbi, FAULT_KVMALLOC);
> +	if (time_to_inject(sbi, FAULT_KVMALLOC))
>   		return NULL;
> -	}
>   
>   	return kvmalloc(size, flags);
>   }
> diff --git a/fs/f2fs/file.c b/fs/f2fs/file.c
> index cad4bdd6f097..ef25b5b14b10 100644
> --- a/fs/f2fs/file.c
> +++ b/fs/f2fs/file.c
> @@ -782,10 +782,8 @@ int f2fs_truncate(struct inode *inode)
>   
>   	trace_f2fs_truncate(inode);
>   
> -	if (time_to_inject(F2FS_I_SB(inode), FAULT_TRUNCATE)) {
> -		f2fs_show_injection_info(F2FS_I_SB(inode), FAULT_TRUNCATE);
> +	if (time_to_inject(F2FS_I_SB(inode), FAULT_TRUNCATE))
>   		return -EIO;
> -	}
>   
>   	err = f2fs_dquot_initialize(inode);
>   	if (err)
> diff --git a/fs/f2fs/gc.c b/fs/f2fs/gc.c
> index 83e68ec7763d..2b9891efcfee 100644
> --- a/fs/f2fs/gc.c
> +++ b/fs/f2fs/gc.c
> @@ -72,11 +72,9 @@ static int gc_thread_func(void *data)
>   			continue;
>   		}
>   
> -		if (time_to_inject(sbi, FAULT_CHECKPOINT)) {
> -			f2fs_show_injection_info(sbi, FAULT_CHECKPOINT);
> +		if (time_to_inject(sbi, FAULT_CHECKPOINT))
>   			f2fs_stop_checkpoint(sbi, false,
>   					STOP_CP_REASON_FAULT_INJECT);
> -		}
>   
>   		if (!sb_start_write_trylock(sbi->sb)) {
>   			stat_other_skip_bggc_count(sbi);
> diff --git a/fs/f2fs/inode.c b/fs/f2fs/inode.c
> index ff6cf66ed46b..01b9e6f85f6b 100644
> --- a/fs/f2fs/inode.c
> +++ b/fs/f2fs/inode.c
> @@ -809,10 +809,8 @@ void f2fs_evict_inode(struct inode *inode)
>   	if (F2FS_HAS_BLOCKS(inode))
>   		err = f2fs_truncate(inode);
>   
> -	if (time_to_inject(sbi, FAULT_EVICT_INODE)) {
> -		f2fs_show_injection_info(sbi, FAULT_EVICT_INODE);
> +	if (time_to_inject(sbi, FAULT_EVICT_INODE))
>   		err = -EIO;
> -	}
>   
>   	if (!err) {
>   		f2fs_lock_op(sbi);
> diff --git a/fs/f2fs/node.c b/fs/f2fs/node.c
> index dde4c0458704..8e87be0fa85e 100644
> --- a/fs/f2fs/node.c
> +++ b/fs/f2fs/node.c
> @@ -2544,10 +2544,8 @@ bool f2fs_alloc_nid(struct f2fs_sb_info *sbi, nid_t *nid)
>   	struct f2fs_nm_info *nm_i = NM_I(sbi);
>   	struct free_nid *i = NULL;
>   retry:
> -	if (time_to_inject(sbi, FAULT_ALLOC_NID)) {
> -		f2fs_show_injection_info(sbi, FAULT_ALLOC_NID);
> +	if (time_to_inject(sbi, FAULT_ALLOC_NID))
>   		return false;
> -	}
>   
>   	spin_lock(&nm_i->nid_list_lock);
>   
> diff --git a/fs/f2fs/segment.c b/fs/f2fs/segment.c
> index 25ddea478fc1..573955d9aed7 100644
> --- a/fs/f2fs/segment.c
> +++ b/fs/f2fs/segment.c
> @@ -384,10 +384,8 @@ int f2fs_commit_atomic_write(struct inode *inode)
>    */
>   void f2fs_balance_fs(struct f2fs_sb_info *sbi, bool need)
>   {
> -	if (time_to_inject(sbi, FAULT_CHECKPOINT)) {
> -		f2fs_show_injection_info(sbi, FAULT_CHECKPOINT);
> +	if (time_to_inject(sbi, FAULT_CHECKPOINT))
>   		f2fs_stop_checkpoint(sbi, false, STOP_CP_REASON_FAULT_INJECT);
> -	}
>   
>   	/* balance_fs_bg is able to be pending */
>   	if (need && excess_cached_nats(sbi))
> @@ -1142,7 +1140,6 @@ static int __submit_discard_cmd(struct f2fs_sb_info *sbi,
>   		dc->len += len;
>   
>   		if (time_to_inject(sbi, FAULT_DISCARD)) {
> -			f2fs_show_injection_info(sbi, FAULT_DISCARD);
>   			err = -EIO;
>   		} else {
>   			err = __blkdev_issue_discard(bdev,
> diff --git a/fs/f2fs/super.c b/fs/f2fs/super.c
> index 1f812b9ce985..73e779d3b2e7 100644
> --- a/fs/f2fs/super.c
> +++ b/fs/f2fs/super.c
> @@ -1372,10 +1372,8 @@ static struct inode *f2fs_alloc_inode(struct super_block *sb)
>   {
>   	struct f2fs_inode_info *fi;
>   
> -	if (time_to_inject(F2FS_SB(sb), FAULT_SLAB_ALLOC)) {
> -		f2fs_show_injection_info(F2FS_SB(sb), FAULT_SLAB_ALLOC);
> +	if (time_to_inject(F2FS_SB(sb), FAULT_SLAB_ALLOC))
>   		return NULL;
> -	}
>   
>   	fi = alloc_inode_sb(sb, f2fs_inode_cachep, GFP_F2FS_ZERO);
>   	if (!fi)
> @@ -2595,10 +2593,8 @@ static ssize_t f2fs_quota_write(struct super_block *sb, int type,
>   
>   int f2fs_dquot_initialize(struct inode *inode)
>   {
> -	if (time_to_inject(F2FS_I_SB(inode), FAULT_DQUOT_INIT)) {
> -		f2fs_show_injection_info(F2FS_I_SB(inode), FAULT_DQUOT_INIT);
> +	if (time_to_inject(F2FS_I_SB(inode), FAULT_DQUOT_INIT))
>   		return -ESRCH;
> -	}
>   
>   	return dquot_initialize(inode);
>   }
  

> After moving f2fs_show_injection_info() core functionality into time_to_inject(),
> __builtin_return_address(0) result changes from return address of caller of
> f2fs_show_injection_info() to return address of time_to_inject().

I tried the __builtin_return_address(1) parameter just now, and no error
was reported when compiling, but a null pointer problem will be triggered
when the kernel is running.

I thought about it, and the print address didn't seem clear enough.
Let's just print the line number of the caller?

#define time_to_inject(sbi, type) __time_to_inject(sbi, type, __func__, __LINE__)
static inline bool __time_to_inject(struct f2fs_sb_info *sbi, int type,
                                    const char *func_name, unsigned int line)
{
    struct f2fs_fault_info *ffi = &F2FS_OPTION(sbi).fault_info;

    if (!ffi->inject_rate)
        return false;

    if (!IS_FAULT_SET(ffi, type))
        return false;

    atomic_inc(&ffi->inject_ops);
    if (atomic_read(&ffi->inject_ops) >= ffi->inject_rate) {
        atomic_set(&ffi->inject_ops, 0);
        printk_ratelimited("%sF2FS-fs (%s) : inject %s in [%s] %d\n",
            KERN_INFO, sbi->sb->s_id, f2fs_fault_name[type],
            func_name, line);
        return true;
    }
    return false;
}

Thx,
Yangtao
  

On 2022/12/16 11:48, Yangtao Li wrote:
>> After moving f2fs_show_injection_info() core functionality into time_to_inject(),
>> __builtin_return_address(0) result changes from return address of caller of
>> f2fs_show_injection_info() to return address of time_to_inject().
> 
> I tried the __builtin_return_address(1) parameter just now, and no error
> was reported when compiling, but a null pointer problem will be triggered
> when the kernel is running.
> 
> I thought about it, and the print address didn't seem clear enough.
> Let's just print the line number of the caller?

That will cause a regression when searching last injection call paths after
bug occurs, since there are many similar callers of time_to_inject, but the
caller's call paths are different. So, IMO, it's useful to keep
__builtin_return_address in the log to distinguish the real call path of
fault injection.

> 
> #define time_to_inject(sbi, type) __time_to_inject(sbi, type, __func__, __LINE__)

Any way to pass __builtin_return_address(0) from parameter in __time_to_inject(...)?

Thanks,

> static inline bool __time_to_inject(struct f2fs_sb_info *sbi, int type,
>                                      const char *func_name, unsigned int line)
> {
>      struct f2fs_fault_info *ffi = &F2FS_OPTION(sbi).fault_info;
> 
>      if (!ffi->inject_rate)
>          return false;
> 
>      if (!IS_FAULT_SET(ffi, type))
>          return false;
> 
>      atomic_inc(&ffi->inject_ops);
>      if (atomic_read(&ffi->inject_ops) >= ffi->inject_rate) {
>          atomic_set(&ffi->inject_ops, 0);
>          printk_ratelimited("%sF2FS-fs (%s) : inject %s in [%s] %d\n",
>              KERN_INFO, sbi->sb->s_id, f2fs_fault_name[type],
>              func_name, line);
>          return true;
>      }
>      return false;
> }
> 
> Thx,
> Yangtao
  

Hi Chao,

> After moving f2fs_show_injection_info() core functionality into time_to_inject(),
> __builtin_return_address(0) result changes from return address of caller of
> f2fs_show_injection_info() to return address of time_to_inject().

It seems you are wrong, and the original patch didn't change the logic here.
Because time_to_inject is an inline function, __builtin_return_address(0)
has not return address of time_to_inject().

My test:

(1). w/ below patch, we found that the value of __builtin_return_address(0) in
	__time_to_inject() is the same as that in f2fs_show_injection_info().

# mount -t f2fs -o fault_type=0xffff,fault_injection=1 /mnt/9p/f2fs.img /mnt/f2fs
[   19.739661] loop0: detected capacity change from 0 to 2097152
[   19.749759]  new F2FS-fs (loop0) : inject kmalloc in f2fs_kmalloc of f2fs_fill_super+0x7eb/0x1780
[   19.750313] raw F2FS-fs (loop0) : inject kmalloc in f2fs_kmalloc of f2fs_fill_super+0x7eb/0x1780
mount: mounting /dev/loop0 on /mnt/f2fs failed: Cannot allocate memory

diff --git a/fs/f2fs/f2fs.h b/fs/f2fs/f2fs.h
index 4035dab1f570..1e5030633f4d 100644
--- a/fs/f2fs/f2fs.h
+++ b/fs/f2fs/f2fs.h
@@ -1876,11 +1876,13 @@ struct f2fs_sb_info {

 #ifdef CONFIG_F2FS_FAULT_INJECTION
 #define f2fs_show_injection_info(sbi, type)                                    \
-       printk_ratelimited("%sF2FS-fs (%s) : inject %s in %s of %pS\n", \
+       printk_ratelimited("%sraw F2FS-fs (%s) : inject %s in %s of %pS\n",     \
                KERN_INFO, sbi->sb->s_id,                               \
                f2fs_fault_name[type],                                  \
                __func__, __builtin_return_address(0))
-static inline bool time_to_inject(struct f2fs_sb_info *sbi, int type)
+
+#define time_to_inject(sbi, type) __time_to_inject(sbi, type, __func__)
+static inline bool __time_to_inject(struct f2fs_sb_info *sbi, int type, const char *func_name)
 {
        struct f2fs_fault_info *ffi = &F2FS_OPTION(sbi).fault_info;

@@ -1893,6 +1895,10 @@ static inline bool time_to_inject(struct f2fs_sb_info *sbi, int type)
        atomic_inc(&ffi->inject_ops);
        if (atomic_read(&ffi->inject_ops) >= ffi->inject_rate) {
                atomic_set(&ffi->inject_ops, 0);
+               printk_ratelimited("%s new F2FS-fs (%s) : inject %s in %s of %pS\n",
+                       KERN_INFO, sbi->sb->s_id,
+                       f2fs_fault_name[type],
+                       func_name, __builtin_return_address(0));
                return true;
        }
        return false;

(2). w/ below patch(remove inline form __time_to_inject()), we found that the value of
	__builtin_return_address(0) in __time_to_inject() is different from that in
	f2fs_show_injection_info().

# mount -t f2fs -o fault_type=0xffff,fault_injection=1 /mnt/9p/f2fs.img /mnt/f2fs
[   81.019451] loop0: detected capacity change from 0 to 2097152
[   81.031058]  new F2FS-fs (loop0) : inject kmalloc in f2fs_kmalloc of f2fs_init_write_merge_io+0x35/0x1c0
[   81.031745] raw F2FS-fs (loop0) : inject kmalloc in f2fs_kmalloc of f2fs_fill_super+0x7eb/0x1710
mount: mounting /dev/loop0 on /mnt/f2fs failed: Cannot allocate memory

diff --git a/fs/f2fs/f2fs.h b/fs/f2fs/f2fs.h
index 4035dab1f570..f15001b5d73b 100644
--- a/fs/f2fs/f2fs.h
+++ b/fs/f2fs/f2fs.h
@@ -1876,11 +1876,13 @@ struct f2fs_sb_info {

 #ifdef CONFIG_F2FS_FAULT_INJECTION
 #define f2fs_show_injection_info(sbi, type)                                    \
-       printk_ratelimited("%sF2FS-fs (%s) : inject %s in %s of %pS\n", \
+       printk_ratelimited("%sraw F2FS-fs (%s) : inject %s in %s of %pS\n",     \
                KERN_INFO, sbi->sb->s_id,                               \
                f2fs_fault_name[type],                                  \
                __func__, __builtin_return_address(0))
-static inline bool time_to_inject(struct f2fs_sb_info *sbi, int type)
+
+#define time_to_inject(sbi, type) __time_to_inject(sbi, type, __func__)
+static bool __time_to_inject(struct f2fs_sb_info *sbi, int type, const char *func_name)
 {
        struct f2fs_fault_info *ffi = &F2FS_OPTION(sbi).fault_info;

@@ -1893,6 +1895,10 @@ static inline bool time_to_inject(struct f2fs_sb_info *sbi, int type)
        atomic_inc(&ffi->inject_ops);
        if (atomic_read(&ffi->inject_ops) >= ffi->inject_rate) {
                atomic_set(&ffi->inject_ops, 0);
+               printk_ratelimited("%s new F2FS-fs (%s) : inject %s in %s of %pS\n",
+                       KERN_INFO, sbi->sb->s_id,
+                       f2fs_fault_name[type],
+                       func_name, __builtin_return_address(0));
                return true;
        }
        return false;

Thx,
Yangtao
  

On 2022/12/17 3:32, Yangtao Li wrote:
> Hi Chao,
> 
>> After moving f2fs_show_injection_info() core functionality into time_to_inject(),
>> __builtin_return_address(0) result changes from return address of caller of
>> f2fs_show_injection_info() to return address of time_to_inject().
> 
> It seems you are wrong, and the original patch didn't change the logic here.
> Because time_to_inject is an inline function, __builtin_return_address(0)
> has not return address of time_to_inject().

Yeah, but it depends on compile option, right? If user use -O0 or -fno-inline
option, inline function will not be expanded...

How about using deterministic implementation?

Thanks,

> 
> My test:
> 
> (1). w/ below patch, we found that the value of __builtin_return_address(0) in
> 	__time_to_inject() is the same as that in f2fs_show_injection_info().
> 
> # mount -t f2fs -o fault_type=0xffff,fault_injection=1 /mnt/9p/f2fs.img /mnt/f2fs
> [   19.739661] loop0: detected capacity change from 0 to 2097152
> [   19.749759]  new F2FS-fs (loop0) : inject kmalloc in f2fs_kmalloc of f2fs_fill_super+0x7eb/0x1780
> [   19.750313] raw F2FS-fs (loop0) : inject kmalloc in f2fs_kmalloc of f2fs_fill_super+0x7eb/0x1780
> mount: mounting /dev/loop0 on /mnt/f2fs failed: Cannot allocate memory
> 
> diff --git a/fs/f2fs/f2fs.h b/fs/f2fs/f2fs.h
> index 4035dab1f570..1e5030633f4d 100644
> --- a/fs/f2fs/f2fs.h
> +++ b/fs/f2fs/f2fs.h
> @@ -1876,11 +1876,13 @@ struct f2fs_sb_info {
> 
>   #ifdef CONFIG_F2FS_FAULT_INJECTION
>   #define f2fs_show_injection_info(sbi, type)                                    \
> -       printk_ratelimited("%sF2FS-fs (%s) : inject %s in %s of %pS\n", \
> +       printk_ratelimited("%sraw F2FS-fs (%s) : inject %s in %s of %pS\n",     \
>                  KERN_INFO, sbi->sb->s_id,                               \
>                  f2fs_fault_name[type],                                  \
>                  __func__, __builtin_return_address(0))
> -static inline bool time_to_inject(struct f2fs_sb_info *sbi, int type)
> +
> +#define time_to_inject(sbi, type) __time_to_inject(sbi, type, __func__)
> +static inline bool __time_to_inject(struct f2fs_sb_info *sbi, int type, const char *func_name)
>   {
>          struct f2fs_fault_info *ffi = &F2FS_OPTION(sbi).fault_info;
> 
> @@ -1893,6 +1895,10 @@ static inline bool time_to_inject(struct f2fs_sb_info *sbi, int type)
>          atomic_inc(&ffi->inject_ops);
>          if (atomic_read(&ffi->inject_ops) >= ffi->inject_rate) {
>                  atomic_set(&ffi->inject_ops, 0);
> +               printk_ratelimited("%s new F2FS-fs (%s) : inject %s in %s of %pS\n",
> +                       KERN_INFO, sbi->sb->s_id,
> +                       f2fs_fault_name[type],
> +                       func_name, __builtin_return_address(0));
>                  return true;
>          }
>          return false;
> 
> (2). w/ below patch(remove inline form __time_to_inject()), we found that the value of
> 	__builtin_return_address(0) in __time_to_inject() is different from that in
> 	f2fs_show_injection_info().
> 
> # mount -t f2fs -o fault_type=0xffff,fault_injection=1 /mnt/9p/f2fs.img /mnt/f2fs
> [   81.019451] loop0: detected capacity change from 0 to 2097152
> [   81.031058]  new F2FS-fs (loop0) : inject kmalloc in f2fs_kmalloc of f2fs_init_write_merge_io+0x35/0x1c0
> [   81.031745] raw F2FS-fs (loop0) : inject kmalloc in f2fs_kmalloc of f2fs_fill_super+0x7eb/0x1710
> mount: mounting /dev/loop0 on /mnt/f2fs failed: Cannot allocate memory
> 
> diff --git a/fs/f2fs/f2fs.h b/fs/f2fs/f2fs.h
> index 4035dab1f570..f15001b5d73b 100644
> --- a/fs/f2fs/f2fs.h
> +++ b/fs/f2fs/f2fs.h
> @@ -1876,11 +1876,13 @@ struct f2fs_sb_info {
> 
>   #ifdef CONFIG_F2FS_FAULT_INJECTION
>   #define f2fs_show_injection_info(sbi, type)                                    \
> -       printk_ratelimited("%sF2FS-fs (%s) : inject %s in %s of %pS\n", \
> +       printk_ratelimited("%sraw F2FS-fs (%s) : inject %s in %s of %pS\n",     \
>                  KERN_INFO, sbi->sb->s_id,                               \
>                  f2fs_fault_name[type],                                  \
>                  __func__, __builtin_return_address(0))
> -static inline bool time_to_inject(struct f2fs_sb_info *sbi, int type)
> +
> +#define time_to_inject(sbi, type) __time_to_inject(sbi, type, __func__)
> +static bool __time_to_inject(struct f2fs_sb_info *sbi, int type, const char *func_name)
>   {
>          struct f2fs_fault_info *ffi = &F2FS_OPTION(sbi).fault_info;
> 
> @@ -1893,6 +1895,10 @@ static inline bool time_to_inject(struct f2fs_sb_info *sbi, int type)
>          atomic_inc(&ffi->inject_ops);
>          if (atomic_read(&ffi->inject_ops) >= ffi->inject_rate) {
>                  atomic_set(&ffi->inject_ops, 0);
> +               printk_ratelimited("%s new F2FS-fs (%s) : inject %s in %s of %pS\n",
> +                       KERN_INFO, sbi->sb->s_id,
> +                       f2fs_fault_name[type],
> +                       func_name, __builtin_return_address(0));
>                  return true;
>          }
>          return false;
> 
> Thx,
> Yangtao
  

Hi Chao,

> Yeah, but it depends on compile option, right? If user use -O0 or -fno-inline option, inline function will not be expanded...

This is not a problem, we just need to mark it as __always_inline.

static __always_inline bool __time_to_inject(struct f2fs_sb_info *sbi, int type,
                                    const char *func_name)

> How about using deterministic implementation?

This way works, or do you have a better suggestion. After marking the function as __always_inline,
there will be no problem you mentioned.

Thx,
Yangtao