Received-SPF: pass (google.com: domain of linux-kernel+bounces-70145-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.48.161 as permitted sender) client-ip=147.75.48.161;
Date: Sun, 18 Feb 2024 10:02:37 +0800
From: Oliver Sang <oliver.sang@intel.com>
To: Chuck Lever <chuck.lever@oracle.com>
CC: Jan Kara <jack@suse.cz>, Chuck Lever <cel@kernel.org>,
	<viro@zeniv.linux.org.uk>, <brauner@kernel.org>, <hughd@google.com>,
	<akpm@linux-foundation.org>, <Liam.Howlett@oracle.com>,
	<feng.tang@intel.com>, <linux-kernel@vger.kernel.org>,
	<linux-fsdevel@vger.kernel.org>, <maple-tree@lists.infradead.org>,
	<linux-mm@kvack.org>, <lkp@intel.com>, <oliver.sang@intel.com>
Subject: Re: [PATCH RFC 6/7] libfs: Convert simple directory offsets to use a
 Maple Tree
Message-ID: <ZdFlPbvexMir0WZO@xsang-OptiPlex-9020>
References: <170785993027.11135.8830043889278631735.stgit@91.116.238.104.host.secureserver.net>
 <170786028128.11135.4581426129369576567.stgit@91.116.238.104.host.secureserver.net>
 <20240215130601.vmafdab57mqbaxrf@quack3>
 <Zc4VfZ4/ejBEOt6s@tissot.1015granger.net>
Content-Type: text/plain; charset="us-ascii"
Content-Disposition: inline
In-Reply-To: <Zc4VfZ4/ejBEOt6s@tissot.1015granger.net>
Precedence: bulk
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?us-ascii?Q?uncXpx3V+8XWrw2agoqEp8ULKq8A47SzIFKkir6JeonS2Gu0b1i4gbB1sVaS?=
 =?us-ascii?Q?1sJ69khxInbqIQdzW61JsNw1Z8csbahb5xYV+NO345Cmp7Ur8Qw2XzI956jf?=
 =?us-ascii?Q?Y01lTfWxxsWaEEzVWhaVsbV1FPRn7bN/XQ1Q6HSdWkcWg+ChjzCmEP4u0Obq?=
 =?us-ascii?Q?wK+PeLF9wvPYOyRPH8k3PXlL7FWgTrP8XJOEsl94E2RxCPAg92pmFaF1+trx?=
 =?us-ascii?Q?veWKM1AcBXJKM3XuLXdsyS5uVCd36BYEwxBE61996qm+FB8mBwRpH1mMtePB?=
 =?us-ascii?Q?V1rDRDLxRF23uRklcIrERoAVc71eOU06PIz6PoenkZDonXO7gLNQ3LW7iLO1?=
 =?us-ascii?Q?JrRvIEvOMauHY2o9EFyzfwdbLh1NzjO2iWCZuZuOATz17EqhO8KzAzMedRKV?=
 =?us-ascii?Q?maCTYywS5OwPT99I0Z0YPLXaMUZSVSIdcB6O0eX2OMLFmaraSoaocX+DKNcA?=
 =?us-ascii?Q?MFd4xfjJ1/1Cxoph0WqrS/4bS/DjwmpUIrS35vcboVMzpN/YYQdffBDULsj/?=
 =?us-ascii?Q?bkMahSjd6EkmVCqD1WyzW04RhghlEwAsUMWrdbnV0PPe0qHHFjlpvu6UPl0d?=
 =?us-ascii?Q?Y2IJS5PgQ9nUayC5BA6vspw6+CLosUcDyoZelTomFe4ZvyiCC7xEGvCaZQ1J?=
 =?us-ascii?Q?+vyg63N0fZcL2deALIOOlbd7X+vua9Vf0wLMVB3RMBKRhoZeHSJey7OW08cO?=
 =?us-ascii?Q?yTIDAU+MuU7C8LKBz0FOlls44eZP1aKsKOsWsF5ANYVm3zAD2nWEpO5c/W/L?=
 =?us-ascii?Q?q+V2ux7igYoljeKf85xNRJuxa56uCs3AeLnrYWWgIBXpnnfxXrCLxSAIfVp9?=
 =?us-ascii?Q?sFuXlZhYAMLfEEHtB/GZdpycNWfDKK7g4+G9ERNttqe5QgCm8pydBhqhyRki?=
 =?us-ascii?Q?pNbQUgmJhIP3chKiZmwXpeqdVpnh+uWiSTfd0BjMv/t13t05yEjKE9Oov5kc?=
 =?us-ascii?Q?beRW3E/RVpopzW7ZiWEjBm4DLfBi1tFWcPK7k+sJE/W8C3vN/u4BdsBcnN0Q?=
 =?us-ascii?Q?zGSDZvuSQmlbOBA+fEEBBO28hY5xIYnJSC5dPfhZq7ZTB+Fcq0yIJTl230t4?=
 =?us-ascii?Q?oT7ksjyRrLcxyPO/bdis0xiSBaxPBVIqSst0DkGkDZ72abhAPiNoNAGGv54j?=
 =?us-ascii?Q?5zxBRcGJdwH23mK2D0PVIq1fXUJn6QEHn4BkcQXmeavLLxeP79wTIK5QU551?=
 =?us-ascii?Q?OX//AzMdv9kO5OEx4jUHtTggUm/mb1keRVS+9ur/xOfpIuJ21gJxlhvpvStz?=
 =?us-ascii?Q?H+DHNa5UHKTN7l3S8mi2lFGukurnpP0qcQOuw7Fbp/vTi3EAxx1j7afh3oex?=
 =?us-ascii?Q?XS8p97veJPDN9uwWf+LGtQHn67FdrnpEUJ4jdWY/T50GM4BuWPNyEQfariUW?=
 =?us-ascii?Q?zZDnic4EpZbSn2+zozbe8/FjxNwGKBHlvGrQSQ3IWPKpSvErYHP7mBxItHrK?=
 =?us-ascii?Q?FRSiT0LqqfuBIq1mE8HOglq3HucmWEC4KGm8ksJ81WpwNeBQzVgzq01zd2Sz?=
 =?us-ascii?Q?n+duzoRMmnSmas6e2a0/cf4DGz65Kr8jY1RYSYqJosmLgk324f+2r4nx1pO3?=
 =?us-ascii?Q?Z2aWBuKbYkTayUrWR15CQarAFWUzCvZCIwdLhjICY1EdrrkAhSTbkpbVSTmP?=
 =?us-ascii?Q?BA=3D=3D?=
