linux-f2fs-devel.lists.sourceforge.net archive mirror
 help / color / mirror / Atom feed
* [f2fs-dev] [PATCH v1] f2fs: fix has_curseg_enough_space to check all data segments for dentry blocks
@ 2025-11-10 13:26 Xiaole He
  2025-11-11  2:44 ` Yongpeng Yang
  0 siblings, 1 reply; 8+ messages in thread
From: Xiaole He @ 2025-11-10 13:26 UTC (permalink / raw)
  To: linux-f2fs-devel; +Cc: jaegeuk, Xiaole He, linux-kernel, stable

When active_logs == 6, dentry blocks can be allocated to HOT, WARM, or
COLD segments based on various conditions in __get_segment_type_6():
- age extent cache (if enabled)
- FI_HOT_DATA flag (set when dirty_pages <= min_hot_blocks)
- rw_hint (defaults to WARM via f2fs_rw_hint_to_seg_type)
- file_is_hot(), FI_NEED_IPU, f2fs_is_cow_file(), etc.

However, has_curseg_enough_space() only checked CURSEG_HOT_DATA segment
for dentry blocks, which could lead to incorrect space calculation when
dentry blocks are actually allocated to WARM or COLD segments.

Reproducer:
Note: This reproducer requires adding a tracepoint to observe segment
type. Add the following tracepoint to include/trace/events/f2fs.h:

TRACE_EVENT(f2fs_allocate_data_block,
        TP_PROTO(struct f2fs_sb_info *sbi, struct inode *inode,
                enum log_type type, block_t blkaddr),

        TP_ARGS(sbi, inode, type, blkaddr),

        TP_STRUCT__entry(
                __field(dev_t, dev)
                __field(ino_t, ino)
                __field(int, type)
                __field(block_t, blkaddr)
                __field(int, is_dir)
        ),

        TP_fast_assign(
                __entry->dev = sbi->sb->s_dev;
                __entry->ino = inode ? inode->i_ino : 0;
                __entry->type = type;
                __entry->blkaddr = blkaddr;
                __entry->is_dir = inode ? S_ISDIR(inode->i_mode) : 0;
        ),

        TP_printk("dev = (%d,%d), ino = %lu, %s, blkaddr = %u, is_dir = %d",
                show_dev(__entry->dev),
                (unsigned long)__entry->ino,
                show_data_type(__entry->type),
                __entry->blkaddr,
                __entry->is_dir)
);

And add the tracepoint call in fs/f2fs/segment.c in
f2fs_allocate_data_block() function. Find the location after
locate_dirty_segment() calls and before IS_DATASEG() check:

	locate_dirty_segment(sbi, GET_SEGNO(sbi, old_blkaddr));
	locate_dirty_segment(sbi, GET_SEGNO(sbi, *new_blkaddr));

	trace_f2fs_allocate_data_block(sbi, folio ? folio->mapping->host : NULL,
					type, *new_blkaddr);

	if (IS_DATASEG(curseg->seg_type))

1. Mount F2FS with active_logs=6 and age extent cache disabled:
   # mkfs.f2fs -f /dev/sdb1
   # mount -t f2fs -o active_logs=6 /dev/sdb1 /mnt/f2fs-test

2. Enable tracing and f2fs_allocate_data_block tracepoint:
   # echo 1 > /sys/kernel/debug/tracing/events/f2fs/f2fs_allocate_data_block/enable
   # echo 1 > /sys/kernel/debug/tracing/tracing_on
   # echo > /sys/kernel/debug/tracing/trace

3. Create a directory and write enough files to trigger dirty_pages >
   min_hot_blocks (default 16), which will clear FI_HOT_DATA flag:
   # mkdir /mnt/f2fs-test/testdir
   # cd /mnt/f2fs-test/testdir
   # seq 1 8192 | xargs touch
   # sync

4. Observe dentry block allocation:
   # cat /sys/kernel/debug/tracing/trace

   The trace output shows dentry blocks (is_dir = 1) allocated to WARM
   segment because FI_HOT_DATA is cleared when dirty_pages >
   min_hot_blocks (default 16). However, has_curseg_enough_space() only
   checked HOT_DATA segment space.

Fix by checking all three data segments (HOT, WARM, COLD) when
active_logs == 6, similar to how __get_segment_type_6() can return
any of these segment types for dentry blocks.

Fixes: ef095d19e82f ("f2fs: write small sized IO to hot log")
Cc: stable@kernel.org
Signed-off-by: Xiaole He <hexiaole1994@126.com>
---
 fs/f2fs/segment.h | 31 +++++++++++++++++++++++++------
 1 file changed, 25 insertions(+), 6 deletions(-)

diff --git a/fs/f2fs/segment.h b/fs/f2fs/segment.h
index 1ce2c8abaf48..c13400a53013 100644
--- a/fs/f2fs/segment.h
+++ b/fs/f2fs/segment.h
@@ -632,15 +632,34 @@ static inline bool has_curseg_enough_space(struct f2fs_sb_info *sbi,
 	}
 
 	/* check current data section for dentry blocks. */
-	segno = CURSEG_I(sbi, CURSEG_HOT_DATA)->segno;
+	if (F2FS_OPTION(sbi).active_logs == 6) {
+		/*
+		 * With active_logs == 6, dentry blocks can be allocated to
+		 * HOT, WARM, or COLD segments based on age extent cache,
+		 * FI_HOT_DATA flag, rw_hint, etc. Check all three.
+		 */
+		for (i = CURSEG_HOT_DATA; i <= CURSEG_COLD_DATA; i++) {
+			segno = CURSEG_I(sbi, i)->segno;
+
+			if (unlikely(segno == NULL_SEGNO))
+				return false;
+
+			left_blocks = get_left_section_blocks(sbi, i, segno);
+
+			if (dent_blocks > left_blocks)
+				return false;
+		}
+	} else {
+		segno = CURSEG_I(sbi, CURSEG_HOT_DATA)->segno;
 
-	if (unlikely(segno == NULL_SEGNO))
-		return false;
+		if (unlikely(segno == NULL_SEGNO))
+			return false;
 
-	left_blocks = get_left_section_blocks(sbi, CURSEG_HOT_DATA, segno);
+		left_blocks = get_left_section_blocks(sbi, CURSEG_HOT_DATA, segno);
 
-	if (dent_blocks > left_blocks)
-		return false;
+		if (dent_blocks > left_blocks)
+			return false;
+	}
 	return true;
 }
 
-- 
2.43.0



_______________________________________________
Linux-f2fs-devel mailing list
Linux-f2fs-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel

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

* Re: [f2fs-dev] [PATCH v1] f2fs: fix has_curseg_enough_space to check all data segments for dentry blocks
  2025-11-10 13:26 [f2fs-dev] [PATCH v1] f2fs: fix has_curseg_enough_space to check all data segments for dentry blocks Xiaole He
@ 2025-11-11  2:44 ` Yongpeng Yang
  2025-11-11  6:00   ` [f2fs-dev] [PATCH v2] " Xiaole He
  0 siblings, 1 reply; 8+ messages in thread
From: Yongpeng Yang @ 2025-11-11  2:44 UTC (permalink / raw)
  To: Xiaole He, linux-f2fs-devel; +Cc: jaegeuk, Yongpeng Yang, linux-kernel, stable

On 11/10/25 21:26, Xiaole He wrote:
> When active_logs == 6, dentry blocks can be allocated to HOT, WARM, or
> COLD segments based on various conditions in __get_segment_type_6():
> - age extent cache (if enabled)
> - FI_HOT_DATA flag (set when dirty_pages <= min_hot_blocks)
> - rw_hint (defaults to WARM via f2fs_rw_hint_to_seg_type)
> - file_is_hot(), FI_NEED_IPU, f2fs_is_cow_file(), etc.
> 
> However, has_curseg_enough_space() only checked CURSEG_HOT_DATA segment
> for dentry blocks, which could lead to incorrect space calculation when
> dentry blocks are actually allocated to WARM or COLD segments.
> 
> Reproducer:
> Note: This reproducer requires adding a tracepoint to observe segment
> type. Add the following tracepoint to include/trace/events/f2fs.h:
> 
> TRACE_EVENT(f2fs_allocate_data_block,
>          TP_PROTO(struct f2fs_sb_info *sbi, struct inode *inode,
>                  enum log_type type, block_t blkaddr),
> 
>          TP_ARGS(sbi, inode, type, blkaddr),
> 
>          TP_STRUCT__entry(
>                  __field(dev_t, dev)
>                  __field(ino_t, ino)
>                  __field(int, type)
>                  __field(block_t, blkaddr)
>                  __field(int, is_dir)
>          ),
> 
>          TP_fast_assign(
>                  __entry->dev = sbi->sb->s_dev;
>                  __entry->ino = inode ? inode->i_ino : 0;
>                  __entry->type = type;
>                  __entry->blkaddr = blkaddr;
>                  __entry->is_dir = inode ? S_ISDIR(inode->i_mode) : 0;
>          ),
> 
>          TP_printk("dev = (%d,%d), ino = %lu, %s, blkaddr = %u, is_dir = %d",
>                  show_dev(__entry->dev),
>                  (unsigned long)__entry->ino,
>                  show_data_type(__entry->type),
>                  __entry->blkaddr,
>                  __entry->is_dir)
> );
> 
> And add the tracepoint call in fs/f2fs/segment.c in
> f2fs_allocate_data_block() function. Find the location after
> locate_dirty_segment() calls and before IS_DATASEG() check:
> 
> 	locate_dirty_segment(sbi, GET_SEGNO(sbi, old_blkaddr));
> 	locate_dirty_segment(sbi, GET_SEGNO(sbi, *new_blkaddr));
> 
> 	trace_f2fs_allocate_data_block(sbi, folio ? folio->mapping->host : NULL,
> 					type, *new_blkaddr);
> 
> 	if (IS_DATASEG(curseg->seg_type))
> 
> 1. Mount F2FS with active_logs=6 and age extent cache disabled:
>     # mkfs.f2fs -f /dev/sdb1
>     # mount -t f2fs -o active_logs=6 /dev/sdb1 /mnt/f2fs-test
> 
> 2. Enable tracing and f2fs_allocate_data_block tracepoint:
>     # echo 1 > /sys/kernel/debug/tracing/events/f2fs/f2fs_allocate_data_block/enable
>     # echo 1 > /sys/kernel/debug/tracing/tracing_on
>     # echo > /sys/kernel/debug/tracing/trace
> 
> 3. Create a directory and write enough files to trigger dirty_pages >
>     min_hot_blocks (default 16), which will clear FI_HOT_DATA flag:
>     # mkdir /mnt/f2fs-test/testdir
>     # cd /mnt/f2fs-test/testdir
>     # seq 1 8192 | xargs touch
>     # sync
> 
> 4. Observe dentry block allocation:
>     # cat /sys/kernel/debug/tracing/trace
> 
>     The trace output shows dentry blocks (is_dir = 1) allocated to WARM
>     segment because FI_HOT_DATA is cleared when dirty_pages >
>     min_hot_blocks (default 16). However, has_curseg_enough_space() only
>     checked HOT_DATA segment space.
> 
> Fix by checking all three data segments (HOT, WARM, COLD) when
> active_logs == 6, similar to how __get_segment_type_6() can return
> any of these segment types for dentry blocks.
> 
> Fixes: ef095d19e82f ("f2fs: write small sized IO to hot log")
> Cc: stable@kernel.org
> Signed-off-by: Xiaole He <hexiaole1994@126.com>
> ---
>   fs/f2fs/segment.h | 31 +++++++++++++++++++++++++------
>   1 file changed, 25 insertions(+), 6 deletions(-)
> 
> diff --git a/fs/f2fs/segment.h b/fs/f2fs/segment.h
> index 1ce2c8abaf48..c13400a53013 100644
> --- a/fs/f2fs/segment.h
> +++ b/fs/f2fs/segment.h
> @@ -632,15 +632,34 @@ static inline bool has_curseg_enough_space(struct f2fs_sb_info *sbi,
>   	}
>   
>   	/* check current data section for dentry blocks. */
> -	segno = CURSEG_I(sbi, CURSEG_HOT_DATA)->segno;
> +	if (F2FS_OPTION(sbi).active_logs == 6) {
> +		/*
> +		 * With active_logs == 6, dentry blocks can be allocated to
> +		 * HOT, WARM, or COLD segments based on age extent cache,
> +		 * FI_HOT_DATA flag, rw_hint, etc. Check all three.
> +		 */
> +		for (i = CURSEG_HOT_DATA; i <= CURSEG_COLD_DATA; i++) {
> +			segno = CURSEG_I(sbi, i)->segno;
> +
> +			if (unlikely(segno == NULL_SEGNO))
> +				return false;
> +
> +			left_blocks = get_left_section_blocks(sbi, i, segno);
> +
> +			if (dent_blocks > left_blocks)
> +				return false;
> +		}

How about merging this for-loop with the one above to avoid duplicated
code? Since both dent_blocks and data_blocks may write to data segments,
should we also check whether data_blocks + dent_blocks > left_blocks?

Yongpeng,


> +	} else {
> +		segno = CURSEG_I(sbi, CURSEG_HOT_DATA)->segno;
>   
> -	if (unlikely(segno == NULL_SEGNO))
> -		return false;
> +		if (unlikely(segno == NULL_SEGNO))
> +			return false;
>   
> -	left_blocks = get_left_section_blocks(sbi, CURSEG_HOT_DATA, segno);
> +		left_blocks = get_left_section_blocks(sbi, CURSEG_HOT_DATA, segno);
>   
> -	if (dent_blocks > left_blocks)
> -		return false;
> +		if (dent_blocks > left_blocks)
> +			return false;
> +	}
>   	return true;
>   }
>   



_______________________________________________
Linux-f2fs-devel mailing list
Linux-f2fs-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel

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

* [f2fs-dev] [PATCH v2] f2fs: fix has_curseg_enough_space to check all data segments for dentry blocks
  2025-11-11  2:44 ` Yongpeng Yang
