Re: [PATCH 1/2] fs: btrfs: implement opendir(), readdir() and closedir()

On Fri, Jun 26, 2026 at 10:38 AM Qu Wenruo <[email protected]> wrote:
>
>
>
> 在 2026/6/26 02:54, Alexey Charkov 写道:
> > Add support for generic directory iteration with opendir(), readdir() and
> > closedir() in the btrfs filesystem driver, following the ext4fs
> > implementation for opendir()/closedir() and the btrfs_iter_dir() function
> > for readdir().
> >
> > Signed-off-by: Alexey Charkov <[email protected]>
> > ---
> >   fs/btrfs/btrfs.c    | 92 +++++++++++++++++++++++++++++++++++++++++++++++++++++
> >   fs/btrfs/ctree.h    |  2 ++
> >   fs/btrfs/dir-item.c | 91 ++++++++++++++++++++++++++++++++++++++++++++++++++++
> >   fs/fs.c             |  4 ++-
> >   include/btrfs.h     |  5 +++
> >   5 files changed, 193 insertions(+), 1 deletion(-)
> >
> > diff --git a/fs/btrfs/btrfs.c b/fs/btrfs/btrfs.c
> > index f3087f690fa4..c647c8dedf4e 100644
> > --- a/fs/btrfs/btrfs.c
> > +++ b/fs/btrfs/btrfs.c
> > @@ -9,6 +9,7 @@
> >   #include <malloc.h>
> >   #include <u-boot/uuid.h>
> >   #include <linux/time.h>
> > +#include <fs.h>
> >   #include "btrfs.h"
> >   #include "crypto/hash.h"
> >   #include "disk-io.h"
> > @@ -159,6 +160,97 @@ int btrfs_ls(const char *path)
> >       return 0;
> >   }
> >
> > +struct btrfs_dir_stream {
> > +     struct fs_dir_stream parent;
> > +     struct fs_dirent dirent;
> > +     char *dirname;
> > +     u64 offset;
>
> This doesn't look correct to me, for an opened dir, we should have at
> least the root id or pointer, and an inode number.
>
> Not just an @dirname and re-do the path resolution again and again.
>
> In fact, I do not even think we should save @dirname here.

Thanks Qu, let me reshuffle these. I'm not a huge FS guru, so I was
just parroting what other existing U-Boot code does (ext4 in this
case). That might well not be the best approach here, even though it
worked for my use case.

> > +};
> > +
> > +int btrfs_opendir(const char *dirname, struct fs_dir_stream **dirsp)
> > +{
> > +     struct btrfs_fs_info *fs_info = current_fs_info;
> > +     struct btrfs_dir_stream *dirs;
> > +     struct btrfs_root *root;
> > +     u64 ino;
> > +     u8 type;
> > +     int ret;
> > +
> > +     *dirsp = NULL;
> > +     ASSERT(fs_info);
> > +
> > +     ret = btrfs_lookup_path(fs_info->fs_root, BTRFS_FIRST_FREE_OBJECTID,
> > +                             dirname, &root, &ino, &type, 40);
>
> You just discard the most important @root and @ino.
>
> So what is the point here?

Thanks for the pointer, I will switch from repeated lookups to a
stored ino reference.

> > +     if (ret < 0)
> > +             return ret;
> > +     if (type != BTRFS_FT_DIR)
> > +             return -ENOTDIR;
> > +
> > +     dirs = calloc(1, sizeof(*dirs));
> > +     if (!dirs)
> > +             return -ENOMEM;
> > +     dirs->dirname = strdup(dirname);
> > +     if (!dirs->dirname) {
> > +             free(dirs);
> > +             return -ENOMEM;
> > +     }
> > +
> > +     *dirsp = (struct fs_dir_stream *)dirs;
> > +     return 0;
> > +}
> > +
> > +int btrfs_readdir(struct fs_dir_stream *fs_dirs, struct fs_dirent **dentp)
> > +{
> > +     struct btrfs_dir_stream *dirs = (struct btrfs_dir_stream *)fs_dirs;
> > +     struct btrfs_fs_info *fs_info = current_fs_info;
> > +     struct fs_dirent *dent = &dirs->dirent;
> > +     struct btrfs_root *root;
> > +     u64 ino;
> > +     u8 type;
> > +     int ret;
> > +
> > +     *dentp = NULL;
> > +     ASSERT(fs_info);
> > +
> > +     ret = btrfs_lookup_path(fs_info->fs_root, BTRFS_FIRST_FREE_OBJECTID,
> > +                             dirs->dirname, &root, &ino, &type, 40);
>
> No, doing the same path resolution again and again is not sane.

Ack

> > +     if (ret < 0)
> > +             return ret;
> > +     if (type != BTRFS_FT_DIR)
> > +             return -ENOTDIR;
> > +
> > +     memset(dent, 0, sizeof(*dent));
> > +     ret = btrfs_next_dir_entry(root, ino, &dirs->offset, dent->name,
> > +                                sizeof(dent->name), &type);
> > +     if (ret < 0)
> > +             return ret;
> > +     if (ret > 0)
> > +             return -ENOENT;
> > +
> > +     switch (type) {
> > +     case BTRFS_FT_DIR:
> > +             dent->type = FS_DT_DIR;
> > +             break;
> > +     case BTRFS_FT_SYMLINK:
> > +             dent->type = FS_DT_LNK;
> > +             break;
> > +     default:
> > +             dent->type = FS_DT_REG;
> > +             break;
>
> So it looks like u-boot only supports the above 3 types, and unlike
> linux kernel the values doesn't match the btrfs internal ones.
>
> In that case, I'd still prefer a proper convertor function.

Hmm, but the convertor function would still be the same switch
statement, and it's only used in this single place. Would factoring it
out actually improve readability?

> [...]
> > +
> > +             *offset = key.offset + 1;
> > +             type = btrfs_dir_type(path.nodes[0], di);
> > +
> > +             /* XATTRs share the key space but are not directory entries. */
> > +             if (type == BTRFS_FT_XATTR) {
>
> This doesn't looks correct again.
>
> XATTR has their own keys, they should not show up among DIR_INDEX keys.

Let me double check and adjust accordingly.

Thanks a lot for your review!

Best regards,
Alexey