On Thu, Nov 14, 2024 at 2:06 PM Bernd Schubert
<bernd.schubert@xxxxxxxxxxx> wrote:
>
>
>
> On 11/14/24 22:29, Joanne Koong wrote:
> > On Thu, Nov 14, 2024 at 1:05 PM Bernd Schubert
> > <bernd.schubert@xxxxxxxxxxx> wrote:
> >>
> >>
> >>
> >> On 11/14/24 21:57, Joanne Koong wrote:
> >>> On Thu, Nov 7, 2024 at 9:04 AM Bernd Schubert <bschubert@xxxxxxx> 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@xxxxxxx>
> >>>> ---
> >>>> 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
>
>