@ 2025-11-11  6:00   ` Xiaole He
  2025-11-11  6:10     ` Xiaole He
  0 siblings, 1 reply; 8+ messages in thread
From: Xiaole He @ 2025-11-11  6:00 UTC (permalink / raw)
  To: linux-f2fs-devel; +Cc: jaegeuk, linux-kernel

Hi Yongpeng,

Thanks for your feedback! I've updated the patch per your suggestions:
- Merged the dentry block check into the main loop to avoid duplication
- Check data_blocks + dent_blocks for data segments since both can write to the same segment

Please see the v2 patch.

Best regards,
Xiaole



_______________________________________________
Linux-f2fs-devel mailing list
Linux-f2fs-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel

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

* [f2fs-dev] [PATCH v2] f2fs: fix has_curseg_enough_space to check all data segments for dentry blocks
  2025-11-11  6:00   ` [f2fs-dev] [PATCH v2] " Xiaole He
@ 2025-11-11  6:10     ` Xiaole He
  2025-11-11  6:29       ` Yongpeng Yang
                         ` (3 more replies)
  0 siblings, 4 replies; 8+ messages in thread
From: Xiaole He @ 2025-11-11  6:10 UTC (permalink / raw)
  To: linux-f2fs-devel; +Cc: jaegeuk, Xiaole He, linux-kernel, stable

