Re: [PATCH v2] ext4: move journal state away from orphan list updates

[Jan Kara]

On Mon 07-09-26 19:12:42, JonasZhou-oc wrote:
> From: Jonas Zhou <jonaszhou@xxxxxxxxxxx>
> 
> In the tested x86-64 layout, s_journal and s_ext4_flags share a cache
> line with s_orphan_lock and s_orphan. Legacy orphan-list updates
> invalidate the same line used by unrelated journal and inode paths.
> 
> Exchange s_journal and s_ext4_flags with s_err_report_sec and
> s_li_request, which are used for error reporting and lazy-init
> management. This separates the common journal and flag readers from
> orphan-list writes without adding padding or shifting the intervening
> allocator fields. Preserve the comment describing s_orphan_lock.
> 
> Only these four member offsets change, and sizeof(struct ext4_sb_info)
> remains 2496 bytes with the tested x86-64 configuration. Compiled layout
> checks with quota disabled, lockdep, PREEMPT_RT and i386 also preserve
> the respective structure sizes and all other member offsets.
> 
> On a system with two Xeon Silver 4208 processors and a filesystem
> without orphan_file enabled, mean throughput improved by 13.52% for
> stress-ng dnotify and 31.70% for unlink.
> 
> This targets users of the legacy orphan list. The orphan_file feature
> avoids this lock in normal operation and remains the main scalability
> improvement. Tests with orphan_file enabled show no clear throughput
> change within the observed boot-to-boot variation.
> 
> Results from repeated tests comparing v7.3-rc1 and v2:
> 
>                        Mean throughput       Mean change
> Feature      Test          base         v2           (%)
> legacy       dnotify    19562.38   22206.66        +13.52
> legacy       unlink        38.82      51.13        +31.70
> legacy       fallocate    233.55     234.14         +0.25
> orphan_file  dnotify    27702.16   27582.55         -0.43
> orphan_file  unlink       116.93     116.52         -0.34
> orphan_file  fallocate    235.20     235.89         +0.29
> 
> Descriptive 95% t intervals from the four paired log ratios (df=3),
> expressed as percentage changes:
>   legacy:      dnotify [+4.83, +22.92], unlink [+22.53, +41.48],
>                fallocate [-1.40, +1.94]
>   orphan_file: dnotify [-5.44, +4.96], unlink [-2.76, +2.13],
>                fallocate [-0.82, +1.42]
> These are intervals for the paired geometric ratio, not the arithmetic
> mean ratio above. Four pairs do not establish performance equivalence.
> 
> For legacy dnotify, perf c2c shows remote HITM samples on the original
> journal/flags offsets, but none on their new cache line after the
> change. Local HITM samples and orphan-lock contention remain. The
> combined HITM sample density of the orphan and journal lines is lower
> than that of the original shared line.
> 
> Signed-off-by: Jonas Zhou <jonaszhou@xxxxxxxxxxx>

Sorry, but we should keep things logically belonging together close. I'd
keep s_journal & s_ext4_flags in the read-mostly part of the sb and perhaps
move orphan-related things (s_orphan_lock, s_orphan, s_orphan_info) later.
Perhaps below s_journal_triggers definition. And add a comment there like:

	/* Orphan inode tracking */

								Honza

> ---
> Changes since v1:
> - Replace explicit cache-line alignment with a four-member exchange.
> - Keep the mutex comment and all other member offsets unchanged.
> - Retest this layout against v7.3-rc1, including orphan_file controls.
> 
> v1:
> https://lore.kernel.org/all/20260903021836.962624-1-jonaszhou-oc@xxxxxxxxxxx/
> 
>  fs/ext4/ext4.h | 15 +++++++++------
>  1 file changed, 9 insertions(+), 6 deletions(-)
> 
> diff --git a/fs/ext4/ext4.h b/fs/ext4/ext4.h
> index 724a27e8be61..16ef01078233 100644
> --- a/fs/ext4/ext4.h
> +++ b/fs/ext4/ext4.h
> @@ -1641,9 +1641,13 @@ struct ext4_sb_info {
>  	struct super_block *s_sb;
>  	struct buffer_head *s_mmp_bh;
>  
> +	/* timeout in seconds for s_err_report; 0 disables the timer. */
> +	unsigned long s_err_report_sec;
> +
> +	/* Lazy inode table initialization info */
> +	struct ext4_li_request *s_li_request;
> +
>  	/* Journaling */
> -	struct journal_s *s_journal;
> -	unsigned long s_ext4_flags;		/* Ext4 superblock flags */
>  	struct mutex s_orphan_lock;	/* Protects on disk list changes */
>  	struct list_head s_orphan;	/* List of orphaned inodes in on disk
>  					   list */
> @@ -1747,11 +1751,10 @@ struct ext4_sb_info {
>  
>  	/* timer for periodic error stats printing */
>  	struct timer_list s_err_report;
> -	/* timeout in seconds for s_err_report; 0 disables the timer. */
> -	unsigned long s_err_report_sec;
> +	/* Keep journal state away from orphan list updates. */
> +	struct journal_s *s_journal;
> +	unsigned long s_ext4_flags;		/* Ext4 superblock flags */
>  
> -	/* Lazy inode table initialization info */
> -	struct ext4_li_request *s_li_request;
>  	/* Wait multiplier for lazy initialization thread */
>  	unsigned int s_li_wait_mult;
>  
> 
> base-commit: cee9395acd8043be0644b25c34bfa86623f2b935
> -- 
> 2.43.0
> 
> 
-- 
Jan Kara <jack@xxxxxxxx>
SUSE Labs, CR