Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.36 as permitted sender) client-ip=23.128.96.36;
Date:   Thu, 12 Oct 2023 13:18:32 -0700
From:   Joel Becker <jlbec@evilplan.org>
To:     Seamus Connor <sconnor@purestorage.com>
Cc:     Christoph Hellwig <hch@lst.de>, linux-kernel@vger.kernel.org
Subject: Re: [PATCH] [WIP] configfs: improve item creation performance
Message-ID: <ZShUmLU3X5QMiWQH@google.com>
Mail-Followup-To: Seamus Connor <sconnor@purestorage.com>,
        Christoph Hellwig <hch@lst.de>, linux-kernel@vger.kernel.org
References: <20231011213919.52267-1-sconnor@purestorage.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20231011213919.52267-1-sconnor@purestorage.com>
Sender: Joel Becker <jlbec@ftp.linux.org.uk>
Precedence: bulk

On Wed, Oct 11, 2023 at 02:39:19PM -0700, Seamus Connor wrote:
> On my machine, creating 40,000 Items in a single directory takes roughly
> 40 seconds. With this patch applied, that time drops down to around 130
> ms.

Nice.

> @@ -207,7 +212,10 @@ static struct configfs_dirent *configfs_new_dirent(struct configfs_dirent *paren
>  		return ERR_PTR(-ENOENT);
>  	}
>  	sd->s_frag = get_fragment(frag);
> -	list_add(&sd->s_sibling, &parent_sd->s_children);
> +	if (configfs_dirent_is_pinned(sd))
> +		list_add_tail(&sd->s_sibling, &parent_sd->s_children);
> +	else
> +		list_add(&sd->s_sibling, &parent_sd->s_children);
>  	spin_unlock(&configfs_dirent_lock);

This is subtle.  Your patch description of course describes why we are
partitioning the items and attributes, but that will get lost into the
memory hole very quickly.  Please add a comment.

> @@ -449,6 +454,10 @@ static struct dentry * configfs_lookup(struct inode *dir,
>  
>  	spin_lock(&configfs_dirent_lock);
>  	list_for_each_entry(sd, &parent_sd->s_children, s_sibling) {
> +
> +		if (configfs_dirent_is_pinned(sd))
> +			break;
> +
>  		if ((sd->s_type & CONFIGFS_NOT_PINNED) &&
>  		    !strcmp(configfs_get_name(sd), dentry->d_name.name)) {
>  			struct configfs_attribute *attr = sd->s_element;

There's a lack of symmetry here.  The pinned check is an inline
function, whereas the `CONFIGFS_NOT_PINNED` check is an open-coded
bitmask.  Why not just:

```
		if (sd->s_type & CONFIGFS_IS_PINNED)
			break;
```

Plus, aren't the pinned/not-pinned checks redundant?  Can't we avoid the
extra conditional?


```
	spin_lock(&configfs_dirent_lock);
	list_for_each_entry(sd, &parent_sd->s_children, s_sibling) {
-		if ((sd->s_type & CONFIGFS_NOT_PINNED) &&
-		    !strcmp(configfs_get_name(sd), dentry->d_name.name)) {
+		/*
+		 * The dirents for config_items are pinned in the
+		 * dcache, so configfs_lookup() should never be called
+		 * for items.  Thus, we're only looking up attributes.
+		 *
+		 * s_children is ordered so that attributes
+		 * (CONFIGFS_NOT_PINNED) come before items (see
+		 * configfs_new_dirent().  If we have reached a child item,
+		 * we are done looking.
+		 */
+		if (!(sd->s_type & CONFIGFS_NOT_PINNED))
+			break;
+
+		if (!strcmp(configfs_get_name(sd), dentry->d_name.name)) {
			struct configfs_attribute *attr = sd->s_element;
			umode_t mode = (attr->ca_mode & S_IALLUGO) | S_IFREG;
```

> -void configfs_hash_and_remove(struct dentry * dir, const char * name)
> -{
> -	struct configfs_dirent * sd;
> -	struct configfs_dirent * parent_sd = dir->d_fsdata;

Man, I thought we removed this years ago:
https://lkml.indiana.edu/hypermail/linux/kernel/0803.0/0905.html.  No
idea why that patch didn't land.

Thanks,
Joel

-- 

Life's Little Instruction Book #222

	"Think twice before burdening a friend with a secret."

			http://www.jlbec.org/
			jlbec@evilplan.org