Re: [RFC PATCH 0/3] create simple libfs directory iterator and make efivarfs use it

[James Bottomley <James.Bottomley@HansenPartnership.com>]

On Tue, 2025-03-18 at 23:45 +0000, Al Viro wrote:
[...]
> I suspect that you are making it too generic for its own good.
> 
> dcache_readdir() needs to cope with the situation when there are
> fuckloads of opened-and-unliked files in there.  That's why we
> play those games with cursors under if (need_resched()) there.

Yes, but those games will never really activate for the efivarfs code
because we expect to be mostly positive.  What I was aiming for was to
make sure there's no duplication of the subtle code,  so someone can't
forget to update it in two places, so doing a callback approach seemed
natural.

> That's not the case for efivarfs.  There you really want just
> "grab a reference to the next positive, drop the reference we
> were given" and that's it.
> 
> IOW, find_next_child() instead of scan_positives().  Export that
> and it becomes just a simple loop -
> 	child = NULL;
> 	while ((child = find_next_child(parent, child)) != NULL) {
> 		struct inode *inode = d_inode(child);
> 		struct efivar_entry *entry = efivar_entry(inode);
> 
> 		err = efivar_entry_size(entry, &size);
> 
> 		inode_lock(inode);
> 		i_size_write(inode, err ? 0 : size + sizeof(__u32));
> 		inode_unlock(inode);
> 
> 		if (err)
> 			simple_recursive_removal(child, NULL);
> 	}
> and that's it.  No callbacks, no cursors, no iterators - just an
> export of helper already there.

So we don't mind the callbacks; we have to do it for efivar_init()
lower down anyway.  However, I did take a cut at doing this based on
simple positive (see below).  I assume you won't like the way I have to
allocate and toss a cursor for each iteration, nor the fact that
there's still the cond_resched() in there?  I think I can fix that but
I'd have to slice apart simple_positive(), which is a bigger
undertaking.

> And we might want a better function name than that...

I went for simple_next_child() ...

Regards,

James

---

 fs/libfs.c         | 41 +++++++++++++++++++++++++++++++++++++++++
 include/linux/fs.h |  2 ++
 2 files changed, 43 insertions(+)

diff --git a/fs/libfs.c b/fs/libfs.c
index 8444f5cc4064..86f29cc2b85a 100644
--- a/fs/libfs.c
+++ b/fs/libfs.c
@@ -146,6 +146,47 @@ static struct dentry *scan_positives(struct dentry *cursor,
 	return found;
 }
 
+/**
+ * simple_next_child - get next child of the parent directory
+ *
+ * @parent: the directory to scan
+ * @child: last child (or NULL to start at the beginning)
+ *
+ * returns the next positive child in sequence with the reference
+ * elevated but if a child is passed in will drop that
+ * reference. Returns NULL on either error or when directory has been
+ * fully scanned.
+ *
+ * The intended use is as an iterator because all the references will
+ * be dropped by the end of the while loop:
+ *
+ *     child = NULL
+ *     while(child = simple_next_child(parent, child)) {
+ *          // do something
+ *     }
+ */
+struct dentry *simple_next_child(struct dentry *parent, struct dentry *child)
+{
+	struct hlist_node **p;
+	struct dentry *cursor = d_alloc_cursor(parent);
+
+	if (!cursor) {
+		dput(child);
+		return NULL;
+	}
+
+	if (child)
+		p = &child->d_sib.next;
+	else
+		p = &parent->d_children.first;
+
+	child = scan_positives(cursor, p, 1, child);
+	dput(cursor);
+
+	return child;
+}
+EXPORT_SYMBOL(simple_next_child);
+
 loff_t dcache_dir_lseek(struct file *file, loff_t offset, int whence)
 {
 	struct dentry *dentry = file->f_path.dentry;
diff --git a/include/linux/fs.h b/include/linux/fs.h
index 2788df98080f..dd84d1c3b8af 100644
--- a/include/linux/fs.h
+++ b/include/linux/fs.h
@@ -3531,6 +3531,8 @@ extern int simple_rename(struct mnt_idmap *, struct inode *,
 			 unsigned int);
 extern void simple_recursive_removal(struct dentry *,
                               void (*callback)(struct dentry *));
+extern struct dentry *simple_next_child(struct dentry *parent,
+					struct dentry *child);
 extern int noop_fsync(struct file *, loff_t, loff_t, int);
 extern ssize_t noop_direct_IO(struct kiocb *iocb, struct iov_iter *iter);
 extern int simple_empty(struct dentry *);
-- 
2.43.0