public inbox for [email protected]
 help / color / mirror / Atom feed
From: Dominique Martinet <[email protected]>
To: Christian Brauner <[email protected]>
Cc: Alexander Viro <[email protected]>,
	Jens Axboe <[email protected]>,
	Pavel Begunkov <[email protected]>,
	Stefan Roesch <[email protected]>, Clay Harris <[email protected]>,
	Dave Chinner <[email protected]>,
	[email protected], [email protected],
	[email protected]
Subject: Re: [PATCH v2 1/6] fs: split off vfs_getdents function of getdents64 syscall
Date: Wed, 24 May 2023 06:13:57 +0900	[thread overview]
Message-ID: <[email protected]> (raw)
In-Reply-To: <20230523-entzug-komodowaran-96d003250f70@brauner>

Christian Brauner wrote on Tue, May 23, 2023 at 05:39:08PM +0200:
> > @@ -362,11 +369,7 @@ SYSCALL_DEFINE3(getdents64, unsigned int, fd,
> >  	};
> >  	int error;
> >  
> > -	f = fdget_pos(fd);
> > -	if (!f.file)
> > -		return -EBADF;
> > -
> > -	error = iterate_dir(f.file, &buf.ctx);
> > +	error = iterate_dir(file, &buf.ctx);
> So afaict this isn't enough.
> If you look into iterate_shared() you should see that it uses and
> updates f_pos. But that can't work for io_uring and also isn't how
> io_uring handles read and write. You probably need to use a local pos
> similar to what io_uring does in rw.c for rw->kiocb.ki_pos. But in
> contrast simply disallow any offsets for getdents completely. Thus not
> relying on f_pos anywhere at all.

Using a private offset from the sqe was the previous implementation
discussed around here[1], and Al Viro pointed out that the iterate
filesystem implementations don't validate the offset makes sense as it's
either costly or for some filesystems downright impossible, so I went
into a don't let users modify it approach.

[1][email protected]/T/#m517583f23502f32b040c819d930359313b3db00c

I agree it's not how io_uring usually works -- it dislikes global
states -- but it works perfectly well as long as you don't have multiple
users on the same file, which the application can take care of.

Not having any offsets would work for small directories but make reading
large directories impossible so some sort of continuation is required,
which means we need to keep the offset around; I also suggested keeping
the offset in argument as the previous version but only allowing the
last known offset (... so ultimately still updating f_pos anyway as we
don't have anywhere else to store it) or 0, but if we're going to do
that it looks much simpler to me to expose the same API as getdents.

Dominique Martinet | Asmadeus

  reply	other threads:[~2023-05-23 21:14 UTC|newest]

Thread overview: 20+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2023-05-10 10:52 [PATCH v2 0/6] io_uring: add getdents support, take 2 Dominique Martinet
2023-05-10 10:52 ` [PATCH v2 1/6] fs: split off vfs_getdents function of getdents64 syscall Dominique Martinet
2023-05-23 15:39   ` Christian Brauner
2023-05-23 21:13     ` Dominique Martinet [this message]
2023-05-10 10:52 ` [PATCH v2 2/6] vfs_getdents/struct dir_context: add flags field Dominique Martinet
2023-05-10 10:52 ` [PATCH v2 3/6] io_uring: add support for getdents Dominique Martinet
2023-05-10 10:52 ` [PATCH v2 4/6] kernfs: implement readdir FMODE_NOWAIT Dominique Martinet
2023-05-11 10:55   ` Dan Carpenter
2023-05-11 11:03     ` Dominique Martinet
2023-05-10 10:52 ` [PATCH v2 5/6] libfs: set FMODE_NOWAIT on dir open Dominique Martinet
2023-05-10 10:52 ` [PATCH v2 6/6] RFC: io_uring getdents: test returning an EOF flag in CQE Dominique Martinet
2023-05-23 14:30   ` Christian Brauner
2023-05-23 21:05     ` Dominique Martinet
2023-05-24 13:52       ` [PATCH v2 1/6] fs: split off vfs_getdents function of getdents64 syscall Christian Brauner
2023-05-24 21:36         ` Dominique Martinet
2023-05-25  9:22           ` Christian Brauner
2023-05-25 11:00             ` Dominique Martinet
2023-05-25 12:33               ` Christian Brauner
2023-07-11  8:17               ` Hao Xu
2023-07-11  8:24                 ` Dominique Martinet

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    [email protected] \
    [email protected] \
    [email protected] \
    [email protected] \
    [email protected] \
    [email protected] \
    [email protected] \
    [email protected] \
    [email protected] \
    [email protected] \
    [email protected] \
    [email protected] \

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox