f2fs: avoid frequent costly fsck triggers

PATCH In review Jaegeuk Kim 3 participants 9 messages Nov 28, 2018 – Dec 7, 2018 F2FS file system

Share & tools

Jaegeuk Kim

author maintainer

2018-11-28 07:31 UTC #0 lore raw

PATCH 1/1 f2fs: avoid frequent costly fsck triggers

If we want to re-enable nat_bits, we rely on fsck which requires full scan of directory tree. Let's do that by regular fsck or unclean shutdown.

Signed-off-by Jaegeuk Kim <[email protected]>

---
 fs/f2fs/f2fs.h | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

fs/f2fs/f2fs.h +5 -1

index c28a9d1cb278..aa500239baf2 100644
@@ -1621,7 +1621,11 @@ static inline void disable_nat_bits(struct f2fs_sb_info *sbi, bool lock)
 {
 	unsigned long flags;
 
-	set_sbi_flag(sbi, SBI_NEED_FSCK);
+	/*
+	 * In order to re-enable nat_bits we need to call fsck.f2fs by
+	 * set_sbi_flag(sbi, SBI_NEED_FSCK). But it may give huge cost,
+	 * so let's rely on regular fsck or unclean shutdown.
+	 */
 
 	if (lock)
 		spin_lock_irqsave(&sbi->cp_lock, flags);

Signature · 1 line

2.19.0.605.g01d371f741-goog

Chao Yu

Reviewed-by on v1 1/1

2018-11-28 07:54 UTC #1 lore raw

Hi Jaeguek,

Quoted 2 lines · Jaegeuk Kim

If we want to re-enable nat_bits, we rely on fsck which requires full scan of directory tree. Let's do that by regular fsck or unclean shutdown.

Reviewed-by Chao Yu <[email protected]>

BTW, I have patch made some month ago...

In order to detect nat_bits disabling, could we introduce one more flag for fsck?

Quoted 1 line

From 86e8bdb2faeec904944bb6621073f4f7de51cc2d Mon Sep 17 00:00:00 2001

From: Chao Yu <[email protected]>
Date: Sun, 9 Sep 2018 05:40:20 +0800
Subject: [PATCH] f2fs: set specified flag after invalidating nat_bits

Signed-off-by Chao Yu <[email protected]>

---
 fs/f2fs/checkpoint.c    | 12 +++++++++++-
 fs/f2fs/f2fs.h          |  3 ++-
 fs/f2fs/node.c          |  3 +++
 include/linux/f2fs_fs.h |  1 +
 4 files changed, 17 insertions(+), 2 deletions(-)

fs/f2fs/checkpoint.c +0 -0

index 7e17bb3dfcb1..f7fb14e0f5f9 100644
@@ -1226,13 +1226,16 @@ static void update_ckpt_flags(struct f2fs_sb_info

*sbi, struct cp_control *cpc)
unsigned long orphan_num = sbi->im[ORPHAN_INO].ino_num; struct f2fs_checkpoint *ckpt = F2FS_CKPT(sbi); unsigned long flags;
+ bool disable_natbits = false;

spin_lock_irqsave(&sbi->cp_lock, flags);

if ((cpc->reason & CP_UMOUNT) &&
le32_to_cpu(ckpt->cp_pack_total_block_count) >
- sbi->blocks_per_seg - NM_I(sbi)->nat_bits_blocks)
+ sbi->blocks_per_seg - NM_I(sbi)->nat_bits_blocks) {
disable_nat_bits(sbi, false);
+ disable_natbits = true;
+ }

if (cpc->reason & CP_TRIMMED)
__set_ckpt_flags(ckpt, CP_TRIMMED_FLAG);
@@ -1270,11 +1273,18 @@ static void update_ckpt_flags(struct f2fs_sb_info *sbi, struct cp_control *cpc)
if (is_sbi_flag_set(sbi, SBI_QUOTA_NEED_REPAIR))
__set_ckpt_flags(ckpt, CP_QUOTA_NEED_FSCK_FLAG);

+ if (is_sbi_flag_set(sbi, SBI_NATBIT_NEED_REPAIR))
+ __set_ckpt_flags(ckpt, CP_NATBIT_NEED_FSCK_FLAG);
+
/* set this flag to activate crc|cp_ver for recovery */ __set_ckpt_flags(ckpt, CP_CRC_RECOVERY_FLAG); __clear_ckpt_flags(ckpt, CP_NOCRC_RECOVERY_FLAG);

spin_unlock_irqrestore(&sbi->cp_lock, flags);
+
+ if (disable_natbits)
+ f2fs_msg(sbi->sb, KERN_NOTICE,
+ "No enough space in CP area, disable nat_bits.");
}

