Re: [PATCH v2] libfs: getdents() should return 0 after reaching EOD

[Chuck Lever III <chuck.lever@oracle.com>]

> On Nov 18, 2023, at 11:28 AM, Al Viro <viro@zeniv.linux.org.uk> wrote:
> 
> On Wed, Nov 15, 2023 at 03:22:52PM -0500, Chuck Lever wrote:
> 
>> static int offset_readdir(struct file *file, struct dir_context *ctx)
>> {
>> + struct dentry *cursor = file->private_data;
>> struct dentry *dir = file->f_path.dentry;
>> 
>> lockdep_assert_held(&d_inode(dir)->i_rwsem);
>> @@ -479,11 +481,19 @@ static int offset_readdir(struct file *file, struct dir_context *ctx)
>> if (!dir_emit_dots(file, ctx))
>> return 0;
>> 
>> - offset_iterate_dir(d_inode(dir), ctx);
>> + if (ctx->pos == 2)
>> + cursor->d_flags &= ~DCACHE_EOD;
>> + else if (cursor->d_flags & DCACHE_EOD)
>> + return 0;
>> +
>> + if (offset_iterate_dir(d_inode(dir), ctx))
>> + cursor->d_flags |= DCACHE_EOD;
> 
> This is simply grotesque - "it's better to keep ->private_data constant,
> so we will allocate a dentry, just to store the one bit of data we need to
> keep track of; oh, and let's grab a bit out of ->d_flags, while we are at it;
> we will ignore the usual locking rules for ->d_flags modifications, 'cause
> it's all serialized on ->f_pos_lock".
> 
> No.  If nothing else, this is harder to follow than the original.

No argument from me.


> It's
> far easier to verify that these struct file instances only use ->private_data
> as a flag and these accesses are serialized on ->f_pos_lock as claimed
> than go through the accesses to ->d_flags, prove that the one above is
> the only one that can happen to such dentries (while they are live, that
> is - once they are in __dentry_kill(), there will be modifications of ->d_flags)
> and that it can't happen to any other instances.
> 
> NAKed-by: Al Viro <viro@zeniv.linux.org.uk>

Fair enough. Are you comfortable enough with v1 to Ack it, or do
you want me to continue looking for another mechanism for marking
the end-of-directory stream?


--
Chuck Lever