Re: [PATCH 24/29] sysctl: Replace root_list with links between sysctl_table_sets.

[Lucian Adrian Grijincu <lucian.grijincu@gmail.com>]

Very nice way to handle netns specific files with links between sets!

You did a much better job than I did at dealing with them.

It took me a while to understand how the code works. I'll try to write
something for Documentation/ because the inner workings are a bit
intertwined.

A few comments bellow.

On Fri, Jan 27, 2012 at 6:52 AM, Eric W. Biederman
<ebiederm@xmission.com> wrote:
> Piecing together directories by looking first in one directory
> tree, than in another directory tree and finally in a third

than -> then

> directory tree makes it hard to verify that some directory
> entries are not multiply defined and makes it hard to create
> efficient implementations the sysctl filesystem.
>
> Replace the sysctl wide list of roots with autogenerated
> links from the core sysctl directory tree to the other
> sysctl directory trees.
>
> This simplifies sysctl directory reading and lookups as now
> only entries in a single sysctl directory tree need to be
> considered.
>
> Benchmark before:
>    make-dummies 0 999 -> 0.44s
>    rmmod dummy        -> 0.065s
>    make-dummies 0 9999 -> 1m36s
>    rmmod dummy         -> 0.4s
>
> Benchmark after:
>    make-dummies 0 999 -> 0.63s
>    rmmod dummy        -> 0.12s
>    make-dummies 0 9999 -> 2m35s
>    rmmod dummy         -> 18s
>
> The slowdown is caused by the lookups used in insert_headers

insert_headers -> insert_header

> and put_links to see if we need to add links or remove links.




>  void register_sysctl_root(struct ctl_table_root *root)
>  {
> -       spin_lock(&sysctl_lock);
> -       list_add_tail(&root->root_list, &sysctl_table_root.root_list);
> -       spin_unlock(&sysctl_lock);
>  }


This function is empty. Can be deleted (there are two callers in
net/sysctl_net.c.


> @@ -400,6 +373,7 @@ static struct dentry *proc_sys_lookup(struct inode *dir, struct dentry *dentry,
>        struct inode *inode;
>        struct dentry *err = ERR_PTR(-ENOENT);
>        struct ctl_dir *ctl_dir;
> +       int ret;
>
>        if (IS_ERR(head))
>                return ERR_CAST(head);
> @@ -410,6 +384,11 @@ static struct dentry *proc_sys_lookup(struct inode *dir, struct dentry *dentry,
>        if (!p)
>                goto out;
>


"sysctl_follow_link" implies that it will follow the link. I would
pull out the check whether the header is a link or not. This wouldn't
save much (a function call), but it would make the code easier to
read:

	/* Get out quickly if not a link */
        if (S_ISLNK(p->mode)) {
		ret = sysctl_follow_link(&h, &p, current->nsproxy);
		err = ERR_PTR(ret);
		if (ret)
		        goto out;
        }



> +       ret = sysctl_follow_link(&h, &p, current->nsproxy);
> +       err = ERR_PTR(ret);
> +       if (ret)
> +               goto out;
> +
>        err = ERR_PTR(-ENOMEM);
>        inode = proc_sys_make_inode(dir->i_sb, h ? h : head, p);
>        if (h)
> @@ -547,6 +526,25 @@ static int proc_sys_fill_cache(struct file *filp, void *dirent,
>        return !!filldir(dirent, qname.name, qname.len, filp->f_pos, ino, type);
>  }
>
> +static int proc_sys_link_fill_cache(struct file *filp, void *dirent,
> +                                   filldir_t filldir,
> +                                   struct ctl_table_header *head,
> +                                   struct ctl_table *table)
> +{
> +       int err, ret = 0;
> +       head = sysctl_head_grab(head);
> +
> +       /* It is not an error if we can not follow the link ignore it */
> +       err = sysctl_follow_link(&head, &table, current->nsproxy);

similar

> +       if (err)
> +               goto out;
> +
> +       ret = proc_sys_fill_cache(filp, dirent, filldir, head, table);
> +out:
> +       sysctl_head_finish(head);
> +       return ret;
> +}
> +




> -static struct ctl_dir *get_subdir(struct ctl_table_set *set,
> -       struct ctl_dir *dir, const char *name, int namelen)
> +static struct ctl_dir *get_subdir(struct ctl_dir *dir,
> +                                 const char *name, int namelen)
>  {
> +       struct ctl_table_set *set = dir->header.set;
>        struct ctl_dir *subdir, *new = NULL;
>
>        spin_lock(&sysctl_lock);
> -       subdir = find_subdir(dir->header.set, dir, name, namelen);
> -       if (!IS_ERR(subdir))
> -               goto found;
> -       if ((PTR_ERR(subdir) == -ENOENT) && set != dir->header.set)
> -               subdir = find_subdir(set, dir, name, namelen);
> +       subdir = find_subdir(dir, name, namelen);
>        if (!IS_ERR(subdir))
>                goto found;
>        if (PTR_ERR(subdir) != -ENOENT)
> @@ -817,13 +815,14 @@ static struct ctl_dir *get_subdir(struct ctl_table_set *set,
>        if (!new)
>                goto failed;
>
> -       subdir = find_subdir(set, dir, name, namelen);
> +       subdir = find_subdir(dir, name, namelen);
>        if (!IS_ERR(subdir))
>                goto found;
>        if (PTR_ERR(subdir) != -ENOENT)
>                goto failed;

I think you're returning the wrong error here. If we got to this point
then subdir == ERR_PTR(-ENOENT).
We want to create a new dir here even if one doesn't exist.
So if we have an error in insert_header() we don't return that error,
but ENOENT.

>
> -       insert_header(dir, &new->header);
> +       if (insert_header(dir, &new->header))
> +               goto failed;
>        subdir = new;

Something like this would fix it:

        if (subdir = insert_header(dir, &new->header))



>  found:
>        subdir->header.nreg++;
> @@ -841,6 +840,57 @@ failed:
>        return subdir;
>  }
>




-- 
 .
..: Lucian