On Wed, 18 Oct 2017 14:19:31 +0000 Helg Bredow <[email protected]> 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. > > 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. > > 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. > > 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. > > Thanks for your feedback mpi@; I've made the changes you suggested. It's safe > to store the fuse struct across calls since this is returned from > fuse_setup() and never changes. Without it, I'm not sure how I would > implement a solution since you need this in the filler function to access the > name dictionary. > > > Index: lib/libfuse/fuse.h > =================================================================== > RCS file: /cvs/src/lib/libfuse/fuse.h,v > retrieving revision 1.12 > diff -u -p -r1.12 fuse.h > --- lib/libfuse/fuse.h 20 Jan 2014 15:01:59 -0000 1.12 > +++ lib/libfuse/fuse.h 18 Oct 2017 13:24:53 -0000 > @@ -89,6 +89,7 @@ typedef int (*fuse_fill_dir_t)(void *, c > off_t); > > typedef struct fuse_dirhandle { > + struct fuse *fuse; > fuse_fill_dir_t filler; > void *buf; > int filled; > Index: lib/libfuse/fuse_ops.c > =================================================================== > RCS file: /cvs/src/lib/libfuse/fuse_ops.c,v > retrieving revision 1.26 > diff -u -p -r1.26 fuse_ops.c > --- lib/libfuse/fuse_ops.c 7 Sep 2016 17:53:35 -0000 1.26 > +++ lib/libfuse/fuse_ops.c 18 Oct 2017 13:24:53 -0000 > @@ -217,17 +217,20 @@ static int > ifuse_fill_readdir(void *dh, const char *name, const struct stat *stbuf, > off_t off) > { > + struct fuse *f; > struct fuse_dirhandle *fd = dh; > + struct fuse_vnode *v; > struct fusebuf *fbuf; > struct dirent *dir; > uint32_t namelen; > uint32_t len; > > + f = fd->fuse; > fbuf = fd->buf; > namelen = strnlen(name, MAXNAMLEN); > len = GENERIC_DIRSIZ(namelen); > > - if (fd->full || (fbuf->fb_len + len > fd->size)) { > + if (fd->full || fbuf->fb_len + len > fd->size) { > fd->full = 1; > return (0); > } > @@ -242,13 +245,21 @@ ifuse_fill_readdir(void *dh, const char > if (off) > fd->filled = 0; > > - if (stbuf) { > - dir->d_fileno = stbuf->st_ino; > + /* TODO Add support for use_ino and readdir_ino */ > + v = get_vn_by_name_and_parent(f, (uint8_t *)name, fbuf->fb_ino); > + if (v == NULL) { > + if (strcmp(name, ".") == 0) > + dir->d_fileno = fbuf->fb_ino; > + else > + dir->d_fileno = 0xffffffff; > + } else > + dir->d_fileno = v->ino; > + > + if (stbuf) > dir->d_type = IFTODT(stbuf->st_mode); > - } else { > - dir->d_fileno = 0xffffffff; > + else > dir->d_type = DT_UNKNOWN; > - } > + > dir->d_reclen = len; > dir->d_off = off + len; /* XXX */ > strlcpy(dir->d_name, name, sizeof(dir->d_name)); > @@ -295,7 +306,7 @@ ifuse_ops_readdir(struct fuse *f, struct > ffi.fh = fbuf->fb_io_fd; > offset = fbuf->fb_io_off; > size = fbuf->fb_io_len; > - startsave = 0; > + startsave = offset; > > fbuf->fb_dat = calloc(1, size); > > @@ -319,6 +330,8 @@ ifuse_ops_readdir(struct fuse *f, struct > vn->fd->size = size; > vn->fd->off = offset; > vn->fd->idx = 0; > + vn->fd->fuse = f; > + vn->fd->start = offset; > startsave = vn->fd->start; > > realname = build_realname(f, vn->ino); > @@ -345,7 +358,8 @@ ifuse_ops_readdir(struct fuse *f, struct > if (fbuf->fb_err) { > fbuf->fb_len = 0; > vn->fd->filled = 1; > - } > + } else if (vn->fd->full && fbuf->fb_len == 0) > + fbuf->fb_err = -ENOBUFS; > > if (fbuf->fb_len == 0) > free(fbuf->fb_dat); > 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); > 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); > } > Does this explain the problem and solution or do you need more information? Is there any interest from the developers in patches to improve fuse? Let me know and I'll continue to work on it. -- Helg <[email protected]>