When active_logs == 6, dentry blocks can be allocated to HOT, WARM, or
COLD segments based on various conditions in __get_segment_type_6():
- age extent cache (if enabled)
- FI_HOT_DATA flag (set when dirty_pages <= min_hot_blocks)
- rw_hint (defaults to WARM via f2fs_rw_hint_to_seg_type)
- file_is_hot(), FI_NEED_IPU, f2fs_is_cow_file(), etc.

However, has_curseg_enough_space() only checked CURSEG_HOT_DATA segment
for dentry blocks, which could lead to incorrect space calculation when
dentry blocks are actually allocated to WARM or COLD segments.

Reproducer:
Note: This reproducer requires adding a tracepoint to observe segment
type. Add the following tracepoint to include/trace/events/f2fs.h:

TRACE_EVENT(f2fs_allocate_data_block,
        TP_PROTO(struct f2fs_sb_info *sbi, struct inode *inode,
                enum log_type type, block_t blkaddr),

        TP_ARGS(sbi, inode, type, blkaddr),

        TP_STRUCT__entry(
                __field(dev_t, dev)
                __field(ino_t, ino)
                __field(int, type)
                __field(block_t, blkaddr)
                __field(int, is_dir)
        ),

        TP_fast_assign(
                __entry->dev = sbi->sb->s_dev;
                __entry->ino = inode ? inode->i_ino : 0;
                __entry->type = type;
                __entry->blkaddr = blkaddr;
                __entry->is_dir = inode ? S_ISDIR(inode->i_mode) : 0;
        ),

        TP_printk("dev = (%d,%d), ino = %lu, %s, blkaddr = %u, is_dir = %d",
                show_dev(__entry->dev),
                (unsigned long)__entry->ino,
                show_data_type(__entry->type),
                __entry->blkaddr,
                __entry->is_dir)
);

