Re: [PATCH RFC 2/2] io_uring: add support for getdents

[Dominique Martinet <asmadeus@codewreck.org>]

Dave Chinner wrote on Mon, May 01, 2023 at 09:32:41AM +1000:
> > I've had a second look and I still don't see anything obvious though;
> > I'd rather avoid adding a new variant of iterate()/iterate_shared() --
> > we could use that as a chance to add a flag to struct file_operation
> > instead? e.g., something like mmap_supported_flags:
> 
> I don't think that makes sense - the eventual goal is to make
> ->iterate() go away entirely and all filesystems use
> ->iterate_shared(). Hence I think adding flags to select iterate vs
> iterate_shared and the locking that is needed is the wrong place to
> start from here.

(The flag could just go away when all filesystems not supporting it are
gone, and it could be made the other way around (e.g. explicit
NOT_SHARED to encourage migrations), so I don't really see the problem
with this but next point makes this moot anyway)

> Whether the filesystem supports non-blocking ->iterate_shared() or
> not is a filesystem implementation option and io_uring needs that
> information to be held on the struct file for efficient
> determination of whether it should use non-blocking operations or
> not.

Right, sorry. I was thinking that since it's fs/op dependant it made
more sense to keep next to the iterate operation, but that'd be a
layering violation to look directly at the file_operation vector
directly from the uring code... So having it in the struct file is
better from that point of view.

> We already set per-filesystem file modes via the ->open method,
> that's how we already tell io_uring that it can do NOWAIT IO, as
> well as async read/write IO for regular files. And now we also use
> it for FMODE_DIO_PARALLEL_WRITE, too.
> 
> See __io_file_supports_nowait()....
> 
> Essentially, io_uring already cwhas the mechanism available to it
> to determine if it should use NOWAIT semantics for getdents
> operations; we just need to set FMODE_NOWAIT correctly for directory
> files via ->open() on the filesystems that support it...

Great, I wasn't aware of FMODE_NOWAIT; things are starting to fall in
place.
I'll send a v2 around Wed or Thurs (yay national holidays)

> [ Hmmmm - we probably need to be more careful in XFS about what
> types of files we set those flags on.... ]

Yes, FMODE_NOWAIT will be set on directories as xfs_dir_open calls
xfs_file_open which sets it inconditionally... So I got to check other
filesystems don't do something similar as a bonus, but it looks like
none that set FMODE_NOWAIT on regular files share the file open path,
so at least that shouldn't be too bad.
Happy to also fold the xfs fix as a prerequisite patch of this series or
to let you do it, just tell me.


Thanks,
-- 
Dominique Martinet | Asmadeus