On Sat, 28 Oct 2017 09:50:35 +0200 Martin Pieuchot <[email protected]> wrote:
> 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". The logic was already there but it wasn't correct for all cases. The library also maintained state and I haven't introduced any new state management variables; I only adjusted how they are maintained. > > 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? > It may be possible to move some of the logic to the kernel, but not much. Note that the library doesn't maintain state across VOP_READDIR (9) calls; it only maintains state between kernel requests to keep filling the fbuf. The change I made is that the library now properly starts scanning from the supplied offset rather than from 0 every time. The situation happens whenever the buffer supplied to VOP_READDIR(9) is too small to contain the entire list of dirents. > > 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? > I don't think it would be difficult to add support for 'use_ino'. It's on my list. The OpenBSD implementation will always behave as if the 'readdir_ino' option was specified. Below is the updated diff without the redundant fb_delete(). 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 29 Oct 2017 13:54:35 -0000 @@ -217,12 +217,15 @@ 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); @@ -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); @@ -314,11 +325,12 @@ ifuse_ops_readdir(struct fuse *f, struct if (!vn->fd->filled) { vn->fd->filler = ifuse_fill_readdir; vn->fd->buf = fbuf; - vn->fd->filled = 0; vn->fd->full = 0; 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 +357,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 29 Oct 2017 13:54:35 -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,17 @@ fusefs_readdir(void *v) if (uio->uio_resid < sizeof(struct dirent)) return (EINVAL); + if (ip->fufh[FUFH_RDONLY].fh_type == FUFH_INVALID) { + error = fusefs_file_open(fmp, ip, FUFH_RDONLY, O_RDONLY, 1, p); + if (error) + return (error); + + diropen = 1; + } + 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 */ - fb_delete(fbuf); - return (error); - } 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 +713,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 +741,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); }
