linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH] f2fs: convert S_IRUGO to 0444
@ 2021-08-14 10:37 Yangtao Li
  2021-08-14 10:37 ` [PATCH v6] f2fs: introduce /sys/fs/f2fs/<disk>/fsck_stack node Yangtao Li
  2021-08-16  1:45 ` [PATCH] f2fs: convert S_IRUGO to 0444 Chao Yu
  0 siblings, 2 replies; 10+ messages in thread
From: Yangtao Li @ 2021-08-14 10:37 UTC (permalink / raw)
  To: jaegeuk, chao; +Cc: linux-f2fs-devel, linux-kernel, Yangtao Li

To fix:
WARNING: Symbolic permissions 'S_IRUGO' are not preferred. Consider using octal permissions '0444'.

Signed-off-by: Yangtao Li <frank.li@vivo.com>
---
 fs/f2fs/debug.c | 2 +-
 fs/f2fs/sysfs.c | 8 ++++----
 2 files changed, 5 insertions(+), 5 deletions(-)

diff --git a/fs/f2fs/debug.c b/fs/f2fs/debug.c
index 833325038ef3..542c20e9ea0c 100644
--- a/fs/f2fs/debug.c
+++ b/fs/f2fs/debug.c
@@ -611,7 +611,7 @@ void __init f2fs_create_root_stats(void)
 #ifdef CONFIG_DEBUG_FS
 	f2fs_debugfs_root = debugfs_create_dir("f2fs", NULL);
 
-	debugfs_create_file("status", S_IRUGO, f2fs_debugfs_root, NULL,
+	debugfs_create_file("status", 0444, f2fs_debugfs_root, NULL,
 			    &stat_fops);
 #endif
 }
