Re: [PATCH 2/3] vfs_getdents/struct dir_context: add flags field

[Dominique Martinet <asmadeus@codewreck.org>]

(replying as that was my code)

Christian Brauner wrote on Wed, Jul 12, 2023 at 01:31:57PM +0200:
> On Tue, Jul 11, 2023 at 07:40:26PM +0800, Hao Xu wrote:
> > diff --git a/fs/readdir.c b/fs/readdir.c
> > index 9592259b7e7f..b80caf4c9321 100644
> > --- a/fs/readdir.c
> > +++ b/fs/readdir.c
> > @@ -358,12 +358,14 @@ static bool filldir64(struct dir_context *ctx, const char *name, int namlen,
> >   * @file    : pointer to file struct of directory
> >   * @dirent  : pointer to user directory structure
> >   * @count   : size of buffer
> > + * @flags   : additional dir_context flags
> 
> Why do you need that flag argument. The ->iterate{_shared}() i_op gets
> passed the file so the filesystem can check
> @file->f_mode & FMODE_NOWAIT, no?

As far as I understand it, it's not because the fd is capable of NOWAIT
that uring will call it in NOWAIT mode:
- if the first getdents call returned -EAGAIN it'll also fall back to
waiting in a separate thread (there's no "getdents poll" implementation,
so there's no other way of rescheduling a non-blocking call)
- it's also possible for the user to specify it wants IOSQE_ASYNC in the
sqe->flags (admitedly I'm not sure why would anyone do this, but that's
useful for benchmarks at least -- it skips the initial NOWAIT call
before falling back to threaded waiting call)

Even outsides of io_uring, a call to getdents64 should block, so even if
the filesystem supports non-blocking it should be explicitely required
by the caller.


> > --- a/include/linux/fs.h
> > +++ b/include/linux/fs.h
> > @@ -1719,8 +1719,16 @@ typedef bool (*filldir_t)(struct dir_context *, const char *, int, loff_t, u64,
> >  struct dir_context {
> >  	filldir_t actor;
> >  	loff_t pos;
> > +	unsigned long flags;
> >  };
> >  
> > +/*
> > + * flags for dir_context flags
> > + * DIR_CONTEXT_F_NOWAIT: Request non-blocking iterate
> > + *                       (requires file->f_mode & FMODE_NOWAIT)
> > + */
> > +#define DIR_CONTEXT_F_NOWAIT	(1 << 0)
> 
> Even if this should be needed, I don't think this needs to use a full
> flags field.

I also got a request to somehow pass back "are there more entries to
read after this call" to the caller in my v1, and I had done this as a
second flag -- in general my understanding was that it's better to add
flags than a specific boolean for extensibility but I have no opinon
here.


-- 
Dominique Martinet | Asmadeus