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

From: Dave Chinner <david@fromorbit.com>
To: Dominique Martinet <asmadeus@codewreck.org>
Cc: Alexander Viro <viro@zeniv.linux.org.uk>,
	Christian Brauner <brauner@kernel.org>,
	Jens Axboe <axboe@kernel.dk>,
	Pavel Begunkov <asml.silence@gmail.com>,
	Stefan Roesch <shr@fb.com>,
	linux-fsdevel@vger.kernel.org, linux-kernel@vger.kernel.org,
	io-uring@vger.kernel.org
Subject: Re: [PATCH RFC 2/2] io_uring: add support for getdents
Date: Mon, 1 May 2023 09:32:41 +1000	[thread overview]
Message-ID: <20230430233241.GC2155823@dread.disaster.area> (raw)
In-Reply-To: <ZEzQRLUnlix1GvbA@codewreck.org>

On Sat, Apr 29, 2023 at 05:07:32PM +0900, Dominique Martinet wrote:
> Dominique Martinet wrote on Fri, Apr 28, 2023 at 03:14:52PM +0900:
> > > AFAICT, the io_uring code wouldn't need to do much more other than
> > > punt to the work queue if it receives a -EAGAIN result. Otherwise
> > > the what the filesystem returns doesn't need to change, and I don't
> > > see that we need to change how the filldir callbacks work, either.
> > > We just keep filling the user buffer until we either run out of
> > > cached directory data or the user buffer is full.
> > 
> > [...] I'd like to confirm what the uring
> > side needs to do before proceeding -- looking at the read/write path
> > there seems to be a polling mechanism in place to tell uring when to
> > look again, and I haven't looked at this part of the code yet to see
> > what happens if no such polling is in place (does uring just retry
> > periodically?)
> 
> Ok so this part can work out as you said, I hadn't understood what you
> meant by "punt to the work queue" but that should work from my new
> understanding of the ring; we can just return EAGAIN if the non-blocking
> variant doesn't have immediate results and call the blocking variant
> when we're called again without IO_URING_F_NONBLOCK in flags.
> (So there's no need to try to add a form of polling, although that is
> possible if we ever become able to do that; I'll just forget about this
> and be happy this part is easy)
> 
> 
> That just leaves deciding if a filesystem handles the blocking variant
> or not; ideally if we can know early (prep time) we can even mark
> REQ_F_FORCE_ASYNC in flags to skip the non-blocking call for filesystems
> that don't handle that and we get the best of both worlds.
> 
> 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.

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.

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...

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

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com