[Virtio-fs] bug in lo_do_readdir()?

[Laszlo Ersek]

On 12/07/20 21:28, Vivek Goyal wrote:
> On Mon, Dec 07, 2020 at 06:32:26PM +0100, Laszlo Ersek wrote:
>> Hi,
>>
>> I believe lo_do_readdir() has a bug: for FUSE_READDIR (not
>> FUSE_READDIRPLUS), it does not call lo_do_lookup() -- which seems fine
>> in itself --, but then it doesn't load the inode into the inode map (?)
>> *at all*.
>>
>> A subsequent FUSE_GETATTR that refers to one of the inode(s) learned via
>> FUSE_READDIR (not FUSE_READDIRPLUS) gets -EBADF, because the requested
>> inode is not in the map.
>>
>> Because the inode is shared with the client through the normal (not
>> "plus") dirent format too (that is, via FUSE_READDIR and not
>> FUSE_READDIRPLUS), the server should remember the inode(s) from the
>> FUSE_READDIR request as well, in my opinion.
>>
> 
> tools/virtiofsd/fuse_lowlevel.h documents that readdir() does not
> affect lookup count.
> 
>      * Returning a directory entry from readdir() does not affect
>      * its lookup count.
> 
> While, readdirplus() does change it.
> 
>      * In contrast to readdir() (which does not affect the lookup counts),
>      * the lookup count of every entry returned by readdirplus(), except "."
>      * and "..", is incremented by one.

Thanks! I'll rework the code with readdirplus.

Laszlo


>> Sorry if I'm mistaken.
>>
>> The reason I'm not using FUSE_READDIRPLUS is three-fold:
>>
>> - one fewer wrapper to implement (I need FUSE_READ anyway, for reading
>> regular files, and FUSE_READDIR is identical, except for the opcode,
>> while FUSE_READDIRPLUS isn't),
>>
>> - FUSE_READDIRPLUS requires feature negotiation, while FUSE_READDIR
>> doesn't, as far as I understand,
>>
>> - performance for the firmware is not critical, so I'm fine calling a
>> separate FUSE_GETATTR on each inode learned through FUSE_READDIR -- I
>> don't need the attributes to come back together with the dirent.
>>
>> Unfortunately, I'm not familiar enough with the virtiofsd internals at
>> this point to propose a patch. It feels like adding a lookup-like, but
>> lighter-weight, call, to the non-plus branch in lo_do_readdir(), should
>> do the trick.
>>
>> Thanks!
>> Laszlo
>>
>> _______________________________________________
>> Virtio-fs mailing list
>> Virtio-fs at redhat.com
>> https://www.redhat.com/mailman/listinfo/virtio-fs