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
