On 23/11/17(Thu) 21:45, Helg wrote:
> On Thu, Nov 23, 2017 at 12:09:34PM +0000, Helg Bredow wrote:
> > ----- Forwarded message from Martin Pieuchot <m...@openbsd.org> -----
> >
> > Date: Sat, 18 Nov 2017 11:03:49 +0100
> > From: Martin Pieuchot <m...@openbsd.org>
> > To: Helg Bredow <xx...@msn.com>
> > CC: "tech@openbsd.org" <tech@openbsd.org>
> > Subject: Re: fuse: vfs create does not map 1:1 to fuse create
> > User-Agent: Mutt/1.9.1 (2017-09-22)
> >
> > On 18/11/17(Sat) 02:22, Helg Bredow wrote:
> > > On Fri, 10 Nov 2017 09:09:32 +0100
> > > Martin Pieuchot <m...@openbsd.org> wrote:
> > >
> > > > On 09/11/17(Thu) 01:20, Helg Bredow wrote:
> > > > > > On 08/11/17(Wed) 14:12, Helg Bredow wrote:
> > > > > > > There is a bug when creating a file in fuse-exfat and then
> > > > > > > deleting it
> > > > > > > again without first unmounting the file system. The reason for
> > > > > > > this is
> > > > > > > that fuse-exfat maintains strict reference counts and fuse
> > > > > > > currently
> > > > > > > calls the file system create and open functions when it should
> > > > > > > only
> > > > > > > call create.
> > > > > > > [...]
> > > > > >
> > > [...]
> > > >
> [...]
> > >
> > > I now this caused some confusion but I believe the patch below is the
> > > correct solution.
> >
> > When you use the work "believing" you're asking us to trust you without
> > argumenting.
> >
> > > Here's the mapping of file system calls so it's clear.
> > > [...]
> >
> > That's irrelevant.
> >
> > There's no such thing as "atomic_open" from the userland point of view.
> > So what you're discussing here is an "implementation details" of Linux
> > VFS.
> >
> > What matters is which syscall the application do. I believe it is
> > open(2). So how its arguments are translated to the userland FS? If
> > I understand your diff correctly, OpenBSD will now do something like
> > below (please correct the graph):
> >
> >
> > USER PROCESS fuse-zip
> > | ^
> > open(2) |
> > | FBT_MKNOD + FBT_OPEN
> > \/ |
> > ---------------------------------
> >
> > KERNEL
> >
> > The only thing I understand is that you're changing the kernel behavior
> > for all open(2) syscall operating on FUSE filesystem. Before the kernel
> > was generating a FBT_CREATE and FBT_OPEN messages, with some arguments.
> > Now you want to generate FTB_MKNOD and FBT_OPEN with other arguments.
> >
> > To me it sounds that you're working around a problem you don't
> > understand. If FUSE FSs want a FUSE 'create' operation then let's
> > give them that! What argument do they expect?
> >
> > So let's start from the beginning:
> >
> > - Which syscall is causing problem with which arguments?
> > - Which FUSE operation(s) FUSE FSs are expecting for this exact syscall
> > with these arguments?
> > - Which FUSE operation(s) OpenBSD FUSE is currently generating with which
> > arguments?
>
> The syscall that is causing the problem is open(2) with the O_CREAT flag.
> With my change, this is the call graph.
>
> open(2)
> |
> vn_open()
> |
> +--VOP_CREATE(9) when O_CREAT
> | |
> | +-->fusefs_create()-->FBT_MKNOD-->ifuse_ops_mknod()
> | |
> | +--> fs mknod()
> |
> +--VOP_SETATTR(9) when O_TRUNC
> | |
> | +-->fusefs_setattr()-->FBT_SETATTR-->ifuse_ops_setattr()
> | |
> | +--> fs truncate()
> |
> +--VOP_OPEN(9) always
> |
> +--fusefs_open()-->FBT_OPEN-->ifuse_ops_open()
> |
> +--> fs open()
>
>
> The file systems are expecting the following arguments.
>
> mknod(path, mode, dev)
> mode is that passed to open(2)
> dev will be 0 for regular files. FUSE mknod can create regular files.
> open(path, fuse_file_info)
> fuse_file_info has (almost all) flags passed to open(2) plus other
> things that are not set by ifuse_ops_open. The FS will also return
> a handle to the file that was opened which it expects to receive
> again for functions that operate on the same file.
> create(path, mode, fuse_file_info)
> mode is that passed to open(2)
> fuse_file_info has (almost all) flags passed to open(2) plus other
> things that are not set by ifuse_ops_open. The FS will also return
> a handle to the file that was opened which it expects to receive
> again for functions that operate on the same file.
>
> OpenBSD FUSE is currently generating the following sequence of FUSE
> calls when creating a file with open(2).
>
> FBT_CREATE + FBT_OPEN
> create arguments are hard-coded to O_CREAT | O_RDWR
> open arguments are either O_RDONLY, O_RDWR, O_WRONLY depending on
> mode
>
> The problem is in vn_open(9). It does not pass the flags to VOP_CREATE
> so we don't have them in fusefs_create(). We could defer calling create
> when fusefs_open() is called but then we don't have the mode. We could
> store the mode in fusefs_create() but create operates on a directory
> vnode and this is not locked when fusefs_open() is called so we would
> have to lock it ourselves. I don't fancy messing with vnode locks
> right now.
>
> FUSE's FS create() needs open(2)'s `flags' and `mode_t'.
>
> These are the options we have.
>
> 1. Pass more arguments to VOP_CREATE(9)
>
> 2. Delay sending FBT_CREATE from kernel to userland and send it instead
> of FBT_OPEN if O_CREAT flag is passed to open(2)
>
> 3. Don't support FBT_CREATE in the OpenBSD FUSE implementation
>
> I've chosen option 3. because this has the least risk and sends a valid
> sequence of messages (FBT_MKNOD + FBT_OPEN) from the kernel to userland.
> This will get us by until FUSE is more stable and I can revisit it
> later.
>
> This patch is the same as I previously sent but with more comments.
I agree option 3 is the best for the moment. We should try to
consolidate our FUSE implementation without extending the VFS
first.
Some comments below.
> Index: sys/miscfs/fuse/fuse_vnops.c
> ===================================================================
> RCS file: /cvs/src/sys/miscfs/fuse/fuse_vnops.c,v
> retrieving revision 1.34
> diff -u -p -r1.34 fuse_vnops.c
> --- sys/miscfs/fuse/fuse_vnops.c 17 Nov 2017 15:45:17 -0000 1.34
> +++ sys/miscfs/fuse/fuse_vnops.c 23 Nov 2017 13:34:53 -0000
> @@ -211,7 +211,7 @@ fusefs_open(void *v)
> struct fusefs_node *ip;
> struct fusefs_mnt *fmp;
> enum fufh_type fufh_type = FUFH_RDONLY;
> - int flags = O_RDONLY;
> + int flags;
> int error;
> int isdir;
>
> @@ -226,24 +226,30 @@ fusefs_open(void *v)
> if (ap->a_vp->v_type == VDIR)
> isdir = 1;
> else {
> - if ((ap->a_mode & FREAD) && (ap->a_mode & FWRITE)) {
> + if ((ap->a_mode & FREAD) && (ap->a_mode & FWRITE))
> fufh_type = FUFH_RDWR;
> - flags = O_RDWR;
> - } else if (ap->a_mode & (FWRITE)) {
> + else if (ap->a_mode & (FWRITE))
> fufh_type = FUFH_WRONLY;
> - flags = O_WRONLY;
> - }
> }
>
> /* already open i think all is ok */
> if (ip->fufh[fufh_type].fh_type != FUFH_INVALID)
> return (0);
>
> + /*
> + * The file has already been created and/or truncated so FUSE dictates
> + * that no creation and truncation flags are passed to open.
> + */
> + flags = OFLAGS(ap->a_mode);
> + flags &= ~O_CREAT;
> + flags &= ~O_EXCL;
> + flags &= ~O_TRUNC;
Can't you write that on a single line?
flags = OFLAGS(ap->a_mode) & ~(O_CREAT|O_EXCL|O_TRUNC);
> +
> error = fusefs_file_open(fmp, ip, fufh_type, flags, isdir, ap->a_p);
> if (error)
> return (error);
>
> - return (error);
> + return (0);
> }
>
> int
> @@ -904,16 +910,15 @@ fusefs_create(void *v)
> goto out;
> }
>
> - if (fmp->undef_op & UNDEF_CREATE) {
> + if (fmp->undef_op & UNDEF_MKNOD) {
> error = ENOSYS;
> goto out;
> }
>
> fbuf = fb_setup(cnp->cn_namelen + 1, ip->ufs_ino.i_number,
> - FBT_CREATE, p);
> + FBT_MKNOD, p);
>
> fbuf->fb_io_mode = mode;
> - fbuf->fb_io_flags = O_CREAT | O_RDWR;
Why are you hardcoding RDWR?
>
> memcpy(fbuf->fb_dat, cnp->cn_nameptr, cnp->cn_namelen);
> fbuf->fb_dat[cnp->cn_namelen] = '\0';
> @@ -921,7 +926,7 @@ fusefs_create(void *v)
> error = fb_queue(fmp->dev, fbuf);
> if (error) {
> if (error == ENOSYS)
> - fmp->undef_op |= UNDEF_CREATE;
> + fmp->undef_op |= UNDEF_MKNOD;
>
> fb_delete(fbuf);
> goto out;
> Index: lib/libfuse/fuse_ops.c
> ===================================================================
> RCS file: /cvs/src/lib/libfuse/fuse_ops.c,v
> retrieving revision 1.27
> diff -u -p -r1.27 fuse_ops.c
> --- lib/libfuse/fuse_ops.c 17 Nov 2017 15:45:17 -0000 1.27
> +++ lib/libfuse/fuse_ops.c 23 Nov 2017 13:34:54 -0000
> @@ -579,6 +579,13 @@ ifuse_ops_write(struct fuse *f, struct f
> return (0);
> }
>
> +#if 0
> +/*
> + * vn_open(9) does not pass the open(2) flags to VOP_CREATE(9) so we can't
> + * support FBT_CREATE yet. The kernel will fall back to FBT_MKNOD + FBT_OPEN.
> + *
> + * See /sys/miscfs/fuse/fuse_vnops.c#fuse_create
> + */
Just remove the function, if people need this they can look at the
history.
> static int
> ifuse_ops_create(struct fuse *f, struct fusebuf *fbuf)
> {
> @@ -611,8 +618,6 @@ ifuse_ops_create(struct fuse *f, struct
>
> if (f->op.create)
> fbuf->fb_err = f->op.create(realname, mode, &ffi);
> - else if (f->op.mknod)
> - fbuf->fb_err = f->op.mknod(realname, S_IFREG | mode, 0);
> else
> fbuf->fb_err = -ENOSYS;
>
> @@ -625,6 +630,7 @@ ifuse_ops_create(struct fuse *f, struct
>
> return (0);
> }
> +#endif
>
> static int
> ifuse_ops_mkdir(struct fuse *f, struct fusebuf *fbuf)
> @@ -1125,9 +1131,11 @@ ifuse_exec_opcode(struct fuse *f, struct
> case FBT_ACCESS:
> ret = ifuse_ops_access(f, fbuf);
> break;
> +#if 0
> case FBT_CREATE:
> ret = ifuse_ops_create(f, fbuf);
> break;
> +#endif
Same here.
> case FBT_SYMLINK:
> ret = ifuse_ops_symlink(f, fbuf);
> break;
>
