linux-kernel - Re: [PATCH RFC 6/7] libfs: Convert simple directory offsets to use a Maple Tree

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20240215130601.vmafdab57mqbaxrf@quack3>
Date: Thu, 15 Feb 2024 14:06:01 +0100
From: Jan Kara <jack@...e.cz>
To: Chuck Lever <cel@...nel.org>
Cc: viro@...iv.linux.org.uk, brauner@...nel.org, jack@...e.cz,
	hughd@...gle.com, akpm@...ux-foundation.org,
	Liam.Howlett@...cle.com, oliver.sang@...el.com, feng.tang@...el.com,
	linux-kernel@...r.kernel.org, linux-fsdevel@...r.kernel.org,
	maple-tree@...ts.infradead.org, linux-mm@...ck.org, lkp@...el.com
Subject: Re: [PATCH RFC 6/7] libfs: Convert simple directory offsets to use a
 Maple Tree

On Tue 13-02-24 16:38:01, Chuck Lever wrote:
> From: Chuck Lever <chuck.lever@...cle.com>
> 
> Test robot reports:
> > kernel test robot noticed a -19.0% regression of aim9.disk_src.ops_per_sec on:
> >
> > commit: a2e459555c5f9da3e619b7e47a63f98574dc75f1 ("shmem: stable directory offsets")
> > https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git master
> 
> Feng Tang further clarifies that:
> > ... the new simple_offset_add()
> > called by shmem_mknod() brings extra cost related with slab,
> > specifically the 'radix_tree_node', which cause the regression.
> 
> Willy's analysis is that, over time, the test workload causes
> xa_alloc_cyclic() to fragment the underlying SLAB cache.
> 
> This patch replaces the offset_ctx's xarray with a Maple Tree in the
> hope that Maple Tree's dense node mode will handle this scenario
> more scalably.
> 
> In addition, we can widen the directory offset to an unsigned long
> everywhere.
> 
> Suggested-by: Matthew Wilcox <willy@...radead.org>
> Reported-by: kernel test robot <oliver.sang@...el.com>
> Closes: https://lore.kernel.org/oe-lkp/202309081306.3ecb3734-oliver.sang@intel.com
> Signed-off-by: Chuck Lever <chuck.lever@...cle.com>

OK, but this will need the performance numbers. Otherwise we have no idea
whether this is worth it or not. Maybe you can ask Oliver Sang? Usually
0-day guys are quite helpful.

> @@ -330,9 +329,9 @@ int simple_offset_empty(struct dentry *dentry)
>  	if (!inode || !S_ISDIR(inode->i_mode))
>  		return ret;
>  
> -	index = 2;
> +	index = DIR_OFFSET_MIN;

This bit should go into the simple_offset_empty() patch...

> @@ -434,15 +433,15 @@ static loff_t offset_dir_llseek(struct file *file, loff_t offset, int whence)
>  
>  	/* In this case, ->private_data is protected by f_pos_lock */
>  	file->private_data = NULL;
> -	return vfs_setpos(file, offset, U32_MAX);
> +	return vfs_setpos(file, offset, MAX_LFS_FILESIZE);
					^^^
Why this? It is ULONG_MAX << PAGE_SHIFT on 32-bit so that doesn't seem
quite right? Why not use ULONG_MAX here directly?

Otherwise the patch looks good to me.

								Honza
-- 
Jan Kara <jack@...e.com>
SUSE Labs, CR