Re: [PATCH] ext4: isolate s_orphan_lock from read-mostly fields

[Jan Kara]

On Thu 03-09-26 10:18:35, JonasZhou-oc wrote:
> From: Jonas Zhou <jonaszhou@xxxxxxxxxxx>
> 
> s_orphan_lock is modified by orphan-list operations, but currently shares
> a cache line with s_journal and s_ext4_flags.  The latter fields are read
> from common ext4 paths, so unrelated accesses can cause the line containing
> the lock to bounce between CPUs.
> 
> On a two-socket Intel Xeon Silver 4208 system with 16 workers, throughput
> improved by 4.6% for stress-ng.dnotify and 1.7% for stress-ng.unlink.  Perf
> c2c showed that HITM events on the affected fields decreased from 3,699 to
> 3,125 (-15.5%) for stress-ng.dnotify and from 1,368 to 571 (-58.3%) for
> stress-ng.unlink.  Remote HITM events decreased by 45.1% and 65.5%,
> respectively.  The journal/flags line had no remote HITM events after the
> change; the remaining remote HITM events were on the orphan-lock line.
> 
> Signed-off-by: Jonas Zhou <jonaszhou@xxxxxxxxxxx>

Ok, that makes some sense. But I have some comments:

1) If you used orphan_file filesystem feature, you'd see much larger wins
(as the orphan lock would get completely out of the picture).

2) You could just place orphan handling related entries to some other place
in the sb which is not so frequently read. That would reduce the contention
as well without wasting space for padding in struct ext4_sb_info.

3) Your patch drops a comment at s_orphan_lock.

In particular given point 1) above, I'm not sure this is really worth it -
if you care about orphan handling scalability, you should just enable
orphan_file feature.

								Honza

> ---
>  fs/ext4/ext4.h | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/fs/ext4/ext4.h b/fs/ext4/ext4.h
> index 724a27e8be61..a3f16a981664 100644
> --- a/fs/ext4/ext4.h
> +++ b/fs/ext4/ext4.h
> @@ -1644,7 +1644,7 @@ struct ext4_sb_info {
>  	/* 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 mutex s_orphan_lock ____cacheline_aligned_in_smp;
>  	struct list_head s_orphan;	/* List of orphaned inodes in on disk
>  					   list */
>  	struct ext4_orphan_info s_orphan_info;
> -- 
> 2.25.1
> 
> 
-- 
Jan Kara <jack@xxxxxxxx>
SUSE Labs, CR