[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <ZShUmLU3X5QMiWQH@google.com>
Date: Thu, 12 Oct 2023 13:18:32 -0700
From: Joel Becker <jlbec@...lplan.org>
To: Seamus Connor <sconnor@...estorage.com>
Cc: Christoph Hellwig <hch@....de>, linux-kernel@...r.kernel.org
Subject: Re: [PATCH] [WIP] configfs: improve item creation performance
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@...lplan.org
Powered by blists - more mailing lists