diff --git a/fs/f2fs/sysfs.c b/fs/f2fs/sysfs.c
index 6642246206bd..204de4c2c818 100644
--- a/fs/f2fs/sysfs.c
+++ b/fs/f2fs/sysfs.c
@@ -1213,13 +1213,13 @@ int f2fs_register_sysfs(struct f2fs_sb_info *sbi)
 		sbi->s_proc = proc_mkdir(sb->s_id, f2fs_proc_root);
 
 	if (sbi->s_proc) {
-		proc_create_single_data("segment_info", S_IRUGO, sbi->s_proc,
+		proc_create_single_data("segment_info", 0444, sbi->s_proc,
 				segment_info_seq_show, sb);
-		proc_create_single_data("segment_bits", S_IRUGO, sbi->s_proc,
+		proc_create_single_data("segment_bits", 0444, sbi->s_proc,
 				segment_bits_seq_show, sb);
-		proc_create_single_data("iostat_info", S_IRUGO, sbi->s_proc,
+		proc_create_single_data("iostat_info", 0444, sbi->s_proc,
 				iostat_info_seq_show, sb);
-		proc_create_single_data("victim_bits", S_IRUGO, sbi->s_proc,
+		proc_create_single_data("victim_bits", 0444, sbi->s_proc,
 				victim_bits_seq_show, sb);
 	}
 	return 0;
-- 
2.32.0


^ permalink raw reply related	[flat|nested] 10+ messages in thread

* [PATCH v6] f2fs: introduce /sys/fs/f2fs/<disk>/fsck_stack node
  2021-08-14 10:37 [PATCH] f2fs: convert S_IRUGO to 0444 Yangtao Li
@ 2021-08-14 10:37 ` Yangtao Li
  2021-08-16  2:53   ` Chao Yu
  2021-08-16  1:45 ` [PATCH] f2fs: convert S_IRUGO to 0444 Chao Yu
  1 sibling, 1 reply; 10+ messages in thread
From: Yangtao Li @ 2021-08-14 10:37 UTC (permalink / raw)
  To: jaegeuk, chao; +Cc: linux-f2fs-devel, linux-kernel, Yangtao Li

SBI_NEED_FSCK is an indicator that fsck.f2fs needs to be triggered,
this flag is set in too many places. For some scenes that are not very
reproducible, adding stack information will help locate the problem.

Let's record all fsck stack history, I added F2FS_FSCK_STACK_TRACE
configuration options and sysfs nodes. After opening the configuration
options and enabling the node, it will start recording. The recorded
stack information will not be clear, and we can get information form
kernel log.

Signed-off-by: Yangtao Li <frank.li@vivo.com>
---
 Documentation/ABI/testing/sysfs-fs-f2fs |  7 ++++
 fs/f2fs/Kconfig                         | 10 ++++++
 fs/f2fs/f2fs.h                          | 45 +++++++++++++++++++++++++
 fs/f2fs/sysfs.c                         | 27 +++++++++++++++
 4 files changed, 89 insertions(+)

diff --git a/Documentation/ABI/testing/sysfs-fs-f2fs b/Documentation/ABI/testing/sysfs-fs-f2fs
index ef4b9218ae1e..047c398093cf 100644
--- a/Documentation/ABI/testing/sysfs-fs-f2fs
+++ b/Documentation/ABI/testing/sysfs-fs-f2fs
@@ -493,3 +493,10 @@ Contact:	"Chao Yu" <yuchao0@huawei.com>
 Description:	When ATGC is on, it controls age threshold to bypass GCing young
 		candidates whose age is not beyond the threshold, by default it was
 		initialized as 604800 seconds (equals to 7 days).
+
+What:		/sys/fs/f2fs/<disk>/fsck_stack
+Date:		August 2021
+Contact:	"Yangtao Li" <frank.li@vivo.com>
+Description:	Controls to enable/disable fsck stack trace, you can get stack
+		information from kernel log. Note that the recorded stack information
+		will not be cleared.
diff --git a/fs/f2fs/Kconfig b/fs/f2fs/Kconfig
index 7669de7b49ce..f451e567e4a8 100644
--- a/fs/f2fs/Kconfig
+++ b/fs/f2fs/Kconfig
@@ -135,3 +135,13 @@ config F2FS_FS_LZORLE
 	default y
 	help
 	  Support LZO-RLE compress algorithm, if unsure, say Y.
+
+config F2FS_FSCK_STACK_TRACE
+	bool "F2FS fsck stack information record"
+	depends on F2FS_FS
+	depends on STACKDEPOT
+	default y
+	help
+	 Support printing out fsck stack history. With this, you have to
+	 turn on "fsck_stack" sysfs node. Then you can get information
+	 from kernel log.
diff --git a/fs/f2fs/f2fs.h b/fs/f2fs/f2fs.h
index ee8eb33e2c25..b2d1d1a5a3fc 100644
--- a/fs/f2fs/f2fs.h
+++ b/fs/f2fs/f2fs.h
@@ -24,6 +24,8 @@
 #include <linux/quotaops.h>
 #include <linux/part_stat.h>
 #include <crypto/hash.h>
+#include <linux/stackdepot.h>
+#include <linux/stacktrace.h>
 
 #include <linux/fscrypt.h>
 #include <linux/fsverity.h>
@@ -117,6 +119,8 @@ typedef u32 nid_t;
 
 #define COMPRESS_EXT_NUM		16
 
+#define FSCK_STACK_DEPTH		64
+
 struct f2fs_mount_info {
 	unsigned int opt;
 	int write_io_size_bits;		/* Write IO size bits */
@@ -1748,6 +1752,11 @@ struct f2fs_sb_info {
 	unsigned int compress_watermark;	/* cache page watermark */
 	atomic_t compress_page_hit;		/* cache hit count */
 #endif
+#ifdef CONFIG_F2FS_FSCK_STACK_TRACE
+	depot_stack_handle_t *fsck_stack_history;
+	unsigned int fsck_count;
+	bool fsck_stack;
+#endif
 };
 
 struct f2fs_private_dio {
@@ -1954,6 +1963,38 @@ static inline struct address_space *NODE_MAPPING(struct f2fs_sb_info *sbi)
 	return sbi->node_inode->i_mapping;
 }
 
+#ifdef CONFIG_F2FS_FSCK_STACK_TRACE
+static void fsck_stack_trace(struct f2fs_sb_info *sbi)
+{
+	unsigned long entries[FSCK_STACK_DEPTH];
+	depot_stack_handle_t stack, *new;
+	unsigned int nr_entries;
+	int i;
+
+	if (!sbi->fsck_stack)
+		return;
+
+	nr_entries = stack_trace_save(entries, ARRAY_SIZE(entries), 0);
+	nr_entries = filter_irq_stacks(entries, nr_entries);
+	stack = stack_depot_save(entries, nr_entries, GFP_KERNEL);
+	if (!stack)
+		return;
+
+	/* Try to find an existing entry for this backtrace */
+	for (i = 0; i < sbi->fsck_count; i++)
+		if (sbi->fsck_stack_history[i] == stack)
+			return;
+
+	new = krealloc(sbi->fsck_stack_history, (sbi->fsck_count + 1) *
+		       sizeof(*sbi->fsck_stack_history), GFP_KERNEL);
+	if (!new)
+		return;
+
+	sbi->fsck_stack_history = new;
+	sbi->fsck_stack_history[sbi->fsck_count++] = stack;
+}
+#endif
+
 static inline bool is_sbi_flag_set(struct f2fs_sb_info *sbi, unsigned int type)
 {
 	return test_bit(type, &sbi->s_flag);
@@ -1962,6 +2003,10 @@ static inline bool is_sbi_flag_set(struct f2fs_sb_info *sbi, unsigned int type)
 static inline void set_sbi_flag(struct f2fs_sb_info *sbi, unsigned int type)
 {
 	set_bit(type, &sbi->s_flag);
+#ifdef CONFIG_F2FS_FSCK_STACK_TRACE
+	if (unlikely(type == SBI_NEED_FSCK))
+		fsck_stack_trace(sbi);
+#endif
 }
 
 static inline void clear_sbi_flag(struct f2fs_sb_info *sbi, unsigned int type)
diff --git a/fs/f2fs/sysfs.c b/fs/f2fs/sysfs.c
index 204de4c2c818..4e786bb797e7 100644
--- a/fs/f2fs/sysfs.c
+++ b/fs/f2fs/sysfs.c
@@ -306,6 +306,26 @@ static ssize_t f2fs_sbi_show(struct f2fs_attr *a,
 	if (!strcmp(a->attr.name, "compr_new_inode"))
 		return sysfs_emit(buf, "%u\n", sbi->compr_new_inode);
 #endif
+#ifdef CONFIG_F2FS_FSCK_STACK_TRACE
+	if (!strcmp(a->attr.name, "fsck_stack")) {
+		unsigned long *entries;
+		unsigned int nr_entries;
+		unsigned int i;
+		int count;
+
+		count = sysfs_emit(buf, "%u\n", sbi->fsck_stack);
+		if (!sbi->fsck_stack)
+			return count;
+
+		for (i = 0; i < sbi->fsck_count; i++) {
+			nr_entries = stack_depot_fetch(sbi->fsck_stack_history[i], &entries);
+			if (!entries)
+				return count;
+			stack_trace_print(entries, nr_entries, 0);
+		}
+		return count;
+	}
+#endif
 
 	ui = (unsigned int *)(ptr + a->offset);
 
@@ -740,6 +760,10 @@ F2FS_RW_ATTR(ATGC_INFO, atgc_management, atgc_candidate_count, max_candidate_cou
 F2FS_RW_ATTR(ATGC_INFO, atgc_management, atgc_age_weight, age_weight);
 F2FS_RW_ATTR(ATGC_INFO, atgc_management, atgc_age_threshold, age_threshold);
 
+#ifdef CONFIG_F2FS_FSCK_STACK_TRACE
+F2FS_RW_ATTR(F2FS_SBI, f2fs_sb_info, fsck_stack, fsck_stack);
+#endif
+
 #define ATTR_LIST(name) (&f2fs_attr_##name.attr)
 static struct attribute *f2fs_attrs[] = {
 	ATTR_LIST(gc_urgent_sleep_time),
@@ -812,6 +836,9 @@ static struct attribute *f2fs_attrs[] = {
 	ATTR_LIST(atgc_candidate_count),
 	ATTR_LIST(atgc_age_weight),
 	ATTR_LIST(atgc_age_threshold),
+#ifdef CONFIG_F2FS_FSCK_STACK_TRACE
+	ATTR_LIST(fsck_stack),
+#endif
 	NULL,
 };
 ATTRIBUTE_GROUPS(f2fs);
-- 
2.32.0


^ permalink raw reply related	[flat|nested] 10+ messages in thread

* Re: [PATCH] f2fs: convert S_IRUGO to 0444
  2021-08-14 10:37 [PATCH] f2fs: convert S_IRUGO to 0444 Yangtao Li
  2021-08-14 10:37 ` [PATCH v6] f2fs: introduce /sys/fs/f2fs/<disk>/fsck_stack node Yangtao Li
@ 2021-08-16  1:45 ` Chao Yu
  1 sibling, 0 replies; 10+ messages in thread
From: Chao Yu @ 2021-08-16  1:45 UTC (permalink / raw)
  To: Yangtao Li, jaegeuk; +Cc: linux-f2fs-devel, linux-kernel

On 2021/8/14 18:37, Yangtao Li wrote:
> To fix:
> WARNING: Symbolic permissions 'S_IRUGO' are not preferred. Consider using octal permissions '0444'.
> 
> Signed-off-by: Yangtao Li <frank.li@vivo.com>

Reviewed-by: Chao Yu <chao@kernel.org>

Thanks,

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH v6] f2fs: introduce /sys/fs/f2fs/<disk>/fsck_stack node
  2021-08-14 10:37 ` [PATCH v6] f2fs: introduce /sys/fs/f2fs/<disk>/fsck_stack node Yangtao Li
@ 2021-08-16  2:53   ` Chao Yu
  2021-08-16  4:02     ` 李扬韬
  0 siblings, 1 reply; 10+ messages in thread
From: Chao Yu @ 2021-08-16  2:53 UTC (permalink / raw)
  To: Yangtao Li, jaegeuk; +Cc: linux-f2fs-devel, linux-kernel

On 2021/8/14 18:37, Yangtao Li wrote:
> SBI_NEED_FSCK is an indicator that fsck.f2fs needs to be triggered,
> this flag is set in too many places. For some scenes that are not very
> reproducible, adding stack information will help locate the problem.
> 
> Let's record all fsck stack history, I added F2FS_FSCK_STACK_TRACE
> configuration options and sysfs nodes. After opening the configuration
> options and enabling the node, it will start recording. The recorded
> stack information will not be clear, and we can get information form
> kernel log.
> 
> Signed-off-by: Yangtao Li <frank.li@vivo.com>
> ---
>   Documentation/ABI/testing/sysfs-fs-f2fs |  7 ++++
>   fs/f2fs/Kconfig                         | 10 ++++++
>   fs/f2fs/f2fs.h                          | 45 +++++++++++++++++++++++++
>   fs/f2fs/sysfs.c                         | 27 +++++++++++++++
>   4 files changed, 89 insertions(+)
> 
> diff --git a/Documentation/ABI/testing/sysfs-fs-f2fs b/Documentation/ABI/testing/sysfs-fs-f2fs
> index ef4b9218ae1e..047c398093cf 100644
> --- a/Documentation/ABI/testing/sysfs-fs-f2fs
> +++ b/Documentation/ABI/testing/sysfs-fs-f2fs
> @@ -493,3 +493,10 @@ Contact:	"Chao Yu" <yuchao0@huawei.com>
>   Description:	When ATGC is on, it controls age threshold to bypass GCing young
>   		candidates whose age is not beyond the threshold, by default it was
>   		initialized as 604800 seconds (equals to 7 days).
> +
> +What:		/sys/fs/f2fs/<disk>/fsck_stack
> +Date:		August 2021
> +Contact:	"Yangtao Li" <frank.li@vivo.com>
> +Description:	Controls to enable/disable fsck stack trace, you can get stack
> +		information from kernel log. Note that the recorded stack information
> +		will not be cleared.

Again, please don't add this into sysfs.

> diff --git a/fs/f2fs/Kconfig b/fs/f2fs/Kconfig
> index 7669de7b49ce..f451e567e4a8 100644
> --- a/fs/f2fs/Kconfig
> +++ b/fs/f2fs/Kconfig
> @@ -135,3 +135,13 @@ config F2FS_FS_LZORLE
>   	default y
>   	help
>   	  Support LZO-RLE compress algorithm, if unsure, say Y.
> +
> +config F2FS_FSCK_STACK_TRACE

I don't think we need another config to wrap this functionality, may be we
can use F2FS_CHECK_FS instead.

> +	bool "F2FS fsck stack information record"
> +	depends on F2FS_FS
> +	depends on STACKDEPOT
> +	default y
> +	help
> +	 Support printing out fsck stack history. With this, you have to
> +	 turn on "fsck_stack" sysfs node. Then you can get information
> +	 from kernel log.
> diff --git a/fs/f2fs/f2fs.h b/fs/f2fs/f2fs.h
> index ee8eb33e2c25..b2d1d1a5a3fc 100644
> --- a/fs/f2fs/f2fs.h
> +++ b/fs/f2fs/f2fs.h
> @@ -24,6 +24,8 @@
>   #include <linux/quotaops.h>
>   #include <linux/part_stat.h>
>   #include <crypto/hash.h>
> +#include <linux/stackdepot.h>
> +#include <linux/stacktrace.h>
>   
>   #include <linux/fscrypt.h>
>   #include <linux/fsverity.h>
> @@ -117,6 +119,8 @@ typedef u32 nid_t;
>   
>   #define COMPRESS_EXT_NUM		16
>   
> +#define FSCK_STACK_DEPTH		64

16?

> +
>   struct f2fs_mount_info {
>   	unsigned int opt;
>   	int write_io_size_bits;		/* Write IO size bits */
> @@ -1748,6 +1752,11 @@ struct f2fs_sb_info {
>   	unsigned int compress_watermark;	/* cache page watermark */
>   	atomic_t compress_page_hit;		/* cache hit count */
>   #endif
> +#ifdef CONFIG_F2FS_FSCK_STACK_TRACE
> +	depot_stack_handle_t *fsck_stack_history;
> +	unsigned int fsck_count;
> +	bool fsck_stack;

IMO, all bug_on()s are corner cases, and catching those stacks won't cost
much, so we can just use CONFIG_XXX to enable/disable this feature.

> +#endif
>   };
>   
>   struct f2fs_private_dio {
> @@ -1954,6 +1963,38 @@ static inline struct address_space *NODE_MAPPING(struct f2fs_sb_info *sbi)
>   	return sbi->node_inode->i_mapping;
>   }
>   
> +#ifdef CONFIG_F2FS_FSCK_STACK_TRACE
> +static void fsck_stack_trace(struct f2fs_sb_info *sbi)
> +{
> +	unsigned long entries[FSCK_STACK_DEPTH];
> +	depot_stack_handle_t stack, *new;
> +	unsigned int nr_entries;
> +	int i;
> +
> +	if (!sbi->fsck_stack)
> +		return;
> +
> +	nr_entries = stack_trace_save(entries, ARRAY_SIZE(entries), 0);
> +	nr_entries = filter_irq_stacks(entries, nr_entries);
> +	stack = stack_depot_save(entries, nr_entries, GFP_KERNEL);
> +	if (!stack)
> +		return;
> +
> +	/* Try to find an existing entry for this backtrace */
> +	for (i = 0; i < sbi->fsck_count; i++)
> +		if (sbi->fsck_stack_history[i] == stack)
> +			return;
> +
> +	new = krealloc(sbi->fsck_stack_history, (sbi->fsck_count + 1) *
> +		       sizeof(*sbi->fsck_stack_history), GFP_KERNEL);
> +	if (!new)
> +		return;
> +
> +	sbi->fsck_stack_history = new;
> +	sbi->fsck_stack_history[sbi->fsck_count++] = stack;

It will case memory leak after f2fs module exits.

> +}
> +#endif
> +
>   static inline bool is_sbi_flag_set(struct f2fs_sb_info *sbi, unsigned int type)
>   {
>   	return test_bit(type, &sbi->s_flag);
> @@ -1962,6 +2003,10 @@ static inline bool is_sbi_flag_set(struct f2fs_sb_info *sbi, unsigned int type)
>   static inline void set_sbi_flag(struct f2fs_sb_info *sbi, unsigned int type)
>   {
>   	set_bit(type, &sbi->s_flag);
> +#ifdef CONFIG_F2FS_FSCK_STACK_TRACE
> +	if (unlikely(type == SBI_NEED_FSCK))
> +		fsck_stack_trace(sbi);
> +#endif
>   }
>   
>   static inline void clear_sbi_flag(struct f2fs_sb_info *sbi, unsigned int type)
> diff --git a/fs/f2fs/sysfs.c b/fs/f2fs/sysfs.c
> index 204de4c2c818..4e786bb797e7 100644
> --- a/fs/f2fs/sysfs.c
> +++ b/fs/f2fs/sysfs.c
> @@ -306,6 +306,26 @@ static ssize_t f2fs_sbi_show(struct f2fs_attr *a,
>   	if (!strcmp(a->attr.name, "compr_new_inode"))
>   		return sysfs_emit(buf, "%u\n", sbi->compr_new_inode);
>   #endif
> +#ifdef CONFIG_F2FS_FSCK_STACK_TRACE
> +	if (!strcmp(a->attr.name, "fsck_stack")) {
> +		unsigned long *entries;
> +		unsigned int nr_entries;
> +		unsigned int i;
> +		int count;
> +
> +		count = sysfs_emit(buf, "%u\n", sbi->fsck_stack);
> +		if (!sbi->fsck_stack)
> +			return count;
> +
> +		for (i = 0; i < sbi->fsck_count; i++) {
> +			nr_entries = stack_depot_fetch(sbi->fsck_stack_history[i], &entries);
> +			if (!entries)
> +				return count;
> +			stack_trace_print(entries, nr_entries, 0);
> +		}
> +		return count;
> +	}
> +#endif
>   
>   	ui = (unsigned int *)(ptr + a->offset);
>   
> @@ -740,6 +760,10 @@ F2FS_RW_ATTR(ATGC_INFO, atgc_management, atgc_candidate_count, max_candidate_cou
>   F2FS_RW_ATTR(ATGC_INFO, atgc_management, atgc_age_weight, age_weight);
>   F2FS_RW_ATTR(ATGC_INFO, atgc_management, atgc_age_threshold, age_threshold);
>   
> +#ifdef CONFIG_F2FS_FSCK_STACK_TRACE
> +F2FS_RW_ATTR(F2FS_SBI, f2fs_sb_info, fsck_stack, fsck_stack);
> +#endif
> +
>   #define ATTR_LIST(name) (&f2fs_attr_##name.attr)
>   static struct attribute *f2fs_attrs[] = {
>   	ATTR_LIST(gc_urgent_sleep_time),
> @@ -812,6 +836,9 @@ static struct attribute *f2fs_attrs[] = {
>   	ATTR_LIST(atgc_candidate_count),
>   	ATTR_LIST(atgc_age_weight),
>   	ATTR_LIST(atgc_age_threshold),
> +#ifdef CONFIG_F2FS_FSCK_STACK_TRACE
> +	ATTR_LIST(fsck_stack),
> +#endif
>   	NULL,
>   };
>   ATTRIBUTE_GROUPS(f2fs);
> 

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re:Re: [PATCH v6] f2fs: introduce /sys/fs/f2fs/<disk>/fsck_stack node
  2021-08-16  2:53   ` Chao Yu
@ 2021-08-16  4:02     ` 李扬韬
  2021-08-16  6:41       ` Chao Yu
  0 siblings, 1 reply; 10+ messages in thread
From: 李扬韬 @ 2021-08-16  4:02 UTC (permalink / raw)
  To: Chao Yu; +Cc: jaegeuk, linux-f2fs-devel, linux-kernel

HI Chao,
>> SBI_NEED_FSCK is an indicator that fsck.f2fs needs to be triggered,
>> this flag is set in too many places. For some scenes that are not very
>> reproducible, adding stack information will help locate the problem.
>> 
>> Let's record all fsck stack history, I added F2FS_FSCK_STACK_TRACE
>> configuration options and sysfs nodes. After opening the configuration
>> options and enabling the node, it will start recording. The recorded
>> stack information will not be clear, and we can get information form
>> kernel log.
>> 
>> Signed-off-by: Yangtao Li <frank.li@vivo.com>
>> ---
>>   Documentation/ABI/testing/sysfs-fs-f2fs |  7 ++++
>>   fs/f2fs/Kconfig                         | 10 ++++++
>>   fs/f2fs/f2fs.h                          | 45 +++++++++++++++++++++++++
>>   fs/f2fs/sysfs.c                         | 27 +++++++++++++++
>>   4 files changed, 89 insertions(+)
>> 
>> diff --git a/Documentation/ABI/testing/sysfs-fs-f2fs b/Documentation/ABI/testing/sysfs-fs-f2fs
>> index ef4b9218ae1e..047c398093cf 100644
>> --- a/Documentation/ABI/testing/sysfs-fs-f2fs
>> +++ b/Documentation/ABI/testing/sysfs-fs-f2fs
>> @@ -493,3 +493,10 @@ Contact:	"Chao Yu" <yuchao0@huawei.com>
>>   Description:	When ATGC is on, it controls age threshold to bypass GCing young
>>   		candidates whose age is not beyond the threshold, by default it was
>>   		initialized as 604800 seconds (equals to 7 days).
>> +
>> +What:		/sys/fs/f2fs/<disk>/fsck_stack
>> +Date:		August 2021
>> +Contact:	"Yangtao Li" <frank.li@vivo.com>
>> +Description:	Controls to enable/disable fsck stack trace, you can get stack
>> +		information from kernel log. Note that the recorded stack information
>> +		will not be cleared.
>
>Again, please don't add this into sysfs.

I added this node, part of the idea is to trigger the export of stack information.
There is no information transmitted through sysfs here, but the record of the stack is switched on and off.
If don't export this information through procfs and sysfs, is there a more appropriate way?

>
>> diff --git a/fs/f2fs/Kconfig b/fs/f2fs/Kconfig
>> index 7669de7b49ce..f451e567e4a8 100644
>> --- a/fs/f2fs/Kconfig
>> +++ b/fs/f2fs/Kconfig
>> @@ -135,3 +135,13 @@ config F2FS_FS_LZORLE
>>   	default y
>>   	help
>>   	  Support LZO-RLE compress algorithm, if unsure, say Y.
>> +
>> +config F2FS_FSCK_STACK_TRACE
>
>I don't think we need another config to wrap this functionality, may be we
>can use F2FS_CHECK_FS instead.

OK.

>
>> +	bool "F2FS fsck stack information record"
>> +	depends on F2FS_FS
>> +	depends on STACKDEPOT
>> +	default y
>> +	help
>> +	 Support printing out fsck stack history. With this, you have to
>> +	 turn on "fsck_stack" sysfs node. Then you can get information
>> +	 from kernel log.
>> diff --git a/fs/f2fs/f2fs.h b/fs/f2fs/f2fs.h
>> index ee8eb33e2c25..b2d1d1a5a3fc 100644
>> --- a/fs/f2fs/f2fs.h
>> +++ b/fs/f2fs/f2fs.h
>> @@ -24,6 +24,8 @@
>>   #include <linux/quotaops.h>
>>   #include <linux/part_stat.h>
>>   #include <crypto/hash.h>
>> +#include <linux/stackdepot.h>
>> +#include <linux/stacktrace.h>
>>   
>>   #include <linux/fscrypt.h>
>>   #include <linux/fsverity.h>
>> @@ -117,6 +119,8 @@ typedef u32 nid_t;
>>   
>>   #define COMPRESS_EXT_NUM		16
>>   
>> +#define FSCK_STACK_DEPTH		64
>
>16?

OK.

>
>> +
>>   struct f2fs_mount_info {
>>   	unsigned int opt;
>>   	int write_io_size_bits;		/* Write IO size bits */
>> @@ -1748,6 +1752,11 @@ struct f2fs_sb_info {
>>   	unsigned int compress_watermark;	/* cache page watermark */
>>   	atomic_t compress_page_hit;		/* cache hit count */
>>   #endif
>> +#ifdef CONFIG_F2FS_FSCK_STACK_TRACE
>> +	depot_stack_handle_t *fsck_stack_history;
>> +	unsigned int fsck_count;
>> +	bool fsck_stack;
>
>IMO, all bug_on()s are corner cases, and catching those stacks won't cost
>much, so we can just use CONFIG_XXX to enable/disable this feature.

F2FS_CHECK_FS ?

>
>> +#endif
>>   };
>>   
>>   struct f2fs_private_dio {
>> @@ -1954,6 +1963,38 @@ static inline struct address_space *NODE_MAPPING(struct f2fs_sb_info *sbi)
>>   	return sbi->node_inode->i_mapping;
>>   }
>>   
>> +#ifdef CONFIG_F2FS_FSCK_STACK_TRACE
>> +static void fsck_stack_trace(struct f2fs_sb_info *sbi)
>> +{
>> +	unsigned long entries[FSCK_STACK_DEPTH];
>> +	depot_stack_handle_t stack, *new;
>> +	unsigned int nr_entries;
>> +	int i;
>> +
>> +	if (!sbi->fsck_stack)
>> +		return;
>> +
>> +	nr_entries = stack_trace_save(entries, ARRAY_SIZE(entries), 0);
>> +	nr_entries = filter_irq_stacks(entries, nr_entries);
>> +	stack = stack_depot_save(entries, nr_entries, GFP_KERNEL);
>> +	if (!stack)
>> +		return;
>> +
>> +	/* Try to find an existing entry for this backtrace */
>> +	for (i = 0; i < sbi->fsck_count; i++)
>> +		if (sbi->fsck_stack_history[i] == stack)
>> +			return;
>> +
>> +	new = krealloc(sbi->fsck_stack_history, (sbi->fsck_count + 1) *
>> +		       sizeof(*sbi->fsck_stack_history), GFP_KERNEL);
>> +	if (!new)
>> +		return;
>> +
>> +	sbi->fsck_stack_history = new;
>> +	sbi->fsck_stack_history[sbi->fsck_count++] = stack;
>
>It will case memory leak after f2fs module exits.

So let's enable this feature when f2fs is not a module and enable F2FS_CHECK_FS.

>
>> +}
>> +#endif
>> +
>>   static inline bool is_sbi_flag_set(struct f2fs_sb_info *sbi, unsigned int type)
>>   {
>>   	return test_bit(type, &sbi->s_flag);
>> @@ -1962,6 +2003,10 @@ static inline bool is_sbi_flag_set(struct f2fs_sb_info *sbi, unsigned int type)
>>   static inline void set_sbi_flag(struct f2fs_sb_info *sbi, unsigned int type)
>>   {
>>   	set_bit(type, &sbi->s_flag);
>> +#ifdef CONFIG_F2FS_FSCK_STACK_TRACE
>> +	if (unlikely(type == SBI_NEED_FSCK))
>> +		fsck_stack_trace(sbi);
>> +#endif
>>   }
>>   
>>   static inline void clear_sbi_flag(struct f2fs_sb_info *sbi, unsigned int type)
>> diff --git a/fs/f2fs/sysfs.c b/fs/f2fs/sysfs.c
>> index 204de4c2c818..4e786bb797e7 100644
>> --- a/fs/f2fs/sysfs.c
>> +++ b/fs/f2fs/sysfs.c
>> @@ -306,6 +306,26 @@ static ssize_t f2fs_sbi_show(struct f2fs_attr *a,
>>   	if (!strcmp(a->attr.name, "compr_new_inode"))
>>   		return sysfs_emit(buf, "%u\n", sbi->compr_new_inode);
>>   #endif
>> +#ifdef CONFIG_F2FS_FSCK_STACK_TRACE
>> +	if (!strcmp(a->attr.name, "fsck_stack")) {
>> +		unsigned long *entries;
>> +		unsigned int nr_entries;
>> +		unsigned int i;
>> +		int count;
>> +
>> +		count = sysfs_emit(buf, "%u\n", sbi->fsck_stack);
>> +		if (!sbi->fsck_stack)
>> +			return count;
>> +
>> +		for (i = 0; i < sbi->fsck_count; i++) {
>> +			nr_entries = stack_depot_fetch(sbi->fsck_stack_history[i], &entries);
>> +			if (!entries)
>> +				return count;
>> +			stack_trace_print(entries, nr_entries, 0);
>> +		}
>> +		return count;
>> +	}
>> +#endif
>>   
>>   	ui = (unsigned int *)(ptr + a->offset);
>>   
>> @@ -740,6 +760,10 @@ F2FS_RW_ATTR(ATGC_INFO, atgc_management, atgc_candidate_count, max_candidate_cou
>>   F2FS_RW_ATTR(ATGC_INFO, atgc_management, atgc_age_weight, age_weight);
>>   F2FS_RW_ATTR(ATGC_INFO, atgc_management, atgc_age_threshold, age_threshold);
>>   
>> +#ifdef CONFIG_F2FS_FSCK_STACK_TRACE
>> +F2FS_RW_ATTR(F2FS_SBI, f2fs_sb_info, fsck_stack, fsck_stack);
>> +#endif
>> +
>>   #define ATTR_LIST(name) (&f2fs_attr_##name.attr)
>>   static struct attribute *f2fs_attrs[] = {
>>   	ATTR_LIST(gc_urgent_sleep_time),
>> @@ -812,6 +836,9 @@ static struct attribute *f2fs_attrs[] = {
>>   	ATTR_LIST(atgc_candidate_count),
>>   	ATTR_LIST(atgc_age_weight),
>>   	ATTR_LIST(atgc_age_threshold),
>> +#ifdef CONFIG_F2FS_FSCK_STACK_TRACE
>> +	ATTR_LIST(fsck_stack),
>> +#endif
>>   	NULL,
>>   };
>>   ATTRIBUTE_GROUPS(f2fs);
>> 

Thx,
Yangtao


^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH v6] f2fs: introduce /sys/fs/f2fs/<disk>/fsck_stack node
  2021-08-16  4:02     ` 李扬韬
@ 2021-08-16  6:41       ` Chao Yu
  2021-08-16  6:53         ` 李扬韬
  2021-08-17  0:27         ` Jaegeuk Kim
  0 siblings, 2 replies; 10+ messages in thread
From: Chao Yu @ 2021-08-16  6:41 UTC (permalink / raw)
  To: 李扬韬, jaegeuk; +Cc: linux-f2fs-devel, linux-kernel

On 2021/8/16 12:02, 李扬韬 wrote:
> HI Chao,
>>> SBI_NEED_FSCK is an indicator that fsck.f2fs needs to be triggered,
>>> this flag is set in too many places. For some scenes that are not very
>>> reproducible, adding stack information will help locate the problem.
>>>
>>> Let's record all fsck stack history, I added F2FS_FSCK_STACK_TRACE
>>> configuration options and sysfs nodes. After opening the configuration
>>> options and enabling the node, it will start recording. The recorded
>>> stack information will not be clear, and we can get information form
>>> kernel log.
>>>
>>> Signed-off-by: Yangtao Li <frank.li@vivo.com>
>>> ---
>>>    Documentation/ABI/testing/sysfs-fs-f2fs |  7 ++++
>>>    fs/f2fs/Kconfig                         | 10 ++++++
>>>    fs/f2fs/f2fs.h                          | 45 +++++++++++++++++++++++++
>>>    fs/f2fs/sysfs.c                         | 27 +++++++++++++++
>>>    4 files changed, 89 insertions(+)
>>>
>>> diff --git a/Documentation/ABI/testing/sysfs-fs-f2fs b/Documentation/ABI/testing/sysfs-fs-f2fs
>>> index ef4b9218ae1e..047c398093cf 100644
>>> --- a/Documentation/ABI/testing/sysfs-fs-f2fs
>>> +++ b/Documentation/ABI/testing/sysfs-fs-f2fs
>>> @@ -493,3 +493,10 @@ Contact:	"Chao Yu" <yuchao0@huawei.com>
>>>    Description:	When ATGC is on, it controls age threshold to bypass GCing young
>>>    		candidates whose age is not beyond the threshold, by default it was
>>>    		initialized as 604800 seconds (equals to 7 days).
>>> +
>>> +What:		/sys/fs/f2fs/<disk>/fsck_stack
>>> +Date:		August 2021
>>> +Contact:	"Yangtao Li" <frank.li@vivo.com>
>>> +Description:	Controls to enable/disable fsck stack trace, you can get stack
>>> +		information from kernel log. Note that the recorded stack information
>>> +		will not be cleared.
>>
>> Again, please don't add this into sysfs.

Oh, I missed to check the details...

> 
> I added this node, part of the idea is to trigger the export of stack information.
> There is no information transmitted through sysfs here, but the record of the stack is switched on and off.
> If don't export this information through procfs and sysfs, is there a more appropriate way?

Well, I doubt why we should export stack info via proc/sysfs node or
sysfs switch.

Those info will always be needed to troubleshoot issues no matter in
user or eng version of Android, can we just print them directly into
kernel message... what I concern is we may lost the bug scene due to
no one can help to trigger dmesg printing via sysfs.

Jaegeuk, thoughts?

> 
>>
>>> diff --git a/fs/f2fs/Kconfig b/fs/f2fs/Kconfig
>>> index 7669de7b49ce..f451e567e4a8 100644
>>> --- a/fs/f2fs/Kconfig
>>> +++ b/fs/f2fs/Kconfig
>>> @@ -135,3 +135,13 @@ config F2FS_FS_LZORLE
>>>    	default y
>>>    	help
>>>    	  Support LZO-RLE compress algorithm, if unsure, say Y.
>>> +
>>> +config F2FS_FSCK_STACK_TRACE
>>
>> I don't think we need another config to wrap this functionality, may be we
>> can use F2FS_CHECK_FS instead.
> 
> OK.
> 
>>
>>> +	bool "F2FS fsck stack information record"
>>> +	depends on F2FS_FS
>>> +	depends on STACKDEPOT
>>> +	default y
>>> +	help
>>> +	 Support printing out fsck stack history. With this, you have to
>>> +	 turn on "fsck_stack" sysfs node. Then you can get information
>>> +	 from kernel log.
>>> diff --git a/fs/f2fs/f2fs.h b/fs/f2fs/f2fs.h
>>> index ee8eb33e2c25..b2d1d1a5a3fc 100644
>>> --- a/fs/f2fs/f2fs.h
>>> +++ b/fs/f2fs/f2fs.h
>>> @@ -24,6 +24,8 @@
>>>    #include <linux/quotaops.h>
>>>    #include <linux/part_stat.h>
>>>    #include <crypto/hash.h>
>>> +#include <linux/stackdepot.h>
>>> +#include <linux/stacktrace.h>
>>>    
>>>    #include <linux/fscrypt.h>
>>>    #include <linux/fsverity.h>
>>> @@ -117,6 +119,8 @@ typedef u32 nid_t;
>>>    
>>>    #define COMPRESS_EXT_NUM		16
>>>    
>>> +#define FSCK_STACK_DEPTH		64
>>
>> 16?
> 
> OK.
> 
>>
>>> +
>>>    struct f2fs_mount_info {
>>>    	unsigned int opt;
>>>    	int write_io_size_bits;		/* Write IO size bits */
>>> @@ -1748,6 +1752,11 @@ struct f2fs_sb_info {
>>>    	unsigned int compress_watermark;	/* cache page watermark */
>>>    	atomic_t compress_page_hit;		/* cache hit count */
>>>    #endif
>>> +#ifdef CONFIG_F2FS_FSCK_STACK_TRACE
>>> +	depot_stack_handle_t *fsck_stack_history;
>>> +	unsigned int fsck_count;
>>> +	bool fsck_stack;
>>
>> IMO, all bug_on()s are corner cases, and catching those stacks won't cost
>> much, so we can just use CONFIG_XXX to enable/disable this feature.
> 
> F2FS_CHECK_FS ?
> 
>>
>>> +#endif
>>>    };
>>>    
>>>    struct f2fs_private_dio {
>>> @@ -1954,6 +1963,38 @@ static inline struct address_space *NODE_MAPPING(struct f2fs_sb_info *sbi)
>>>    	return sbi->node_inode->i_mapping;
>>>    }
>>>    
>>> +#ifdef CONFIG_F2FS_FSCK_STACK_TRACE
>>> +static void fsck_stack_trace(struct f2fs_sb_info *sbi)
>>> +{
>>> +	unsigned long entries[FSCK_STACK_DEPTH];
>>> +	depot_stack_handle_t stack, *new;
>>> +	unsigned int nr_entries;
>>> +	int i;
>>> +
>>> +	if (!sbi->fsck_stack)
>>> +		return;
>>> +
>>> +	nr_entries = stack_trace_save(entries, ARRAY_SIZE(entries), 0);
>>> +	nr_entries = filter_irq_stacks(entries, nr_entries);
>>> +	stack = stack_depot_save(entries, nr_entries, GFP_KERNEL);
>>> +	if (!stack)
>>> +		return;
>>> +
>>> +	/* Try to find an existing entry for this backtrace */
>>> +	for (i = 0; i < sbi->fsck_count; i++)
>>> +		if (sbi->fsck_stack_history[i] == stack)
>>> +			return;
>>> +
>>> +	new = krealloc(sbi->fsck_stack_history, (sbi->fsck_count + 1) *
>>> +		       sizeof(*sbi->fsck_stack_history), GFP_KERNEL);
>>> +	if (!new)
>>> +		return;
>>> +
>>> +	sbi->fsck_stack_history = new;
>>> +	sbi->fsck_stack_history[sbi->fsck_count++] = stack;
>>
>> It will case memory leak after f2fs module exits.
> 
> So let's enable this feature when f2fs is not a module and enable F2FS_CHECK_FS.

I mean it needs to free .fsck_stack_history during umount().

Thanks,

> 
>>
>>> +}
>>> +#endif
>>> +
>>>    static inline bool is_sbi_flag_set(struct f2fs_sb_info *sbi, unsigned int type)
>>>    {
>>>    	return test_bit(type, &sbi->s_flag);
>>> @@ -1962,6 +2003,10 @@ static inline bool is_sbi_flag_set(struct f2fs_sb_info *sbi, unsigned int type)
>>>    static inline void set_sbi_flag(struct f2fs_sb_info *sbi, unsigned int type)
>>>    {
>>>    	set_bit(type, &sbi->s_flag);
>>> +#ifdef CONFIG_F2FS_FSCK_STACK_TRACE
>>> +	if (unlikely(type == SBI_NEED_FSCK))
>>> +		fsck_stack_trace(sbi);
>>> +#endif
>>>    }
>>>    
>>>    static inline void clear_sbi_flag(struct f2fs_sb_info *sbi, unsigned int type)
>>> diff --git a/fs/f2fs/sysfs.c b/fs/f2fs/sysfs.c
>>> index 204de4c2c818..4e786bb797e7 100644
>>> --- a/fs/f2fs/sysfs.c
>>> +++ b/fs/f2fs/sysfs.c
>>> @@ -306,6 +306,26 @@ static ssize_t f2fs_sbi_show(struct f2fs_attr *a,
>>>    	if (!strcmp(a->attr.name, "compr_new_inode"))
>>>    		return sysfs_emit(buf, "%u\n", sbi->compr_new_inode);
>>>    #endif
>>> +#ifdef CONFIG_F2FS_FSCK_STACK_TRACE
>>> +	if (!strcmp(a->attr.name, "fsck_stack")) {
>>> +		unsigned long *entries;
>>> +		unsigned int nr_entries;
>>> +		unsigned int i;
>>> +		int count;
>>> +
>>> +		count = sysfs_emit(buf, "%u\n", sbi->fsck_stack);
>>> +		if (!sbi->fsck_stack)
>>> +			return count;
>>> +
>>> +		for (i = 0; i < sbi->fsck_count; i++) {
>>> +			nr_entries = stack_depot_fetch(sbi->fsck_stack_history[i], &entries);
>>> +			if (!entries)
>>> +				return count;
>>> +			stack_trace_print(entries, nr_entries, 0);
>>> +		}
>>> +		return count;
>>> +	}
>>> +#endif
>>>    
>>>    	ui = (unsigned int *)(ptr + a->offset);
>>>    
>>> @@ -740,6 +760,10 @@ F2FS_RW_ATTR(ATGC_INFO, atgc_management, atgc_candidate_count, max_candidate_cou
>>>    F2FS_RW_ATTR(ATGC_INFO, atgc_management, atgc_age_weight, age_weight);
>>>    F2FS_RW_ATTR(ATGC_INFO, atgc_management, atgc_age_threshold, age_threshold);
>>>    
>>> +#ifdef CONFIG_F2FS_FSCK_STACK_TRACE
>>> +F2FS_RW_ATTR(F2FS_SBI, f2fs_sb_info, fsck_stack, fsck_stack);
>>> +#endif
>>> +
>>>    #define ATTR_LIST(name) (&f2fs_attr_##name.attr)
>>>    static struct attribute *f2fs_attrs[] = {
>>>    	ATTR_LIST(gc_urgent_sleep_time),
>>> @@ -812,6 +836,9 @@ static struct attribute *f2fs_attrs[] = {
>>>    	ATTR_LIST(atgc_candidate_count),
>>>    	ATTR_LIST(atgc_age_weight),
>>>    	ATTR_LIST(atgc_age_threshold),
>>> +#ifdef CONFIG_F2FS_FSCK_STACK_TRACE
>>> +	ATTR_LIST(fsck_stack),
>>> +#endif
>>>    	NULL,
>>>    };
>>>    ATTRIBUTE_GROUPS(f2fs);
>>>
> 
> Thx,
> Yangtao
> 

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re:Re: [PATCH v6] f2fs: introduce /sys/fs/f2fs/<disk>/fsck_stack node
  2021-08-16  6:41       ` Chao Yu
@ 2021-08-16  6:53         ` 李扬韬
  2021-08-17  0:27         ` Jaegeuk Kim
  1 sibling, 0 replies; 10+ messages in thread
From: 李扬韬 @ 2021-08-16  6:53 UTC (permalink / raw)
  To: Chao Yu; +Cc: jaegeuk, linux-f2fs-devel, linux-kernel


>> HI Chao,
>>>> SBI_NEED_FSCK is an indicator that fsck.f2fs needs to be triggered,
>>>> this flag is set in too many places. For some scenes that are not very
>>>> reproducible, adding stack information will help locate the problem.
>>>>
>>>> Let's record all fsck stack history, I added F2FS_FSCK_STACK_TRACE
>>>> configuration options and sysfs nodes. After opening the configuration
>>>> options and enabling the node, it will start recording. The recorded
>>>> stack information will not be clear, and we can get information form
>>>> kernel log.
>>>>
>>>> Signed-off-by: Yangtao Li <frank.li@vivo.com>
>>>> ---
>>>>    Documentation/ABI/testing/sysfs-fs-f2fs |  7 ++++
>>>>    fs/f2fs/Kconfig                         | 10 ++++++
>>>>    fs/f2fs/f2fs.h                          | 45 +++++++++++++++++++++++++
>>>>    fs/f2fs/sysfs.c                         | 27 +++++++++++++++
>>>>    4 files changed, 89 insertions(+)
>>>>
>>>> diff --git a/Documentation/ABI/testing/sysfs-fs-f2fs b/Documentation/ABI/testing/sysfs-fs-f2fs
>>>> index ef4b9218ae1e..047c398093cf 100644
>>>> --- a/Documentation/ABI/testing/sysfs-fs-f2fs
>>>> +++ b/Documentation/ABI/testing/sysfs-fs-f2fs
>>>> @@ -493,3 +493,10 @@ Contact:	"Chao Yu" <yuchao0@huawei.com>
>>>>    Description:	When ATGC is on, it controls age threshold to bypass GCing young
>>>>    		candidates whose age is not beyond the threshold, by default it was
>>>>    		initialized as 604800 seconds (equals to 7 days).
>>>> +
>>>> +What:		/sys/fs/f2fs/<disk>/fsck_stack
>>>> +Date:		August 2021
>>>> +Contact:	"Yangtao Li" <frank.li@vivo.com>
>>>> +Description:	Controls to enable/disable fsck stack trace, you can get stack
>>>> +		information from kernel log. Note that the recorded stack information
>>>> +		will not be cleared.
>>>
>>> Again, please don't add this into sysfs.
>
>Oh, I missed to check the details...
>
>> 
>> I added this node, part of the idea is to trigger the export of stack information.
>> There is no information transmitted through sysfs here, but the record of the stack is switched on and off.
>> If don't export this information through procfs and sysfs, is there a more appropriate way?
>
>Well, I doubt why we should export stack info via proc/sysfs node or

>sysfs switch.>
>Those info will always be needed to troubleshoot issues no matter in
>user or eng version of Android, can we just print them directly into
>kernel message... what I concern is we may lost the bug scene due to

>no one can help to trigger dmesg printing via sysfs.


We can collect these stacks in user space by monitoring whether the sbi status contains fsck.

In addition, I noticed that there are some places, f2fs_bug_on is set in some places,
and set_sbi_flag(sbi, SBI_NEED_FSCK) is used in a small part. Why not change everything
to f2fs_bug_on? In this way, when CONFIG_F2FS_CHECK_FS is turned on, it will actively
panic, allowing us to discover problems in time. If follow the above approach, it is better
to have a separate F2FS_FSCK_STACK_TRACE configuration?


Thx,>
>Jaegeuk, thoughts?
>
>> 
>>>
>>>> diff --git a/fs/f2fs/Kconfig b/fs/f2fs/Kconfig
>>>> index 7669de7b49ce..f451e567e4a8 100644
>>>> --- a/fs/f2fs/Kconfig
>>>> +++ b/fs/f2fs/Kconfig
>>>> @@ -135,3 +135,13 @@ config F2FS_FS_LZORLE
>>>>    	default y
>>>>    	help
>>>>    	  Support LZO-RLE compress algorithm, if unsure, say Y.
>>>> +
>>>> +config F2FS_FSCK_STACK_TRACE
>>>
>>> I don't think we need another config to wrap this functionality, may be we
>>> can use F2FS_CHECK_FS instead.
>> 
>> OK.
>> 
>>>
>>>> +	bool "F2FS fsck stack information record"
>>>> +	depends on F2FS_FS
>>>> +	depends on STACKDEPOT
>>>> +	default y
>>>> +	help
>>>> +	 Support printing out fsck stack history. With this, you have to
>>>> +	 turn on "fsck_stack" sysfs node. Then you can get information
>>>> +	 from kernel log.
>>>> diff --git a/fs/f2fs/f2fs.h b/fs/f2fs/f2fs.h
>>>> index ee8eb33e2c25..b2d1d1a5a3fc 100644
>>>> --- a/fs/f2fs/f2fs.h
>>>> +++ b/fs/f2fs/f2fs.h
>>>> @@ -24,6 +24,8 @@
>>>>    #include <linux/quotaops.h>
>>>>    #include <linux/part_stat.h>
>>>>    #include <crypto/hash.h>
>>>> +#include <linux/stackdepot.h>
>>>> +#include <linux/stacktrace.h>
>>>>    
>>>>    #include <linux/fscrypt.h>
>>>>    #include <linux/fsverity.h>
>>>> @@ -117,6 +119,8 @@ typedef u32 nid_t;
>>>>    
>>>>    #define COMPRESS_EXT_NUM		16
>>>>    
>>>> +#define FSCK_STACK_DEPTH		64
>>>
>>> 16?
>> 
>> OK.
>> 
>>>
>>>> +
>>>>    struct f2fs_mount_info {
>>>>    	unsigned int opt;
>>>>    	int write_io_size_bits;		/* Write IO size bits */
>>>> @@ -1748,6 +1752,11 @@ struct f2fs_sb_info {
>>>>    	unsigned int compress_watermark;	/* cache page watermark */
>>>>    	atomic_t compress_page_hit;		/* cache hit count */
>>>>    #endif
>>>> +#ifdef CONFIG_F2FS_FSCK_STACK_TRACE
>>>> +	depot_stack_handle_t *fsck_stack_history;
>>>> +	unsigned int fsck_count;
>>>> +	bool fsck_stack;
>>>
>>> IMO, all bug_on()s are corner cases, and catching those stacks won't cost
>>> much, so we can just use CONFIG_XXX to enable/disable this feature.
>> 
>> F2FS_CHECK_FS ?
>> 
>>>
>>>> +#endif
>>>>    };
>>>>    
>>>>    struct f2fs_private_dio {
>>>> @@ -1954,6 +1963,38 @@ static inline struct address_space *NODE_MAPPING(struct f2fs_sb_info *sbi)
>>>>    	return sbi->node_inode->i_mapping;
>>>>    }
>>>>    
>>>> +#ifdef CONFIG_F2FS_FSCK_STACK_TRACE
>>>> +static void fsck_stack_trace(struct f2fs_sb_info *sbi)
>>>> +{
>>>> +	unsigned long entries[FSCK_STACK_DEPTH];
>>>> +	depot_stack_handle_t stack, *new;
>>>> +	unsigned int nr_entries;
>>>> +	int i;
>>>> +
>>>> +	if (!sbi->fsck_stack)
>>>> +		return;
>>>> +
>>>> +	nr_entries = stack_trace_save(entries, ARRAY_SIZE(entries), 0);
>>>> +	nr_entries = filter_irq_stacks(entries, nr_entries);
>>>> +	stack = stack_depot_save(entries, nr_entries, GFP_KERNEL);
>>>> +	if (!stack)
>>>> +		return;
>>>> +
>>>> +	/* Try to find an existing entry for this backtrace */
>>>> +	for (i = 0; i < sbi->fsck_count; i++)
>>>> +		if (sbi->fsck_stack_history[i] == stack)
>>>> +			return;
>>>> +
>>>> +	new = krealloc(sbi->fsck_stack_history, (sbi->fsck_count + 1) *
>>>> +		       sizeof(*sbi->fsck_stack_history), GFP_KERNEL);
>>>> +	if (!new)
>>>> +		return;
>>>> +
>>>> +	sbi->fsck_stack_history = new;
>>>> +	sbi->fsck_stack_history[sbi->fsck_count++] = stack;
>>>
>>> It will case memory leak after f2fs module exits.
>> 
>> So let's enable this feature when f2fs is not a module and enable F2FS_CHECK_FS.
>
>I mean it needs to free .fsck_stack_history during umount().
>
>Thanks,
>
>> 
>>>
>>>> +}
>>>> +#endif
>>>> +
>>>>    static inline bool is_sbi_flag_set(struct f2fs_sb_info *sbi, unsigned int type)
>>>>    {
>>>>    	return test_bit(type, &sbi->s_flag);
>>>> @@ -1962,6 +2003,10 @@ static inline bool is_sbi_flag_set(struct f2fs_sb_info *sbi, unsigned int type)
>>>>    static inline void set_sbi_flag(struct f2fs_sb_info *sbi, unsigned int type)
>>>>    {
>>>>    	set_bit(type, &sbi->s_flag);
>>>> +#ifdef CONFIG_F2FS_FSCK_STACK_TRACE
>>>> +	if (unlikely(type == SBI_NEED_FSCK))
>>>> +		fsck_stack_trace(sbi);
>>>> +#endif
>>>>    }
>>>>    
>>>>    static inline void clear_sbi_flag(struct f2fs_sb_info *sbi, unsigned int type)
>>>> diff --git a/fs/f2fs/sysfs.c b/fs/f2fs/sysfs.c
>>>> index 204de4c2c818..4e786bb797e7 100644
>>>> --- a/fs/f2fs/sysfs.c
>>>> +++ b/fs/f2fs/sysfs.c
>>>> @@ -306,6 +306,26 @@ static ssize_t f2fs_sbi_show(struct f2fs_attr *a,
>>>>    	if (!strcmp(a->attr.name, "compr_new_inode"))
>>>>    		return sysfs_emit(buf, "%u\n", sbi->compr_new_inode);
>>>>    #endif
>>>> +#ifdef CONFIG_F2FS_FSCK_STACK_TRACE
>>>> +	if (!strcmp(a->attr.name, "fsck_stack")) {
>>>> +		unsigned long *entries;
>>>> +		unsigned int nr_entries;
>>>> +		unsigned int i;
>>>> +		int count;
>>>> +
>>>> +		count = sysfs_emit(buf, "%u\n", sbi->fsck_stack);
>>>> +		if (!sbi->fsck_stack)
>>>> +			return count;
>>>> +
>>>> +		for (i = 0; i < sbi->fsck_count; i++) {
>>>> +			nr_entries = stack_depot_fetch(sbi->fsck_stack_history[i], &entries);
>>>> +			if (!entries)
>>>> +				return count;
>>>> +			stack_trace_print(entries, nr_entries, 0);
>>>> +		}
>>>> +		return count;
>>>> +	}
>>>> +#endif
>>>>    
>>>>    	ui = (unsigned int *)(ptr + a->offset);
>>>>    
>>>> @@ -740,6 +760,10 @@ F2FS_RW_ATTR(ATGC_INFO, atgc_management, atgc_candidate_count, max_candidate_cou
>>>>    F2FS_RW_ATTR(ATGC_INFO, atgc_management, atgc_age_weight, age_weight);
>>>>    F2FS_RW_ATTR(ATGC_INFO, atgc_management, atgc_age_threshold, age_threshold);
>>>>    
>>>> +#ifdef CONFIG_F2FS_FSCK_STACK_TRACE
>>>> +F2FS_RW_ATTR(F2FS_SBI, f2fs_sb_info, fsck_stack, fsck_stack);
>>>> +#endif
>>>> +
>>>>    #define ATTR_LIST(name) (&f2fs_attr_##name.attr)
>>>>    static struct attribute *f2fs_attrs[] = {
>>>>    	ATTR_LIST(gc_urgent_sleep_time),
>>>> @@ -812,6 +836,9 @@ static struct attribute *f2fs_attrs[] = {
>>>>    	ATTR_LIST(atgc_candidate_count),
>>>>    	ATTR_LIST(atgc_age_weight),
>>>>    	ATTR_LIST(atgc_age_threshold),
>>>> +#ifdef CONFIG_F2FS_FSCK_STACK_TRACE
>>>> +	ATTR_LIST(fsck_stack),
>>>> +#endif
>>>>    	NULL,
>>>>    };
>>>>    ATTRIBUTE_GROUPS(f2fs);
>>>>
>> 
>> Thx,
>> Yangtao
>> 



^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH v6] f2fs: introduce /sys/fs/f2fs/<disk>/fsck_stack node
  2021-08-16  6:41       ` Chao Yu
  2021-08-16  6:53         ` 李扬韬
@ 2021-08-17  0:27         ` Jaegeuk Kim
  2021-08-20  9:12           ` Chao Yu
  1 sibling, 1 reply; 10+ messages in thread
From: Jaegeuk Kim @ 2021-08-17  0:27 UTC (permalink / raw)
  To: Chao Yu; +Cc: 李扬韬, linux-f2fs-devel, linux-kernel

On 08/16, Chao Yu wrote:
> On 2021/8/16 12:02, 李扬韬 wrote:
> > HI Chao,
> > > > SBI_NEED_FSCK is an indicator that fsck.f2fs needs to be triggered,
> > > > this flag is set in too many places. For some scenes that are not very
> > > > reproducible, adding stack information will help locate the problem.
> > > > 
> > > > Let's record all fsck stack history, I added F2FS_FSCK_STACK_TRACE
> > > > configuration options and sysfs nodes. After opening the configuration
> > > > options and enabling the node, it will start recording. The recorded
> > > > stack information will not be clear, and we can get information form
> > > > kernel log.
> > > > 
> > > > Signed-off-by: Yangtao Li <frank.li@vivo.com>
> > > > ---
> > > >    Documentation/ABI/testing/sysfs-fs-f2fs |  7 ++++
> > > >    fs/f2fs/Kconfig                         | 10 ++++++
> > > >    fs/f2fs/f2fs.h                          | 45 +++++++++++++++++++++++++
> > > >    fs/f2fs/sysfs.c                         | 27 +++++++++++++++
> > > >    4 files changed, 89 insertions(+)
> > > > 
> > > > diff --git a/Documentation/ABI/testing/sysfs-fs-f2fs b/Documentation/ABI/testing/sysfs-fs-f2fs
> > > > index ef4b9218ae1e..047c398093cf 100644
> > > > --- a/Documentation/ABI/testing/sysfs-fs-f2fs
> > > > +++ b/Documentation/ABI/testing/sysfs-fs-f2fs
> > > > @@ -493,3 +493,10 @@ Contact:	"Chao Yu" <yuchao0@huawei.com>
> > > >    Description:	When ATGC is on, it controls age threshold to bypass GCing young
> > > >    		candidates whose age is not beyond the threshold, by default it was
> > > >    		initialized as 604800 seconds (equals to 7 days).
> > > > +
> > > > +What:		/sys/fs/f2fs/<disk>/fsck_stack
> > > > +Date:		August 2021
> > > > +Contact:	"Yangtao Li" <frank.li@vivo.com>
> > > > +Description:	Controls to enable/disable fsck stack trace, you can get stack
> > > > +		information from kernel log. Note that the recorded stack information
> > > > +		will not be cleared.
> > > 
> > > Again, please don't add this into sysfs.
> 
> Oh, I missed to check the details...
> 
> > 
> > I added this node, part of the idea is to trigger the export of stack information.
> > There is no information transmitted through sysfs here, but the record of the stack is switched on and off.
> > If don't export this information through procfs and sysfs, is there a more appropriate way?
> 
> Well, I doubt why we should export stack info via proc/sysfs node or
> sysfs switch.
> 
> Those info will always be needed to troubleshoot issues no matter in
> user or eng version of Android, can we just print them directly into
> kernel message... what I concern is we may lost the bug scene due to
> no one can help to trigger dmesg printing via sysfs.
> 
> Jaegeuk, thoughts?

I thought that it'd be good to have an error history in debufs. For example,
we can have a set of reasons defined as enum and put the error counts per
SBI_NEED_FSCK along with error number like ENOMEM, EIO, and so on. The goal
would be to understand whether we're getting SBI_NEED_FSCK which triggers
fsck in practical.

> 
> > 
> > > 
> > > > diff --git a/fs/f2fs/Kconfig b/fs/f2fs/Kconfig
> > > > index 7669de7b49ce..f451e567e4a8 100644
> > > > --- a/fs/f2fs/Kconfig
> > > > +++ b/fs/f2fs/Kconfig
> > > > @@ -135,3 +135,13 @@ config F2FS_FS_LZORLE
> > > >    	default y
> > > >    	help
> > > >    	  Support LZO-RLE compress algorithm, if unsure, say Y.
> > > > +
> > > > +config F2FS_FSCK_STACK_TRACE
> > > 
> > > I don't think we need another config to wrap this functionality, may be we
> > > can use F2FS_CHECK_FS instead.
> > 
> > OK.
> > 
> > > 
> > > > +	bool "F2FS fsck stack information record"
> > > > +	depends on F2FS_FS
> > > > +	depends on STACKDEPOT
> > > > +	default y
> > > > +	help
> > > > +	 Support printing out fsck stack history. With this, you have to
> > > > +	 turn on "fsck_stack" sysfs node. Then you can get information
> > > > +	 from kernel log.
> > > > diff --git a/fs/f2fs/f2fs.h b/fs/f2fs/f2fs.h
> > > > index ee8eb33e2c25..b2d1d1a5a3fc 100644
> > > > --- a/fs/f2fs/f2fs.h
> > > > +++ b/fs/f2fs/f2fs.h
> > > > @@ -24,6 +24,8 @@
> > > >    #include <linux/quotaops.h>
> > > >    #include <linux/part_stat.h>
> > > >    #include <crypto/hash.h>
> > > > +#include <linux/stackdepot.h>
> > > > +#include <linux/stacktrace.h>
> > > >    #include <linux/fscrypt.h>
> > > >    #include <linux/fsverity.h>
> > > > @@ -117,6 +119,8 @@ typedef u32 nid_t;
> > > >    #define COMPRESS_EXT_NUM		16
> > > > +#define FSCK_STACK_DEPTH		64
> > > 
> > > 16?
> > 
> > OK.
> > 
> > > 
> > > > +
> > > >    struct f2fs_mount_info {
> > > >    	unsigned int opt;
> > > >    	int write_io_size_bits;		/* Write IO size bits */
> > > > @@ -1748,6 +1752,11 @@ struct f2fs_sb_info {
> > > >    	unsigned int compress_watermark;	/* cache page watermark */
> > > >    	atomic_t compress_page_hit;		/* cache hit count */
> > > >    #endif
> > > > +#ifdef CONFIG_F2FS_FSCK_STACK_TRACE
> > > > +	depot_stack_handle_t *fsck_stack_history;
> > > > +	unsigned int fsck_count;
> > > > +	bool fsck_stack;
> > > 
> > > IMO, all bug_on()s are corner cases, and catching those stacks won't cost
> > > much, so we can just use CONFIG_XXX to enable/disable this feature.
> > 
> > F2FS_CHECK_FS ?
> > 
> > > 
> > > > +#endif
> > > >    };
> > > >    struct f2fs_private_dio {
> > > > @@ -1954,6 +1963,38 @@ static inline struct address_space *NODE_MAPPING(struct f2fs_sb_info *sbi)
> > > >    	return sbi->node_inode->i_mapping;
> > > >    }
> > > > +#ifdef CONFIG_F2FS_FSCK_STACK_TRACE
> > > > +static void fsck_stack_trace(struct f2fs_sb_info *sbi)
> > > > +{
> > > > +	unsigned long entries[FSCK_STACK_DEPTH];
> > > > +	depot_stack_handle_t stack, *new;
> > > > +	unsigned int nr_entries;
> > > > +	int i;
> > > > +
> > > > +	if (!sbi->fsck_stack)
> > > > +		return;
> > > > +
> > > > +	nr_entries = stack_trace_save(entries, ARRAY_SIZE(entries), 0);
> > > > +	nr_entries = filter_irq_stacks(entries, nr_entries);
> > > > +	stack = stack_depot_save(entries, nr_entries, GFP_KERNEL);
> > > > +	if (!stack)
> > > > +		return;
> > > > +
> > > > +	/* Try to find an existing entry for this backtrace */
> > > > +	for (i = 0; i < sbi->fsck_count; i++)
> > > > +		if (sbi->fsck_stack_history[i] == stack)
> > > > +			return;
> > > > +
> > > > +	new = krealloc(sbi->fsck_stack_history, (sbi->fsck_count + 1) *
> > > > +		       sizeof(*sbi->fsck_stack_history), GFP_KERNEL);
> > > > +	if (!new)
> > > > +		return;
> > > > +
> > > > +	sbi->fsck_stack_history = new;
> > > > +	sbi->fsck_stack_history[sbi->fsck_count++] = stack;
> > > 
> > > It will case memory leak after f2fs module exits.
> > 
> > So let's enable this feature when f2fs is not a module and enable F2FS_CHECK_FS.
> 
> I mean it needs to free .fsck_stack_history during umount().
> 
> Thanks,
> 
> > 
> > > 
> > > > +}
> > > > +#endif
> > > > +
> > > >    static inline bool is_sbi_flag_set(struct f2fs_sb_info *sbi, unsigned int type)
> > > >    {
> > > >    	return test_bit(type, &sbi->s_flag);
> > > > @@ -1962,6 +2003,10 @@ static inline bool is_sbi_flag_set(struct f2fs_sb_info *sbi, unsigned int type)
> > > >    static inline void set_sbi_flag(struct f2fs_sb_info *sbi, unsigned int type)
> > > >    {
> > > >    	set_bit(type, &sbi->s_flag);
> > > > +#ifdef CONFIG_F2FS_FSCK_STACK_TRACE
> > > > +	if (unlikely(type == SBI_NEED_FSCK))
> > > > +		fsck_stack_trace(sbi);
> > > > +#endif
> > > >    }
> > > >    static inline void clear_sbi_flag(struct f2fs_sb_info *sbi, unsigned int type)
> > > > diff --git a/fs/f2fs/sysfs.c b/fs/f2fs/sysfs.c
> > > > index 204de4c2c818..4e786bb797e7 100644
> > > > --- a/fs/f2fs/sysfs.c
> > > > +++ b/fs/f2fs/sysfs.c
> > > > @@ -306,6 +306,26 @@ static ssize_t f2fs_sbi_show(struct f2fs_attr *a,
> > > >    	if (!strcmp(a->attr.name, "compr_new_inode"))
> > > >    		return sysfs_emit(buf, "%u\n", sbi->compr_new_inode);
> > > >    #endif
> > > > +#ifdef CONFIG_F2FS_FSCK_STACK_TRACE
> > > > +	if (!strcmp(a->attr.name, "fsck_stack")) {
> > > > +		unsigned long *entries;
> > > > +		unsigned int nr_entries;
> > > > +		unsigned int i;
> > > > +		int count;
> > > > +
> > > > +		count = sysfs_emit(buf, "%u\n", sbi->fsck_stack);
> > > > +		if (!sbi->fsck_stack)
> > > > +			return count;
> > > > +
> > > > +		for (i = 0; i < sbi->fsck_count; i++) {
> > > > +			nr_entries = stack_depot_fetch(sbi->fsck_stack_history[i], &entries);
> > > > +			if (!entries)
> > > > +				return count;
> > > > +			stack_trace_print(entries, nr_entries, 0);
> > > > +		}
> > > > +		return count;
> > > > +	}
> > > > +#endif
> > > >    	ui = (unsigned int *)(ptr + a->offset);
> > > > @@ -740,6 +760,10 @@ F2FS_RW_ATTR(ATGC_INFO, atgc_management, atgc_candidate_count, max_candidate_cou
> > > >    F2FS_RW_ATTR(ATGC_INFO, atgc_management, atgc_age_weight, age_weight);
> > > >    F2FS_RW_ATTR(ATGC_INFO, atgc_management, atgc_age_threshold, age_threshold);
> > > > +#ifdef CONFIG_F2FS_FSCK_STACK_TRACE
> > > > +F2FS_RW_ATTR(F2FS_SBI, f2fs_sb_info, fsck_stack, fsck_stack);
> > > > +#endif
> > > > +
> > > >    #define ATTR_LIST(name) (&f2fs_attr_##name.attr)
> > > >    static struct attribute *f2fs_attrs[] = {
> > > >    	ATTR_LIST(gc_urgent_sleep_time),
> > > > @@ -812,6 +836,9 @@ static struct attribute *f2fs_attrs[] = {
> > > >    	ATTR_LIST(atgc_candidate_count),
> > > >    	ATTR_LIST(atgc_age_weight),
> > > >    	ATTR_LIST(atgc_age_threshold),
> > > > +#ifdef CONFIG_F2FS_FSCK_STACK_TRACE
> > > > +	ATTR_LIST(fsck_stack),
> > > > +#endif
> > > >    	NULL,
> > > >    };
> > > >    ATTRIBUTE_GROUPS(f2fs);
> > > > 
> > 
> > Thx,
> > Yangtao
> > 

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH v6] f2fs: introduce /sys/fs/f2fs/<disk>/fsck_stack node
  2021-08-17  0:27         ` Jaegeuk Kim
@ 2021-08-20  9:12           ` Chao Yu
  0 siblings, 0 replies; 10+ messages in thread
From: Chao Yu @ 2021-08-20  9:12 UTC (permalink / raw)
  To: Jaegeuk Kim; +Cc: 李扬韬, linux-f2fs-devel, linux-kernel

On 2021/8/17 8:27, Jaegeuk Kim wrote:
> On 08/16, Chao Yu wrote:
>> On 2021/8/16 12:02, 李扬韬 wrote:
>>> HI Chao,
>>>>> SBI_NEED_FSCK is an indicator that fsck.f2fs needs to be triggered,
>>>>> this flag is set in too many places. For some scenes that are not very
>>>>> reproducible, adding stack information will help locate the problem.
>>>>>
>>>>> Let's record all fsck stack history, I added F2FS_FSCK_STACK_TRACE
>>>>> configuration options and sysfs nodes. After opening the configuration
>>>>> options and enabling the node, it will start recording. The recorded
>>>>> stack information will not be clear, and we can get information form
>>>>> kernel log.
>>>>>
>>>>> Signed-off-by: Yangtao Li <frank.li@vivo.com>
>>>>> ---
>>>>>     Documentation/ABI/testing/sysfs-fs-f2fs |  7 ++++
>>>>>     fs/f2fs/Kconfig                         | 10 ++++++
>>>>>     fs/f2fs/f2fs.h                          | 45 +++++++++++++++++++++++++
>>>>>     fs/f2fs/sysfs.c                         | 27 +++++++++++++++
>>>>>     4 files changed, 89 insertions(+)
>>>>>
>>>>> diff --git a/Documentation/ABI/testing/sysfs-fs-f2fs b/Documentation/ABI/testing/sysfs-fs-f2fs
>>>>> index ef4b9218ae1e..047c398093cf 100644
>>>>> --- a/Documentation/ABI/testing/sysfs-fs-f2fs
>>>>> +++ b/Documentation/ABI/testing/sysfs-fs-f2fs
>>>>> @@ -493,3 +493,10 @@ Contact:	"Chao Yu" <yuchao0@huawei.com>
>>>>>     Description:	When ATGC is on, it controls age threshold to bypass GCing young
>>>>>     		candidates whose age is not beyond the threshold, by default it was
>>>>>     		initialized as 604800 seconds (equals to 7 days).
>>>>> +
>>>>> +What:		/sys/fs/f2fs/<disk>/fsck_stack
>>>>> +Date:		August 2021
>>>>> +Contact:	"Yangtao Li" <frank.li@vivo.com>
>>>>> +Description:	Controls to enable/disable fsck stack trace, you can get stack
>>>>> +		information from kernel log. Note that the recorded stack information
>>>>> +		will not be cleared.
>>>>
>>>> Again, please don't add this into sysfs.
>>
>> Oh, I missed to check the details...
>>
>>>
>>> I added this node, part of the idea is to trigger the export of stack information.
>>> There is no information transmitted through sysfs here, but the record of the stack is switched on and off.
>>> If don't export this information through procfs and sysfs, is there a more appropriate way?
>>
>> Well, I doubt why we should export stack info via proc/sysfs node or
>> sysfs switch.
>>
>> Those info will always be needed to troubleshoot issues no matter in
>> user or eng version of Android, can we just print them directly into
>> kernel message... what I concern is we may lost the bug scene due to
>> no one can help to trigger dmesg printing via sysfs.
>>
>> Jaegeuk, thoughts?
> 
> I thought that it'd be good to have an error history in debufs. For example,
> we can have a set of reasons defined as enum and put the error counts per
> SBI_NEED_FSCK along with error number like ENOMEM, EIO, and so on. The goal
> would be to understand whether we're getting SBI_NEED_FSCK which triggers
> fsck in practical.

In most of the cases we set SBI_NEED_FSCK, the reason is in-mem or on-disk
data becomes inconsistent, so it looks it's not easy to distinguish the
reason based on error number like ENOMEM, EIO... if I get your point correctly
here.

Thanks,

> 
>>
>>>
>>>>
>>>>> diff --git a/fs/f2fs/Kconfig b/fs/f2fs/Kconfig
>>>>> index 7669de7b49ce..f451e567e4a8 100644
>>>>> --- a/fs/f2fs/Kconfig
>>>>> +++ b/fs/f2fs/Kconfig
>>>>> @@ -135,3 +135,13 @@ config F2FS_FS_LZORLE
>>>>>     	default y
>>>>>     	help
>>>>>     	  Support LZO-RLE compress algorithm, if unsure, say Y.
>>>>> +
>>>>> +config F2FS_FSCK_STACK_TRACE
>>>>
>>>> I don't think we need another config to wrap this functionality, may be we
>>>> can use F2FS_CHECK_FS instead.
>>>
>>> OK.
>>>
>>>>
>>>>> +	bool "F2FS fsck stack information record"
>>>>> +	depends on F2FS_FS
>>>>> +	depends on STACKDEPOT
>>>>> +	default y
>>>>> +	help
>>>>> +	 Support printing out fsck stack history. With this, you have to
>>>>> +	 turn on "fsck_stack" sysfs node. Then you can get information
>>>>> +	 from kernel log.
>>>>> diff --git a/fs/f2fs/f2fs.h b/fs/f2fs/f2fs.h
>>>>> index ee8eb33e2c25..b2d1d1a5a3fc 100644
>>>>> --- a/fs/f2fs/f2fs.h
>>>>> +++ b/fs/f2fs/f2fs.h
>>>>> @@ -24,6 +24,8 @@
>>>>>     #include <linux/quotaops.h>
>>>>>     #include <linux/part_stat.h>
>>>>>     #include <crypto/hash.h>
>>>>> +#include <linux/stackdepot.h>
>>>>> +#include <linux/stacktrace.h>
>>>>>     #include <linux/fscrypt.h>
>>>>>     #include <linux/fsverity.h>
>>>>> @@ -117,6 +119,8 @@ typedef u32 nid_t;
>>>>>     #define COMPRESS_EXT_NUM		16
>>>>> +#define FSCK_STACK_DEPTH		64
>>>>
>>>> 16?
>>>
>>> OK.
>>>
>>>>
>>>>> +
>>>>>     struct f2fs_mount_info {
>>>>>     	unsigned int opt;
>>>>>     	int write_io_size_bits;		/* Write IO size bits */
>>>>> @@ -1748,6 +1752,11 @@ struct f2fs_sb_info {
>>>>>     	unsigned int compress_watermark;	/* cache page watermark */
>>>>>     	atomic_t compress_page_hit;		/* cache hit count */
>>>>>     #endif
>>>>> +#ifdef CONFIG_F2FS_FSCK_STACK_TRACE
>>>>> +	depot_stack_handle_t *fsck_stack_history;
>>>>> +	unsigned int fsck_count;
>>>>> +	bool fsck_stack;
>>>>
>>>> IMO, all bug_on()s are corner cases, and catching those stacks won't cost
>>>> much, so we can just use CONFIG_XXX to enable/disable this feature.
>>>
>>> F2FS_CHECK_FS ?
>>>
>>>>
>>>>> +#endif
>>>>>     };
>>>>>     struct f2fs_private_dio {
>>>>> @@ -1954,6 +1963,38 @@ static inline struct address_space *NODE_MAPPING(struct f2fs_sb_info *sbi)
>>>>>     	return sbi->node_inode->i_mapping;
>>>>>     }
>>>>> +#ifdef CONFIG_F2FS_FSCK_STACK_TRACE
>>>>> +static void fsck_stack_trace(struct f2fs_sb_info *sbi)
>>>>> +{
>>>>> +	unsigned long entries[FSCK_STACK_DEPTH];
>>>>> +	depot_stack_handle_t stack, *new;
>>>>> +	unsigned int nr_entries;
>>>>> +	int i;
>>>>> +
>>>>> +	if (!sbi->fsck_stack)
>>>>> +		return;
>>>>> +
>>>>> +	nr_entries = stack_trace_save(entries, ARRAY_SIZE(entries), 0);
>>>>> +	nr_entries = filter_irq_stacks(entries, nr_entries);
>>>>> +	stack = stack_depot_save(entries, nr_entries, GFP_KERNEL);
>>>>> +	if (!stack)
>>>>> +		return;
>>>>> +
>>>>> +	/* Try to find an existing entry for this backtrace */
>>>>> +	for (i = 0; i < sbi->fsck_count; i++)
>>>>> +		if (sbi->fsck_stack_history[i] == stack)
>>>>> +			return;
>>>>> +
>>>>> +	new = krealloc(sbi->fsck_stack_history, (sbi->fsck_count + 1) *
>>>>> +		       sizeof(*sbi->fsck_stack_history), GFP_KERNEL);
>>>>> +	if (!new)
>>>>> +		return;
>>>>> +
>>>>> +	sbi->fsck_stack_history = new;
>>>>> +	sbi->fsck_stack_history[sbi->fsck_count++] = stack;
>>>>
>>>> It will case memory leak after f2fs module exits.
>>>
>>> So let's enable this feature when f2fs is not a module and enable F2FS_CHECK_FS.
>>
>> I mean it needs to free .fsck_stack_history during umount().
>>
>> Thanks,
>>
>>>
>>>>
>>>>> +}
>>>>> +#endif
>>>>> +
>>>>>     static inline bool is_sbi_flag_set(struct f2fs_sb_info *sbi, unsigned int type)
>>>>>     {
>>>>>     	return test_bit(type, &sbi->s_flag);
>>>>> @@ -1962,6 +2003,10 @@ static inline bool is_sbi_flag_set(struct f2fs_sb_info *sbi, unsigned int type)
>>>>>     static inline void set_sbi_flag(struct f2fs_sb_info *sbi, unsigned int type)
>>>>>     {
>>>>>     	set_bit(type, &sbi->s_flag);
>>>>> +#ifdef CONFIG_F2FS_FSCK_STACK_TRACE
>>>>> +	if (unlikely(type == SBI_NEED_FSCK))
>>>>> +		fsck_stack_trace(sbi);
>>>>> +#endif
>>>>>     }
>>>>>     static inline void clear_sbi_flag(struct f2fs_sb_info *sbi, unsigned int type)
>>>>> diff --git a/fs/f2fs/sysfs.c b/fs/f2fs/sysfs.c
>>>>> index 204de4c2c818..4e786bb797e7 100644
>>>>> --- a/fs/f2fs/sysfs.c
>>>>> +++ b/fs/f2fs/sysfs.c
>>>>> @@ -306,6 +306,26 @@ static ssize_t f2fs_sbi_show(struct f2fs_attr *a,
>>>>>     	if (!strcmp(a->attr.name, "compr_new_inode"))
>>>>>     		return sysfs_emit(buf, "%u\n", sbi->compr_new_inode);
>>>>>     #endif
>>>>> +#ifdef CONFIG_F2FS_FSCK_STACK_TRACE
>>>>> +	if (!strcmp(a->attr.name, "fsck_stack")) {
>>>>> +		unsigned long *entries;
>>>>> +		unsigned int nr_entries;
>>>>> +		unsigned int i;
>>>>> +		int count;
>>>>> +
>>>>> +		count = sysfs_emit(buf, "%u\n", sbi->fsck_stack);
>>>>> +		if (!sbi->fsck_stack)
>>>>> +			return count;
>>>>> +
>>>>> +		for (i = 0; i < sbi->fsck_count; i++) {
>>>>> +			nr_entries = stack_depot_fetch(sbi->fsck_stack_history[i], &entries);
>>>>> +			if (!entries)
>>>>> +				return count;
>>>>> +			stack_trace_print(entries, nr_entries, 0);
>>>>> +		}
>>>>> +		return count;
>>>>> +	}
>>>>> +#endif
>>>>>     	ui = (unsigned int *)(ptr + a->offset);
>>>>> @@ -740,6 +760,10 @@ F2FS_RW_ATTR(ATGC_INFO, atgc_management, atgc_candidate_count, max_candidate_cou
>>>>>     F2FS_RW_ATTR(ATGC_INFO, atgc_management, atgc_age_weight, age_weight);
>>>>>     F2FS_RW_ATTR(ATGC_INFO, atgc_management, atgc_age_threshold, age_threshold);
>>>>> +#ifdef CONFIG_F2FS_FSCK_STACK_TRACE
>>>>> +F2FS_RW_ATTR(F2FS_SBI, f2fs_sb_info, fsck_stack, fsck_stack);
>>>>> +#endif
>>>>> +
>>>>>     #define ATTR_LIST(name) (&f2fs_attr_##name.attr)
>>>>>     static struct attribute *f2fs_attrs[] = {
>>>>>     	ATTR_LIST(gc_urgent_sleep_time),
>>>>> @@ -812,6 +836,9 @@ static struct attribute *f2fs_attrs[] = {
>>>>>     	ATTR_LIST(atgc_candidate_count),
>>>>>     	ATTR_LIST(atgc_age_weight),
>>>>>     	ATTR_LIST(atgc_age_threshold),
>>>>> +#ifdef CONFIG_F2FS_FSCK_STACK_TRACE
>>>>> +	ATTR_LIST(fsck_stack),
>>>>> +#endif
>>>>>     	NULL,
>>>>>     };
>>>>>     ATTRIBUTE_GROUPS(f2fs);
>>>>>
>>>
>>> Thx,
>>> Yangtao
>>>

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH v6] f2fs: introduce /sys/fs/f2fs/<disk>/fsck_stack node
       [not found] <AEYAigA8D7ZftrI0MRYeIqoY.3.1629098634558.Hmail.frank.li@vivo.com>
@ 2021-08-19  8:04 ` Chao Yu
  0 siblings, 0 replies; 10+ messages in thread
From: Chao Yu @ 2021-08-19  8:04 UTC (permalink / raw)
  To: 李扬韬; +Cc: jaegeuk, linux-f2fs-devel, linux-kernel

On 2021/8/16 15:23, 李扬韬 wrote:
>>> HI Chao,
>>>>> SBI_NEED_FSCK is an indicator that fsck.f2fs needs to be triggered,
>>>>> this flag is set in too many places. For some scenes that are not very
>>>>> reproducible, adding stack information will help locate the problem.
>>>>>
>>>>> Let's record all fsck stack history, I added F2FS_FSCK_STACK_TRACE
>>>>> configuration options and sysfs nodes. After opening the configuration
>>>>> options and enabling the node, it will start recording. The recorded
>>>>> stack information will not be clear, and we can get information form
>>>>> kernel log.
>>>>>
>>>>> Signed-off-by: Yangtao Li <frank.li@vivo.com>
>>>>> ---
>>>>>    Documentation/ABI/testing/sysfs-fs-f2fs |  7 ++++
>>>>>    fs/f2fs/Kconfig                         | 10 ++++++
>>>>>    fs/f2fs/f2fs.h                          | 45 +++++++++++++++++++++++++
>>>>>    fs/f2fs/sysfs.c                         | 27 +++++++++++++++
>>>>>    4 files changed, 89 insertions(+)
>>>>>
>>>>> diff --git a/Documentation/ABI/testing/sysfs-fs-f2fs b/Documentation/ABI/testing/sysfs-fs-f2fs
>>>>> index ef4b9218ae1e..047c398093cf 100644
>>>>> --- a/Documentation/ABI/testing/sysfs-fs-f2fs
>>>>> +++ b/Documentation/ABI/testing/sysfs-fs-f2fs
>>>>> @@ -493,3 +493,10 @@ Contact:	"Chao Yu" <yuchao0@huawei.com>
>>>>>    Description:	When ATGC is on, it controls age threshold to bypass GCing young
>>>>>    		candidates whose age is not beyond the threshold, by default it was
>>>>>    		initialized as 604800 seconds (equals to 7 days).
>>>>> +
>>>>> +What:		/sys/fs/f2fs/<disk>/fsck_stack
>>>>> +Date:		August 2021
>>>>> +Contact:	"Yangtao Li" <frank.li@vivo.com>
>>>>> +Description:	Controls to enable/disable fsck stack trace, you can get stack
>>>>> +		information from kernel log. Note that the recorded stack information
>>>>> +		will not be cleared.
>>>>
>>>> Again, please don't add this into sysfs.
>>
>>Oh, I missed to check the details...
>>
>>> 
>>> I added this node, part of the idea is to trigger the export of stack information.
>>> There is no information transmitted through sysfs here, but the record of the stack is switched on and off.
>>> If don't export this information through procfs and sysfs, is there a more appropriate way?
>>
>>Well, I doubt why we should export stack info via proc/sysfs node or
>>sysfs switch.
>>
>>Those info will always be needed to troubleshoot issues no matter in
>>user or eng version of Android, can we just print them directly into
>>kernel message... what I concern is we may lost the bug scene due to
>>no one can help to trigger dmesg printing via sysfs.
>>
>>Jaegeuk, thoughts?
>>
>>> 
>>>>
>>>>> diff --git a/fs/f2fs/Kconfig b/fs/f2fs/Kconfig
>>>>> index 7669de7b49ce..f451e567e4a8 100644
>>>>> --- a/fs/f2fs/Kconfig
>>>>> +++ b/fs/f2fs/Kconfig
>>>>> @@ -135,3 +135,13 @@ config F2FS_FS_LZORLE
>>>>>    	default y
>>>>>    	help
>>>>>    	  Support LZO-RLE compress algorithm, if unsure, say Y.
>>>>> +
>>>>> +config F2FS_FSCK_STACK_TRACE
>>>>
>>>> I don't think we need another config to wrap this functionality, may be we
>>>> can use F2FS_CHECK_FS instead.
>>> 
>>> OK.
>>> 
>>>>
>>>>> +	bool "F2FS fsck stack information record"
>>>>> +	depends on F2FS_FS
>>>>> +	depends on STACKDEPOT
>>>>> +	default y
>>>>> +	help
>>>>> +	 Support printing out fsck stack history. With this, you have to
>>>>> +	 turn on "fsck_stack" sysfs node. Then you can get information
>>>>> +	 from kernel log.
>>>>> diff --git a/fs/f2fs/f2fs.h b/fs/f2fs/f2fs.h
>>>>> index ee8eb33e2c25..b2d1d1a5a3fc 100644
>>>>> --- a/fs/f2fs/f2fs.h
>>>>> +++ b/fs/f2fs/f2fs.h
>>>>> @@ -24,6 +24,8 @@
>>>>>    #include <linux/quotaops.h>
>>>>>    #include <linux/part_stat.h>
>>>>>    #include <crypto/hash.h>
>>>>> +#include <linux/stackdepot.h>
>>>>> +#include <linux/stacktrace.h>
>>>>>    
>>>>>    #include <linux/fscrypt.h>
>>>>>    #include <linux/fsverity.h>
>>>>> @@ -117,6 +119,8 @@ typedef u32 nid_t;
>>>>>    
>>>>>    #define COMPRESS_EXT_NUM		16
>>>>>    
>>>>> +#define FSCK_STACK_DEPTH		64
>>>>
>>>> 16?
>>> 
>>> OK.
>>> 
>>>>
>>>>> +
>>>>>    struct f2fs_mount_info {
>>>>>    	unsigned int opt;
>>>>>    	int write_io_size_bits;		/* Write IO size bits */
>>>>> @@ -1748,6 +1752,11 @@ struct f2fs_sb_info {
>>>>>    	unsigned int compress_watermark;	/* cache page watermark */
>>>>>    	atomic_t compress_page_hit;		/* cache hit count */
>>>>>    #endif
>>>>> +#ifdef CONFIG_F2FS_FSCK_STACK_TRACE
>>>>> +	depot_stack_handle_t *fsck_stack_history;
>>>>> +	unsigned int fsck_count;
>>>>> +	bool fsck_stack;
>>>>
>>>> IMO, all bug_on()s are corner cases, and catching those stacks won't cost
>>>> much, so we can just use CONFIG_XXX to enable/disable this feature.
>>> 
>>> F2FS_CHECK_FS ?
>>> 
>>>>
>>>>> +#endif
>>>>>    };
>>>>>    
>>>>>    struct f2fs_private_dio {
>>>>> @@ -1954,6 +1963,38 @@ static inline struct address_space *NODE_MAPPING(struct f2fs_sb_info *sbi)
>>>>>    	return sbi->node_inode->i_mapping;
>>>>>    }
>>>>>    
>>>>> +#ifdef CONFIG_F2FS_FSCK_STACK_TRACE
>>>>> +static void fsck_stack_trace(struct f2fs_sb_info *sbi)
>>>>> +{
>>>>> +	unsigned long entries[FSCK_STACK_DEPTH];
>>>>> +	depot_stack_handle_t stack, *new;
>>>>> +	unsigned int nr_entries;
>>>>> +	int i;
>>>>> +
>>>>> +	if (!sbi->fsck_stack)
>>>>> +		return;
>>>>> +
>>>>> +	nr_entries = stack_trace_save(entries, ARRAY_SIZE(entries), 0);
>>>>> +	nr_entries = filter_irq_stacks(entries, nr_entries);
>>>>> +	stack = stack_depot_save(entries, nr_entries, GFP_KERNEL);
>>>>> +	if (!stack)
>>>>> +		return;
>>>>> +
>>>>> +	/* Try to find an existing entry for this backtrace */
>>>>> +	for (i = 0; i < sbi->fsck_count; i++)
>>>>> +		if (sbi->fsck_stack_history[i] == stack)
>>>>> +			return;
>>>>> +
>>>>> +	new = krealloc(sbi->fsck_stack_history, (sbi->fsck_count + 1) *
>>>>> +		       sizeof(*sbi->fsck_stack_history), GFP_KERNEL);
>>>>> +	if (!new)
>>>>> +		return;
>>>>> +
>>>>> +	sbi->fsck_stack_history = new;
>>>>> +	sbi->fsck_stack_history[sbi->fsck_count++] = stack;
>>>>
>>>> It will case memory leak after f2fs module exits.
>>> 
>>> So let's enable this feature when f2fs is not a module and enable F2FS_CHECK_FS.
>>
>>I mean it needs to free .fsck_stack_history during umount().
>  >
> 
> So this is a double-edged sword, and the person who uses it must be aware of this issue.
> The release seems unrealistic under the current circumstances. This is also the reason

You mean the handles allocated from stack_depot_save() should not be released, right?

> why I add a switch, instead of enabling it by default after opening the configuration.

Well, I still didn't get why we can not release the memory allocated by krealloc().

Thanks,

> 
> Yangtao
> 
> 

^ permalink raw reply	[flat|nested] 10+ messages in thread

end of thread, other threads:[~2021-08-20  9:13 UTC | newest]

Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-08-14 10:37 [PATCH] f2fs: convert S_IRUGO to 0444 Yangtao Li
2021-08-14 10:37 ` [PATCH v6] f2fs: introduce /sys/fs/f2fs/<disk>/fsck_stack node Yangtao Li
2021-08-16  2:53   ` Chao Yu
2021-08-16  4:02     ` 李扬韬
2021-08-16  6:41       ` Chao Yu
2021-08-16  6:53         ` 李扬韬
2021-08-17  0:27         ` Jaegeuk Kim
2021-08-20  9:12           ` Chao Yu
2021-08-16  1:45 ` [PATCH] f2fs: convert S_IRUGO to 0444 Chao Yu
     [not found] <AEYAigA8D7ZftrI0MRYeIqoY.3.1629098634558.Hmail.frank.li@vivo.com>
2021-08-19  8:04 ` [PATCH v6] f2fs: introduce /sys/fs/f2fs/<disk>/fsck_stack node Chao Yu

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).