Messages in this thread | | | Date | Thu, 12 Oct 2023 13:18:32 -0700 | From | Joel Becker <> | 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@evilplan.org
| |