Messages in this thread | | | Date | Sat, 14 Oct 2023 18:09:28 -0700 | From | Joel Becker <> | Subject | Re: [PATCH v2] configfs: improve item creation performance |
| |
On Fri, Oct 13, 2023 at 02:11:29PM -0700, Seamus Connor wrote: > As the size of a directory increases item creation slows down. > Optimizing access to s_children removes this bottleneck. > > dirents are already pinned into the cache, there is no need to scan the > s_children list looking for duplicate Items. The configfs_dirent_exists > check is moved to a location where it is called only during subsystem > initialization. > > d_lookup will only need to call configfs_lookup in the case where the > item in question is not pinned to dcache. The only items not pinned to > dcache are attributes. These are placed at the front of the s_children > list, whilst pinned items are inserted at the back. configfs_lookup > stops scanning when it encounters the first pinned entry in s_children. > > The assumption of the above optimizations is that there will be few > attributes, but potentially many Items in a given directory. > > Signed-off-by: Seamus Connor <sconnor@purestorage.com> > Cc: Joel Becker <jlbec@evilplan.org> > Cc: Christoph Hellwig <hch@lst.de> > Cc: linux-kernel@vger.kernel.org Reviewed-by: Joel Becker <jlbec@evilplan.org>
> --- > > In this revision I've added some commentary describing the changes, and > I have removed a helper function. > > fs/configfs/configfs_internal.h | 3 +-- > fs/configfs/dir.c | 39 +++++++++++++++++++++++++-------- > fs/configfs/inode.c | 24 -------------------- > 3 files changed, 31 insertions(+), 35 deletions(-) > > diff --git a/fs/configfs/configfs_internal.h b/fs/configfs/configfs_internal.h > index c0395363eab9..b91036fd71b1 100644 > --- a/fs/configfs/configfs_internal.h > +++ b/fs/configfs/configfs_internal.h > @@ -55,6 +55,7 @@ struct configfs_dirent { > #define CONFIGFS_USET_IN_MKDIR 0x0200 > #define CONFIGFS_USET_CREATING 0x0400 > #define CONFIGFS_NOT_PINNED (CONFIGFS_ITEM_ATTR | CONFIGFS_ITEM_BIN_ATTR) > +#define CONFIGFS_PINNED (CONFIGFS_ROOT | CONFIGFS_DIR | CONFIGFS_ITEM_LINK) > > extern struct mutex configfs_symlink_mutex; > extern spinlock_t configfs_dirent_lock; > @@ -73,8 +74,6 @@ extern int configfs_make_dirent(struct configfs_dirent *, struct dentry *, > void *, umode_t, int, struct configfs_fragment *); > extern int configfs_dirent_is_ready(struct configfs_dirent *); > > -extern void configfs_hash_and_remove(struct dentry * dir, const char * name); > - > extern const unsigned char * configfs_get_name(struct configfs_dirent *sd); > extern void configfs_drop_dentry(struct configfs_dirent *sd, struct dentry *parent); > extern int configfs_setattr(struct user_namespace *mnt_userns, > diff --git a/fs/configfs/dir.c b/fs/configfs/dir.c > index d1f9d2632202..64a0eac83b90 100644 > --- a/fs/configfs/dir.c > +++ b/fs/configfs/dir.c > @@ -207,7 +207,17 @@ 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); > + > + /* > + * configfs_lookup scans only for unpinned items. s_children is > + * partitioned so that configfs_lookup can bail out early. > + * CONFIGFS_PINNED and CONFIGFS_NOT_PINNED are not symmetrical. readdir > + * cursors still need to be inserted at the front of the list. > + */ > + if (sd->s_type & CONFIGFS_PINNED) > + 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); > > return sd; > @@ -220,10 +230,11 @@ static struct configfs_dirent *configfs_new_dirent(struct configfs_dirent *paren > * > * called with parent inode's i_mutex held > */ > -static int configfs_dirent_exists(struct configfs_dirent *parent_sd, > - const unsigned char *new) > +static int configfs_dirent_exists(struct dentry *dentry) > { > - struct configfs_dirent * sd; > + struct configfs_dirent *parent_sd = dentry->d_parent->d_fsdata; > + const unsigned char *new = dentry->d_name.name; > + struct configfs_dirent *sd; > > list_for_each_entry(sd, &parent_sd->s_children, s_sibling) { > if (sd->s_element) { > @@ -289,10 +300,6 @@ static int configfs_create_dir(struct config_item *item, struct dentry *dentry, > > BUG_ON(!item); > > - error = configfs_dirent_exists(p->d_fsdata, dentry->d_name.name); > - if (unlikely(error)) > - return error; > - > error = configfs_make_dirent(p->d_fsdata, dentry, item, mode, > CONFIGFS_DIR | CONFIGFS_USET_CREATING, > frag); > @@ -449,6 +456,18 @@ static struct dentry * configfs_lookup(struct inode *dir, > > spin_lock(&configfs_dirent_lock); > list_for_each_entry(sd, &parent_sd->s_children, s_sibling) { > + > + /* > + * s_children is partitioned, see configfs_new_dirent. The first > + * pinned item indicates we can stop scanning. > + */ > + if (sd->s_type & CONFIGFS_PINNED) > + break; > + > + /* > + * Note: CONFIGFS_PINNED and CONFIGFS_NOT_PINNED are asymmetric. > + * there may be a readdir cursor in this list > + */ > if ((sd->s_type & CONFIGFS_NOT_PINNED) && > !strcmp(configfs_get_name(sd), dentry->d_name.name)) { > struct configfs_attribute *attr = sd->s_element; > @@ -1878,7 +1897,9 @@ int configfs_register_subsystem(struct configfs_subsystem *subsys) > if (dentry) { > d_add(dentry, NULL); > > - err = configfs_attach_group(sd->s_element, &group->cg_item, > + err = configfs_dirent_exists(dentry); > + if (!err) > + err = configfs_attach_group(sd->s_element, &group->cg_item, > dentry, frag); > if (err) { > BUG_ON(d_inode(dentry)); > diff --git a/fs/configfs/inode.c b/fs/configfs/inode.c > index b601610e9907..15964e62329d 100644 > --- a/fs/configfs/inode.c > +++ b/fs/configfs/inode.c > @@ -218,27 +218,3 @@ void configfs_drop_dentry(struct configfs_dirent * sd, struct dentry * parent) > } > } > > -void configfs_hash_and_remove(struct dentry * dir, const char * name) > -{ > - struct configfs_dirent * sd; > - struct configfs_dirent * parent_sd = dir->d_fsdata; > - > - if (d_really_is_negative(dir)) > - /* no inode means this hasn't been made visible yet */ > - return; > - > - inode_lock(d_inode(dir)); > - list_for_each_entry(sd, &parent_sd->s_children, s_sibling) { > - if (!sd->s_element) > - continue; > - if (!strcmp(configfs_get_name(sd), name)) { > - spin_lock(&configfs_dirent_lock); > - list_del_init(&sd->s_sibling); > - spin_unlock(&configfs_dirent_lock); > - configfs_drop_dentry(sd, dir); > - configfs_put(sd); > - break; > - } > - } > - inode_unlock(d_inode(dir)); > -} > -- > 2.37.0 >
--
"Glory is fleeting, but obscurity is forever." - Napoleon Bonaparte
http://www.jlbec.org/ jlbec@evilplan.org
| |