| Message ID | 20241107-fuse-uring-for-6-10-rfc4-v5-5-e8660a991499@ddn.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | fuse: fuse-over-io-uring | expand |
On Thu, Nov 7, 2024 at 9:04 AM Bernd Schubert <bschubert@ddn.com> wrote: > > This change sets up FUSE operations to have headers in args.in_args[0], > even for opcodes without an actual header. We do this to prepare for > cleanly separating payload from headers in the future. > > For opcodes without a header, we use a zero-sized struct as a > placeholder. This approach: > - Keeps things consistent across all FUSE operations > - Will help with payload alignment later > - Avoids future issues when header sizes change > > Signed-off-by: Bernd Schubert <bschubert@ddn.com> > --- > fs/fuse/dax.c | 13 ++++++++----- > fs/fuse/dev.c | 24 ++++++++++++++++++++---- > fs/fuse/dir.c | 41 +++++++++++++++++++++++++++-------------- > fs/fuse/fuse_i.h | 7 +++++++ > fs/fuse/xattr.c | 9 ++++++--- > 5 files changed, 68 insertions(+), 26 deletions(-) > > diff --git a/fs/fuse/dax.c b/fs/fuse/dax.c > index 12ef91d170bb3091ac35a33d2b9dc38330b00948..e459b8134ccb089f971bebf8da1f7fc5199c1271 100644 > --- a/fs/fuse/dax.c > +++ b/fs/fuse/dax.c > @@ -237,14 +237,17 @@ static int fuse_send_removemapping(struct inode *inode, > struct fuse_inode *fi = get_fuse_inode(inode); > struct fuse_mount *fm = get_fuse_mount(inode); > FUSE_ARGS(args); > + struct fuse_zero_in zero_arg; > > args.opcode = FUSE_REMOVEMAPPING; > args.nodeid = fi->nodeid; > - args.in_numargs = 2; > - args.in_args[0].size = sizeof(*inargp); > - args.in_args[0].value = inargp; > - args.in_args[1].size = inargp->count * sizeof(*remove_one); > - args.in_args[1].value = remove_one; > + args.in_numargs = 3; > + args.in_args[0].size = sizeof(zero_arg); > + args.in_args[0].value = &zero_arg; > + args.in_args[1].size = sizeof(*inargp); > + args.in_args[1].value = inargp; > + args.in_args[2].size = inargp->count * sizeof(*remove_one); > + args.in_args[2].value = remove_one; > return fuse_simple_request(fm, &args); > } > > diff --git a/fs/fuse/dev.c b/fs/fuse/dev.c > index dbc222f9b0f0e590ce3ef83077e6b4cff03cff65..6effef4073da3dad2f6140761eca98147a41d88d 100644 > --- a/fs/fuse/dev.c > +++ b/fs/fuse/dev.c > @@ -1007,6 +1007,19 @@ static int fuse_copy_args(struct fuse_copy_state *cs, unsigned numargs, > > for (i = 0; !err && i < numargs; i++) { > struct fuse_arg *arg = &args[i]; > + > + /* zero headers */ > + if (arg->size == 0) { > + if (WARN_ON_ONCE(i != 0)) { > + if (cs->req) > + pr_err_once( > + "fuse: zero size header in opcode %d\n", > + cs->req->in.h.opcode); > + return -EINVAL; > + } > + continue; > + } > + > if (i == numargs - 1 && argpages) > err = fuse_copy_pages(cs, arg->size, zeroing); > else > @@ -1662,6 +1675,7 @@ static int fuse_retrieve(struct fuse_mount *fm, struct inode *inode, > size_t args_size = sizeof(*ra); > struct fuse_args_pages *ap; > struct fuse_args *args; > + struct fuse_zero_in zero_arg; > > offset = outarg->offset & ~PAGE_MASK; > file_size = i_size_read(inode); > @@ -1688,7 +1702,7 @@ static int fuse_retrieve(struct fuse_mount *fm, struct inode *inode, > args = &ap->args; > args->nodeid = outarg->nodeid; > args->opcode = FUSE_NOTIFY_REPLY; > - args->in_numargs = 2; > + args->in_numargs = 3; > args->in_pages = true; > args->end = fuse_retrieve_end; > > @@ -1715,9 +1729,11 @@ static int fuse_retrieve(struct fuse_mount *fm, struct inode *inode, > } > ra->inarg.offset = outarg->offset; > ra->inarg.size = total_len; > - args->in_args[0].size = sizeof(ra->inarg); > - args->in_args[0].value = &ra->inarg; > - args->in_args[1].size = total_len; > + args->in_args[0].size = sizeof(zero_arg); > + args->in_args[0].value = &zero_arg; > + args->in_args[1].size = sizeof(ra->inarg); > + args->in_args[1].value = &ra->inarg; > + args->in_args[2].size = total_len; > > err = fuse_simple_notify_reply(fm, args, outarg->notify_unique); > if (err) Do we also need to add a zero arg header for FUSE_READLINK, FUSE_DESTROY, and FUSE_BATCH_FORGET requests as well? Thanks, Joanne > diff --git a/fs/fuse/dir.c b/fs/fuse/dir.c > index 2b0d4781f39484d50d1fd7f4f673d8b08c5fd7cf..6d67d7f8e6b4460c759df3fb293e169bcc78a897 100644 > --- a/fs/fuse/dir.c > +++ b/fs/fuse/dir.c > @@ -172,12 +172,16 @@ static void fuse_lookup_init(struct fuse_conn *fc, struct fuse_args *args, > u64 nodeid, const struct qstr *name, > struct fuse_entry_out *outarg) > { > + struct fuse_zero_in zero_arg; > + > memset(outarg, 0, sizeof(struct fuse_entry_out)); > args->opcode = FUSE_LOOKUP; > args->nodeid = nodeid; > - args->in_numargs = 1; > - args->in_args[0].size = name->len + 1; > - args->in_args[0].value = name->name; > + args->in_numargs = 2; > + args->in_args[0].size = sizeof(zero_arg); > + args->in_args[0].value = &zero_arg; > + args->in_args[1].size = name->len + 1; > + args->in_args[1].value = name->name; > args->out_numargs = 1; > args->out_args[0].size = sizeof(struct fuse_entry_out); > args->out_args[0].value = outarg; > @@ -915,16 +919,19 @@ static int fuse_mkdir(struct mnt_idmap *idmap, struct inode *dir, > static int fuse_symlink(struct mnt_idmap *idmap, struct inode *dir, > struct dentry *entry, const char *link) > { > + struct fuse_zero_in zero_arg; > struct fuse_mount *fm = get_fuse_mount(dir); > unsigned len = strlen(link) + 1; > FUSE_ARGS(args); > > args.opcode = FUSE_SYMLINK; > - args.in_numargs = 2; > - args.in_args[0].size = entry->d_name.len + 1; > - args.in_args[0].value = entry->d_name.name; > - args.in_args[1].size = len; > - args.in_args[1].value = link; > + args.in_numargs = 3; > + args.in_args[0].size = sizeof(zero_arg); > + args.in_args[0].value = &zero_arg; > + args.in_args[1].size = entry->d_name.len + 1; > + args.in_args[1].value = entry->d_name.name; > + args.in_args[2].size = len; > + args.in_args[2].value = link; > return create_new_entry(fm, &args, dir, entry, S_IFLNK); > } > > @@ -975,6 +982,7 @@ static void fuse_entry_unlinked(struct dentry *entry) > > static int fuse_unlink(struct inode *dir, struct dentry *entry) > { > + struct fuse_zero_in inarg; > int err; > struct fuse_mount *fm = get_fuse_mount(dir); > FUSE_ARGS(args); > @@ -984,9 +992,11 @@ static int fuse_unlink(struct inode *dir, struct dentry *entry) > > args.opcode = FUSE_UNLINK; > args.nodeid = get_node_id(dir); > - args.in_numargs = 1; > - args.in_args[0].size = entry->d_name.len + 1; > - args.in_args[0].value = entry->d_name.name; > + args.in_numargs = 2; > + args.in_args[0].size = sizeof(inarg); > + args.in_args[0].value = &inarg; > + args.in_args[1].size = entry->d_name.len + 1; > + args.in_args[1].value = entry->d_name.name; > err = fuse_simple_request(fm, &args); > if (!err) { > fuse_dir_changed(dir); > @@ -998,6 +1008,7 @@ static int fuse_unlink(struct inode *dir, struct dentry *entry) > > static int fuse_rmdir(struct inode *dir, struct dentry *entry) > { > + struct fuse_zero_in zero_arg; > int err; > struct fuse_mount *fm = get_fuse_mount(dir); > FUSE_ARGS(args); > @@ -1007,9 +1018,11 @@ static int fuse_rmdir(struct inode *dir, struct dentry *entry) > > args.opcode = FUSE_RMDIR; > args.nodeid = get_node_id(dir); > - args.in_numargs = 1; > - args.in_args[0].size = entry->d_name.len + 1; > - args.in_args[0].value = entry->d_name.name; > + args.in_numargs = 2; > + args.in_args[0].size = sizeof(zero_arg); > + args.in_args[0].value = &zero_arg; > + args.in_args[1].size = entry->d_name.len + 1; > + args.in_args[1].value = entry->d_name.name; > err = fuse_simple_request(fm, &args); > if (!err) { > fuse_dir_changed(dir); > diff --git a/fs/fuse/fuse_i.h b/fs/fuse/fuse_i.h > index f2391961031374d8d55916c326c6472f0c03aae6..e2d1d90dfdb13b2c3e7de4789501ee45d3bf7794 100644 > --- a/fs/fuse/fuse_i.h > +++ b/fs/fuse/fuse_i.h > @@ -941,6 +941,13 @@ struct fuse_mount { > struct rcu_head rcu; > }; > > +/* > + * Empty header for FUSE opcodes without specific header needs. > + * Used as a placeholder in args->in_args[0] for consistency > + * across all FUSE operations, simplifying request handling. > + */ > +struct fuse_zero_in {}; > + > static inline struct fuse_mount *get_fuse_mount_super(struct super_block *sb) > { > return sb->s_fs_info; > diff --git a/fs/fuse/xattr.c b/fs/fuse/xattr.c > index 5b423fdbb13f8f17c3982e96dd0de836662092b0..2df1efd2e9bdb46571148f484d7927044f31c184 100644 > --- a/fs/fuse/xattr.c > +++ b/fs/fuse/xattr.c > @@ -158,15 +158,18 @@ int fuse_removexattr(struct inode *inode, const char *name) > struct fuse_mount *fm = get_fuse_mount(inode); > FUSE_ARGS(args); > int err; > + struct fuse_zero_in zero_arg; > > if (fm->fc->no_removexattr) > return -EOPNOTSUPP; > > args.opcode = FUSE_REMOVEXATTR; > args.nodeid = get_node_id(inode); > - args.in_numargs = 1; > - args.in_args[0].size = strlen(name) + 1; > - args.in_args[0].value = name; > + args.in_numargs = 2; > + args.in_args[0].size = sizeof(zero_arg); > + args.in_args[0].value = &zero_arg; > + args.in_args[1].size = strlen(name) + 1; > + args.in_args[1].value = name; > err = fuse_simple_request(fm, &args); > if (err == -ENOSYS) { > fm->fc->no_removexattr = 1; > > -- > 2.43.0 >
On 11/14/24 21:57, Joanne Koong wrote: > On Thu, Nov 7, 2024 at 9:04 AM Bernd Schubert <bschubert@ddn.com> wrote: >> >> This change sets up FUSE operations to have headers in args.in_args[0], >> even for opcodes without an actual header. We do this to prepare for >> cleanly separating payload from headers in the future. >> >> For opcodes without a header, we use a zero-sized struct as a >> placeholder. This approach: >> - Keeps things consistent across all FUSE operations >> - Will help with payload alignment later >> - Avoids future issues when header sizes change >> >> Signed-off-by: Bernd Schubert <bschubert@ddn.com> >> --- >> fs/fuse/dax.c | 13 ++++++++----- >> fs/fuse/dev.c | 24 ++++++++++++++++++++---- >> fs/fuse/dir.c | 41 +++++++++++++++++++++++++++-------------- >> fs/fuse/fuse_i.h | 7 +++++++ >> fs/fuse/xattr.c | 9 ++++++--- >> 5 files changed, 68 insertions(+), 26 deletions(-) >> >> diff --git a/fs/fuse/dax.c b/fs/fuse/dax.c >> index 12ef91d170bb3091ac35a33d2b9dc38330b00948..e459b8134ccb089f971bebf8da1f7fc5199c1271 100644 >> --- a/fs/fuse/dax.c >> +++ b/fs/fuse/dax.c >> @@ -237,14 +237,17 @@ static int fuse_send_removemapping(struct inode *inode, >> struct fuse_inode *fi = get_fuse_inode(inode); >> struct fuse_mount *fm = get_fuse_mount(inode); >> FUSE_ARGS(args); >> + struct fuse_zero_in zero_arg; >> >> args.opcode = FUSE_REMOVEMAPPING; >> args.nodeid = fi->nodeid; >> - args.in_numargs = 2; >> - args.in_args[0].size = sizeof(*inargp); >> - args.in_args[0].value = inargp; >> - args.in_args[1].size = inargp->count * sizeof(*remove_one); >> - args.in_args[1].value = remove_one; >> + args.in_numargs = 3; >> + args.in_args[0].size = sizeof(zero_arg); >> + args.in_args[0].value = &zero_arg; >> + args.in_args[1].size = sizeof(*inargp); >> + args.in_args[1].value = inargp; >> + args.in_args[2].size = inargp->count * sizeof(*remove_one); >> + args.in_args[2].value = remove_one; >> return fuse_simple_request(fm, &args); >> } >> >> diff --git a/fs/fuse/dev.c b/fs/fuse/dev.c >> index dbc222f9b0f0e590ce3ef83077e6b4cff03cff65..6effef4073da3dad2f6140761eca98147a41d88d 100644 >> --- a/fs/fuse/dev.c >> +++ b/fs/fuse/dev.c >> @@ -1007,6 +1007,19 @@ static int fuse_copy_args(struct fuse_copy_state *cs, unsigned numargs, >> >> for (i = 0; !err && i < numargs; i++) { >> struct fuse_arg *arg = &args[i]; >> + >> + /* zero headers */ >> + if (arg->size == 0) { >> + if (WARN_ON_ONCE(i != 0)) { >> + if (cs->req) >> + pr_err_once( >> + "fuse: zero size header in opcode %d\n", >> + cs->req->in.h.opcode); >> + return -EINVAL; >> + } >> + continue; >> + } >> + >> if (i == numargs - 1 && argpages) >> err = fuse_copy_pages(cs, arg->size, zeroing); >> else >> @@ -1662,6 +1675,7 @@ static int fuse_retrieve(struct fuse_mount *fm, struct inode *inode, >> size_t args_size = sizeof(*ra); >> struct fuse_args_pages *ap; >> struct fuse_args *args; >> + struct fuse_zero_in zero_arg; >> >> offset = outarg->offset & ~PAGE_MASK; >> file_size = i_size_read(inode); >> @@ -1688,7 +1702,7 @@ static int fuse_retrieve(struct fuse_mount *fm, struct inode *inode, >> args = &ap->args; >> args->nodeid = outarg->nodeid; >> args->opcode = FUSE_NOTIFY_REPLY; >> - args->in_numargs = 2; >> + args->in_numargs = 3; >> args->in_pages = true; >> args->end = fuse_retrieve_end; >> >> @@ -1715,9 +1729,11 @@ static int fuse_retrieve(struct fuse_mount *fm, struct inode *inode, >> } >> ra->inarg.offset = outarg->offset; >> ra->inarg.size = total_len; >> - args->in_args[0].size = sizeof(ra->inarg); >> - args->in_args[0].value = &ra->inarg; >> - args->in_args[1].size = total_len; >> + args->in_args[0].size = sizeof(zero_arg); >> + args->in_args[0].value = &zero_arg; >> + args->in_args[1].size = sizeof(ra->inarg); >> + args->in_args[1].value = &ra->inarg; >> + args->in_args[2].size = total_len; >> >> err = fuse_simple_notify_reply(fm, args, outarg->notify_unique); >> if (err) > > Do we also need to add a zero arg header for FUSE_READLINK, > FUSE_DESTROY, and FUSE_BATCH_FORGET requests as well? > Thanks for looking at the patch! I should have added to the commit message that I didn't modify these, as they don't have an in argument at all. Thanks, Bernd
On Thu, Nov 14, 2024 at 1:05 PM Bernd Schubert <bernd.schubert@fastmail.fm> wrote: > > > > On 11/14/24 21:57, Joanne Koong wrote: > > On Thu, Nov 7, 2024 at 9:04 AM Bernd Schubert <bschubert@ddn.com> wrote: > >> > >> This change sets up FUSE operations to have headers in args.in_args[0], > >> even for opcodes without an actual header. We do this to prepare for > >> cleanly separating payload from headers in the future. > >> > >> For opcodes without a header, we use a zero-sized struct as a > >> placeholder. This approach: > >> - Keeps things consistent across all FUSE operations > >> - Will help with payload alignment later > >> - Avoids future issues when header sizes change > >> > >> Signed-off-by: Bernd Schubert <bschubert@ddn.com> > >> --- > >> fs/fuse/dax.c | 13 ++++++++----- > >> fs/fuse/dev.c | 24 ++++++++++++++++++++---- > >> fs/fuse/dir.c | 41 +++++++++++++++++++++++++++-------------- > >> fs/fuse/fuse_i.h | 7 +++++++ > >> fs/fuse/xattr.c | 9 ++++++--- > >> 5 files changed, 68 insertions(+), 26 deletions(-) > >> > >> diff --git a/fs/fuse/dax.c b/fs/fuse/dax.c > >> index 12ef91d170bb3091ac35a33d2b9dc38330b00948..e459b8134ccb089f971bebf8da1f7fc5199c1271 100644 > >> --- a/fs/fuse/dax.c > >> +++ b/fs/fuse/dax.c > >> @@ -237,14 +237,17 @@ static int fuse_send_removemapping(struct inode *inode, > >> struct fuse_inode *fi = get_fuse_inode(inode); > >> struct fuse_mount *fm = get_fuse_mount(inode); > >> FUSE_ARGS(args); > >> + struct fuse_zero_in zero_arg; > >> > >> args.opcode = FUSE_REMOVEMAPPING; > >> args.nodeid = fi->nodeid; > >> - args.in_numargs = 2; > >> - args.in_args[0].size = sizeof(*inargp); > >> - args.in_args[0].value = inargp; > >> - args.in_args[1].size = inargp->count * sizeof(*remove_one); > >> - args.in_args[1].value = remove_one; > >> + args.in_numargs = 3; > >> + args.in_args[0].size = sizeof(zero_arg); > >> + args.in_args[0].value = &zero_arg; > >> + args.in_args[1].size = sizeof(*inargp); > >> + args.in_args[1].value = inargp; > >> + args.in_args[2].size = inargp->count * sizeof(*remove_one); > >> + args.in_args[2].value = remove_one; > >> return fuse_simple_request(fm, &args); > >> } > >> > >> diff --git a/fs/fuse/dev.c b/fs/fuse/dev.c > >> index dbc222f9b0f0e590ce3ef83077e6b4cff03cff65..6effef4073da3dad2f6140761eca98147a41d88d 100644 > >> --- a/fs/fuse/dev.c > >> +++ b/fs/fuse/dev.c > >> @@ -1007,6 +1007,19 @@ static int fuse_copy_args(struct fuse_copy_state *cs, unsigned numargs, > >> > >> for (i = 0; !err && i < numargs; i++) { > >> struct fuse_arg *arg = &args[i]; > >> + > >> + /* zero headers */ > >> + if (arg->size == 0) { > >> + if (WARN_ON_ONCE(i != 0)) { > >> + if (cs->req) > >> + pr_err_once( > >> + "fuse: zero size header in opcode %d\n", > >> + cs->req->in.h.opcode); > >> + return -EINVAL; > >> + } > >> + continue; > >> + } > >> + > >> if (i == numargs - 1 && argpages) > >> err = fuse_copy_pages(cs, arg->size, zeroing); > >> else > >> @@ -1662,6 +1675,7 @@ static int fuse_retrieve(struct fuse_mount *fm, struct inode *inode, > >> size_t args_size = sizeof(*ra); > >> struct fuse_args_pages *ap; > >> struct fuse_args *args; > >> + struct fuse_zero_in zero_arg; > >> > >> offset = outarg->offset & ~PAGE_MASK; > >> file_size = i_size_read(inode); > >> @@ -1688,7 +1702,7 @@ static int fuse_retrieve(struct fuse_mount *fm, struct inode *inode, > >> args = &ap->args; > >> args->nodeid = outarg->nodeid; > >> args->opcode = FUSE_NOTIFY_REPLY; > >> - args->in_numargs = 2; > >> + args->in_numargs = 3; > >> args->in_pages = true; > >> args->end = fuse_retrieve_end; > >> > >> @@ -1715,9 +1729,11 @@ static int fuse_retrieve(struct fuse_mount *fm, struct inode *inode, > >> } > >> ra->inarg.offset = outarg->offset; > >> ra->inarg.size = total_len; > >> - args->in_args[0].size = sizeof(ra->inarg); > >> - args->in_args[0].value = &ra->inarg; > >> - args->in_args[1].size = total_len; > >> + args->in_args[0].size = sizeof(zero_arg); > >> + args->in_args[0].value = &zero_arg; > >> + args->in_args[1].size = sizeof(ra->inarg); > >> + args->in_args[1].value = &ra->inarg; > >> + args->in_args[2].size = total_len; > >> > >> err = fuse_simple_notify_reply(fm, args, outarg->notify_unique); > >> if (err) > > > > Do we also need to add a zero arg header for FUSE_READLINK, > > FUSE_DESTROY, and FUSE_BATCH_FORGET requests as well? > > > > Thanks for looking at the patch! I should have added to the commit message > that I didn't modify these, as they don't have an in argument at all. > Thanks for clarifying! (and apologies for the late review. I haven't been keeping up with these patches since RFC v3 but I'm planning to get up to speed and take a deeper look at these tomorrow + next week). I think the FUSE_BATCH_FORGET request does use in args, depending on the number of forget requests. Thanks, Joanne > > Thanks, > Bernd
On 11/14/24 22:29, Joanne Koong wrote: > On Thu, Nov 14, 2024 at 1:05 PM Bernd Schubert > <bernd.schubert@fastmail.fm> wrote: >> >> >> >> On 11/14/24 21:57, Joanne Koong wrote: >>> On Thu, Nov 7, 2024 at 9:04 AM Bernd Schubert <bschubert@ddn.com> wrote: >>>> >>>> This change sets up FUSE operations to have headers in args.in_args[0], >>>> even for opcodes without an actual header. We do this to prepare for >>>> cleanly separating payload from headers in the future. >>>> >>>> For opcodes without a header, we use a zero-sized struct as a >>>> placeholder. This approach: >>>> - Keeps things consistent across all FUSE operations >>>> - Will help with payload alignment later >>>> - Avoids future issues when header sizes change >>>> >>>> Signed-off-by: Bernd Schubert <bschubert@ddn.com> >>>> --- >>>> fs/fuse/dax.c | 13 ++++++++----- >>>> fs/fuse/dev.c | 24 ++++++++++++++++++++---- >>>> fs/fuse/dir.c | 41 +++++++++++++++++++++++++++-------------- >>>> fs/fuse/fuse_i.h | 7 +++++++ >>>> fs/fuse/xattr.c | 9 ++++++--- >>>> 5 files changed, 68 insertions(+), 26 deletions(-) >>>> >>>> diff --git a/fs/fuse/dax.c b/fs/fuse/dax.c >>>> index 12ef91d170bb3091ac35a33d2b9dc38330b00948..e459b8134ccb089f971bebf8da1f7fc5199c1271 100644 >>>> --- a/fs/fuse/dax.c >>>> +++ b/fs/fuse/dax.c >>>> @@ -237,14 +237,17 @@ static int fuse_send_removemapping(struct inode *inode, >>>> struct fuse_inode *fi = get_fuse_inode(inode); >>>> struct fuse_mount *fm = get_fuse_mount(inode); >>>> FUSE_ARGS(args); >>>> + struct fuse_zero_in zero_arg; >>>> >>>> args.opcode = FUSE_REMOVEMAPPING; >>>> args.nodeid = fi->nodeid; >>>> - args.in_numargs = 2; >>>> - args.in_args[0].size = sizeof(*inargp); >>>> - args.in_args[0].value = inargp; >>>> - args.in_args[1].size = inargp->count * sizeof(*remove_one); >>>> - args.in_args[1].value = remove_one; >>>> + args.in_numargs = 3; >>>> + args.in_args[0].size = sizeof(zero_arg); >>>> + args.in_args[0].value = &zero_arg; >>>> + args.in_args[1].size = sizeof(*inargp); >>>> + args.in_args[1].value = inargp; >>>> + args.in_args[2].size = inargp->count * sizeof(*remove_one); >>>> + args.in_args[2].value = remove_one; >>>> return fuse_simple_request(fm, &args); >>>> } >>>> >>>> diff --git a/fs/fuse/dev.c b/fs/fuse/dev.c >>>> index dbc222f9b0f0e590ce3ef83077e6b4cff03cff65..6effef4073da3dad2f6140761eca98147a41d88d 100644 >>>> --- a/fs/fuse/dev.c >>>> +++ b/fs/fuse/dev.c >>>> @@ -1007,6 +1007,19 @@ static int fuse_copy_args(struct fuse_copy_state *cs, unsigned numargs, >>>> >>>> for (i = 0; !err && i < numargs; i++) { >>>> struct fuse_arg *arg = &args[i]; >>>> + >>>> + /* zero headers */ >>>> + if (arg->size == 0) { >>>> + if (WARN_ON_ONCE(i != 0)) { >>>> + if (cs->req) >>>> + pr_err_once( >>>> + "fuse: zero size header in opcode %d\n", >>>> + cs->req->in.h.opcode); >>>> + return -EINVAL; >>>> + } >>>> + continue; >>>> + } >>>> + >>>> if (i == numargs - 1 && argpages) >>>> err = fuse_copy_pages(cs, arg->size, zeroing); >>>> else >>>> @@ -1662,6 +1675,7 @@ static int fuse_retrieve(struct fuse_mount *fm, struct inode *inode, >>>> size_t args_size = sizeof(*ra); >>>> struct fuse_args_pages *ap; >>>> struct fuse_args *args; >>>> + struct fuse_zero_in zero_arg; >>>> >>>> offset = outarg->offset & ~PAGE_MASK; >>>> file_size = i_size_read(inode); >>>> @@ -1688,7 +1702,7 @@ static int fuse_retrieve(struct fuse_mount *fm, struct inode *inode, >>>> args = &ap->args; >>>> args->nodeid = outarg->nodeid; >>>> args->opcode = FUSE_NOTIFY_REPLY; >>>> - args->in_numargs = 2; >>>> + args->in_numargs = 3; >>>> args->in_pages = true; >>>> args->end = fuse_retrieve_end; >>>> >>>> @@ -1715,9 +1729,11 @@ static int fuse_retrieve(struct fuse_mount *fm, struct inode *inode, >>>> } >>>> ra->inarg.offset = outarg->offset; >>>> ra->inarg.size = total_len; >>>> - args->in_args[0].size = sizeof(ra->inarg); >>>> - args->in_args[0].value = &ra->inarg; >>>> - args->in_args[1].size = total_len; >>>> + args->in_args[0].size = sizeof(zero_arg); >>>> + args->in_args[0].value = &zero_arg; >>>> + args->in_args[1].size = sizeof(ra->inarg); >>>> + args->in_args[1].value = &ra->inarg; >>>> + args->in_args[2].size = total_len; >>>> >>>> err = fuse_simple_notify_reply(fm, args, outarg->notify_unique); >>>> if (err) >>> >>> Do we also need to add a zero arg header for FUSE_READLINK, >>> FUSE_DESTROY, and FUSE_BATCH_FORGET requests as well? >>> >> >> Thanks for looking at the patch! I should have added to the commit message >> that I didn't modify these, as they don't have an in argument at all. >> > > Thanks for clarifying! (and apologies for the late review. I haven't > been keeping up with these patches since RFC v3 but I'm planning to > get up to speed and take a deeper look at these tomorrow + next week). No worries at all... I'm also very late with reviewing your patches. I'm close for the next fuse-io-version, just fixing some bg accounting issues that had been in all rfc versions so far. > > I think the FUSE_BATCH_FORGET request does use in args, depending on > the number of forget requests. Ah right, but it does not use fuse_copy_args and args->in_args[idx] - is very special. And just looking it up again, the header is in the right place. Issue would be more for over-io-uring to copy into the payload. However, current over-io-uring patches don't handle forgets at all - it goes over /dev/fuse. Unless you disagree, I think we can do forgets later on over io-uring as optimization. Thanks, Bernd
On Thu, Nov 14, 2024 at 2:06 PM Bernd Schubert <bernd.schubert@fastmail.fm> wrote: > > > > On 11/14/24 22:29, Joanne Koong wrote: > > On Thu, Nov 14, 2024 at 1:05 PM Bernd Schubert > > <bernd.schubert@fastmail.fm> wrote: > >> > >> > >> > >> On 11/14/24 21:57, Joanne Koong wrote: > >>> On Thu, Nov 7, 2024 at 9:04 AM Bernd Schubert <bschubert@ddn.com> wrote: > >>>> > >>>> This change sets up FUSE operations to have headers in args.in_args[0], > >>>> even for opcodes without an actual header. We do this to prepare for > >>>> cleanly separating payload from headers in the future. > >>>> > >>>> For opcodes without a header, we use a zero-sized struct as a > >>>> placeholder. This approach: > >>>> - Keeps things consistent across all FUSE operations > >>>> - Will help with payload alignment later > >>>> - Avoids future issues when header sizes change > >>>> > >>>> Signed-off-by: Bernd Schubert <bschubert@ddn.com> > >>>> --- > >>>> fs/fuse/dax.c | 13 ++++++++----- > >>>> fs/fuse/dev.c | 24 ++++++++++++++++++++---- > >>>> fs/fuse/dir.c | 41 +++++++++++++++++++++++++++-------------- > >>>> fs/fuse/fuse_i.h | 7 +++++++ > >>>> fs/fuse/xattr.c | 9 ++++++--- > >>>> 5 files changed, 68 insertions(+), 26 deletions(-) > >>>> > >>>> diff --git a/fs/fuse/dax.c b/fs/fuse/dax.c > >>>> index 12ef91d170bb3091ac35a33d2b9dc38330b00948..e459b8134ccb089f971bebf8da1f7fc5199c1271 100644 > >>>> --- a/fs/fuse/dax.c > >>>> +++ b/fs/fuse/dax.c > >>>> @@ -237,14 +237,17 @@ static int fuse_send_removemapping(struct inode *inode, > >>>> struct fuse_inode *fi = get_fuse_inode(inode); > >>>> struct fuse_mount *fm = get_fuse_mount(inode); > >>>> FUSE_ARGS(args); > >>>> + struct fuse_zero_in zero_arg; > >>>> > >>>> args.opcode = FUSE_REMOVEMAPPING; > >>>> args.nodeid = fi->nodeid; > >>>> - args.in_numargs = 2; > >>>> - args.in_args[0].size = sizeof(*inargp); > >>>> - args.in_args[0].value = inargp; > >>>> - args.in_args[1].size = inargp->count * sizeof(*remove_one); > >>>> - args.in_args[1].value = remove_one; > >>>> + args.in_numargs = 3; > >>>> + args.in_args[0].size = sizeof(zero_arg); > >>>> + args.in_args[0].value = &zero_arg; > >>>> + args.in_args[1].size = sizeof(*inargp); > >>>> + args.in_args[1].value = inargp; > >>>> + args.in_args[2].size = inargp->count * sizeof(*remove_one); > >>>> + args.in_args[2].value = remove_one; > >>>> return fuse_simple_request(fm, &args); > >>>> } > >>>> > >>>> diff --git a/fs/fuse/dev.c b/fs/fuse/dev.c > >>>> index dbc222f9b0f0e590ce3ef83077e6b4cff03cff65..6effef4073da3dad2f6140761eca98147a41d88d 100644 > >>>> --- a/fs/fuse/dev.c > >>>> +++ b/fs/fuse/dev.c > >>>> @@ -1007,6 +1007,19 @@ static int fuse_copy_args(struct fuse_copy_state *cs, unsigned numargs, > >>>> > >>>> for (i = 0; !err && i < numargs; i++) { > >>>> struct fuse_arg *arg = &args[i]; > >>>> + > >>>> + /* zero headers */ > >>>> + if (arg->size == 0) { > >>>> + if (WARN_ON_ONCE(i != 0)) { > >>>> + if (cs->req) > >>>> + pr_err_once( > >>>> + "fuse: zero size header in opcode %d\n", > >>>> + cs->req->in.h.opcode); > >>>> + return -EINVAL; > >>>> + } > >>>> + continue; > >>>> + } > >>>> + > >>>> if (i == numargs - 1 && argpages) > >>>> err = fuse_copy_pages(cs, arg->size, zeroing); > >>>> else > >>>> @@ -1662,6 +1675,7 @@ static int fuse_retrieve(struct fuse_mount *fm, struct inode *inode, > >>>> size_t args_size = sizeof(*ra); > >>>> struct fuse_args_pages *ap; > >>>> struct fuse_args *args; > >>>> + struct fuse_zero_in zero_arg; > >>>> > >>>> offset = outarg->offset & ~PAGE_MASK; > >>>> file_size = i_size_read(inode); > >>>> @@ -1688,7 +1702,7 @@ static int fuse_retrieve(struct fuse_mount *fm, struct inode *inode, > >>>> args = &ap->args; > >>>> args->nodeid = outarg->nodeid; > >>>> args->opcode = FUSE_NOTIFY_REPLY; > >>>> - args->in_numargs = 2; > >>>> + args->in_numargs = 3; > >>>> args->in_pages = true; > >>>> args->end = fuse_retrieve_end; > >>>> > >>>> @@ -1715,9 +1729,11 @@ static int fuse_retrieve(struct fuse_mount *fm, struct inode *inode, > >>>> } > >>>> ra->inarg.offset = outarg->offset; > >>>> ra->inarg.size = total_len; > >>>> - args->in_args[0].size = sizeof(ra->inarg); > >>>> - args->in_args[0].value = &ra->inarg; > >>>> - args->in_args[1].size = total_len; > >>>> + args->in_args[0].size = sizeof(zero_arg); > >>>> + args->in_args[0].value = &zero_arg; > >>>> + args->in_args[1].size = sizeof(ra->inarg); > >>>> + args->in_args[1].value = &ra->inarg; > >>>> + args->in_args[2].size = total_len; > >>>> > >>>> err = fuse_simple_notify_reply(fm, args, outarg->notify_unique); > >>>> if (err) > >>> > >>> Do we also need to add a zero arg header for FUSE_READLINK, > >>> FUSE_DESTROY, and FUSE_BATCH_FORGET requests as well? > >>> > >> > >> Thanks for looking at the patch! I should have added to the commit message > >> that I didn't modify these, as they don't have an in argument at all. > >> > > > > Thanks for clarifying! (and apologies for the late review. I haven't > > been keeping up with these patches since RFC v3 but I'm planning to > > get up to speed and take a deeper look at these tomorrow + next week). > > No worries at all... I'm also very late with reviewing your patches. > I'm close for the next fuse-io-version, just fixing some bg accounting > issues that had been in all rfc versions so far. > Awesome, I'll wait until your next fuse io version to review then. Thanks for trucking along on this - I'm very excited to use this. > > > > I think the FUSE_BATCH_FORGET request does use in args, depending on > > the number of forget requests. > > Ah right, but it does not use fuse_copy_args and args->in_args[idx] - > is very special. And just looking it up again, the header is in the > right place. Issue would be more for over-io-uring to copy into the > payload. However, current over-io-uring patches don't handle forgets > at all - it goes over /dev/fuse. Unless you disagree, I think we can > do forgets later on over io-uring as optimization. > Not important at all - was just noting it in case you had meant to include it as part of this patch. Thanks, Joanne > > Thanks, > Bernd > >
diff --git a/fs/fuse/dax.c b/fs/fuse/dax.c index 12ef91d170bb3091ac35a33d2b9dc38330b00948..e459b8134ccb089f971bebf8da1f7fc5199c1271 100644 --- a/fs/fuse/dax.c +++ b/fs/fuse/dax.c @@ -237,14 +237,17 @@ static int fuse_send_removemapping(struct inode *inode, struct fuse_inode *fi = get_fuse_inode(inode); struct fuse_mount *fm = get_fuse_mount(inode); FUSE_ARGS(args); + struct fuse_zero_in zero_arg; args.opcode = FUSE_REMOVEMAPPING; args.nodeid = fi->nodeid; - args.in_numargs = 2; - args.in_args[0].size = sizeof(*inargp); - args.in_args[0].value = inargp; - args.in_args[1].size = inargp->count * sizeof(*remove_one); - args.in_args[1].value = remove_one; + args.in_numargs = 3; + args.in_args[0].size = sizeof(zero_arg); + args.in_args[0].value = &zero_arg; + args.in_args[1].size = sizeof(*inargp); + args.in_args[1].value = inargp; + args.in_args[2].size = inargp->count * sizeof(*remove_one); + args.in_args[2].value = remove_one; return fuse_simple_request(fm, &args); } diff --git a/fs/fuse/dev.c b/fs/fuse/dev.c index dbc222f9b0f0e590ce3ef83077e6b4cff03cff65..6effef4073da3dad2f6140761eca98147a41d88d 100644 --- a/fs/fuse/dev.c +++ b/fs/fuse/dev.c @@ -1007,6 +1007,19 @@ static int fuse_copy_args(struct fuse_copy_state *cs, unsigned numargs, for (i = 0; !err && i < numargs; i++) { struct fuse_arg *arg = &args[i]; + + /* zero headers */ + if (arg->size == 0) { + if (WARN_ON_ONCE(i != 0)) { + if (cs->req) + pr_err_once( + "fuse: zero size header in opcode %d\n", + cs->req->in.h.opcode); + return -EINVAL; + } + continue; + } + if (i == numargs - 1 && argpages) err = fuse_copy_pages(cs, arg->size, zeroing); else @@ -1662,6 +1675,7 @@ static int fuse_retrieve(struct fuse_mount *fm, struct inode *inode, size_t args_size = sizeof(*ra); struct fuse_args_pages *ap; struct fuse_args *args; + struct fuse_zero_in zero_arg; offset = outarg->offset & ~PAGE_MASK; file_size = i_size_read(inode); @@ -1688,7 +1702,7 @@ static int fuse_retrieve(struct fuse_mount *fm, struct inode *inode, args = &ap->args; args->nodeid = outarg->nodeid; args->opcode = FUSE_NOTIFY_REPLY; - args->in_numargs = 2; + args->in_numargs = 3; args->in_pages = true; args->end = fuse_retrieve_end; @@ -1715,9 +1729,11 @@ static int fuse_retrieve(struct fuse_mount *fm, struct inode *inode, } ra->inarg.offset = outarg->offset; ra->inarg.size = total_len; - args->in_args[0].size = sizeof(ra->inarg); - args->in_args[0].value = &ra->inarg; - args->in_args[1].size = total_len; + args->in_args[0].size = sizeof(zero_arg); + args->in_args[0].value = &zero_arg; + args->in_args[1].size = sizeof(ra->inarg); + args->in_args[1].value = &ra->inarg; + args->in_args[2].size = total_len; err = fuse_simple_notify_reply(fm, args, outarg->notify_unique); if (err) diff --git a/fs/fuse/dir.c b/fs/fuse/dir.c index 2b0d4781f39484d50d1fd7f4f673d8b08c5fd7cf..6d67d7f8e6b4460c759df3fb293e169bcc78a897 100644 --- a/fs/fuse/dir.c +++ b/fs/fuse/dir.c @@ -172,12 +172,16 @@ static void fuse_lookup_init(struct fuse_conn *fc, struct fuse_args *args, u64 nodeid, const struct qstr *name, struct fuse_entry_out *outarg) { + struct fuse_zero_in zero_arg; + memset(outarg, 0, sizeof(struct fuse_entry_out)); args->opcode = FUSE_LOOKUP; args->nodeid = nodeid; - args->in_numargs = 1; - args->in_args[0].size = name->len + 1; - args->in_args[0].value = name->name; + args->in_numargs = 2; + args->in_args[0].size = sizeof(zero_arg); + args->in_args[0].value = &zero_arg; + args->in_args[1].size = name->len + 1; + args->in_args[1].value = name->name; args->out_numargs = 1; args->out_args[0].size = sizeof(struct fuse_entry_out); args->out_args[0].value = outarg; @@ -915,16 +919,19 @@ static int fuse_mkdir(struct mnt_idmap *idmap, struct inode *dir, static int fuse_symlink(struct mnt_idmap *idmap, struct inode *dir, struct dentry *entry, const char *link) { + struct fuse_zero_in zero_arg; struct fuse_mount *fm = get_fuse_mount(dir); unsigned len = strlen(link) + 1; FUSE_ARGS(args); args.opcode = FUSE_SYMLINK; - args.in_numargs = 2; - args.in_args[0].size = entry->d_name.len + 1; - args.in_args[0].value = entry->d_name.name; - args.in_args[1].size = len; - args.in_args[1].value = link; + args.in_numargs = 3; + args.in_args[0].size = sizeof(zero_arg); + args.in_args[0].value = &zero_arg; + args.in_args[1].size = entry->d_name.len + 1; + args.in_args[1].value = entry->d_name.name; + args.in_args[2].size = len; + args.in_args[2].value = link; return create_new_entry(fm, &args, dir, entry, S_IFLNK); } @@ -975,6 +982,7 @@ static void fuse_entry_unlinked(struct dentry *entry) static int fuse_unlink(struct inode *dir, struct dentry *entry) { + struct fuse_zero_in inarg; int err; struct fuse_mount *fm = get_fuse_mount(dir); FUSE_ARGS(args); @@ -984,9 +992,11 @@ static int fuse_unlink(struct inode *dir, struct dentry *entry) args.opcode = FUSE_UNLINK; args.nodeid = get_node_id(dir); - args.in_numargs = 1; - args.in_args[0].size = entry->d_name.len + 1; - args.in_args[0].value = entry->d_name.name; + args.in_numargs = 2; + args.in_args[0].size = sizeof(inarg); + args.in_args[0].value = &inarg; + args.in_args[1].size = entry->d_name.len + 1; + args.in_args[1].value = entry->d_name.name; err = fuse_simple_request(fm, &args); if (!err) { fuse_dir_changed(dir); @@ -998,6 +1008,7 @@ static int fuse_unlink(struct inode *dir, struct dentry *entry) static int fuse_rmdir(struct inode *dir, struct dentry *entry) { + struct fuse_zero_in zero_arg; int err; struct fuse_mount *fm = get_fuse_mount(dir); FUSE_ARGS(args); @@ -1007,9 +1018,11 @@ static int fuse_rmdir(struct inode *dir, struct dentry *entry) args.opcode = FUSE_RMDIR; args.nodeid = get_node_id(dir); - args.in_numargs = 1; - args.in_args[0].size = entry->d_name.len + 1; - args.in_args[0].value = entry->d_name.name; + args.in_numargs = 2; + args.in_args[0].size = sizeof(zero_arg); + args.in_args[0].value = &zero_arg; + args.in_args[1].size = entry->d_name.len + 1; + args.in_args[1].value = entry->d_name.name; err = fuse_simple_request(fm, &args); if (!err) { fuse_dir_changed(dir); diff --git a/fs/fuse/fuse_i.h b/fs/fuse/fuse_i.h index f2391961031374d8d55916c326c6472f0c03aae6..e2d1d90dfdb13b2c3e7de4789501ee45d3bf7794 100644 --- a/fs/fuse/fuse_i.h +++ b/fs/fuse/fuse_i.h @@ -941,6 +941,13 @@ struct fuse_mount { struct rcu_head rcu; }; +/* + * Empty header for FUSE opcodes without specific header needs. + * Used as a placeholder in args->in_args[0] for consistency + * across all FUSE operations, simplifying request handling. + */ +struct fuse_zero_in {}; + static inline struct fuse_mount *get_fuse_mount_super(struct super_block *sb) { return sb->s_fs_info; diff --git a/fs/fuse/xattr.c b/fs/fuse/xattr.c index 5b423fdbb13f8f17c3982e96dd0de836662092b0..2df1efd2e9bdb46571148f484d7927044f31c184 100644 --- a/fs/fuse/xattr.c +++ b/fs/fuse/xattr.c @@ -158,15 +158,18 @@ int fuse_removexattr(struct inode *inode, const char *name) struct fuse_mount *fm = get_fuse_mount(inode); FUSE_ARGS(args); int err; + struct fuse_zero_in zero_arg; if (fm->fc->no_removexattr) return -EOPNOTSUPP; args.opcode = FUSE_REMOVEXATTR; args.nodeid = get_node_id(inode); - args.in_numargs = 1; - args.in_args[0].size = strlen(name) + 1; - args.in_args[0].value = name; + args.in_numargs = 2; + args.in_args[0].size = sizeof(zero_arg); + args.in_args[0].value = &zero_arg; + args.in_args[1].size = strlen(name) + 1; + args.in_args[1].value = name; err = fuse_simple_request(fm, &args); if (err == -ENOSYS) { fm->fc->no_removexattr = 1;
This change sets up FUSE operations to have headers in args.in_args[0], even for opcodes without an actual header. We do this to prepare for cleanly separating payload from headers in the future. For opcodes without a header, we use a zero-sized struct as a placeholder. This approach: - Keeps things consistent across all FUSE operations - Will help with payload alignment later - Avoids future issues when header sizes change Signed-off-by: Bernd Schubert <bschubert@ddn.com> --- fs/fuse/dax.c | 13 ++++++++----- fs/fuse/dev.c | 24 ++++++++++++++++++++---- fs/fuse/dir.c | 41 +++++++++++++++++++++++++++-------------- fs/fuse/fuse_i.h | 7 +++++++ fs/fuse/xattr.c | 9 ++++++--- 5 files changed, 68 insertions(+), 26 deletions(-)