| Message ID | 20240820211735.2098951-1-bschubert@ddn.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | fuse: Add open-gettr for fuse-file-open | expand |
On Tue, 20 Aug 2024 at 23:18, Bernd Schubert <bschubert@ddn.com> wrote: > > This is to update attributes on open to achieve close-to-open > coherency even if an inode has a attribute cache timeout. > > Signed-off-by: Bernd Schubert <bschubert@ddn.com> > > --- > libfuse patch: > https://github.com/libfuse/libfuse/pull/1020 > (FUSE_OPENDIR_GETATTR still missing at time of writing) > > Note: This does not make use of existing atomic-open patches > as these are more complex than two new opcodes for open-getattr. The format of the requests would be very similar to the atomic open ones, right? Thanks, Miklos
On 8/21/24 16:28, Miklos Szeredi wrote: > On Tue, 20 Aug 2024 at 23:18, Bernd Schubert <bschubert@ddn.com> wrote: >> >> This is to update attributes on open to achieve close-to-open >> coherency even if an inode has a attribute cache timeout. >> >> Signed-off-by: Bernd Schubert <bschubert@ddn.com> >> >> --- >> libfuse patch: >> https://github.com/libfuse/libfuse/pull/1020 >> (FUSE_OPENDIR_GETATTR still missing at time of writing) >> >> Note: This does not make use of existing atomic-open patches >> as these are more complex than two new opcodes for open-getattr. > > The format of the requests would be very similar to the atomic open ones, right? Atomic open patches are using fuse_open_out + fuse_entry_out open-getattr is using fuse_open_out + attr_outarg We could switch open-getattr to the atomic-open format, but then need to introduce a flag to tell fuse-server that this is a plain atomic and much simpler than atomic-open (atomic-open is also on the server side rather complex). For open-getattr we need don't need to revalidate the entry with all the fields provided by fuse_entry_out. Fine with me if you prefer a new struct to be used by atomic-open and open-getattr with a flag and padding. Like enum atomic_open_flags { ATOMIC_OPEN_IS_OPEN_GETATTR = 1ULL < 0; }; struct atomic_open { uint64_t atomic_open_flags; struct fuse_open_out open_out; uint8_t future_padding1[16]; struct fuse_entry_out entry_out; uint8_t future_padding2[16]; } What do you think? Thanks, Bernd
On Wed, 21 Aug 2024 at 16:44, Bernd Schubert <bernd.schubert@fastmail.fm> wrote: > struct atomic_open > { > uint64_t atomic_open_flags; > struct fuse_open_out open_out; > uint8_t future_padding1[16]; > struct fuse_entry_out entry_out; > uint8_t future_padding2[16]; > } > > > What do you think? I'm wondering if something like the "compound procedure" in NFSv4 would work for fuse as well? Thanks, Miklos
On 8/21/24 17:03, Miklos Szeredi wrote: > On Wed, 21 Aug 2024 at 16:44, Bernd Schubert <bernd.schubert@fastmail.fm> wrote: > >> struct atomic_open >> { >> uint64_t atomic_open_flags; >> struct fuse_open_out open_out; >> uint8_t future_padding1[16]; >> struct fuse_entry_out entry_out; >> uint8_t future_padding2[16]; >> } >> >> >> What do you think? > > I'm wondering if something like the "compound procedure" in NFSv4 > would work for fuse as well? I need to figure out how that actually works in NFS. Do you have a suggestion how to bundle open + getattr in one request with compounds? My thinking is that we need something like #define MAX_OPS 8 // abitrary value #define MAX_OP_OUT_SZ struct op { uint32_t opcode; uint32_t arg_sz; void *arg; }; struct fuse_compound { uint32_t n_ops; struct op ops[MAX_OPS]; } But how would fuse_file_open() know how to send a sequence of requests? I don't see an issue to decode that on the server side into multiple requests, but how do we process the result in fuse-client? For fg requests we have exactly request that gets processed by the vfs operaration sending a request - how would that look like with compounds? Or do I totally misunderstand you and you want to use compounds to avoid the uber struct for atomic-open? At least we still need to add in the ATOMIC_OPEN_IS_OPEN_GETATTR flag there and actually another like ATOMIC_OPEN_IS_DIRECTORY. Thanks, Bernd
On Wed, 21 Aug 2024 at 17:55, Bernd Schubert <bernd.schubert@fastmail.fm> wrote: > But how would fuse_file_open() know how to send a sequence of requests? It would just send a FUSE_COMPOUND request, which contains a FUSE_GETATTR and a FUSE_OPEN request. > I don't see an issue to decode that on the server side into multiple > requests, but how do we process the result in fuse-client? For fg > requests we have exactly request that gets processed by the vfs > operaration sending a request - how would that look like with compounds? AFAIU compunds in NFS4 bundle multiple request into one which the server processes sequentially and the results are also returned in a bundle. That's just the protocol, the server and the client can use this in any way that conforms to the protocol. > > Or do I totally misunderstand you and you want to use compounds to avoid > the uber struct for atomic-open? At least we still need to add in the > ATOMIC_OPEN_IS_OPEN_GETATTR flag there and actually another like > ATOMIC_OPEN_IS_DIRECTORY. Yes, the main advantage would be to avoid having to add new request types for things that are actually just the aggregation of multiple existing request types. Thanks, Miklos
On 8/21/24 18:18, Miklos Szeredi wrote: > On Wed, 21 Aug 2024 at 17:55, Bernd Schubert <bernd.schubert@fastmail.fm> wrote: > >> But how would fuse_file_open() know how to send a sequence of requests? > > It would just send a FUSE_COMPOUND request, which contains a > FUSE_GETATTR and a FUSE_OPEN request. > >> I don't see an issue to decode that on the server side into multiple >> requests, but how do we process the result in fuse-client? For fg >> requests we have exactly request that gets processed by the vfs >> operaration sending a request - how would that look like with compounds? > > AFAIU compunds in NFS4 bundle multiple request into one which the > server processes sequentially and the results are also returned in a > bundle. > > That's just the protocol, the server and the client can use this in > any way that conforms to the protocol. > >> >> Or do I totally misunderstand you and you want to use compounds to avoid >> the uber struct for atomic-open? At least we still need to add in the >> ATOMIC_OPEN_IS_OPEN_GETATTR flag there and actually another like >> ATOMIC_OPEN_IS_DIRECTORY. > > Yes, the main advantage would be to avoid having to add new request > types for things that are actually just the aggregation of multiple > existing request types. Thanks, that simplifies things for fuse kernel. I'm still thinking about what is the best way for fuse server. Traveling (back home) the next days - will come with a new patch next week. Thanks, Bernd
On Tue, Aug 20, 2024 at 2:18 PM Bernd Schubert <bschubert@ddn.com> wrote: > > This is to update attributes on open to achieve close-to-open > coherency even if an inode has a attribute cache timeout. > > Signed-off-by: Bernd Schubert <bschubert@ddn.com> > > --- > libfuse patch: > https://github.com/libfuse/libfuse/pull/1020 > (FUSE_OPENDIR_GETATTR still missing at time of writing) > > Note: This does not make use of existing atomic-open patches > as these are more complex than two new opcodes for open-getattr. > > Note2: This is an alternative to Joannes patch that adds > FOPEN_FETCH_ATTR, which would need to kernel/userspace transitions > https://lore.kernel.org/all/20240813212149.1909627-1-joannelkoong@gmail.com/ > > Question for reviewers: > - Should this better use statx fields? Probably not needed for > coherency? > - Should this introduce a new struct that contains > struct fuse_open_out and struct fuse_attr_out, with > additional padding between them to avoid incompat issues > if either struct should be extended? > --- > fs/fuse/file.c | 94 ++++++++++++++++++++++++++++++++++++++- > fs/fuse/fuse_i.h | 7 +++ > fs/fuse/ioctl.c | 2 +- > include/uapi/linux/fuse.h | 5 +++ > 4 files changed, 105 insertions(+), 3 deletions(-) > > diff --git a/fs/fuse/file.c b/fs/fuse/file.c > index f39456c65ed7..d470e6a2b3d4 100644 > --- a/fs/fuse/file.c > +++ b/fs/fuse/file.c > @@ -51,6 +51,78 @@ static int fuse_send_open(struct fuse_mount *fm, u64 nodeid, > return fuse_simple_request(fm, &args); > } > > +/* > + * Open the file and update inode attributes > + */ > +static int fuse_file_open_getattr(struct fuse_mount *fm, u64 nodeid, > + struct inode *inode, unsigned int open_flags, > + int opcode, > + struct fuse_open_out *open_outargp) > +{ > + struct fuse_conn *fc = fm->fc; > + u64 attr_version = fuse_get_attr_version(fc); > + struct fuse_open_in inarg; > + struct fuse_attr_out attr_outarg; > + FUSE_ARGS(args); > + int err; > + > + /* convert the opcode from plain open to open-with-getattr */ > + if (opcode == FUSE_OPEN) { > + if (fc->no_open_getattr) > + return -ENOSYS; > + opcode = FUSE_OPEN_GETATTR; > + } else { > + if (fc->no_opendir_getattr) > + return -ENOSYS; > + opcode = FUSE_OPENDIR_GETATTR; > + } > + > + memset(&inarg, 0, sizeof(inarg)); > + inarg.flags = open_flags & ~(O_CREAT | O_EXCL | O_NOCTTY); > + if (!fm->fc->atomic_o_trunc) > + inarg.flags &= ~O_TRUNC; > + > + if (fm->fc->handle_killpriv_v2 && > + (inarg.flags & O_TRUNC) && !capable(CAP_FSETID)) { > + inarg.open_flags |= FUSE_OPEN_KILL_SUIDGID; > + } > + > + args.opcode = opcode; > + args.nodeid = nodeid; > + args.in_numargs = 1; > + args.in_args[0].size = sizeof(inarg); > + args.in_args[0].value = &inarg; > + args.out_numargs = 2; > + args.out_args[0].size = sizeof(*open_outargp); > + args.out_args[0].value = open_outargp; > + args.out_args[1].size = sizeof(attr_outarg); > + args.out_args[1].value = &attr_outarg; > + > + err = fuse_simple_request(fm, &args); > + if (err) { > + if (err == -ENOSYS) { > + if (opcode == FUSE_OPEN) > + fc->no_open_getattr = 1; > + else > + fc->no_opendir_getattr = 1; > + } > + return err; > + } > + > + err = -EIO; > + if (fuse_invalid_attr(&attr_outarg.attr) || > + inode_wrong_type(inode, attr_outarg.attr.mode)) { > + fuse_make_bad(inode); > + return err; > + } > + > + fuse_change_attributes(inode, &attr_outarg.attr, NULL, > + ATTR_TIMEOUT(&attr_outarg), attr_version); > + Hi Bernrd, For my use case, it'd be preferred if this could be gated by an FOPEN flag that the server can set in the reply if it doesn't want to opt into the refreshed attributes (eg if this flag is set, then the getattr values in the reply are blank and the kernel should skip fuse_change_attributes()). For the use case I have, the attributes only need to be fetched and changed for O_APPENDs where it'd be ideal if we could skip this overhead for non O_APPENDs. This flag can be added separately after your patch lands, but just wanted to note this now as a heads-up. I'm also happy to add this in later if it's better to do this later than as part of this patch. Thanks, Joanne
diff --git a/fs/fuse/file.c b/fs/fuse/file.c index f39456c65ed7..d470e6a2b3d4 100644 --- a/fs/fuse/file.c +++ b/fs/fuse/file.c @@ -51,6 +51,78 @@ static int fuse_send_open(struct fuse_mount *fm, u64 nodeid, return fuse_simple_request(fm, &args); } +/* + * Open the file and update inode attributes + */ +static int fuse_file_open_getattr(struct fuse_mount *fm, u64 nodeid, + struct inode *inode, unsigned int open_flags, + int opcode, + struct fuse_open_out *open_outargp) +{ + struct fuse_conn *fc = fm->fc; + u64 attr_version = fuse_get_attr_version(fc); + struct fuse_open_in inarg; + struct fuse_attr_out attr_outarg; + FUSE_ARGS(args); + int err; + + /* convert the opcode from plain open to open-with-getattr */ + if (opcode == FUSE_OPEN) { + if (fc->no_open_getattr) + return -ENOSYS; + opcode = FUSE_OPEN_GETATTR; + } else { + if (fc->no_opendir_getattr) + return -ENOSYS; + opcode = FUSE_OPENDIR_GETATTR; + } + + memset(&inarg, 0, sizeof(inarg)); + inarg.flags = open_flags & ~(O_CREAT | O_EXCL | O_NOCTTY); + if (!fm->fc->atomic_o_trunc) + inarg.flags &= ~O_TRUNC; + + if (fm->fc->handle_killpriv_v2 && + (inarg.flags & O_TRUNC) && !capable(CAP_FSETID)) { + inarg.open_flags |= FUSE_OPEN_KILL_SUIDGID; + } + + args.opcode = opcode; + args.nodeid = nodeid; + args.in_numargs = 1; + args.in_args[0].size = sizeof(inarg); + args.in_args[0].value = &inarg; + args.out_numargs = 2; + args.out_args[0].size = sizeof(*open_outargp); + args.out_args[0].value = open_outargp; + args.out_args[1].size = sizeof(attr_outarg); + args.out_args[1].value = &attr_outarg; + + err = fuse_simple_request(fm, &args); + if (err) { + if (err == -ENOSYS) { + if (opcode == FUSE_OPEN) + fc->no_open_getattr = 1; + else + fc->no_opendir_getattr = 1; + } + return err; + } + + err = -EIO; + if (fuse_invalid_attr(&attr_outarg.attr) || + inode_wrong_type(inode, attr_outarg.attr.mode)) { + fuse_make_bad(inode); + return err; + } + + fuse_change_attributes(inode, &attr_outarg.attr, NULL, + ATTR_TIMEOUT(&attr_outarg), attr_version); + + return 0; +} + + struct fuse_file *fuse_file_alloc(struct fuse_mount *fm, bool release) { struct fuse_file *ff; @@ -123,7 +195,12 @@ static void fuse_file_put(struct fuse_file *ff, bool sync) } } +/* + * @inode might be NULL, the caller indicates the attr updates are not + * needed on open + */ struct fuse_file *fuse_file_open(struct fuse_mount *fm, u64 nodeid, + struct inode *inode, unsigned int open_flags, bool isdir) { struct fuse_conn *fc = fm->fc; @@ -143,7 +220,19 @@ struct fuse_file *fuse_file_open(struct fuse_mount *fm, u64 nodeid, struct fuse_open_out *outargp = &ff->args->open_outarg; int err; - err = fuse_send_open(fm, nodeid, open_flags, opcode, outargp); + err = -ENOSYS; + if (inode) { + /* + * open-with-getattr preferred for + * close-to-open coherency + */ + err = fuse_file_open_getattr(fm, nodeid, inode, + open_flags, opcode, + outargp); + } + if (err == -ENOSYS) + err = fuse_send_open(fm, nodeid, open_flags, opcode, + outargp); if (!err) { ff->fh = outargp->fh; ff->open_flags = outargp->open_flags; @@ -172,7 +261,8 @@ struct fuse_file *fuse_file_open(struct fuse_mount *fm, u64 nodeid, int fuse_do_open(struct fuse_mount *fm, u64 nodeid, struct file *file, bool isdir) { - struct fuse_file *ff = fuse_file_open(fm, nodeid, file->f_flags, isdir); + struct fuse_file *ff = fuse_file_open(fm, nodeid, file_inode(file), + file->f_flags, isdir); if (!IS_ERR(ff)) file->private_data = ff; diff --git a/fs/fuse/fuse_i.h b/fs/fuse/fuse_i.h index f23919610313..5c0ea7ce0b4a 100644 --- a/fs/fuse/fuse_i.h +++ b/fs/fuse/fuse_i.h @@ -731,9 +731,15 @@ struct fuse_conn { /** Is open/release not implemented by fs? */ unsigned no_open:1; + /** Is open-getattr not implemented by fs */ + unsigned no_open_getattr:1; + /** Is opendir/releasedir not implemented by fs? */ unsigned no_opendir:1; + /** Is opendir-getattr not implemented by fs? */ + unsigned no_opendir_getattr:1; + /** Is fsync not implemented by fs? */ unsigned no_fsync:1; @@ -1404,6 +1410,7 @@ void fuse_file_io_release(struct fuse_file *ff, struct inode *inode); /* file.c */ struct fuse_file *fuse_file_open(struct fuse_mount *fm, u64 nodeid, + struct inode *inode, unsigned int open_flags, bool isdir); void fuse_file_release(struct inode *inode, struct fuse_file *ff, unsigned int open_flags, fl_owner_t id, bool isdir); diff --git a/fs/fuse/ioctl.c b/fs/fuse/ioctl.c index 572ce8a82ceb..d5322f6a904d 100644 --- a/fs/fuse/ioctl.c +++ b/fs/fuse/ioctl.c @@ -493,7 +493,7 @@ static struct fuse_file *fuse_priv_ioctl_prepare(struct inode *inode) if (!S_ISREG(inode->i_mode) && !isdir) return ERR_PTR(-ENOTTY); - return fuse_file_open(fm, get_node_id(inode), O_RDONLY, isdir); + return fuse_file_open(fm, get_node_id(inode), NULL, O_RDONLY, isdir); } static void fuse_priv_ioctl_cleanup(struct inode *inode, struct fuse_file *ff) diff --git a/include/uapi/linux/fuse.h b/include/uapi/linux/fuse.h index d08b99d60f6f..34b06cf62c16 100644 --- a/include/uapi/linux/fuse.h +++ b/include/uapi/linux/fuse.h @@ -217,6 +217,9 @@ * - add backing_id to fuse_open_out, add FOPEN_PASSTHROUGH open flag * - add FUSE_NO_EXPORT_SUPPORT init flag * - add FUSE_NOTIFY_RESEND, add FUSE_HAS_RESEND init flag + * + * 7.41 + * - add FUSE_OPEN_GETATTR/FUSE_OPENDIR_GETATTR */ #ifndef _LINUX_FUSE_H @@ -633,6 +636,8 @@ enum fuse_opcode { FUSE_SYNCFS = 50, FUSE_TMPFILE = 51, FUSE_STATX = 52, + FUSE_OPEN_GETATTR = 53, + FUSE_OPENDIR_GETATTR = 54, /* CUSE specific operations */ CUSE_INIT = 4096,
This is to update attributes on open to achieve close-to-open coherency even if an inode has a attribute cache timeout. Signed-off-by: Bernd Schubert <bschubert@ddn.com> --- libfuse patch: https://github.com/libfuse/libfuse/pull/1020 (FUSE_OPENDIR_GETATTR still missing at time of writing) Note: This does not make use of existing atomic-open patches as these are more complex than two new opcodes for open-getattr. Note2: This is an alternative to Joannes patch that adds FOPEN_FETCH_ATTR, which would need to kernel/userspace transitions https://lore.kernel.org/all/20240813212149.1909627-1-joannelkoong@gmail.com/ Question for reviewers: - Should this better use statx fields? Probably not needed for coherency? - Should this introduce a new struct that contains struct fuse_open_out and struct fuse_attr_out, with additional padding between them to avoid incompat issues if either struct should be extended? --- fs/fuse/file.c | 94 ++++++++++++++++++++++++++++++++++++++- fs/fuse/fuse_i.h | 7 +++ fs/fuse/ioctl.c | 2 +- include/uapi/linux/fuse.h | 5 +++ 4 files changed, 105 insertions(+), 3 deletions(-)