And add the tracepoint call in fs/f2fs/segment.c in
f2fs_allocate_data_block() function. Find the location after
locate_dirty_segment() calls and before IS_DATASEG() check:

        locate_dirty_segment(sbi, GET_SEGNO(sbi, old_blkaddr));
        locate_dirty_segment(sbi, GET_SEGNO(sbi, *new_blkaddr));

        trace_f2fs_allocate_data_block(sbi, folio ? folio->mapping->host : NULL,
                                        type, *new_blkaddr);

        if (IS_DATASEG(curseg->seg_type))

1. Mount F2FS with active_logs=6 and age extent cache disabled:
   # mkfs.f2fs -f /dev/sdb1
   # mount -t f2fs -o active_logs=6 /dev/sdb1 /mnt/f2fs-test

2. Enable tracing and f2fs_allocate_data_block tracepoint:
   # echo 1 > /sys/kernel/debug/tracing/events/f2fs/f2fs_allocate_data_block/enable
   # echo 1 > /sys/kernel/debug/tracing/tracing_on
   # echo > /sys/kernel/debug/tracing/trace

3. Create a directory and write enough files to trigger dirty_pages >
   min_hot_blocks (default 16), which will clear FI_HOT_DATA flag:
   # mkdir /mnt/f2fs-test/testdir
   # cd /mnt/f2fs-test/testdir
   # seq 1 8192 | xargs touch
   # sync

4. Observe dentry block allocation:
   # cat /sys/kernel/debug/tracing/trace

   The trace output shows dentry blocks (is_dir = 1) allocated to WARM
   segment because FI_HOT_DATA is cleared when dirty_pages >
   min_hot_blocks (default 16). However, has_curseg_enough_space() only
   checked HOT_DATA segment space.

Fix by merging the dentry block check into the main data/node block
check loop and checking data_blocks + dent_blocks for data segments,
since both regular data blocks and dentry blocks can be written to the
same segment. When active_logs == 6, dentry blocks can be allocated to
any of the three data segments (HOT, WARM, COLD), so all three segments
need to account for dentry blocks. When active_logs != 6, dentry blocks
are always allocated to HOT_DATA segment only, so only HOT_DATA segment
needs to account for dentry blocks, while WARM and COLD segments only
check data_blocks.

Fixes: ef095d19e82f ("f2fs: write small sized IO to hot log")
Cc: stable@kernel.org
Signed-off-by: Xiaole He <hexiaole1994@126.com>
---
Changes in v2 (per Yongpeng's feedback):
- Merged dentry block check into the main loop to avoid duplication
- Check data_blocks + dent_blocks for data segments (both can write to same segment)
---
 fs/f2fs/segment.h | 24 ++++++++++++------------
 1 file changed, 12 insertions(+), 12 deletions(-)

diff --git a/fs/f2fs/segment.h b/fs/f2fs/segment.h
index 1ce2c8abaf48..acda720a54eb 100644
--- a/fs/f2fs/segment.h
+++ b/fs/f2fs/segment.h
@@ -626,21 +626,21 @@ static inline bool has_curseg_enough_space(struct f2fs_sb_info *sbi,
 
 		left_blocks = get_left_section_blocks(sbi, i, segno);
 
-		blocks = i <= CURSEG_COLD_DATA ? data_blocks : node_blocks;
+		if (i <= CURSEG_COLD_DATA) {
+			blocks = data_blocks;
+			/*
+			 * With active_logs == 6, dentry blocks can be allocated to
+			 * any data segment. With active_logs != 6, dentry blocks
+			 * are always allocated to HOT_DATA segment.
+			 */
+			if ((F2FS_OPTION(sbi).active_logs == 6) || (i == CURSEG_HOT_DATA))
+				blocks += dent_blocks;
+		} else {
+			blocks = node_blocks;
+		}
 		if (blocks > left_blocks)
 			return false;
 	}
-
-	/* check current data section for dentry blocks. */
-	segno = CURSEG_I(sbi, CURSEG_HOT_DATA)->segno;
-
-	if (unlikely(segno == NULL_SEGNO))
-		return false;
-
-	left_blocks = get_left_section_blocks(sbi, CURSEG_HOT_DATA, segno);
-
-	if (dent_blocks > left_blocks)
-		return false;
 	return true;
 }
 
-- 
2.43.0



_______________________________________________
Linux-f2fs-devel mailing list
Linux-f2fs-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel

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

* Re: [f2fs-dev] [PATCH v2] f2fs: fix has_curseg_enough_space to check all data segments for dentry blocks
  2025-11-11  6:10     ` Xiaole He
@ 2025-11-11  6:29       ` Yongpeng Yang
  2025-11-11  7:17       ` Chao Yu via Linux-f2fs-devel
                         ` (2 subsequent siblings)
  3 siblings, 0 replies; 8+ messages in thread
From: Yongpeng Yang @ 2025-11-11  6:29 UTC (permalink / raw)
  To: Xiaole He, linux-f2fs-devel; +Cc: jaegeuk, Yongpeng Yang, linux-kernel, stable

