On 18/10/17(Wed) 14:19, Helg Bredow wrote:
> On Wed, 18 Oct 2017 10:47:59 +0200
> Martin Pieuchot <[email protected]> wrote:
>
> > On 16/10/17(Mon) 14:37, Helg Bredow wrote:
> > > [...]
> > > Patch is below.
> >
> > Do you mind explaining the problem and the solution you implemented?
> >
>
> There is a regression introduced in 6.2 that can cause a call to getcwd(3) to
> result in an endless loop. fusefs_readdir() has a TODO that returns success
> even if a directory is not open, so getcwd(3) keeps calling it to fetch more
> dirents but none ever get supplied. This patch opens and closes the directory
> if it's not already open. The check for an invalid file handle was inside the
> loop but I've moved it outside since it's not going to become invalid once
> it's been opened.
Nice. You can remove a call to fb_delete() in the first error path.
> This doesn't resolve the problem however. This is because ifuse_ops_readdir()
> does not support being called across multiple calls to VOP_READDIR(9). This
> occurs if the buffer is too small to contain all the entries and getcwd(3)
> only supplies a 512 byte buffer. If the current directory is not returned in
> the first call to VOP_READDIR(9) then getcwd(3) will continue to call it but
> the eofflag is never set. ifuse_ops_readdir() now correctly starts scanning
> at the next dirent even across multiple VOP_READDIR(9) calls.
Why did you decide to put the logic in the library? Now the library
needs to remember some "state".
Would it be possible to keep a dumb ifuse_ops_readdir() and make the
kernel ask for the next dirent? How often does that happen in
practise? Only when strlen(dirent) > 512?
> In addition, if the entries don't evenly fit into the buffer then
> fusefs_readdir() would prematurely detect that the entire directory had been
> scanned and set the eofflag. This fix handles this condition so the entire
> directory is always scanned.
That's good.
> ifuse_fill_readdir() returns the ino of the file system instead of its own
> ino, which it returns from other calls such as stat(2). getcwd(3) compares
> these inos when scanning the directory and will never find a match since they
> are sourced from different file systems. fuse has the use_ino option to allow
> a file system to use its own inos but this is not implemented in OpenBSD yet.
> This patch forces the fuse ino to be used in ifuse_fill_readdir(). This is
> still only paritally implemented though, it will only set a valid ino if it
> can find the file or directory in the name cache. This is sufficient for
> getcwd(3) since the parent directory will always be in the cache.
How much work would it be to support the `use_ino' option?
> Index: sys/miscfs/fuse/fuse_vnops.c
> ===================================================================
> RCS file: /cvs/src/sys/miscfs/fuse/fuse_vnops.c,v
> retrieving revision 1.33
> diff -u -p -r1.33 fuse_vnops.c
> --- sys/miscfs/fuse/fuse_vnops.c 7 Sep 2016 17:53:35 -0000 1.33
> +++ sys/miscfs/fuse/fuse_vnops.c 18 Oct 2017 13:25:04 -0000
> @@ -680,7 +680,7 @@ fusefs_readdir(void *v)
> struct vnode *vp;
> struct proc *p;
> struct uio *uio;
> - int error = 0, eofflag = 0;
> + int error = 0, eofflag = 0, diropen = 0;
>
> vp = ap->a_vp;
> uio = ap->a_uio;
> @@ -695,14 +695,18 @@ fusefs_readdir(void *v)
> if (uio->uio_resid < sizeof(struct dirent))
> return (EINVAL);
>
> - while (uio->uio_resid > 0) {
> - fbuf = fb_setup(0, ip->ufs_ino.i_number, FBT_READDIR, p);
> -
> - if (ip->fufh[FUFH_RDONLY].fh_type == FUFH_INVALID) {
> - /* TODO open the file */
> + if (ip->fufh[FUFH_RDONLY].fh_type == FUFH_INVALID) {
> + error = fusefs_file_open(fmp, ip, FUFH_RDONLY, O_RDONLY, 1, p);
> + if (error) {
> fb_delete(fbuf);
No need for fb_delete() here.
> return (error);
> }
> + diropen = 1;
> + }
> +
> + while (uio->uio_resid > 0) {
> + fbuf = fb_setup(0, ip->ufs_ino.i_number, FBT_READDIR, p);
> +
> fbuf->fb_io_fd = ip->fufh[FUFH_RDONLY].fh_id;
> fbuf->fb_io_off = uio->uio_offset;
> fbuf->fb_io_len = MIN(uio->uio_resid, fmp->max_read);
> @@ -710,6 +714,13 @@ fusefs_readdir(void *v)
> error = fb_queue(fmp->dev, fbuf);
>
> if (error) {
> + /*
> + * dirent was larger than residual space left in
> + * buffer.
> + */
> + if (error == ENOBUFS && fbuf->fb_len == 0)
> + error = 0;
> +
> fb_delete(fbuf);
> break;
> }
> @@ -731,6 +742,9 @@ fusefs_readdir(void *v)
>
> if (!error && ap->a_eofflag != NULL)
> *ap->a_eofflag = eofflag;
> +
> + if (diropen)
> + fusefs_file_close(fmp, ip, FUFH_RDONLY, O_RDONLY, 1, p);
>
> return (error);
> }
>
