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 >> [email protected] >> https://www.redhat.com/mailman/listinfo/virtio-fs _______________________________________________ Virtio-fs mailing list [email protected] https://www.redhat.com/mailman/listinfo/virtio-fs