On 11/11/25 14:10, Xiaole He wrote:
> When active_logs == 6, dentry blocks can be allocated to HOT, WARM, or
> COLD segments based on various conditions in __get_segment_type_6():
> - age extent cache (if enabled)
> - FI_HOT_DATA flag (set when dirty_pages <= min_hot_blocks)
> - rw_hint (defaults to WARM via f2fs_rw_hint_to_seg_type)
> - file_is_hot(), FI_NEED_IPU, f2fs_is_cow_file(), etc.
> 
> However, has_curseg_enough_space() only checked CURSEG_HOT_DATA segment
> for dentry blocks, which could lead to incorrect space calculation when
> dentry blocks are actually allocated to WARM or COLD segments.
> 
> Reproducer:
> Note: This reproducer requires adding a tracepoint to observe segment
> type. Add the following tracepoint to include/trace/events/f2fs.h:
> 
> TRACE_EVENT(f2fs_allocate_data_block,
>          TP_PROTO(struct f2fs_sb_info *sbi, struct inode *inode,
>                  enum log_type type, block_t blkaddr),
> 
>          TP_ARGS(sbi, inode, type, blkaddr),
> 
>          TP_STRUCT__entry(
>                  __field(dev_t, dev)
>                  __field(ino_t, ino)
>                  __field(int, type)
>                  __field(block_t, blkaddr)
>                  __field(int, is_dir)
>          ),
> 
>          TP_fast_assign(
>                  __entry->dev = sbi->sb->s_dev;
>                  __entry->ino = inode ? inode->i_ino : 0;
>                  __entry->type = type;
>                  __entry->blkaddr = blkaddr;
>                  __entry->is_dir = inode ? S_ISDIR(inode->i_mode) : 0;
>          ),
> 
>          TP_printk("dev = (%d,%d), ino = %lu, %s, blkaddr = %u, is_dir = %d",
>                  show_dev(__entry->dev),
>                  (unsigned long)__entry->ino,
>                  show_data_type(__entry->type),
>                  __entry->blkaddr,
>                  __entry->is_dir)
> );
> 
> And add the tracepoint call in fs/f2fs/segment.c in
> f2fs_allocate_data_block() function. Find the location after
> locate_dirty_segment() calls and before IS_DATASEG() check:
> 
>          locate_dirty_segment(sbi, GET_SEGNO(sbi, old_blkaddr));
>          locate_dirty_segment(sbi, GET_SEGNO(sbi, *new_blkaddr));
> 
>          trace_f2fs_allocate_data_block(sbi, folio ? folio->mapping->host : NULL,
>                                          type, *new_blkaddr);
> 
>          if (IS_DATASEG(curseg->seg_type))
> 
> 1. Mount F2FS with active_logs=6 and age extent cache disabled:
>     # mkfs.f2fs -f /dev/sdb1
>     # mount -t f2fs -o active_logs=6 /dev/sdb1 /mnt/f2fs-test
> 
> 2. Enable tracing and f2fs_allocate_data_block tracepoint:
>     # echo 1 > /sys/kernel/debug/tracing/events/f2fs/f2fs_allocate_data_block/enable
>     # echo 1 > /sys/kernel/debug/tracing/tracing_on
>     # echo > /sys/kernel/debug/tracing/trace
> 
> 3. Create a directory and write enough files to trigger dirty_pages >
>     min_hot_blocks (default 16), which will clear FI_HOT_DATA flag:
>     # mkdir /mnt/f2fs-test/testdir
>     # cd /mnt/f2fs-test/testdir
>     # seq 1 8192 | xargs touch
>     # sync
> 
> 4. Observe dentry block allocation:
>     # cat /sys/kernel/debug/tracing/trace
> 
>     The trace output shows dentry blocks (is_dir = 1) allocated to WARM
>     segment because FI_HOT_DATA is cleared when dirty_pages >
>     min_hot_blocks (default 16). However, has_curseg_enough_space() only
>     checked HOT_DATA segment space.
> 
> Fix by merging the dentry block check into the main data/node block
> check loop and checking data_blocks + dent_blocks for data segments,
> since both regular data blocks and dentry blocks can be written to the
> same segment. When active_logs == 6, dentry blocks can be allocated to
> any of the three data segments (HOT, WARM, COLD), so all three segments
> need to account for dentry blocks. When active_logs != 6, dentry blocks
> are always allocated to HOT_DATA segment only, so only HOT_DATA segment
> needs to account for dentry blocks, while WARM and COLD segments only
> check data_blocks.
> 
> Fixes: ef095d19e82f ("f2fs: write small sized IO to hot log")
> Cc: stable@kernel.org
> Signed-off-by: Xiaole He <hexiaole1994@126.com>
> ---
> Changes in v2 (per Yongpeng's feedback):
> - Merged dentry block check into the main loop to avoid duplication
> - Check data_blocks + dent_blocks for data segments (both can write to same segment)
> ---
>   fs/f2fs/segment.h | 24 ++++++++++++------------
>   1 file changed, 12 insertions(+), 12 deletions(-)
> 
> diff --git a/fs/f2fs/segment.h b/fs/f2fs/segment.h
> index 1ce2c8abaf48..acda720a54eb 100644
> --- a/fs/f2fs/segment.h
> +++ b/fs/f2fs/segment.h
> @@ -626,21 +626,21 @@ static inline bool has_curseg_enough_space(struct f2fs_sb_info *sbi,
>   
>   		left_blocks = get_left_section_blocks(sbi, i, segno);
>   
> -		blocks = i <= CURSEG_COLD_DATA ? data_blocks : node_blocks;
> +		if (i <= CURSEG_COLD_DATA) {
> +			blocks = data_blocks;
> +			/*
> +			 * With active_logs == 6, dentry blocks can be allocated to
> +			 * any data segment. With active_logs != 6, dentry blocks
> +			 * are always allocated to HOT_DATA segment.
> +			 */
> +			if ((F2FS_OPTION(sbi).active_logs == 6) || (i == CURSEG_HOT_DATA))
> +				blocks += dent_blocks;
> +		} else {
> +			blocks = node_blocks;
> +		}
>   		if (blocks > left_blocks)
>   			return false;
>   	}
> -
> -	/* check current data section for dentry blocks. */
> -	segno = CURSEG_I(sbi, CURSEG_HOT_DATA)->segno;
> -
> -	if (unlikely(segno == NULL_SEGNO))
> -		return false;
> -
> -	left_blocks = get_left_section_blocks(sbi, CURSEG_HOT_DATA, segno);
> -
> -	if (dent_blocks > left_blocks)
> -		return false;
>   	return true;
>   }
>   

