Re: [PATCH v2 0/3] io_uring: add getdents64 support

[Al Viro <viro@zeniv.linux.org.uk>]

On Wed, Dec 15, 2021 at 02:09:52PM -0700, Jens Axboe wrote:
> On 11/24/21 4:16 PM, Stefan Roesch wrote:
> > This series adds support for getdents64 in liburing. The intent is to
> > provide a more complete I/O interface for io_uring.
> > 
> > Patch 1: fs: add parameter use_fpos to iterate_dir()
> >   This adds a new parameter to the function iterate_dir() so the
> >   caller can specify if the position is the file position or the
> >   position stored in the buffer context.
> > 
> > Patch 2: fs: split off vfs_getdents function from getdents64 system call
> >   This splits of the iterate_dir part of the syscall in its own
> >   dedicated function. This allows to call the function directly from
> >   liburing.
> > 
> > Patch 3: io_uring: add support for getdents64
> >   Adds the functions to io_uring to support getdents64.
> > 
> > There is also a patch series for the changes to liburing. This includes
> > a new test. The patch series is called "liburing: add getdents support."
> 
> Al, ping on this one as well, same question on the VFS side.

First of all, apologies for being MIA - had been sick for a couple of
months, so right now I'm digging through the huge pile of mail.

The problem is not with VFS side - it's with ->iterate_dir() *instances*.
The logics for validation of file position in there is not pretty, and
it's based upon the assumption that all position changes (other than
those from ->iterate_dir() itself) come through lseek(2), with its
per-opened-file exclusion with getdents(2).

Your API just shoves an arbitrary number at them, with no indication
as far as struct file instance is concerned that something might need
to be checked.  Hell, the file might've been just opened with no
directory-changing operations done since then; of course its position
is valid.  And has nothing whatsoever with the number you put into
ctx->pos.

Normalization is sufficiently costly to show up on real-world loads.
Even "assume it bogus" flag somewhere in ctx will seriously cost on
io_uring side.  And that doesn't help with the case of tmpfs et.al.
where position is ignored - there's a per-instance cursor moved
around on lseek/readdir and that's the authoritative thing there.

So please, use a saner userland ABI - pread-like stuff is a really,
really bad idea for directories.