Re: [PATCH 1/2 v3] eventfs: Remove eventfs_file and just use eventfs_inode
From: Google
Date: Tue Sep 19 2023 - 05:41:25 EST
On Mon, 18 Sep 2023 21:04:56 -0400
Steven Rostedt <rostedt@xxxxxxxxxxx> wrote:
> > > @@ -144,18 +129,18 @@ static struct dentry *create_dir(const char *name, struct dentry *parent, void *
> > > }
> > >
> > > /**
> > > - * eventfs_set_ef_status_free - set the ef->status to free
> > > + * eventfs_set_ei_status_free - remove the dentry reference from an eventfs_inode
> > > * @ti: the tracefs_inode of the dentry
> > > - * @dentry: dentry who's status to be freed
> > > + * @dentry: dentry which has the reference to remove.
> > > *
> > > - * eventfs_set_ef_status_free will be called if no more
> > > - * references remain
> > > + * Remove the association between a dentry from an eventfs_inode.
> > > */
> > > -void eventfs_set_ef_status_free(struct tracefs_inode *ti, struct dentry *dentry)
> > > +void eventfs_set_ei_status_free(struct tracefs_inode *ti, struct dentry *dentry)
> > > {
> > > struct tracefs_inode *ti_parent;
> > > + struct eventfs_inode *ei_child, *tmp;
> > > struct eventfs_inode *ei;
> > > - struct eventfs_file *ef, *tmp;
> > > + int i;
> > >
> > > /* The top level events directory may be freed by this */
> > > if (unlikely(ti->flags & TRACEFS_EVENT_TOP_INODE)) {
> > > @@ -166,9 +151,9 @@ void eventfs_set_ef_status_free(struct tracefs_inode *ti, struct dentry *dentry)
> > > ei = ti->private;
> > >
> > > /* Record all the top level files */
> > > - list_for_each_entry_srcu(ef, &ei->e_top_files, list,
> > > + list_for_each_entry_srcu(ei_child, &ei->children, list,
> > > lockdep_is_held(&eventfs_mutex)) {
> > > - list_add_tail(&ef->del_list, &ef_del_list);
> > > + list_add_tail(&ei_child->del_list, &ef_del_list);
> > > }
> >
> > Do we really need to move all ei::children to ef_del_list (this must be
> > ei_del_list)? Because we removed ei from ti->private under eventfs_mutex
> > is locked, no one can access the ei::childrenn anymore?
>
> Part of this is paranoia, just like why I set ti->private to NULL again. It
> shouldn't be accessed, but just in case it is, I want to make sure it
> doesn't cause any hard to find bugs. I rather have this than to chase down
> some hard to find bug if this ever becomes not the case.
>
> And I wanted to be consistent. As ei->children should only be iterated with
> srcu or eventfs_mutex held. I really don't want to have that list iterated
> without any locking.
>
> Note, this is a slow path, where it's better to be safe than efficient.
OK, so this is done intentionally. I got it.
>
> >
> > >
> > > /* Nothing should access this, but just in case! */
> > > @@ -177,11 +162,13 @@ void eventfs_set_ef_status_free(struct tracefs_inode *ti, struct dentry *dentry)
> > > mutex_unlock(&eventfs_mutex);
> > >
> > > /* Now safely free the top level files and their children */
> > > - list_for_each_entry_safe(ef, tmp, &ef_del_list, del_list) {
> > > - list_del(&ef->del_list);
> > > - eventfs_remove(ef);
> > > + list_for_each_entry_safe(ei_child, tmp, &ef_del_list, del_list) {
> >
> > I mean now we can safely use ei->children here.
>
> Like I said, I don't want to iterate it without a lock.
> eventfs_remove_dir() grabs the eventfs_lock, so it can't be held during the
> loop.
OK.
>
> >
> > > + list_del(&ei_child->del_list);
> > > + eventfs_remove_dir(ei_child);
> > > }
> > >
> > > + kfree_const(ei->name);
> > > + kfree(ei->d_children);
> > > kfree(ei);
> > > return;
> > > }
> > > @@ -192,68 +179,162 @@ void eventfs_set_ef_status_free(struct tracefs_inode *ti, struct dentry *dentry)
> > > if (!ti_parent || !(ti_parent->flags & TRACEFS_EVENT_INODE))
> > > goto out;
> > >
> > > - ef = dentry->d_fsdata;
> > > - if (!ef)
> > > + ei = dentry->d_fsdata;
> > > + if (!ei)
> > > goto out;
> > >
> > > /*
> > > - * If ef was freed, then the LSB bit is set for d_fsdata.
> > > + * If ei was freed, then the LSB bit is set for d_fsdata.
> > > * But this should not happen, as it should still have a
> > > * ref count that prevents it. Warn in case it does.
> > > */
> > > - if (WARN_ON_ONCE((unsigned long)ef & 1))
> > > + if (WARN_ON_ONCE((unsigned long)ei & 1))
> > > goto out;
> > >
> > > + /* This could belong to one of the files of the ei */
> > > + if (ei->dentry != dentry) {
> > > + for (i = 0; i < ei->nr_entries; i++) {
> > > + if (ei->d_children[i] == dentry)
> > > + break;
> > > + }
> > > + if (WARN_ON_ONCE(i == ei->nr_entries))
> > > + goto out;
> > > + ei->d_children[i] = NULL;
> > > + } else {
> > > + ei->dentry = NULL;
> > > + }
> > > +
> > > dentry->d_fsdata = NULL;
> > > - ef->dentry = NULL;
> > > -out:
> > > + out:
> > > + mutex_unlock(&eventfs_mutex);
> > > +}
> > > +
>
>
>
> > > /**
> > > - * create_dentry - helper function to create dentry
> > > - * @ef: eventfs_file of file or directory to create
> > > - * @parent: parent dentry
> > > - * @lookup: true if called from lookup routine
> > > + * create_dir_dentry - Create a directory dentry for the eventfs_inode
> > > + * @ei: The eventfs_inode to create the directory for
> > > + * @parent: The dentry of the parent of this directory
> > > + * @lookup: True if this is called by the lookup code
> > > *
> > > - * Used to create a dentry for file/dir, executes post dentry creation routine
> > > + * This creates and attaches a directory dentry to the eventfs_inode @ei.
> > > */
> > > static struct dentry *
> > > -create_dentry(struct eventfs_file *ef, struct dentry *parent, bool lookup)
> > > +create_dir_dentry(struct eventfs_inode *ei, struct dentry *parent, bool lookup)
> > > {
> > > bool invalidate = false;
> > > - struct dentry *dentry;
> > > + struct dentry *dentry = NULL;
> > >
> > > mutex_lock(&eventfs_mutex);
> > > - if (ef->is_freed) {
> > > - mutex_unlock(&eventfs_mutex);
> > > - return NULL;
> > > - }
> > > - if (ef->dentry) {
> > > - dentry = ef->dentry;
> > > - /* On dir open, up the ref count */
> > > + if (ei->dentry) {
> > > + /* If the dentry already has a dentry, use it */
> > > + dentry = ei->dentry;
> > > + /* lookup does not need to up the ref count */
> > > if (!lookup)
> > > dget(dentry);
> > > mutex_unlock(&eventfs_mutex);
> > > @@ -261,42 +342,50 @@ create_dentry(struct eventfs_file *ef, struct dentry *parent, bool lookup)
> > > }
> > > mutex_unlock(&eventfs_mutex);
> > >
> > > + /* The lookup already has the parent->d_inode locked */
> > > if (!lookup)
> > > inode_lock(parent->d_inode);
> > >
> > > - if (ef->ei)
> > > - dentry = create_dir(ef->name, parent, ef->data);
> > > - else
> > > - dentry = create_file(ef->name, ef->mode, parent,
> > > - ef->data, ef->fop);
> > > + dentry = create_dir(ei->name, parent);
> > >
> > > if (!lookup)
> > > inode_unlock(parent->d_inode);
> > >
> > > mutex_lock(&eventfs_mutex);
> > > - if (IS_ERR_OR_NULL(dentry)) {
> > > - /* If the ef was already updated get it */
> > > - dentry = ef->dentry;
> > > +
> > > + if (IS_ERR_OR_NULL(dentry) && !ei->is_freed) {
> > > + /*
> > > + * When the mutex was released, something else could have
> > > + * created the dentry for this e_dentry. In which case
> > > + * use that one.
> > > + *
> > > + * Note, with the mutex held, the e_dentry cannot have content
> > > + * and the ei->is_freed be true at the same time.
> > > + */
> > > + dentry = ei->dentry;
> > > if (dentry && !lookup)
> > > dget(dentry);
> > > mutex_unlock(&eventfs_mutex);
> > > return dentry;
> > > }
> > >
> > > - if (!ef->dentry && !ef->is_freed) {
> > > - ef->dentry = dentry;
> > > - if (ef->ei)
> > > - eventfs_post_create_dir(ef);
> > > - dentry->d_fsdata = ef;
> > > + if (!ei->dentry && !ei->is_freed) {
> > > + ei->dentry = dentry;
> > > + eventfs_post_create_dir(ei);
> > > + dentry->d_fsdata = ei;
> > > } else {
> > > - /* A race here, should try again (unless freed) */
> > > + /*
> > > + * Should never happen unless we get here due to being freed.
> > > + * Otherwise it means two dentries exist with the same name.
> > > + */
> > > + WARN_ON_ONCE(!ei->is_freed);
> > > invalidate = true;
> > >
> > > /*
> > > * Should never happen unless we get here due to being freed.
> > > * Otherwise it means two dentries exist with the same name.
> > > */
> > > - WARN_ON_ONCE(!ef->is_freed);
> > > + WARN_ON_ONCE(!ei->is_freed);
> >
> > Do we need to repeat this same WARN_ON_ONCE() again?
>
> Nope. That probably happened from my many rebases I did no this code.
>
> >
> > > }
> > > mutex_unlock(&eventfs_mutex);
> > > if (invalidate)
> > > @@ -308,50 +397,70 @@ create_dentry(struct eventfs_file *ef, struct dentry *parent, bool lookup)
> > > return invalidate ? NULL : dentry;
> > > }
> > >
> > > -static bool match_event_file(struct eventfs_file *ef, const char *name)
> > > -{
> > > - bool ret;
> > > -
> > > - mutex_lock(&eventfs_mutex);
> > > - ret = !ef->is_freed && strcmp(ef->name, name) == 0;
> > > - mutex_unlock(&eventfs_mutex);
> > > -
> > > - return ret;
> > > -}
> > > -
> > > /**
> > > * eventfs_root_lookup - lookup routine to create file/dir
> > > * @dir: in which a lookup is being done
> > > * @dentry: file/dir dentry
> > > - * @flags: to pass as flags parameter to simple lookup
> > > + * @flags: Just passed to simple_lookup()
> > > *
> > > - * Used to create a dynamic file/dir within @dir. Use the eventfs_inode
> > > - * list of meta data to find the information needed to create the file/dir.
> > > + * Used to create dynamic file/dir with-in @dir, search with-in @ei
> > > + * list, if @dentry found go ahead and create the file/dir
> > > */
> > > +
> > > static struct dentry *eventfs_root_lookup(struct inode *dir,
> > > struct dentry *dentry,
> > > unsigned int flags)
> > > {
> > > + const struct file_operations *fops;
> > > + const struct eventfs_entry *entry;
> > > + struct eventfs_inode *ei_child;
> > > struct tracefs_inode *ti;
> > > struct eventfs_inode *ei;
> > > - struct eventfs_file *ef;
> > > struct dentry *ret = NULL;
> > > + const char *name = dentry->d_name.name;
> > > + bool created = false;
> > > + umode_t mode;
> > > + void *data;
> > > int idx;
> > > + int i;
> > > + int r;
> > >
> > > ti = get_tracefs(dir);
> > > if (!(ti->flags & TRACEFS_EVENT_INODE))
> > > return NULL;
> > >
> > > ei = ti->private;
> >
> > Just for make sure, can we access 'ti->private' safely here, because I saw
> > eventfs_set_ef_status_free() modifies it under eventfs_mutex.
> > I guess this is called with some inode reference so it is not removed.
> > (but in that case, why we need eventfs_mutex in eventfs_set_ef_status_free()...?)
>
> eventfs_mutex is used to protect modifying the list. srcu is used to
> protect the iteration of the list.
>
> This code is called via the vfs layer. Hmm, maybe I should take the eventfs_mutex() here:
>
> idx = srcu_read_lock(&eventfs_srcu);
> mutex_lock(eventfs_mutex);
> ei = ti->private;
> mutex_unlock(eventfs_mutex);
>
> if (!ei)
> goto out;
>
> Just in case there's a way that ei can be deleted while the list is being walked.
>
> Hmm.
Yeah. If we can 'get' the refcount on 'ei' while holding the eventfs_mutex,
it ensures 'ei' is there until we 'put' it.
>
> >
> > > + data = ei->data;
> > > +
> > > idx = srcu_read_lock(&eventfs_srcu);
> > > - list_for_each_entry_srcu(ef, &ei->e_top_files, list,
> > > + list_for_each_entry_srcu(ei_child, &ei->children, list,
> > > srcu_read_lock_held(&eventfs_srcu)) {
> > > - if (!match_event_file(ef, dentry->d_name.name))
> > > + if (strcmp(ei_child->name, name) != 0)
> > > continue;
> > > ret = simple_lookup(dir, dentry, flags);
> > > - create_dentry(ef, ef->d_parent, true);
> > > + create_dir_dentry(ei_child, ei->dentry, true);
> > > + created = true;
> > > break;
> > > }
> > > +
> > > + if (created)
> > > + goto out;
> > > +
> > > + for (i = 0; i < ei->nr_entries; i++) {
> > > + entry = &ei->entries[i];
> > > + if (strcmp(name, entry->name) == 0) {
> > > + void *cdata = data;
> > > + r = entry->callback(name, &mode, &cdata, &fops);
> > > + if (r <= 0)
> > > + continue;
> > > + ret = simple_lookup(dir, dentry, flags);
> > > + create_file_dentry(ei, &ei->d_children[i],
> > > + ei->dentry, name, mode, cdata,
> > > + fops, true);
> > > + break;
> > > + }
> > > + }
> > > + out:
> > > srcu_read_unlock(&eventfs_srcu, idx);
> > > return ret;
> > > }
> > > @@ -363,11 +472,12 @@ static struct dentry *eventfs_root_lookup(struct inode *dir,
> > > */
> > > static int eventfs_release(struct inode *inode, struct file *file)
> > > {
> > > + struct eventfs_inode *ei_child;
> > > struct tracefs_inode *ti;
> > > struct eventfs_inode *ei;
> > > - struct eventfs_file *ef;
> > > struct dentry *dentry;
> > > int idx;
> > > + int i;
> > >
> > > ti = get_tracefs(inode);
> > > if (!(ti->flags & TRACEFS_EVENT_INODE))
> > > @@ -375,10 +485,18 @@ static int eventfs_release(struct inode *inode, struct file *file)
> > >
> > > ei = ti->private;
> > > idx = srcu_read_lock(&eventfs_srcu);
> > > - list_for_each_entry_srcu(ef, &ei->e_top_files, list,
> > > + list_for_each_entry_srcu(ei_child, &ei->children, list,
> > > srcu_read_lock_held(&eventfs_srcu)) {
> > > mutex_lock(&eventfs_mutex);
> > > - dentry = ef->dentry;
> > > + dentry = ei_child->dentry;
> > > + mutex_unlock(&eventfs_mutex);
> >
> > If someone add a directory via eventfs_create_dir() in parallel, is this
> > local mutex_lock able to protect from that? (usually it may not happen.)
>
> That would require an event being added and created at the same time. Not
> sure that is possible.
>
> We could try it?
Not sure, but both eventfs_release() and eventfs_create_dir() will be
called from dynamic events, right? But the dynamic events will protect
the create/delete operation with a mutex, so it should not happen if
I understand correctly.
But if the eventfs requires such external exclusion for the operation,
it should be commented.
>
> >
> > > + if (dentry)
> > > + dput(dentry);
> > > + }
> > > +
> > > + for (i = 0; i < ei->nr_entries; i++) {
> > > + mutex_lock(&eventfs_mutex);
> > > + dentry = ei->d_children[i];
> > > mutex_unlock(&eventfs_mutex);
> >
> > Ditto. Maybe I'm misunderstanding how eventfs_mutex is used.
>
> I'll have to go back and look at this part on why I had this. I think it
> was to make sure ei->d_children existed. But it may also need a test too. I
> don't remember. :-/
Thanks :)
>
>
> >
> > > if (dentry)
> > > dput(dentry);
> > > @@ -390,94 +508,140 @@ static int eventfs_release(struct inode *inode, struct file *file)
> > > /**
> > > * dcache_dir_open_wrapper - eventfs open wrapper
> > > * @inode: not used
> > > - * @file: dir to be opened (to create its child)
> > > + * @file: dir to be opened (to create it's children)
> > > *
> > > - * Used to dynamically create the file/dir within @file. @file is really a
> > > - * directory and all the files/dirs of the children within @file will be
> > > - * created. If any of the files/dirs have already been created, their
> > > - * reference count will be incremented.
> > > + * Used to dynamic create file/dir with-in @file, all the
> > > + * file/dir will be created. If already created then references
> > > + * will be increased
> > > */
> > > static int dcache_dir_open_wrapper(struct inode *inode, struct file *file)
> > > {
> > > + const struct file_operations *fops;
> > > + const struct eventfs_entry *entry;
> > > + struct eventfs_inode *ei_child;
> > > struct tracefs_inode *ti;
> > > struct eventfs_inode *ei;
> > > - struct eventfs_file *ef;
> > > - struct dentry *dentry = file_dentry(file);
> > > + struct dentry *parent = file_dentry(file);
> > > struct inode *f_inode = file_inode(file);
> > > + const char *name = parent->d_name.name;
> > > + umode_t mode;
> > > + void *data;
> > > int idx;
> > > + int i;
> > > + int r;
> > >
> > > ti = get_tracefs(f_inode);
> > > if (!(ti->flags & TRACEFS_EVENT_INODE))
> > > return -EINVAL;
> > >
> > > ei = ti->private;
> > > + data = ei->data;
> > > +
> > > idx = srcu_read_lock(&eventfs_srcu);
> > > - list_for_each_entry_srcu(ef, &ei->e_top_files, list,
> > > + list_for_each_entry_srcu(ei_child, &ei->children, list,
> > > srcu_read_lock_held(&eventfs_srcu)) {
> > > - create_dentry(ef, dentry, false);
> > > + create_dir_dentry(ei_child, parent, false);
> > > + }
> > > +
> > > + for (i = 0; i < ei->nr_entries; i++) {
> > > + void *cdata = data;
> > > + entry = &ei->entries[i];
> > > + name = entry->name;
> > > + r = entry->callback(name, &mode, &cdata, &fops);
> > > + if (r <= 0)
> > > + continue;
> > > + create_file_dentry(ei, &ei->d_children[i],
> > > + parent, name, mode, cdata, fops, false);
> > > }
> > > srcu_read_unlock(&eventfs_srcu, idx);
> > > return dcache_dir_open(inode, file);
> > > }
> > >
>
>
> > > /**
> > > * eventfs_remove - remove eventfs dir or file from list
> > > - * @ef: eventfs_file to be removed.
> > > + * @ei: eventfs_inode to be removed.
> > > *
> > > * This function acquire the eventfs_mutex lock and call eventfs_remove_rec()
> > > */
> > > -void eventfs_remove(struct eventfs_file *ef)
> > > +void eventfs_remove_dir(struct eventfs_inode *ei)
> > > {
> > > - struct eventfs_file *tmp;
> > > - LIST_HEAD(ef_del_list);
> > > + struct eventfs_inode *tmp;
> > > + LIST_HEAD(ei_del_list);
> > > struct dentry *dentry_list = NULL;
> > > struct dentry *dentry;
> > > + int i;
> > >
> > > - if (!ef)
> > > + if (!ei)
> > > return;
> > >
> > > mutex_lock(&eventfs_mutex);
> > > - eventfs_remove_rec(ef, &ef_del_list, 0);
> > > - list_for_each_entry_safe(ef, tmp, &ef_del_list, del_list) {
> > > - if (ef->dentry) {
> > > - unsigned long ptr = (unsigned long)dentry_list;
> > > -
> > > - /* Keep the dentry from being freed yet */
> > > - dget(ef->dentry);
> > > -
> > > - /*
> > > - * Paranoid: The dget() above should prevent the dentry
> > > - * from being freed and calling eventfs_set_ef_status_free().
> > > - * But just in case, set the link list LSB pointer to 1
> > > - * and have eventfs_set_ef_status_free() check that to
> > > - * make sure that if it does happen, it will not think
> > > - * the d_fsdata is an event_file.
> > > - *
> > > - * For this to work, no event_file should be allocated
> > > - * on a odd space, as the ef should always be allocated
> > > - * to be at least word aligned. Check for that too.
> > > - */
> > > - WARN_ON_ONCE(ptr & 1);
> > > -
> > > - ef->dentry->d_fsdata = (void *)(ptr | 1);
> > > - dentry_list = ef->dentry;
> > > - ef->dentry = NULL;
> > > - }
> > > - call_srcu(&eventfs_srcu, &ef->rcu, free_ef);
> > > + eventfs_remove_rec(ei, &ei_del_list, 0);
> > > +
> > > + list_for_each_entry_safe(ei, tmp, &ei_del_list, del_list) {
> > > + for (i = 0; i < ei->nr_entries; i++)
> > > + unhook_dentry(&ei->d_children[i], &dentry_list);
> > > + unhook_dentry(&ei->dentry, &dentry_list);
> > > + call_srcu(&eventfs_srcu, &ei->rcu, free_ei);
> > > }
> > > mutex_unlock(&eventfs_mutex);
> > >
> > > @@ -783,8 +814,8 @@ void eventfs_remove(struct eventfs_file *ef)
> > > mutex_lock(&eventfs_mutex);
> > > /* dentry should now have at least a single reference */
> > > WARN_ONCE((int)d_count(dentry) < 1,
> > > - "dentry %p less than one reference (%d) after invalidate\n",
> > > - dentry, d_count(dentry));
> > > + "dentry %px (%s) less than one reference (%d) after invalidate\n",
> > > + dentry, dentry->d_name.name, d_count(dentry));
> >
> > Do we need to check this? Even if someone can access to this dentry, it will
> > use a dput(), so *that dput()* or *the below dput()* will release the dentry,
> > doesn't it?
>
> I triggered this warning a lot in my development, so I'd like to keep it!
OK, I was just curious why we are doing this here.
>
> >
> >
> > > mutex_unlock(&eventfs_mutex);
> > > dput(dentry);
> > > }
> > > diff --git a/fs/tracefs/event_show.c b/fs/tracefs/event_show.c
> > > index 66dece7cc810..35e8f5d805ea 100644
> > > --- a/fs/tracefs/event_show.c
> > > +++ b/fs/tracefs/event_show.c
> > > @@ -25,7 +25,7 @@ static void *e_next(struct seq_file *m, void *v, loff_t *pos)
> > > int level = elist->level;
> > > struct list_head *head = elist->head[level];
> > > struct list_head *next = elist->next[level];
> > > - struct eventfs_file *ef;
> > > + struct eventfs_inode *ei;
> > >
> > > (*pos)++;
> > >
> > > @@ -42,12 +42,12 @@ static void *e_next(struct seq_file *m, void *v, loff_t *pos)
> > > elist->next[level] = next;
> > > }
> > >
> > > - ef = list_entry(next, struct eventfs_file, list);
> > > + ei = list_entry(next, struct eventfs_inode, list);
> > >
> > > /* For each entry (not at the bottom) do a breadth first search */
> > > - if (ef->ei && !list_empty(&ef->ei->e_top_files) && level < 2) {
> > > + if (!list_empty(&ei->children) && level < 2) {
> > > elist->level = ++level;
> > > - head = &ef->ei->e_top_files;
> > > + head = &ei->children;
> > > elist->head[level] = head;
> > > next = head;
> > > /*
> > > @@ -57,13 +57,13 @@ static void *e_next(struct seq_file *m, void *v, loff_t *pos)
> > > }
> > >
> > > elist->next[level] = next->next;
> > > - return ef;
> > > + return ei;
> > > }
> > >
> > > static void *e_start(struct seq_file *m, loff_t *pos)
> > > {
> > > struct event_list *elist = m->private;
> > > - struct eventfs_file *ef = NULL;
> > > + struct eventfs_inode *ei = NULL;
> > > loff_t l;
> > >
> > > mutex_lock(&eventfs_mutex);
> > > @@ -72,25 +72,31 @@ static void *e_start(struct seq_file *m, loff_t *pos)
> > > elist->next[0] = elist->head[0]->next;
> > >
> > > for (l = 0; l <= *pos; ) {
> > > - ef = e_next(m, ef, &l);
> > > - if (!ef)
> > > + ei = e_next(m, ei, &l);
> > > + if (!ei)
> > > break;
> > > }
> > > - return ef;
> > > + return ei;
> > > }
> > >
> > > static int e_show(struct seq_file *m, void *v)
> > > {
> > > - struct eventfs_file *ef = v;
> > > + struct eventfs_inode *ei = v;
> > > + int i;
> > >
> > > - seq_printf(m, "%s", ef->name);
> > > - if (ef->ei)
> > > - seq_putc(m, '/');
> > > + seq_printf(m, "%s", ei->name);
> > >
> > > - if (ef->dentry)
> > > - seq_printf(m, " dentry: (%d)", d_count(ef->dentry));
> > > + if (ei->dentry)
> > > + seq_printf(m, " dentry: (%d)", d_count(ei->dentry));
> > > seq_putc(m, '\n');
> > >
> > > + for (i = 0; i < ei->nr_entries; i++) {
> > > + struct dentry *dentry = ei->d_children[i];
> > > + if (dentry) {
> > > + seq_printf(m, " %s dentry: %px (%d)\n",
> > > + ei->entries[i].name, dentry, d_count(dentry));
> > > + }
> > > + }
> > > return 0;
> > > }
> > >
> > > @@ -111,30 +117,25 @@ eventfs_show_dentry_open(struct inode *inode, struct file *file)
> > > {
> > > const struct seq_operations *seq_ops = &eventfs_show_dentry_seq_ops;
> > > struct event_list *elist;
> > > - struct tracefs_inode *ti;
> > > struct eventfs_inode *ei;
> > > - struct dentry *dentry;
> > >
> > > - /* The inode private should have the dentry of the "events" directory */
> > > - dentry = inode->i_private;
> > > - if (!dentry)
> > > + /* The inode private should have the eventfs_inode of the "events" directory */
> > > + ei = inode->i_private;
> > > + if (!ei)
> > > return -EINVAL;
> > >
> > > elist = __seq_open_private(file, seq_ops, sizeof(*elist));
> > > if (!elist)
> > > return -ENOMEM;
> > >
> > > - ti = get_tracefs(dentry->d_inode);
> > > - ei = ti->private;
> > > -
> > > /*
> > > * Start off at level 0 (/sys/kernel/tracing/events)
> > > * Initialize head to the events files and next to the
> > > * first file.
> > > */
> > > elist->level = 0;
> > > - elist->head[0] = &ei->e_top_files;
> > > - elist->next[0] = ei->e_top_files.next;
> > > + elist->head[0] = &ei->children;
> > > + elist->next[0] = ei->children.next;
> > >
> > > return 0;
> > > }
> > > diff --git a/fs/tracefs/inode.c b/fs/tracefs/inode.c
> > > index 891653ba9cf3..34ffb2f8114e 100644
> > > --- a/fs/tracefs/inode.c
> > > +++ b/fs/tracefs/inode.c
> > > @@ -385,7 +385,7 @@ static void tracefs_dentry_iput(struct dentry *dentry, struct inode *inode)
> > >
> > > ti = get_tracefs(inode);
> > > if (ti && ti->flags & TRACEFS_EVENT_INODE)
> > > - eventfs_set_ef_status_free(ti, dentry);
> > > + eventfs_set_ei_status_free(ti, dentry);
> > > iput(inode);
> > > }
> > >
> > > diff --git a/fs/tracefs/internal.h b/fs/tracefs/internal.h
> > > index 461920f0133f..ea3b01c0971a 100644
> > > --- a/fs/tracefs/internal.h
> > > +++ b/fs/tracefs/internal.h
> > > @@ -9,35 +9,36 @@ enum {
> > > TRACEFS_EVENT_TOP_INODE = BIT(2),
> > > };
> > >
> > > -struct eventfs_inode {
> > > - struct list_head e_top_files;
> > > +struct tracefs_inode {
> > > + unsigned long flags;
> > > + void *private;
> > > + struct inode vfs_inode;
> > > };
> > >
> > > /*
> > > - * struct eventfs_file - hold the properties of the eventfs files and
> > > - * directories.
> > > - * @name: the name of the file or directory to create
> > > - * @d_parent: holds parent's dentry
> > > - * @dentry: once accessed holds dentry
> > > - * @list: file or directory to be added to parent directory
> > > - * @ei: list of files and directories within directory
> > > - * @fop: file_operations for file or directory
> > > - * @iop: inode_operations for file or directory
> > > - * @data: something that the caller will want to get to later on
> > > - * @mode: the permission that the file or directory should have
> > > + * struct eventfs_inode - hold the properties of the eventfs directories.
> > > + * @list: link list into the parent directory
> > > + * @entries: the array of entries representing the files in the directory
> > > + * @name: the name of the directory to create
> >
> > @children: link list into the child eventfs_inode
>
> Thanks! This got chopped a few times during rebasing.
>
> >
> > > + * @dentry: the dentry of the directory
> > > + * @d_parent: pointer to the parent's dentry
> > > + * @d_children: The array of dentries to represent the files when created
> > > + * @data: The private data to pass to the callbacks
> > > + * @nr_entries: The number of items in @entries
> > > */
> > > -struct eventfs_file {
> > > +struct eventfs_inode {
> > > + struct list_head list;
> > > + const struct eventfs_entry *entries;
> > > const char *name;
> > > - struct dentry *d_parent;
> > > + struct list_head children;
> > > struct dentry *dentry;
> > > - struct list_head list;
> > > - struct eventfs_inode *ei;
> > > - const struct file_operations *fop;
> > > - const struct inode_operations *iop;
> > > + struct dentry *d_parent;
> > > + struct dentry **d_children;
> > > + void *data;
> > > /*
> > > * Union - used for deletion
> > > - * @del_list: list of eventfs_file to delete
> > > - * @rcu: eventfs_file to delete in RCU
> > > + * @del_list: list of eventfs_inode to delete
> > > + * @rcu: eventfs_indoe to delete in RCU
> > > * @is_freed: node is freed if one of the above is set
> > > */
> > > union {
> >
> > Thank you,
> >
>
> Thank you for reviewing!
>
> I'll need to send a v2.
>
> I'll have to do that anyway, as Linus nacked the show_eventfs_dentries file :-(
Thank you,
--
Masami Hiramatsu (Google) <mhiramat@xxxxxxxxxx>