Looks good to me,
Reviewed-by: Yongpeng Yang <yangyongpeng@xiaomi.com>

Thanks
Yongpeng,


_______________________________________________
Linux-f2fs-devel mailing list
Linux-f2fs-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel

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

* Re: [f2fs-dev] [PATCH v2] f2fs: fix has_curseg_enough_space to check all data segments for dentry blocks
  2025-11-11  6:10     ` Xiaole He
  2025-11-11  6:29       ` Yongpeng Yang
@ 2025-11-11  7:17       ` Chao Yu via Linux-f2fs-devel
  2025-11-24 18:50       ` patchwork-bot+f2fs--- via Linux-f2fs-devel
  2025-12-02 18:00       ` Jaegeuk Kim via Linux-f2fs-devel
  3 siblings, 0 replies; 8+ messages in thread
From: Chao Yu via Linux-f2fs-devel @ 2025-11-11  7:17 UTC (permalink / raw)
  To: Xiaole He, linux-f2fs-devel; +Cc: jaegeuk, linux-kernel, stable

On 11/11/25 14:10, Xiaole He wrote:
> When active_logs == 6, dentry blocks can be allocated to HOT, WARM, or
> COLD segments based on various conditions in __get_segment_type_6():
> - age extent cache (if enabled)
> - FI_HOT_DATA flag (set when dirty_pages <= min_hot_blocks)
> - rw_hint (defaults to WARM via f2fs_rw_hint_to_seg_type)
> - file_is_hot(), FI_NEED_IPU, f2fs_is_cow_file(), etc.
> 
> However, has_curseg_enough_space() only checked CURSEG_HOT_DATA segment
> for dentry blocks, which could lead to incorrect space calculation when
> dentry blocks are actually allocated to WARM or COLD segments.
> 
> Reproducer:
> Note: This reproducer requires adding a tracepoint to observe segment
> type. Add the following tracepoint to include/trace/events/f2fs.h:
> 
> TRACE_EVENT(f2fs_allocate_data_block,
>         TP_PROTO(struct f2fs_sb_info *sbi, struct inode *inode,
>                 enum log_type type, block_t blkaddr),
> 
>         TP_ARGS(sbi, inode, type, blkaddr),
> 
>         TP_STRUCT__entry(
>                 __field(dev_t, dev)
>                 __field(ino_t, ino)
>                 __field(int, type)
>                 __field(block_t, blkaddr)
>                 __field(int, is_dir)
>         ),
> 
>         TP_fast_assign(
>                 __entry->dev = sbi->sb->s_dev;
>                 __entry->ino = inode ? inode->i_ino : 0;
>                 __entry->type = type;
>                 __entry->blkaddr = blkaddr;
>                 __entry->is_dir = inode ? S_ISDIR(inode->i_mode) : 0;
>         ),
> 
>         TP_printk("dev = (%d,%d), ino = %lu, %s, blkaddr = %u, is_dir = %d",
>                 show_dev(__entry->dev),
>                 (unsigned long)__entry->ino,
>                 show_data_type(__entry->type),
>                 __entry->blkaddr,
>                 __entry->is_dir)
> );
> 
> And add the tracepoint call in fs/f2fs/segment.c in
> f2fs_allocate_data_block() function. Find the location after
> locate_dirty_segment() calls and before IS_DATASEG() check:
> 
>         locate_dirty_segment(sbi, GET_SEGNO(sbi, old_blkaddr));
>         locate_dirty_segment(sbi, GET_SEGNO(sbi, *new_blkaddr));
> 
>         trace_f2fs_allocate_data_block(sbi, folio ? folio->mapping->host : NULL,
>                                         type, *new_blkaddr);
> 
>         if (IS_DATASEG(curseg->seg_type))
> 
> 1. Mount F2FS with active_logs=6 and age extent cache disabled:
>    # mkfs.f2fs -f /dev/sdb1
>    # mount -t f2fs -o active_logs=6 /dev/sdb1 /mnt/f2fs-test
> 
> 2. Enable tracing and f2fs_allocate_data_block tracepoint:
>    # echo 1 > /sys/kernel/debug/tracing/events/f2fs/f2fs_allocate_data_block/enable
>    # echo 1 > /sys/kernel/debug/tracing/tracing_on
>    # echo > /sys/kernel/debug/tracing/trace
> 
> 3. Create a directory and write enough files to trigger dirty_pages >
>    min_hot_blocks (default 16), which will clear FI_HOT_DATA flag:
>    # mkdir /mnt/f2fs-test/testdir
>    # cd /mnt/f2fs-test/testdir
>    # seq 1 8192 | xargs touch
>    # sync
> 
> 4. Observe dentry block allocation:
>    # cat /sys/kernel/debug/tracing/trace
> 
>    The trace output shows dentry blocks (is_dir = 1) allocated to WARM
>    segment because FI_HOT_DATA is cleared when dirty_pages >
>    min_hot_blocks (default 16). However, has_curseg_enough_space() only
>    checked HOT_DATA segment space.
> 
> Fix by merging the dentry block check into the main data/node block
> check loop and checking data_blocks + dent_blocks for data segments,
> since both regular data blocks and dentry blocks can be written to the
> same segment. When active_logs == 6, dentry blocks can be allocated to
> any of the three data segments (HOT, WARM, COLD), so all three segments
> need to account for dentry blocks. When active_logs != 6, dentry blocks
> are always allocated to HOT_DATA segment only, so only HOT_DATA segment
> needs to account for dentry blocks, while WARM and COLD segments only
> check data_blocks.
> 
> Fixes: ef095d19e82f ("f2fs: write small sized IO to hot log")
> Cc: stable@kernel.org
> Signed-off-by: Xiaole He <hexiaole1994@126.com>

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

Thanks,


_______________________________________________
Linux-f2fs-devel mailing list
Linux-f2fs-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel

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

* Re: [f2fs-dev] [PATCH v2] f2fs: fix has_curseg_enough_space to check all data segments for dentry blocks
  2025-11-11  6:10     ` Xiaole He
  2025-11-11  6:29       ` Yongpeng Yang
  2025-11-11  7:17       ` Chao Yu via Linux-f2fs-devel
