On Mon, Nov 27, 2017 at 10:48:04AM +0100, Martin Pieuchot wrote:
> 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.
Thanks for all your patience so far mpi@, I really appreciate your help.
I've replaced the patch with an updated version so I'll address your
comments directly here.
> Can't you write that on a single line?
>
> flags = OFLAGS(ap->a_mode) & ~(O_CREAT|O_EXCL|O_TRUNC);
Done.
> > - fbuf->fb_io_flags = O_CREAT | O_RDWR;
>
> Why are you hardcoding RDWR?
I'm not. I deleted this line because FBT_MKNOD doesn't accept flags.
> Just remove the function, if people need this they can look at the
> history.
Done. I've also removed FBT_CREATE from sys/sys/fusebuf.h and
share/man/man9/fb_setup.9
Index: lib/libfuse/fuse_ops.c
===================================================================
RCS file: /cvs/src/lib/libfuse/fuse_ops.c,v
retrieving revision 1.27
diff -u -p -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 28 Nov 2017 14:43:48 -0000
@@ -580,53 +580,6 @@ ifuse_ops_write(struct fuse *f, struct f
}
static int
-ifuse_ops_create(struct fuse *f, struct fusebuf *fbuf)
-{
- struct fuse_file_info ffi;
- struct fuse_vnode *vn;
- uint32_t mode;
-
- char *realname;
-
- DPRINTF("Opcode:\tcreate\n");
- DPRINTF("Inode:\t%llu\n", (unsigned long long)fbuf->fb_ino);
-
- bzero(&ffi, sizeof(ffi));
- ffi.flags = fbuf->fb_io_flags;
- mode = fbuf->fb_io_mode;
-
- vn = get_vn_by_name_and_parent(f, fbuf->fb_dat, fbuf->fb_ino);
- if (vn == NULL) {
- fbuf->fb_err = -errno;
- free(fbuf->fb_dat);
- return (0);
- }
-
- free(fbuf->fb_dat);
- realname = build_realname(f, vn->ino);
- if (realname == NULL) {
- fbuf->fb_err = -errno;
- return (0);
- }
-
- 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;
-
- if (!fbuf->fb_err) {
- fbuf->fb_err = update_attr(f, &fbuf->fb_attr, realname, vn);
- fbuf->fb_ino = fbuf->fb_attr.st_ino;
- fbuf->fb_io_mode = fbuf->fb_attr.st_mode;
- }
- free(realname);
-
- return (0);
-}
-
-static int
ifuse_ops_mkdir(struct fuse *f, struct fusebuf *fbuf)
{
struct fuse_vnode *vn;
@@ -1124,9 +1077,6 @@ ifuse_exec_opcode(struct fuse *f, struct
break;
case FBT_ACCESS:
ret = ifuse_ops_access(f, fbuf);
- break;
- case FBT_CREATE:
- ret = ifuse_ops_create(f, fbuf);
break;
case FBT_SYMLINK:
ret = ifuse_ops_symlink(f, fbuf);
Index: sys/miscfs/fuse/fuse_vnops.c
===================================================================
RCS file: /cvs/src/sys/miscfs/fuse/fuse_vnops.c,v
retrieving revision 1.36
diff -u -p -u -p -r1.36 fuse_vnops.c
--- sys/miscfs/fuse/fuse_vnops.c 27 Nov 2017 22:55:17 -0000 1.36
+++ sys/miscfs/fuse/fuse_vnops.c 28 Nov 2017 14:43:49 -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,27 @@ 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) & ~(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 +907,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;
memcpy(fbuf->fb_dat, cnp->cn_nameptr, cnp->cn_namelen);
fbuf->fb_dat[cnp->cn_namelen] = '\0';
@@ -921,7 +923,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: sys/sys/fusebuf.h
===================================================================
RCS file: /cvs/src/sys/sys/fusebuf.h,v
retrieving revision 1.11
diff -u -p -u -p -r1.11 fusebuf.h
--- sys/sys/fusebuf.h 30 Aug 2016 16:45:54 -0000 1.11
+++ sys/sys/fusebuf.h 28 Nov 2017 14:43:49 -0000
@@ -126,7 +126,6 @@ struct fusebuf {
#define FBT_RELEASEDIR 22
#define FBT_FSYNCDIR 23
#define FBT_ACCESS 24
-#define FBT_CREATE 25
#define FBT_DESTROY 26
#define FBT_RECLAIM 27
Index: share/man/man9/fb_setup.9
===================================================================
RCS file: /cvs/src/share/man/man9/fb_setup.9,v
retrieving revision 1.6
diff -u -p -u -p -r1.6 fb_setup.9
--- share/man/man9/fb_setup.9 30 Aug 2016 16:45:54 -0000 1.6
+++ share/man/man9/fb_setup.9 28 Nov 2017 14:43:49 -0000
@@ -184,8 +184,6 @@ The fusebuf is a close dir operation.
The fusebuf is a dir sync operation.
.It Dv FBT_ACCESS
The fusebuf is an access operation.
-.It Dv FBT_CREATE
-The fusebuf is a create file operation.
.It Dv FBT_DESTROY
The fusebuf closes the FUSE connection.
.El
