Re: [PATCH RFC v3 05/17] fuse: Add a uring config ioctl

[Joanne Koong <joannelkoong@gmail.com>]

On Wed, Sep 4, 2024 at 3:24 PM Bernd Schubert
<bernd.schubert@fastmail.fm> wrote:
>
> On 9/4/24 02:43, Joanne Koong wrote:
> > On Sun, Sep 1, 2024 at 6:37 AM Bernd Schubert <bschubert@ddn.com> wrote:
> >>
> >> This only adds the initial ioctl for basic fuse-uring initialization.
> >> More ioctl types will be added later to initialize queues.
...
> >
> >> +               return -EINVAL;
> >> +       }
> >> +
> >> +       if (rcfg->nr_queues > 1 && rcfg->nr_queues != num_present_cpus()) {
> >
> > Will it always be that nr_queues must be the number of CPUs on the
> > system or will that constraint be relaxed in the future?
>
> In all my testing performance rather suffered when any kind of cpu switching was involved. I guess we should first find a good reason to relax it and then need to think about which queue to use, when a request comes on a different core. Do you have a use case?

Ah, gotcha. I don't have a use case in mind, just thought it'd be
common for some users to want more than 1 queue but not as many queues
as they have cores. This could always be added later in the future
though if this use case actually comes up.

>
> >> diff --git a/fs/fuse/fuse_dev_i.h b/fs/fuse/fuse_dev_i.h
> >> index 6c506f040d5f..e6289bafb788 100644
> >> --- a/fs/fuse/fuse_dev_i.h
> >> +++ b/fs/fuse/fuse_dev_i.h
> >> @@ -7,6 +7,7 @@
> >>  #define _FS_FUSE_DEV_I_H
> >>
> >>  #include <linux/types.h>
> >> +#include <linux/fs.h>
> >
> > I think you accidentally included this.
> >
>
> When I remove it:
>
> bschubert2@imesrv6 linux.git>make M=fs/fuse/
>   CC [M]  fs/fuse/dev_uring.o
> In file included from fs/fuse/dev_uring.c:7:
> fs/fuse/fuse_dev_i.h:15:52: warning: declaration of 'struct file' will not be visible outside of this function [-Wvisibility]
> static inline struct fuse_dev *fuse_get_dev(struct file *file)
>                                                    ^
> fs/fuse/fuse_dev_i.h:21:9: error: call to undeclared function 'READ_ONCE'; ISO C99 and later do not support implicit function declarations [-Wimplicit-function-declaration]
>         return READ_ONCE(file->private_data);
>                ^
> fs/fuse/fuse_dev_i.h:21:23: error: incomplete definition of type 'struct file'
>         return READ_ONCE(file->private_data);
>                          ~~~~^
>
>
> I could also include <linux/fs.h> in dev_uring.c, but isn't it cleaner
> to have the include in fuse_dev_i.h as it is that file that
> adds dependencies?
>

You're totally right, I had missed that this patch adds in a new
caller of this header (dev_uring.c) - sorry for the noise!

> >>
...
> >> +
> >>  #endif /* _LINUX_FUSE_H */
> >>
> >> --
> >> 2.43.0
> >>
>
> I will get it all fixed later this week! I will also review my own
> patches before v4, I just wanted to get v3 out asap as it was already
> taking so much time after v2.
>

Gotcha, I'll wait until v4 to review the other patches in this set then.

Excited to follow all the progress on this!


Thanks,
Joanne

>
> Thanks,
> Bernd
>