X-MS-Exchange-CrossTenant-Network-Message-Id: d7c6e8db-c5f2-4de1-84a6-08dc3025b401
X-MS-Exchange-CrossTenant-AuthSource: LV3PR11MB8603.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 18 Feb 2024 02:02:47.2598
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 46c98d88-e344-4ed4-8496-4ed7712e255d
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: vTVMbvyCQ0gHkXMtnVK05PZB8edGvWsedjGrIX+fG4Vs0Pa8LisRSNC4URNZisuZ8/u5dteM4XZ8f/VX7GfO7g==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: SJ0PR11MB5868
X-OriginatorOrg: intel.com

hi, Chuck Lever,

On Thu, Feb 15, 2024 at 08:45:33AM -0500, Chuck Lever wrote:
> On Thu, Feb 15, 2024 at 02:06:01PM +0100, Jan Kara wrote:
> > On Tue 13-02-24 16:38:01, Chuck Lever wrote:
> > > From: Chuck Lever <chuck.lever@oracle.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@infradead.org>
> > > Reported-by: kernel test robot <oliver.sang@intel.com>
> > > Closes: https://lore.kernel.org/oe-lkp/202309081306.3ecb3734-oliver.sang@intel.com
> > > Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
> > 
> > OK, but this will need the performance numbers.
> 
> Yes, I totally concur. The point of this posting was to get some
> early review and start the ball rolling.
> 
> Actually we expect roughly the same performance numbers now. "Dense
> node" support in Maple Tree is supposed to be the real win, but
> I'm not sure it's ready yet.
> 
> 
> > 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.
> 
> Oliver and Feng were copied on this series.

we are in holidays last week, now we are back.

I noticed there is v2 for this patch set
https://lore.kernel.org/all/170820145616.6328.12620992971699079156.stgit@91.116.238.104.host.secureserver.net/

and you also put it in a branch:
https://git.kernel.org/pub/scm/linux/kernel/git/cel/linux.git
"simple-offset-maple" branch.

we will test aim9 performance based on this branch. Thanks

> 
> 
> > > @@ -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?
> 
> I initially changed U32_MAX to ULONG_MAX, but for some reason, the
> length checking in vfs_setpos() fails. There is probably a sign
> extension thing happening here that I don't understand.
> 
> 
> > Otherwise the patch looks good to me.
> 
> As always, thank you for your review.
> 
> 
> -- 
> Chuck Lever