Re: [PATCH 2/2] kernfs: Allow creation with external gen + ino numbers

[Namhyung Kim]

On Fri, Oct 25, 2019 at 2:52 AM Tejun Heo <tj@xxxxxxxxxx> wrote:
>
> On Wed, Oct 16, 2019 at 09:50:19PM +0900, Namhyung Kim wrote:
> > diff --git a/arch/x86/kernel/cpu/resctrl/rdtgroup.c b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
> > diff --git a/fs/kernfs/dir.c b/fs/kernfs/dir.c
> > index 6ebae6bbe6a5..f2e54532c110 100644
> > --- a/fs/kernfs/dir.c
> > +++ b/fs/kernfs/dir.c
> > @@ -618,10 +618,10 @@ static struct kernfs_node *__kernfs_new_node(struct kernfs_root *root,
> >                                            struct kernfs_node *parent,
> >                                            const char *name, umode_t mode,
> >                                            kuid_t uid, kgid_t gid,
> > +                                          u32 gen, int ino,
>
> Shouldn't this be ino_t so that we can use 64bit uniq id as ino
> directly where possible?  Also, it might make more sense if ino comes
> before gen.

Will change the order.

>
> > @@ -635,11 +635,24 @@ static struct kernfs_node *__kernfs_new_node(struct kernfs_root *root,
> >
> >       idr_preload(GFP_KERNEL);
> >       spin_lock(&kernfs_idr_lock);
> > -     cursor = idr_get_cursor(&root->ino_idr);
> > -     ret = idr_alloc_cyclic(&root->ino_idr, kn, 1, 0, GFP_ATOMIC);
> > -     if (ret >= 0 && ret < cursor)
> > -             root->next_generation++;
> > -     gen = root->next_generation;
> > +
> > +     if (ino == 0) {
> > +             cursor = idr_get_cursor(&root->ino_idr);
> > +             ret = idr_alloc_cyclic(&root->ino_idr, kn, 1, 0, GFP_ATOMIC);
> > +             if (ret >= 0 && ret < cursor)
> > +                     root->next_generation++;
> > +             gen = root->next_generation;
> > +     } else {
> > +             ret = idr_alloc(&root->ino_idr, kn, ino, ino + 1, GFP_ATOMIC);
> > +             if (ret != ino) {
> > +                     WARN_ONCE(1, "kernfs ino was used: %d", ino);
> > +                     ret = -EINVAL;
> > +             } else {
> > +                     WARN_ON(root->next_generation > gen);
> > +                     root->next_generation = gen;
> > +             }
> > +     }
>
> Oh, I see, so the code is still depending on idr and thus the use of
> 32bit ino.  Hmm....

Right.

>
> >  static inline struct kernfs_node *
> > diff --git a/kernel/cgroup/cgroup.c b/kernel/cgroup/cgroup.c
> > index 44c67d26c1fe..13d0d181a9f8 100644
> > --- a/kernel/cgroup/cgroup.c
> > +++ b/kernel/cgroup/cgroup.c
> > @@ -3916,16 +3916,22 @@ static int cgroup_add_file(struct cgroup_subsys_state *css, struct cgroup *cgrp,
> >       char name[CGROUP_FILE_NAME_MAX];
> >       struct kernfs_node *kn;
> >       struct lock_class_key *key = NULL;
> > +     struct cgroup_root *root = cgrp->root;
> > +     int ino, gen;
> >       int ret;
> >
> >  #ifdef CONFIG_DEBUG_LOCK_ALLOC
> >       key = &cft->lockdep_key;
> >  #endif
> > +
> > +     ino = cgroup_idr_alloc(&root->cgroup_idr, NULL, false, GFP_KERNEL);
> > +     gen = root->cgroup_idr.generation;
> > +
> >       kn = __kernfs_create_file(cgrp->kn, cgroup_file_name(cgrp, cft, name),
> >                                 cgroup_file_mode(cft),
> >                                 GLOBAL_ROOT_UID, GLOBAL_ROOT_GID,
> >                                 0, cft->kf_ops, cft,
> > -                               NULL, key);
> > +                               NULL, key, gen, ino);
>
> Can we move this to a separate patch so that this patch can be an
> identity conversion?

Will do.

Thanks
Namhyung