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

[Hao Xu <hao.xu@linux.dev>]

Hi Christian and Dominique,


On 7/13/23 00:02, Dominique Martinet wrote:
> (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.


Hi Christian,

My understanding of FMODE_NOWAIT is "this file support nowait IO". Just 
like what Dominique

said, io_uring issue a request two rounds(let's simplify it here since 
no apoll or task work involved),

and the first round is  a nowait/nonblock try, the second one is an 
offload-ed block try. So besides

a "ability" flag(FMODE_NOWAIT), we still need a "one-round" flag to 
point out that "we do need to

do nowait IO this time".


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


I've no strong opinion here, I kept it here as a flag variable to make it

more extendable in the future.


Thanks,

Hao