static void commit_checkpoint(struct f2fs_sb_info *sbi,

fs/f2fs/f2fs.h +1 -0

index f0cedbe0c536..b55341c269b2 100644
@@ -1107,6 +1107,7 @@ enum {
 	SBI_QUOTA_NEED_FLUSH,			/* need to flush quota info in CP */
 	SBI_QUOTA_SKIP_FLUSH,			/* skip flushing quota in current CP */
 	SBI_QUOTA_NEED_REPAIR,			/* quota file may be corrupted */
+	SBI_NATBIT_NEED_REPAIR,			/* nat full/empty bitmaps need repair */
 };
 
 enum {

@@ -1628,7 +1629,7 @@ static inline void disable_nat_bits(struct

f2fs_sb_info *sbi, bool lock)
{
unsigned long flags;

- set_sbi_flag(sbi, SBI_NEED_FSCK);
+ set_sbi_flag(sbi, SBI_NATBIT_NEED_REPAIR);

if (lock)
spin_lock_irqsave(&sbi->cp_lock, flags);

fs/f2fs/node.c +3 -0

index e57add1e8966..0c6f8312a087 100644
@@ -2902,6 +2902,9 @@ static int __get_nat_bitmaps(struct f2fs_sb_info *sbi)
 
 	cp_ver |= (cur_cp_crc(ckpt) << 32);
 	if (cpu_to_le64(cp_ver) != *(__le64 *)nm_i->nat_bits) {
+		f2fs_msg(sbi->sb, KERN_NOTICE,
+			"Disable nat_bits due to incorrect cp_ver (%llu, %llu)",
+			cp_ver, le64_to_cpu(*(__le64 *)nm_i->nat_bits));
 		disable_nat_bits(sbi, true);
 		return 0;
 	}

include/linux/f2fs_fs.h +1 -0

index 7196653833fa..1f3ae1504573 100644
@@ -117,6 +117,7 @@ struct f2fs_super_block {
 /*
  * For checkpoint
  */
+#define CP_NATBIT_NEED_FSCK_FLAG	0X00002000
 #define CP_DISABLED_FLAG		0x00001000
 #define CP_QUOTA_NEED_FSCK_FLAG		0x00000800
 #define CP_LARGE_NAT_BITMAP_FLAG	0x00000400

Signature · 1 line

2.18.0.rc1

Quoted 24 lines

Signed-off-by Jaegeuk Kim <[email protected]>

---
 fs/f2fs/f2fs.h | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

fs/f2fs/f2fs.h +5 -1

index c28a9d1cb278..aa500239baf2 100644
@@ -1621,7 +1621,11 @@ static inline void disable_nat_bits(struct f2fs_sb_info *sbi, bool lock)
 {
 	unsigned long flags;
 
-	set_sbi_flag(sbi, SBI_NEED_FSCK);
+	/*
+	 * In order to re-enable nat_bits we need to call fsck.f2fs by
+	 * set_sbi_flag(sbi, SBI_NEED_FSCK). But it may give huge cost,
+	 * so let's rely on regular fsck or unclean shutdown.
+	 */
 
 	if (lock)
 		spin_lock_irqsave(&sbi->cp_lock, flags);

Jaegeuk Kim

author maintainer

2018-11-28 08:10 UTC #2 lore raw

Quoted 6 lines · Chao Yu

Hi Jaeguek,
Quoted 2 lines · Jaegeuk Kim
If we want to re-enable nat_bits, we rely on fsck which requires full scan of directory tree. Let's do that by regular fsck or unclean shutdown.

Quoted 6 lines

Reviewed-by Chao Yu <[email protected]>
BTW, I have patch made some month ago...
In order to detect nat_bits disabling, could we introduce one more flag for fsck?

Do we have a way to enable nat_bits very quickly in fsck?

Quoted 134 lines

Quoted 2 lines

From 86e8bdb2faeec904944bb6621073f4f7de51cc2d Mon Sep 17 00:00:00 2001

From: Chao Yu <[email protected]>
Date: Sun, 9 Sep 2018 05:40:20 +0800
Subject: [PATCH] f2fs: set specified flag after invalidating nat_bits

Signed-off-by Chao Yu <[email protected]>

---
 fs/f2fs/checkpoint.c    | 12 +++++++++++-
 fs/f2fs/f2fs.h          |  3 ++-
 fs/f2fs/node.c          |  3 +++
 include/linux/f2fs_fs.h |  1 +
 4 files changed, 17 insertions(+), 2 deletions(-)

fs/f2fs/checkpoint.c +4 -1

index 7e17bb3dfcb1..f7fb14e0f5f9 100644
@@ -1226,13 +1226,16 @@ static void update_ckpt_flags(struct f2fs_sb_info
 *sbi, struct cp_control *cpc)
 	unsigned long orphan_num = sbi->im[ORPHAN_INO].ino_num;
 	struct f2fs_checkpoint *ckpt = F2FS_CKPT(sbi);
 	unsigned long flags;
+	bool disable_natbits = false;
 
 	spin_lock_irqsave(&sbi->cp_lock, flags);
 
 	if ((cpc->reason & CP_UMOUNT) &&
 			le32_to_cpu(ckpt->cp_pack_total_block_count) >
-			sbi->blocks_per_seg - NM_I(sbi)->nat_bits_blocks)
+			sbi->blocks_per_seg - NM_I(sbi)->nat_bits_blocks) {
 		disable_nat_bits(sbi, false);
+		disable_natbits = true;
+	}
 
 	if (cpc->reason & CP_TRIMMED)

fs/f2fs/checkpoint.c +7 -0

 		__set_ckpt_flags(ckpt, CP_TRIMMED_FLAG);

@@ -1270,11 +1273,18 @@ static void update_ckpt_flags(struct f2fs_sb_info
 *sbi, struct cp_control *cpc)
 	if (is_sbi_flag_set(sbi, SBI_QUOTA_NEED_REPAIR))
 		__set_ckpt_flags(ckpt, CP_QUOTA_NEED_FSCK_FLAG);
 
+	if (is_sbi_flag_set(sbi, SBI_NATBIT_NEED_REPAIR))
+		__set_ckpt_flags(ckpt, CP_NATBIT_NEED_FSCK_FLAG);
+
 	/* set this flag to activate crc|cp_ver for recovery */
 	__set_ckpt_flags(ckpt, CP_CRC_RECOVERY_FLAG);
 	__clear_ckpt_flags(ckpt, CP_NOCRC_RECOVERY_FLAG);
 
 	spin_unlock_irqrestore(&sbi->cp_lock, flags);
+
+	if (disable_natbits)
+		f2fs_msg(sbi->sb, KERN_NOTICE,
+			"No enough space in CP area, disable nat_bits.");
 }

static void commit_checkpoint(struct f2fs_sb_info *sbi,

fs/f2fs/f2fs.h +2 -1

index f0cedbe0c536..b55341c269b2 100644
@@ -1107,6 +1107,7 @@ enum {
 	SBI_QUOTA_NEED_FLUSH,			/* need to flush quota info in CP */
 	SBI_QUOTA_SKIP_FLUSH,			/* skip flushing quota in current CP */
 	SBI_QUOTA_NEED_REPAIR,			/* quota file may be corrupted */
+	SBI_NATBIT_NEED_REPAIR,			/* nat full/empty bitmaps need repair */
 };
 
 enum {

@@ -1628,7 +1629,7 @@ static inline void disable_nat_bits(struct
 f2fs_sb_info *sbi, bool lock)
 {
 	unsigned long flags;
 
-	set_sbi_flag(sbi, SBI_NEED_FSCK);
+	set_sbi_flag(sbi, SBI_NATBIT_NEED_REPAIR);
 
 	if (lock)

fs/f2fs/f2fs.h +0 -0

 		spin_lock_irqsave(&sbi->cp_lock, flags);

fs/f2fs/node.c +3 -0

index e57add1e8966..0c6f8312a087 100644
@@ -2902,6 +2902,9 @@ static int __get_nat_bitmaps(struct f2fs_sb_info *sbi)
 
 	cp_ver |= (cur_cp_crc(ckpt) << 32);
 	if (cpu_to_le64(cp_ver) != *(__le64 *)nm_i->nat_bits) {
+		f2fs_msg(sbi->sb, KERN_NOTICE,
+			"Disable nat_bits due to incorrect cp_ver (%llu, %llu)",
+			cp_ver, le64_to_cpu(*(__le64 *)nm_i->nat_bits));
 		disable_nat_bits(sbi, true);
 		return 0;
 	}

include/linux/f2fs_fs.h +1 -0

index 7196653833fa..1f3ae1504573 100644
@@ -117,6 +117,7 @@ struct f2fs_super_block {
 /*
  * For checkpoint
  */
+#define CP_NATBIT_NEED_FSCK_FLAG	0X00002000
 #define CP_DISABLED_FLAG		0x00001000
 #define CP_QUOTA_NEED_FSCK_FLAG		0x00000800
 #define CP_LARGE_NAT_BITMAP_FLAG	0x00000400

Signature · 1 line

2.18.0.rc1

Quoted 24 lines

Signed-off-by Jaegeuk Kim <[email protected]>

---
 fs/f2fs/f2fs.h | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

fs/f2fs/f2fs.h +5 -1

index c28a9d1cb278..aa500239baf2 100644
@@ -1621,7 +1621,11 @@ static inline void disable_nat_bits(struct f2fs_sb_info *sbi, bool lock)
 {
 	unsigned long flags;
 
-	set_sbi_flag(sbi, SBI_NEED_FSCK);
+	/*
+	 * In order to re-enable nat_bits we need to call fsck.f2fs by
+	 * set_sbi_flag(sbi, SBI_NEED_FSCK). But it may give huge cost,
+	 * so let's rely on regular fsck or unclean shutdown.
+	 */
 
 	if (lock)
 		spin_lock_irqsave(&sbi->cp_lock, flags);

Chao Yu

2018-11-28 08:19 UTC #3 lore raw

Quoted 7 lines · Jaegeuk Kim

Quoted 6 lines · Chao Yu
Hi Jaeguek,
Quoted 2 lines · Jaegeuk Kim
If we want to re-enable nat_bits, we rely on fsck which requires full scan of directory tree. Let's do that by regular fsck or unclean shutdown.

Quoted 8 lines

Quoted 6 lines
Reviewed-by Chao Yu <[email protected]>
BTW, I have patch made some month ago...
In order to detect nat_bits disabling, could we introduce one more flag for fsck?
Do we have a way to enable nat_bits very quickly in fsck?

For image with SBI_NATBIT_NEED_REPAIR flag, can we just check metadata and rebuild nat_bits based on verified nat blocks/journals?

Thanks,

Quoted 138 lines

Quoted 135 lines

Quoted 3 lines

From 86e8bdb2faeec904944bb6621073f4f7de51cc2d Mon Sep 17 00:00:00 2001

From: Chao Yu <[email protected]>
Date: Sun, 9 Sep 2018 05:40:20 +0800
Subject: [PATCH] f2fs: set specified flag after invalidating nat_bits

Signed-off-by Chao Yu <[email protected]>

---
 fs/f2fs/checkpoint.c    | 12 +++++++++++-
 fs/f2fs/f2fs.h          |  3 ++-
 fs/f2fs/node.c          |  3 +++
 include/linux/f2fs_fs.h |  1 +
 4 files changed, 17 insertions(+), 2 deletions(-)

fs/f2fs/checkpoint.c +4 -1

index 7e17bb3dfcb1..f7fb14e0f5f9 100644
@@ -1226,13 +1226,16 @@ static void update_ckpt_flags(struct f2fs_sb_info
 *sbi, struct cp_control *cpc)
 	unsigned long orphan_num = sbi->im[ORPHAN_INO].ino_num;
 	struct f2fs_checkpoint *ckpt = F2FS_CKPT(sbi);
 	unsigned long flags;
+	bool disable_natbits = false;
 
 	spin_lock_irqsave(&sbi->cp_lock, flags);
 
 	if ((cpc->reason & CP_UMOUNT) &&
 			le32_to_cpu(ckpt->cp_pack_total_block_count) >
-			sbi->blocks_per_seg - NM_I(sbi)->nat_bits_blocks)
+			sbi->blocks_per_seg - NM_I(sbi)->nat_bits_blocks) {
 		disable_nat_bits(sbi, false);
+		disable_natbits = true;
+	}
 
 	if (cpc->reason & CP_TRIMMED)

fs/f2fs/checkpoint.c +7 -0

 		__set_ckpt_flags(ckpt, CP_TRIMMED_FLAG);

@@ -1270,11 +1273,18 @@ static void update_ckpt_flags(struct f2fs_sb_info
 *sbi, struct cp_control *cpc)
 	if (is_sbi_flag_set(sbi, SBI_QUOTA_NEED_REPAIR))
 		__set_ckpt_flags(ckpt, CP_QUOTA_NEED_FSCK_FLAG);
 
+	if (is_sbi_flag_set(sbi, SBI_NATBIT_NEED_REPAIR))
+		__set_ckpt_flags(ckpt, CP_NATBIT_NEED_FSCK_FLAG);
+
 	/* set this flag to activate crc|cp_ver for recovery */
 	__set_ckpt_flags(ckpt, CP_CRC_RECOVERY_FLAG);
 	__clear_ckpt_flags(ckpt, CP_NOCRC_RECOVERY_FLAG);
 
 	spin_unlock_irqrestore(&sbi->cp_lock, flags);
+
+	if (disable_natbits)
+		f2fs_msg(sbi->sb, KERN_NOTICE,
+			"No enough space in CP area, disable nat_bits.");
 }

static void commit_checkpoint(struct f2fs_sb_info *sbi,

fs/f2fs/f2fs.h +2 -1

index f0cedbe0c536..b55341c269b2 100644
@@ -1107,6 +1107,7 @@ enum {
 	SBI_QUOTA_NEED_FLUSH,			/* need to flush quota info in CP */
 	SBI_QUOTA_SKIP_FLUSH,			/* skip flushing quota in current CP */
 	SBI_QUOTA_NEED_REPAIR,			/* quota file may be corrupted */
+	SBI_NATBIT_NEED_REPAIR,			/* nat full/empty bitmaps need repair */
 };
 
 enum {

@@ -1628,7 +1629,7 @@ static inline void disable_nat_bits(struct
 f2fs_sb_info *sbi, bool lock)
 {
 	unsigned long flags;
 
-	set_sbi_flag(sbi, SBI_NEED_FSCK);
+	set_sbi_flag(sbi, SBI_NATBIT_NEED_REPAIR);
 
 	if (lock)

fs/f2fs/f2fs.h +0 -0

 		spin_lock_irqsave(&sbi->cp_lock, flags);

fs/f2fs/node.c +3 -0

index e57add1e8966..0c6f8312a087 100644
@@ -2902,6 +2902,9 @@ static int __get_nat_bitmaps(struct f2fs_sb_info *sbi)
 
 	cp_ver |= (cur_cp_crc(ckpt) << 32);
 	if (cpu_to_le64(cp_ver) != *(__le64 *)nm_i->nat_bits) {
+		f2fs_msg(sbi->sb, KERN_NOTICE,
+			"Disable nat_bits due to incorrect cp_ver (%llu, %llu)",
+			cp_ver, le64_to_cpu(*(__le64 *)nm_i->nat_bits));
 		disable_nat_bits(sbi, true);
 		return 0;
 	}

include/linux/f2fs_fs.h +1 -0

index 7196653833fa..1f3ae1504573 100644
@@ -117,6 +117,7 @@ struct f2fs_super_block {
 /*
  * For checkpoint
  */
+#define CP_NATBIT_NEED_FSCK_FLAG	0X00002000
 #define CP_DISABLED_FLAG		0x00001000
 #define CP_QUOTA_NEED_FSCK_FLAG		0x00000800
 #define CP_LARGE_NAT_BITMAP_FLAG	0x00000400

Signature · 1 line

2.18.0.rc1

Quoted 14 lines

Signed-off-by Jaegeuk Kim <[email protected]>

---
 fs/f2fs/f2fs.h | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

fs/f2fs/f2fs.h +0 -0

index c28a9d1cb278..aa500239baf2 100644
@@ -1621,7 +1621,11 @@ static inline void disable_nat_bits(struct f2fs_sb_info *sbi, bool lock)
 {
 	unsigned long flags;

fs/f2fs/f2fs.h static inline void disable_nat_bits(struct f2fs_sb_info *sbi, bool lock) · line 1631

Quoted 10 lines

fs/f2fs/f2fs.h +5 -1

 
-	set_sbi_flag(sbi, SBI_NEED_FSCK);
+	/*
+	 * In order to re-enable nat_bits we need to call fsck.f2fs by
+	 * set_sbi_flag(sbi, SBI_NEED_FSCK). But it may give huge cost,
+	 * so let's rely on regular fsck or unclean shutdown.
+	 */
 
 	if (lock)
 		spin_lock_irqsave(&sbi->cp_lock, flags);

Jaegeuk Kim

author maintainer

2018-11-28 17:48 UTC #4 lore raw

Quoted 12 lines · Chao Yu

Quoted 11 lines · Jaegeuk Kim
Quoted 10 lines · Chao Yu
Hi Jaeguek,
Quoted 2 lines · Jaegeuk Kim
If we want to re-enable nat_bits, we rely on fsck which requires full scan of directory tree. Let's do that by regular fsck or unclean shutdown.
Reviewed-by Chao Yu <[email protected]>
BTW, I have patch made some month ago...

Quoted 7 lines

Quoted 4 lines
Quoted 2 lines
In order to detect nat_bits disabling, could we introduce one more flag for fsck?
Do we have a way to enable nat_bits very quickly in fsck?
For image with SBI_NATBIT_NEED_REPAIR flag, can we just check metadata and rebuild nat_bits based on verified nat blocks/journals?

I'm leaning to rely on full scan to enable nat_bits again. We may add a mount count or timer to trigger fsck regularly?

Quoted 141 lines

Thanks,

Quoted 138 lines

Quoted 135 lines

Quoted 3 lines

From 86e8bdb2faeec904944bb6621073f4f7de51cc2d Mon Sep 17 00:00:00 2001

From: Chao Yu <[email protected]>
Date: Sun, 9 Sep 2018 05:40:20 +0800
Subject: [PATCH] f2fs: set specified flag after invalidating nat_bits

Signed-off-by Chao Yu <[email protected]>

---
 fs/f2fs/checkpoint.c    | 12 +++++++++++-
 fs/f2fs/f2fs.h          |  3 ++-
 fs/f2fs/node.c          |  3 +++
 include/linux/f2fs_fs.h |  1 +
 4 files changed, 17 insertions(+), 2 deletions(-)

fs/f2fs/checkpoint.c +4 -1

index 7e17bb3dfcb1..f7fb14e0f5f9 100644
@@ -1226,13 +1226,16 @@ static void update_ckpt_flags(struct f2fs_sb_info
 *sbi, struct cp_control *cpc)
 	unsigned long orphan_num = sbi->im[ORPHAN_INO].ino_num;
 	struct f2fs_checkpoint *ckpt = F2FS_CKPT(sbi);
 	unsigned long flags;
+	bool disable_natbits = false;
 
 	spin_lock_irqsave(&sbi->cp_lock, flags);
 
 	if ((cpc->reason & CP_UMOUNT) &&
 			le32_to_cpu(ckpt->cp_pack_total_block_count) >
-			sbi->blocks_per_seg - NM_I(sbi)->nat_bits_blocks)
+			sbi->blocks_per_seg - NM_I(sbi)->nat_bits_blocks) {
 		disable_nat_bits(sbi, false);
+		disable_natbits = true;
+	}
 
 	if (cpc->reason & CP_TRIMMED)

fs/f2fs/checkpoint.c +7 -0

 		__set_ckpt_flags(ckpt, CP_TRIMMED_FLAG);

@@ -1270,11 +1273,18 @@ static void update_ckpt_flags(struct f2fs_sb_info
 *sbi, struct cp_control *cpc)
 	if (is_sbi_flag_set(sbi, SBI_QUOTA_NEED_REPAIR))
 		__set_ckpt_flags(ckpt, CP_QUOTA_NEED_FSCK_FLAG);
 
+	if (is_sbi_flag_set(sbi, SBI_NATBIT_NEED_REPAIR))
+		__set_ckpt_flags(ckpt, CP_NATBIT_NEED_FSCK_FLAG);
+
 	/* set this flag to activate crc|cp_ver for recovery */
 	__set_ckpt_flags(ckpt, CP_CRC_RECOVERY_FLAG);
 	__clear_ckpt_flags(ckpt, CP_NOCRC_RECOVERY_FLAG);
 
 	spin_unlock_irqrestore(&sbi->cp_lock, flags);
+
+	if (disable_natbits)
+		f2fs_msg(sbi->sb, KERN_NOTICE,
+			"No enough space in CP area, disable nat_bits.");
 }

static void commit_checkpoint(struct f2fs_sb_info *sbi,

fs/f2fs/f2fs.h +2 -1

index f0cedbe0c536..b55341c269b2 100644
@@ -1107,6 +1107,7 @@ enum {
 	SBI_QUOTA_NEED_FLUSH,			/* need to flush quota info in CP */
 	SBI_QUOTA_SKIP_FLUSH,			/* skip flushing quota in current CP */
 	SBI_QUOTA_NEED_REPAIR,			/* quota file may be corrupted */
+	SBI_NATBIT_NEED_REPAIR,			/* nat full/empty bitmaps need repair */
 };
 
 enum {

@@ -1628,7 +1629,7 @@ static inline void disable_nat_bits(struct
 f2fs_sb_info *sbi, bool lock)
 {
 	unsigned long flags;
 
-	set_sbi_flag(sbi, SBI_NEED_FSCK);
+	set_sbi_flag(sbi, SBI_NATBIT_NEED_REPAIR);
 
 	if (lock)

fs/f2fs/f2fs.h +0 -0

 		spin_lock_irqsave(&sbi->cp_lock, flags);

fs/f2fs/node.c +3 -0

index e57add1e8966..0c6f8312a087 100644
@@ -2902,6 +2902,9 @@ static int __get_nat_bitmaps(struct f2fs_sb_info *sbi)
 
 	cp_ver |= (cur_cp_crc(ckpt) << 32);
 	if (cpu_to_le64(cp_ver) != *(__le64 *)nm_i->nat_bits) {
+		f2fs_msg(sbi->sb, KERN_NOTICE,
+			"Disable nat_bits due to incorrect cp_ver (%llu, %llu)",
+			cp_ver, le64_to_cpu(*(__le64 *)nm_i->nat_bits));
 		disable_nat_bits(sbi, true);
 		return 0;
 	}

include/linux/f2fs_fs.h +1 -0

index 7196653833fa..1f3ae1504573 100644
@@ -117,6 +117,7 @@ struct f2fs_super_block {
 /*
  * For checkpoint
  */
+#define CP_NATBIT_NEED_FSCK_FLAG	0X00002000
 #define CP_DISABLED_FLAG		0x00001000
 #define CP_QUOTA_NEED_FSCK_FLAG		0x00000800
 #define CP_LARGE_NAT_BITMAP_FLAG	0x00000400

Signature · 1 line

2.18.0.rc1

Quoted 14 lines

Signed-off-by Jaegeuk Kim <[email protected]>

---
 fs/f2fs/f2fs.h | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

fs/f2fs/f2fs.h +0 -0

index c28a9d1cb278..aa500239baf2 100644
@@ -1621,7 +1621,11 @@ static inline void disable_nat_bits(struct f2fs_sb_info *sbi, bool lock)
 {
 	unsigned long flags;

fs/f2fs/f2fs.h static inline void disable_nat_bits(struct f2fs_sb_info *sbi, bool lock) · line 1631

Quoted 10 lines

fs/f2fs/f2fs.h +5 -1

 
-	set_sbi_flag(sbi, SBI_NEED_FSCK);
+	/*
+	 * In order to re-enable nat_bits we need to call fsck.f2fs by
+	 * set_sbi_flag(sbi, SBI_NEED_FSCK). But it may give huge cost,
+	 * so let's rely on regular fsck or unclean shutdown.
+	 */
 
 	if (lock)
 		spin_lock_irqsave(&sbi->cp_lock, flags);

Sheng Yong

2018-11-30 02:35 UTC #5 lore raw

Hi, Jaegeuk and Chao,

Quoted 16 lines · Jaegeuk Kim

Quoted 15 lines · Chao Yu
Quoted 14 lines · Jaegeuk Kim
Quoted 12 lines · Chao Yu
Hi Jaeguek,
Quoted 2 lines · Jaegeuk Kim
If we want to re-enable nat_bits, we rely on fsck which requires full scan of directory tree. Let's do that by regular fsck or unclean shutdown.
Reviewed-by Chao Yu <[email protected]>
BTW, I have patch made some month ago...
In order to detect nat_bits disabling, could we introduce one more flag for fsck?

Quoted 7 lines

Quoted 4 lines
Quoted 1 line
Do we have a way to enable nat_bits very quickly in fsck?
For image with SBI_NATBIT_NEED_REPAIR flag, can we just check metadata and rebuild nat_bits based on verified nat blocks/journals?
I'm leaning to rely on full scan to enable nat_bits again. We may add a mount count or timer to trigger fsck regularly?

I'm afraid regular full fsck would give us bad experience of booting time. FYI, 256GB storage, which is filled with small files, costs almost 10 min to do a full fsck. And it seems larger capacity storages are on the way. So, is it worth doing that only to enable nat_bits (plus checking f2fs consistent not that necessarily)?

Thanks

Quoted 151 lines

Quoted 142 lines

Thanks,

Quoted 138 lines

Quoted 135 lines

Quoted 3 lines

From 86e8bdb2faeec904944bb6621073f4f7de51cc2d Mon Sep 17 00:00:00 2001

From: Chao Yu <[email protected]>
Date: Sun, 9 Sep 2018 05:40:20 +0800
Subject: [PATCH] f2fs: set specified flag after invalidating nat_bits

Signed-off-by Chao Yu <[email protected]>

---
  fs/f2fs/checkpoint.c    | 12 +++++++++++-
  fs/f2fs/f2fs.h          |  3 ++-
  fs/f2fs/node.c          |  3 +++
  include/linux/f2fs_fs.h |  1 +
  4 files changed, 17 insertions(+), 2 deletions(-)

fs/f2fs/checkpoint.c +4 -1

index 7e17bb3dfcb1..f7fb14e0f5f9 100644
@@ -1226,13 +1226,16 @@ static void update_ckpt_flags(struct f2fs_sb_info
 *sbi, struct cp_control *cpc)
  	unsigned long orphan_num = sbi->im[ORPHAN_INO].ino_num;
  	struct f2fs_checkpoint *ckpt = F2FS_CKPT(sbi);
  	unsigned long flags;
+	bool disable_natbits = false;
 
  	spin_lock_irqsave(&sbi->cp_lock, flags);
 
  	if ((cpc->reason & CP_UMOUNT) &&
  			le32_to_cpu(ckpt->cp_pack_total_block_count) >
-			sbi->blocks_per_seg - NM_I(sbi)->nat_bits_blocks)
+			sbi->blocks_per_seg - NM_I(sbi)->nat_bits_blocks) {
  		disable_nat_bits(sbi, false);
+		disable_natbits = true;
+	}
 
  	if (cpc->reason & CP_TRIMMED)

  		__set_ckpt_flags(ckpt, CP_TRIMMED_FLAG);

fs/f2fs/checkpoint.c +7 -0

@@ -1270,11 +1273,18 @@ static void update_ckpt_flags(struct f2fs_sb_info
 *sbi, struct cp_control *cpc)
  	if (is_sbi_flag_set(sbi, SBI_QUOTA_NEED_REPAIR))
  		__set_ckpt_flags(ckpt, CP_QUOTA_NEED_FSCK_FLAG);
 
+	if (is_sbi_flag_set(sbi, SBI_NATBIT_NEED_REPAIR))
+		__set_ckpt_flags(ckpt, CP_NATBIT_NEED_FSCK_FLAG);
+
  	/* set this flag to activate crc|cp_ver for recovery */
  	__set_ckpt_flags(ckpt, CP_CRC_RECOVERY_FLAG);
  	__clear_ckpt_flags(ckpt, CP_NOCRC_RECOVERY_FLAG);
 
  	spin_unlock_irqrestore(&sbi->cp_lock, flags);
+
+	if (disable_natbits)
+		f2fs_msg(sbi->sb, KERN_NOTICE,
+			"No enough space in CP area, disable nat_bits.");
  }

static void commit_checkpoint(struct f2fs_sb_info *sbi,

fs/f2fs/f2fs.h +2 -1

index f0cedbe0c536..b55341c269b2 100644
@@ -1107,6 +1107,7 @@ enum {
  	SBI_QUOTA_NEED_FLUSH,			/* need to flush quota info in CP */
  	SBI_QUOTA_SKIP_FLUSH,			/* skip flushing quota in current CP */
  	SBI_QUOTA_NEED_REPAIR,			/* quota file may be corrupted */
+	SBI_NATBIT_NEED_REPAIR,			/* nat full/empty bitmaps need repair */
  };
 
  enum {

@@ -1628,7 +1629,7 @@ static inline void disable_nat_bits(struct
 f2fs_sb_info *sbi, bool lock)
  {
  	unsigned long flags;
 
-	set_sbi_flag(sbi, SBI_NEED_FSCK);
+	set_sbi_flag(sbi, SBI_NATBIT_NEED_REPAIR);
 
  	if (lock)

  		spin_lock_irqsave(&sbi->cp_lock, flags);

fs/f2fs/node.c +3 -0

index e57add1e8966..0c6f8312a087 100644
@@ -2902,6 +2902,9 @@ static int __get_nat_bitmaps(struct f2fs_sb_info *sbi)
 
  	cp_ver |= (cur_cp_crc(ckpt) << 32);
  	if (cpu_to_le64(cp_ver) != *(__le64 *)nm_i->nat_bits) {
+		f2fs_msg(sbi->sb, KERN_NOTICE,
+			"Disable nat_bits due to incorrect cp_ver (%llu, %llu)",
+			cp_ver, le64_to_cpu(*(__le64 *)nm_i->nat_bits));
  		disable_nat_bits(sbi, true);
  		return 0;
  	}

include/linux/f2fs_fs.h +1 -0

index 7196653833fa..1f3ae1504573 100644
@@ -117,6 +117,7 @@ struct f2fs_super_block {
  /*
   * For checkpoint
   */
+#define CP_NATBIT_NEED_FSCK_FLAG	0X00002000
  #define CP_DISABLED_FLAG		0x00001000
  #define CP_QUOTA_NEED_FSCK_FLAG		0x00000800
  #define CP_LARGE_NAT_BITMAP_FLAG	0x00000400

Signature · 1 line

2.18.0.rc1

Quoted 14 lines

Signed-off-by Jaegeuk Kim <[email protected]>

---
  fs/f2fs/f2fs.h | 6 +++++-
  1 file changed, 5 insertions(+), 1 deletion(-)

fs/f2fs/f2fs.h +0 -0

index c28a9d1cb278..aa500239baf2 100644
@@ -1621,7 +1621,11 @@ static inline void disable_nat_bits(struct f2fs_sb_info *sbi, bool lock)
  {
  	unsigned long flags;

fs/f2fs/f2fs.h static inline void disable_nat_bits(struct f2fs_sb_info *sbi, bool lock) · line 1631

Quoted 10 lines

fs/f2fs/f2fs.h +5 -1

 
-	set_sbi_flag(sbi, SBI_NEED_FSCK);
+	/*
+	 * In order to re-enable nat_bits we need to call fsck.f2fs by
+	 * set_sbi_flag(sbi, SBI_NEED_FSCK). But it may give huge cost,
+	 * so let's rely on regular fsck or unclean shutdown.
+	 */
 
  	if (lock)
  		spin_lock_irqsave(&sbi->cp_lock, flags);

_______________________________________________
Linux-f2fs-devel mailing list
[email protected]
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel

Chao Yu

2018-11-30 03:10 UTC #6 lore raw

Quoted 24 lines · Sheng Yong

Hi, Jaegeuk and Chao,
Quoted 21 lines · Jaegeuk Kim
Quoted 19 lines · Chao Yu
Quoted 7 lines · Jaegeuk Kim
Quoted 6 lines · Chao Yu
Hi Jaeguek,
Quoted 2 lines · Jaegeuk Kim
If we want to re-enable nat_bits, we rely on fsck which requires full scan of directory tree. Let's do that by regular fsck or unclean shutdown.
Quoted 8 lines
Quoted 6 lines
Reviewed-by Chao Yu <[email protected]>
BTW, I have patch made some month ago...
In order to detect nat_bits disabling, could we introduce one more flag for fsck?
Do we have a way to enable nat_bits very quickly in fsck?
For image with SBI_NATBIT_NEED_REPAIR flag, can we just check metadata and rebuild nat_bits based on verified nat blocks/journals?

Quoted 6 lines

Quoted 2 lines
I'm leaning to rely on full scan to enable nat_bits again. We may add a mount count or timer to trigger fsck regularly?
I'm afraid regular full fsck would give us bad experience of booting time. FYI, 256GB storage, which is filled with small files, costs almost 10 min to do a full fsck. And it seems larger capacity storages are on the way.

Agreed.

Quoted 2 lines

So, is it worth doing that only to enable nat_bits (plus checking f2fs consistent not that necessarily)?

In android environment, I think it may be too expensive for adding nat_bits by triggering full scan by fsck during boot time.

If we can update all nat bitmap in free time after mount, maybe we can rebuild nat_bits based on full nat bitmap during umount, which can be cheaper than rebuiding in userspace.

Thanks,

Quoted 157 lines

Thanks

Quoted 144 lines

Quoted 142 lines

Thanks,

Quoted 138 lines

Quoted 135 lines

Quoted 3 lines

From 86e8bdb2faeec904944bb6621073f4f7de51cc2d Mon Sep 17 00:00:00 2001

From: Chao Yu <[email protected]>
Date: Sun, 9 Sep 2018 05:40:20 +0800
Subject: [PATCH] f2fs: set specified flag after invalidating nat_bits

Signed-off-by Chao Yu <[email protected]>

---
  fs/f2fs/checkpoint.c    | 12 +++++++++++-
  fs/f2fs/f2fs.h          |  3 ++-
  fs/f2fs/node.c          |  3 +++
  include/linux/f2fs_fs.h |  1 +
  4 files changed, 17 insertions(+), 2 deletions(-)

fs/f2fs/checkpoint.c +4 -1

index 7e17bb3dfcb1..f7fb14e0f5f9 100644
@@ -1226,13 +1226,16 @@ static void update_ckpt_flags(struct f2fs_sb_info
 *sbi, struct cp_control *cpc)
  	unsigned long orphan_num = sbi->im[ORPHAN_INO].ino_num;
  	struct f2fs_checkpoint *ckpt = F2FS_CKPT(sbi);
  	unsigned long flags;
+	bool disable_natbits = false;
 
  	spin_lock_irqsave(&sbi->cp_lock, flags);
 
  	if ((cpc->reason & CP_UMOUNT) &&
  			le32_to_cpu(ckpt->cp_pack_total_block_count) >
-			sbi->blocks_per_seg - NM_I(sbi)->nat_bits_blocks)
+			sbi->blocks_per_seg - NM_I(sbi)->nat_bits_blocks) {
  		disable_nat_bits(sbi, false);
+		disable_natbits = true;
+	}
 
  	if (cpc->reason & CP_TRIMMED)

  		__set_ckpt_flags(ckpt, CP_TRIMMED_FLAG);

fs/f2fs/checkpoint.c +7 -0

@@ -1270,11 +1273,18 @@ static void update_ckpt_flags(struct f2fs_sb_info
 *sbi, struct cp_control *cpc)
  	if (is_sbi_flag_set(sbi, SBI_QUOTA_NEED_REPAIR))
  		__set_ckpt_flags(ckpt, CP_QUOTA_NEED_FSCK_FLAG);
 
+	if (is_sbi_flag_set(sbi, SBI_NATBIT_NEED_REPAIR))
+		__set_ckpt_flags(ckpt, CP_NATBIT_NEED_FSCK_FLAG);
+
  	/* set this flag to activate crc|cp_ver for recovery */
  	__set_ckpt_flags(ckpt, CP_CRC_RECOVERY_FLAG);
  	__clear_ckpt_flags(ckpt, CP_NOCRC_RECOVERY_FLAG);
 
  	spin_unlock_irqrestore(&sbi->cp_lock, flags);
+
+	if (disable_natbits)
+		f2fs_msg(sbi->sb, KERN_NOTICE,
+			"No enough space in CP area, disable nat_bits.");
  }

static void commit_checkpoint(struct f2fs_sb_info *sbi,

fs/f2fs/f2fs.h +2 -1

index f0cedbe0c536..b55341c269b2 100644
@@ -1107,6 +1107,7 @@ enum {
  	SBI_QUOTA_NEED_FLUSH,			/* need to flush quota info in CP */
  	SBI_QUOTA_SKIP_FLUSH,			/* skip flushing quota in current CP */
  	SBI_QUOTA_NEED_REPAIR,			/* quota file may be corrupted */
+	SBI_NATBIT_NEED_REPAIR,			/* nat full/empty bitmaps need repair */
  };
 
  enum {

@@ -1628,7 +1629,7 @@ static inline void disable_nat_bits(struct
 f2fs_sb_info *sbi, bool lock)
  {
  	unsigned long flags;
 
-	set_sbi_flag(sbi, SBI_NEED_FSCK);
+	set_sbi_flag(sbi, SBI_NATBIT_NEED_REPAIR);
 
  	if (lock)

  		spin_lock_irqsave(&sbi->cp_lock, flags);

fs/f2fs/node.c +3 -0

index e57add1e8966..0c6f8312a087 100644
@@ -2902,6 +2902,9 @@ static int __get_nat_bitmaps(struct f2fs_sb_info *sbi)
 
  	cp_ver |= (cur_cp_crc(ckpt) << 32);
  	if (cpu_to_le64(cp_ver) != *(__le64 *)nm_i->nat_bits) {
+		f2fs_msg(sbi->sb, KERN_NOTICE,
+			"Disable nat_bits due to incorrect cp_ver (%llu, %llu)",
+			cp_ver, le64_to_cpu(*(__le64 *)nm_i->nat_bits));
  		disable_nat_bits(sbi, true);
  		return 0;
  	}

include/linux/f2fs_fs.h +1 -0

index 7196653833fa..1f3ae1504573 100644
@@ -117,6 +117,7 @@ struct f2fs_super_block {
  /*
   * For checkpoint
   */
+#define CP_NATBIT_NEED_FSCK_FLAG	0X00002000
  #define CP_DISABLED_FLAG		0x00001000
  #define CP_QUOTA_NEED_FSCK_FLAG		0x00000800
  #define CP_LARGE_NAT_BITMAP_FLAG	0x00000400

Signature · 1 line

2.18.0.rc1

Quoted 14 lines

Signed-off-by Jaegeuk Kim <[email protected]>

---
  fs/f2fs/f2fs.h | 6 +++++-
  1 file changed, 5 insertions(+), 1 deletion(-)

fs/f2fs/f2fs.h +0 -0

index c28a9d1cb278..aa500239baf2 100644
@@ -1621,7 +1621,11 @@ static inline void disable_nat_bits(struct f2fs_sb_info *sbi, bool lock)
  {
  	unsigned long flags;

fs/f2fs/f2fs.h static inline void disable_nat_bits(struct f2fs_sb_info *sbi, bool lock) · line 1631

Quoted 10 lines

fs/f2fs/f2fs.h +5 -1

 
-	set_sbi_flag(sbi, SBI_NEED_FSCK);
+	/*
+	 * In order to re-enable nat_bits we need to call fsck.f2fs by
+	 * set_sbi_flag(sbi, SBI_NEED_FSCK). But it may give huge cost,
+	 * so let's rely on regular fsck or unclean shutdown.
+	 */
 
  	if (lock)
  		spin_lock_irqsave(&sbi->cp_lock, flags);

Quoted 7 lines

_______________________________________________
Linux-f2fs-devel mailing list
[email protected]
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel
.

Jaegeuk Kim

author maintainer

2018-11-30 20:28 UTC #7 lore raw

Quoted 25 lines · Chao Yu

Quoted 24 lines · Sheng Yong
Hi, Jaegeuk and Chao,
Quoted 21 lines · Jaegeuk Kim
Quoted 19 lines · Chao Yu
Quoted 7 lines · Jaegeuk Kim
Quoted 6 lines · Chao Yu
Hi Jaeguek,
Quoted 2 lines · Jaegeuk Kim
If we want to re-enable nat_bits, we rely on fsck which requires full scan of directory tree. Let's do that by regular fsck or unclean shutdown.
Quoted 8 lines
Quoted 6 lines
Reviewed-by Chao Yu <[email protected]>
BTW, I have patch made some month ago...
In order to detect nat_bits disabling, could we introduce one more flag for fsck?
Do we have a way to enable nat_bits very quickly in fsck?
For image with SBI_NATBIT_NEED_REPAIR flag, can we just check metadata and rebuild nat_bits based on verified nat blocks/journals?

Quoted 8 lines

Quoted 6 lines
Quoted 2 lines
I'm leaning to rely on full scan to enable nat_bits again. We may add a mount count or timer to trigger fsck regularly?
I'm afraid regular full fsck would give us bad experience of booting time. FYI, 256GB storage, which is filled with small files, costs almost 10 min to do a full fsck. And it seems larger capacity storages are on the way.
Agreed.

Agreed. So, that's why I wrote this patch.

Quoted 6 lines

Quoted 3 lines
So, is it worth doing that only to enable nat_bits (plus checking f2fs consistent not that necessarily)?
In android environment, I think it may be too expensive for adding nat_bits by triggering full scan by fsck during boot time.

That's why I'd like to enable this only when we need full scan.

Quoted 4 lines

If we can update all nat bitmap in free time after mount, maybe we can rebuild nat_bits based on full nat bitmap during umount, which can be cheaper than rebuiding in userspace.

Yeah, rebuiling nat_bits in run time would be better, but can be applied in future. But, since Android reboot procedure uses a timeout, if we exceed it, we'll get unclean unmount which triggers another fsck, which doesn't make sense at all.

Quoted 160 lines

Thanks,

Quoted 157 lines

Thanks

Quoted 144 lines

Quoted 142 lines

Thanks,

Quoted 138 lines

Quoted 135 lines

Quoted 3 lines

From 86e8bdb2faeec904944bb6621073f4f7de51cc2d Mon Sep 17 00:00:00 2001

From: Chao Yu <[email protected]>
Date: Sun, 9 Sep 2018 05:40:20 +0800
Subject: [PATCH] f2fs: set specified flag after invalidating nat_bits

Signed-off-by Chao Yu <[email protected]>

---
  fs/f2fs/checkpoint.c    | 12 +++++++++++-
  fs/f2fs/f2fs.h          |  3 ++-
  fs/f2fs/node.c          |  3 +++
  include/linux/f2fs_fs.h |  1 +
  4 files changed, 17 insertions(+), 2 deletions(-)

fs/f2fs/checkpoint.c +4 -1

index 7e17bb3dfcb1..f7fb14e0f5f9 100644
@@ -1226,13 +1226,16 @@ static void update_ckpt_flags(struct f2fs_sb_info
 *sbi, struct cp_control *cpc)
  	unsigned long orphan_num = sbi->im[ORPHAN_INO].ino_num;
  	struct f2fs_checkpoint *ckpt = F2FS_CKPT(sbi);
  	unsigned long flags;
+	bool disable_natbits = false;
 
  	spin_lock_irqsave(&sbi->cp_lock, flags);
 
  	if ((cpc->reason & CP_UMOUNT) &&
  			le32_to_cpu(ckpt->cp_pack_total_block_count) >
-			sbi->blocks_per_seg - NM_I(sbi)->nat_bits_blocks)
+			sbi->blocks_per_seg - NM_I(sbi)->nat_bits_blocks) {
  		disable_nat_bits(sbi, false);
+		disable_natbits = true;
+	}
 
  	if (cpc->reason & CP_TRIMMED)

  		__set_ckpt_flags(ckpt, CP_TRIMMED_FLAG);

fs/f2fs/checkpoint.c +7 -0

@@ -1270,11 +1273,18 @@ static void update_ckpt_flags(struct f2fs_sb_info
 *sbi, struct cp_control *cpc)
  	if (is_sbi_flag_set(sbi, SBI_QUOTA_NEED_REPAIR))
  		__set_ckpt_flags(ckpt, CP_QUOTA_NEED_FSCK_FLAG);
 
+	if (is_sbi_flag_set(sbi, SBI_NATBIT_NEED_REPAIR))
+		__set_ckpt_flags(ckpt, CP_NATBIT_NEED_FSCK_FLAG);
+
  	/* set this flag to activate crc|cp_ver for recovery */
  	__set_ckpt_flags(ckpt, CP_CRC_RECOVERY_FLAG);
  	__clear_ckpt_flags(ckpt, CP_NOCRC_RECOVERY_FLAG);
 
  	spin_unlock_irqrestore(&sbi->cp_lock, flags);
+
+	if (disable_natbits)
+		f2fs_msg(sbi->sb, KERN_NOTICE,
+			"No enough space in CP area, disable nat_bits.");
  }

static void commit_checkpoint(struct f2fs_sb_info *sbi,

fs/f2fs/f2fs.h +2 -1

index f0cedbe0c536..b55341c269b2 100644
@@ -1107,6 +1107,7 @@ enum {
  	SBI_QUOTA_NEED_FLUSH,			/* need to flush quota info in CP */
  	SBI_QUOTA_SKIP_FLUSH,			/* skip flushing quota in current CP */
  	SBI_QUOTA_NEED_REPAIR,			/* quota file may be corrupted */
+	SBI_NATBIT_NEED_REPAIR,			/* nat full/empty bitmaps need repair */
  };
 
  enum {

@@ -1628,7 +1629,7 @@ static inline void disable_nat_bits(struct
 f2fs_sb_info *sbi, bool lock)
  {
  	unsigned long flags;
 
-	set_sbi_flag(sbi, SBI_NEED_FSCK);
+	set_sbi_flag(sbi, SBI_NATBIT_NEED_REPAIR);
 
  	if (lock)

  		spin_lock_irqsave(&sbi->cp_lock, flags);

fs/f2fs/node.c +3 -0

index e57add1e8966..0c6f8312a087 100644
@@ -2902,6 +2902,9 @@ static int __get_nat_bitmaps(struct f2fs_sb_info *sbi)
 
  	cp_ver |= (cur_cp_crc(ckpt) << 32);
  	if (cpu_to_le64(cp_ver) != *(__le64 *)nm_i->nat_bits) {
+		f2fs_msg(sbi->sb, KERN_NOTICE,
+			"Disable nat_bits due to incorrect cp_ver (%llu, %llu)",
+			cp_ver, le64_to_cpu(*(__le64 *)nm_i->nat_bits));
  		disable_nat_bits(sbi, true);
  		return 0;
  	}

include/linux/f2fs_fs.h +1 -0

index 7196653833fa..1f3ae1504573 100644
@@ -117,6 +117,7 @@ struct f2fs_super_block {
  /*
   * For checkpoint
   */
+#define CP_NATBIT_NEED_FSCK_FLAG	0X00002000
  #define CP_DISABLED_FLAG		0x00001000
  #define CP_QUOTA_NEED_FSCK_FLAG		0x00000800
  #define CP_LARGE_NAT_BITMAP_FLAG	0x00000400

Signature · 1 line

2.18.0.rc1

Quoted 14 lines

Signed-off-by Jaegeuk Kim <[email protected]>

---
  fs/f2fs/f2fs.h | 6 +++++-
  1 file changed, 5 insertions(+), 1 deletion(-)

fs/f2fs/f2fs.h +0 -0

index c28a9d1cb278..aa500239baf2 100644
@@ -1621,7 +1621,11 @@ static inline void disable_nat_bits(struct f2fs_sb_info *sbi, bool lock)
  {
  	unsigned long flags;

fs/f2fs/f2fs.h static inline void disable_nat_bits(struct f2fs_sb_info *sbi, bool lock) · line 1631

Quoted 10 lines

fs/f2fs/f2fs.h +5 -1

 
-	set_sbi_flag(sbi, SBI_NEED_FSCK);
+	/*
+	 * In order to re-enable nat_bits we need to call fsck.f2fs by
+	 * set_sbi_flag(sbi, SBI_NEED_FSCK). But it may give huge cost,
+	 * so let's rely on regular fsck or unclean shutdown.
+	 */
 
  	if (lock)
  		spin_lock_irqsave(&sbi->cp_lock, flags);

Quoted 7 lines

_______________________________________________
Linux-f2fs-devel mailing list
[email protected]
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel
.

Chao Yu

2018-12-07 09:41 UTC #8 lore raw

Quoted 29 lines · Jaegeuk Kim

Quoted 28 lines · Chao Yu
Quoted 27 lines · Sheng Yong
Hi, Jaegeuk and Chao,
Quoted 23 lines · Jaegeuk Kim
Quoted 12 lines · Chao Yu
Quoted 11 lines · Jaegeuk Kim
Quoted 10 lines · Chao Yu
Hi Jaeguek,
Quoted 2 lines · Jaegeuk Kim
If we want to re-enable nat_bits, we rely on fsck which requires full scan of directory tree. Let's do that by regular fsck or unclean shutdown.
Reviewed-by Chao Yu <[email protected]>
BTW, I have patch made some month ago...
Quoted 7 lines
Quoted 4 lines
Quoted 2 lines
In order to detect nat_bits disabling, could we introduce one more flag for fsck?
Do we have a way to enable nat_bits very quickly in fsck?
For image with SBI_NATBIT_NEED_REPAIR flag, can we just check metadata and rebuild nat_bits based on verified nat blocks/journals?
I'm leaning to rely on full scan to enable nat_bits again. We may add a mount count or timer to trigger fsck regularly?

Quoted 7 lines

Quoted 5 lines
Quoted 3 lines
I'm afraid regular full fsck would give us bad experience of booting time. FYI, 256GB storage, which is filled with small files, costs almost 10 min to do a full fsck. And it seems larger capacity storages are on the way.
Agreed.
Agreed. So, that's why I wrote this patch.

I see.

Quoted 11 lines

Quoted 7 lines
Quoted 4 lines
So, is it worth doing that only to enable nat_bits (plus checking f2fs consistent not that necessarily)?
In android environment, I think it may be too expensive for adding nat_bits by triggering full scan by fsck during boot time.
That's why I'd like to enable this only when we need full scan.
Quoted 1 line

Quoted 6 lines

Quoted 3 lines
If we can update all nat bitmap in free time after mount, maybe we can rebuild nat_bits based on full nat bitmap during umount, which can be cheaper than rebuiding in userspace.
Yeah, rebuiling nat_bits in run time would be better, but can be applied in future. But, since Android reboot procedure uses a timeout, if we exceed

It only need to write additional nat_bits blocks during last umount checkpoint, why it can exceed the timeout period?

Thanks,

Quoted 166 lines

it, we'll get unclean unmount which triggers another fsck, which doesn't make sense at all.

Quoted 160 lines

Thanks,

Quoted 157 lines

Thanks

Quoted 144 lines

Quoted 142 lines

Thanks,

Quoted 138 lines

Quoted 135 lines

Quoted 3 lines

From 86e8bdb2faeec904944bb6621073f4f7de51cc2d Mon Sep 17 00:00:00 2001

From: Chao Yu <[email protected]>
Date: Sun, 9 Sep 2018 05:40:20 +0800
Subject: [PATCH] f2fs: set specified flag after invalidating nat_bits

Signed-off-by Chao Yu <[email protected]>

---
  fs/f2fs/checkpoint.c    | 12 +++++++++++-
  fs/f2fs/f2fs.h          |  3 ++-
  fs/f2fs/node.c          |  3 +++
  include/linux/f2fs_fs.h |  1 +
  4 files changed, 17 insertions(+), 2 deletions(-)

fs/f2fs/checkpoint.c +4 -1

index 7e17bb3dfcb1..f7fb14e0f5f9 100644
@@ -1226,13 +1226,16 @@ static void update_ckpt_flags(struct f2fs_sb_info
 *sbi, struct cp_control *cpc)
  	unsigned long orphan_num = sbi->im[ORPHAN_INO].ino_num;
  	struct f2fs_checkpoint *ckpt = F2FS_CKPT(sbi);
  	unsigned long flags;
+	bool disable_natbits = false;
 
  	spin_lock_irqsave(&sbi->cp_lock, flags);
 
  	if ((cpc->reason & CP_UMOUNT) &&
  			le32_to_cpu(ckpt->cp_pack_total_block_count) >
-			sbi->blocks_per_seg - NM_I(sbi)->nat_bits_blocks)
+			sbi->blocks_per_seg - NM_I(sbi)->nat_bits_blocks) {
  		disable_nat_bits(sbi, false);
+		disable_natbits = true;
+	}
 
  	if (cpc->reason & CP_TRIMMED)

  		__set_ckpt_flags(ckpt, CP_TRIMMED_FLAG);

fs/f2fs/checkpoint.c +7 -0

@@ -1270,11 +1273,18 @@ static void update_ckpt_flags(struct f2fs_sb_info
 *sbi, struct cp_control *cpc)
  	if (is_sbi_flag_set(sbi, SBI_QUOTA_NEED_REPAIR))
  		__set_ckpt_flags(ckpt, CP_QUOTA_NEED_FSCK_FLAG);
 
+	if (is_sbi_flag_set(sbi, SBI_NATBIT_NEED_REPAIR))
+		__set_ckpt_flags(ckpt, CP_NATBIT_NEED_FSCK_FLAG);
+
  	/* set this flag to activate crc|cp_ver for recovery */
  	__set_ckpt_flags(ckpt, CP_CRC_RECOVERY_FLAG);
  	__clear_ckpt_flags(ckpt, CP_NOCRC_RECOVERY_FLAG);
 
  	spin_unlock_irqrestore(&sbi->cp_lock, flags);
+
+	if (disable_natbits)
+		f2fs_msg(sbi->sb, KERN_NOTICE,
+			"No enough space in CP area, disable nat_bits.");
  }

static void commit_checkpoint(struct f2fs_sb_info *sbi,

fs/f2fs/f2fs.h +2 -1

index f0cedbe0c536..b55341c269b2 100644
@@ -1107,6 +1107,7 @@ enum {
  	SBI_QUOTA_NEED_FLUSH,			/* need to flush quota info in CP */
  	SBI_QUOTA_SKIP_FLUSH,			/* skip flushing quota in current CP */
  	SBI_QUOTA_NEED_REPAIR,			/* quota file may be corrupted */
+	SBI_NATBIT_NEED_REPAIR,			/* nat full/empty bitmaps need repair */
  };
 
  enum {

@@ -1628,7 +1629,7 @@ static inline void disable_nat_bits(struct
 f2fs_sb_info *sbi, bool lock)
  {
  	unsigned long flags;
 
-	set_sbi_flag(sbi, SBI_NEED_FSCK);
+	set_sbi_flag(sbi, SBI_NATBIT_NEED_REPAIR);
 
  	if (lock)

  		spin_lock_irqsave(&sbi->cp_lock, flags);

fs/f2fs/node.c +3 -0

index e57add1e8966..0c6f8312a087 100644
@@ -2902,6 +2902,9 @@ static int __get_nat_bitmaps(struct f2fs_sb_info *sbi)
 
  	cp_ver |= (cur_cp_crc(ckpt) << 32);
  	if (cpu_to_le64(cp_ver) != *(__le64 *)nm_i->nat_bits) {
+		f2fs_msg(sbi->sb, KERN_NOTICE,
+			"Disable nat_bits due to incorrect cp_ver (%llu, %llu)",
+			cp_ver, le64_to_cpu(*(__le64 *)nm_i->nat_bits));
  		disable_nat_bits(sbi, true);
  		return 0;
  	}

include/linux/f2fs_fs.h +1 -0

index 7196653833fa..1f3ae1504573 100644
@@ -117,6 +117,7 @@ struct f2fs_super_block {
  /*
   * For checkpoint
   */
+#define CP_NATBIT_NEED_FSCK_FLAG	0X00002000
  #define CP_DISABLED_FLAG		0x00001000
  #define CP_QUOTA_NEED_FSCK_FLAG		0x00000800
  #define CP_LARGE_NAT_BITMAP_FLAG	0x00000400

Signature · 1 line

2.18.0.rc1

Quoted 14 lines

Signed-off-by Jaegeuk Kim <[email protected]>

---
  fs/f2fs/f2fs.h | 6 +++++-
  1 file changed, 5 insertions(+), 1 deletion(-)

fs/f2fs/f2fs.h +0 -0

index c28a9d1cb278..aa500239baf2 100644
@@ -1621,7 +1621,11 @@ static inline void disable_nat_bits(struct f2fs_sb_info *sbi, bool lock)
  {
  	unsigned long flags;

fs/f2fs/f2fs.h static inline void disable_nat_bits(struct f2fs_sb_info *sbi, bool lock) · line 1631

Quoted 10 lines

fs/f2fs/f2fs.h +5 -1

 
-	set_sbi_flag(sbi, SBI_NEED_FSCK);
+	/*
+	 * In order to re-enable nat_bits we need to call fsck.f2fs by
+	 * set_sbi_flag(sbi, SBI_NEED_FSCK). But it may give huge cost,
+	 * so let's rely on regular fsck or unclean shutdown.
+	 */
 
  	if (lock)
  		spin_lock_irqsave(&sbi->cp_lock, flags);

Quoted 7 lines

_______________________________________________
Linux-f2fs-devel mailing list
[email protected]
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel
.