@ 2025-11-24 18:50       ` patchwork-bot+f2fs--- via Linux-f2fs-devel
  2025-12-02 18:00       ` Jaegeuk Kim via Linux-f2fs-devel
  3 siblings, 0 replies; 8+ messages in thread
From: patchwork-bot+f2fs--- via Linux-f2fs-devel @ 2025-11-24 18:50 UTC (permalink / raw)
  To: Xiaole He; +Cc: jaegeuk, stable, linux-kernel, linux-f2fs-devel

Hello:

This patch was applied to jaegeuk/f2fs.git (dev)
by Jaegeuk Kim <jaegeuk@kernel.org>:

On Tue, 11 Nov 2025 14:10:51 +0800 you wrote:
> When active_logs == 6, dentry blocks can be allocated to HOT, WARM, or
> COLD segments based on various conditions in __get_segment_type_6():
> - age extent cache (if enabled)
> - FI_HOT_DATA flag (set when dirty_pages <= min_hot_blocks)
> - rw_hint (defaults to WARM via f2fs_rw_hint_to_seg_type)
> - file_is_hot(), FI_NEED_IPU, f2fs_is_cow_file(), etc.
> 
> [...]

Here is the summary with links:
  - [f2fs-dev,v2] f2fs: fix has_curseg_enough_space to check all data segments for dentry blocks
    https://git.kernel.org/jaegeuk/f2fs/c/6d87364f7e94

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html




_______________________________________________
Linux-f2fs-devel mailing list
Linux-f2fs-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel

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

* Re: [f2fs-dev] [PATCH v2] f2fs: fix has_curseg_enough_space to check all data segments for dentry blocks
  2025-11-11  6:10     ` Xiaole He
                         ` (2 preceding siblings ...)
  2025-11-24 18:50       ` patchwork-bot+f2fs--- via Linux-f2fs-devel
@ 2025-12-02 18:00       ` Jaegeuk Kim via Linux-f2fs-devel
  3 siblings, 0 replies; 8+ messages in thread
From: Jaegeuk Kim via Linux-f2fs-devel @ 2025-12-02 18:00 UTC (permalink / raw)
  To: Xiaole He; +Cc: stable, linux-kernel, linux-f2fs-devel

I need to drop this patch, since this breaks xfstests such as f2fs/005.

Please revisit the issue.

On 11/11, Xiaole He wrote:
> When active_logs == 6, dentry blocks can be allocated to HOT, WARM, or
> COLD segments based on various conditions in __get_segment_type_6():
> - age extent cache (if enabled)
> - FI_HOT_DATA flag (set when dirty_pages <= min_hot_blocks)
> - rw_hint (defaults to WARM via f2fs_rw_hint_to_seg_type)
> - file_is_hot(), FI_NEED_IPU, f2fs_is_cow_file(), etc.
> 
> However, has_curseg_enough_space() only checked CURSEG_HOT_DATA segment
> for dentry blocks, which could lead to incorrect space calculation when
> dentry blocks are actually allocated to WARM or COLD segments.
> 
> Reproducer:
> Note: This reproducer requires adding a tracepoint to observe segment
> type. Add the following tracepoint to include/trace/events/f2fs.h:
> 
> TRACE_EVENT(f2fs_allocate_data_block,
>         TP_PROTO(struct f2fs_sb_info *sbi, struct inode *inode,
>                 enum log_type type, block_t blkaddr),
> 
>         TP_ARGS(sbi, inode, type, blkaddr),
> 
>         TP_STRUCT__entry(
>                 __field(dev_t, dev)
>                 __field(ino_t, ino)
>                 __field(int, type)
>                 __field(block_t, blkaddr)
>                 __field(int, is_dir)
>         ),
> 
>         TP_fast_assign(
>                 __entry->dev = sbi->sb->s_dev;
>                 __entry->ino = inode ? inode->i_ino : 0;
>                 __entry->type = type;
>                 __entry->blkaddr = blkaddr;
>                 __entry->is_dir = inode ? S_ISDIR(inode->i_mode) : 0;
>         ),
> 
>         TP_printk("dev = (%d,%d), ino = %lu, %s, blkaddr = %u, is_dir = %d",
>                 show_dev(__entry->dev),
>                 (unsigned long)__entry->ino,
>                 show_data_type(__entry->type),
>                 __entry->blkaddr,
>                 __entry->is_dir)
> );
> 
> And add the tracepoint call in fs/f2fs/segment.c in
> f2fs_allocate_data_block() function. Find the location after
> locate_dirty_segment() calls and before IS_DATASEG() check:
> 
>         locate_dirty_segment(sbi, GET_SEGNO(sbi, old_blkaddr));
>         locate_dirty_segment(sbi, GET_SEGNO(sbi, *new_blkaddr));
> 
>         trace_f2fs_allocate_data_block(sbi, folio ? folio->mapping->host : NULL,
>                                         type, *new_blkaddr);
> 
>         if (IS_DATASEG(curseg->seg_type))
> 
> 1. Mount F2FS with active_logs=6 and age extent cache disabled:
>    # mkfs.f2fs -f /dev/sdb1
>    # mount -t f2fs -o active_logs=6 /dev/sdb1 /mnt/f2fs-test
> 
> 2. Enable tracing and f2fs_allocate_data_block tracepoint:
>    # echo 1 > /sys/kernel/debug/tracing/events/f2fs/f2fs_allocate_data_block/enable
>    # echo 1 > /sys/kernel/debug/tracing/tracing_on
>    # echo > /sys/kernel/debug/tracing/trace
> 
> 3. Create a directory and write enough files to trigger dirty_pages >
>    min_hot_blocks (default 16), which will clear FI_HOT_DATA flag:
>    # mkdir /mnt/f2fs-test/testdir
>    # cd /mnt/f2fs-test/testdir
>    # seq 1 8192 | xargs touch
>    # sync
> 
> 4. Observe dentry block allocation:
>    # cat /sys/kernel/debug/tracing/trace
> 
>    The trace output shows dentry blocks (is_dir = 1) allocated to WARM
>    segment because FI_HOT_DATA is cleared when dirty_pages >
>    min_hot_blocks (default 16). However, has_curseg_enough_space() only
>    checked HOT_DATA segment space.
> 
> Fix by merging the dentry block check into the main data/node block
> check loop and checking data_blocks + dent_blocks for data segments,
> since both regular data blocks and dentry blocks can be written to the
> same segment. When active_logs == 6, dentry blocks can be allocated to
> any of the three data segments (HOT, WARM, COLD), so all three segments
> need to account for dentry blocks. When active_logs != 6, dentry blocks
> are always allocated to HOT_DATA segment only, so only HOT_DATA segment
> needs to account for dentry blocks, while WARM and COLD segments only
> check data_blocks.
> 
> Fixes: ef095d19e82f ("f2fs: write small sized IO to hot log")
> Cc: stable@kernel.org
> Signed-off-by: Xiaole He <hexiaole1994@126.com>
> ---
> Changes in v2 (per Yongpeng's feedback):
> - Merged dentry block check into the main loop to avoid duplication
> - Check data_blocks + dent_blocks for data segments (both can write to same segment)
> ---
>  fs/f2fs/segment.h | 24 ++++++++++++------------
>  1 file changed, 12 insertions(+), 12 deletions(-)
> 
> diff --git a/fs/f2fs/segment.h b/fs/f2fs/segment.h
> index 1ce2c8abaf48..acda720a54eb 100644
> --- a/fs/f2fs/segment.h
> +++ b/fs/f2fs/segment.h
> @@ -626,21 +626,21 @@ static inline bool has_curseg_enough_space(struct f2fs_sb_info *sbi,
>  
>  		left_blocks = get_left_section_blocks(sbi, i, segno);
>  
> -		blocks = i <= CURSEG_COLD_DATA ? data_blocks : node_blocks;
> +		if (i <= CURSEG_COLD_DATA) {
> +			blocks = data_blocks;
> +			/*
> +			 * With active_logs == 6, dentry blocks can be allocated to
> +			 * any data segment. With active_logs != 6, dentry blocks
> +			 * are always allocated to HOT_DATA segment.
> +			 */
> +			if ((F2FS_OPTION(sbi).active_logs == 6) || (i == CURSEG_HOT_DATA))
> +				blocks += dent_blocks;
> +		} else {
> +			blocks = node_blocks;
> +		}
>  		if (blocks > left_blocks)
>  			return false;
>  	}
> -
> -	/* check current data section for dentry blocks. */
> -	segno = CURSEG_I(sbi, CURSEG_HOT_DATA)->segno;
> -
> -	if (unlikely(segno == NULL_SEGNO))
> -		return false;
> -
> -	left_blocks = get_left_section_blocks(sbi, CURSEG_HOT_DATA, segno);
> -
> -	if (dent_blocks > left_blocks)
> -		return false;
>  	return true;
>  }
>  
> -- 
> 2.43.0


_______________________________________________
Linux-f2fs-devel mailing list
Linux-f2fs-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel

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

end of thread, other threads:[~2025-12-02 18:00 UTC | newest]

Thread overview: 8+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2025-11-10 13:26 [f2fs-dev] [PATCH v1] f2fs: fix has_curseg_enough_space to check all data segments for dentry blocks Xiaole He
2025-11-11  2:44 ` Yongpeng Yang
2025-11-11  6:00   ` [f2fs-dev] [PATCH v2] " Xiaole He
2025-11-11  6:10     ` Xiaole He
2025-11-11  6:29       ` Yongpeng Yang
2025-11-11  7:17       ` Chao Yu via Linux-f2fs-devel
2025-11-24 18:50       ` patchwork-bot+f2fs--- via Linux-f2fs-devel
2025-12-02 18:00       ` Jaegeuk Kim via Linux-f2fs-devel

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).