On Wed, Jul 3, 2024 at 10:30 AM Josef Bacik <josef@xxxxxxxxxxxxxx> wrote:
>
> On Wed, Jul 03, 2024 at 05:58:20PM +0200, Bernd Schubert wrote:
> >
> >
> > On 7/3/24 17:15, Josef Bacik wrote:
> > > On Tue, Jul 02, 2024 at 06:31:08PM +0200, Bernd Schubert wrote:
> > >> Read/writes IOs should be page aligned as fuse server
> > >> might need to copy data to another buffer otherwise in
> > >> order to fulfill network or device storage requirements.
> > >>
> > >> Simple reproducer is with libfuse, example/passthrough*
> > >> and opening a file with O_DIRECT - without this change
> > >> writing to that file failed with -EINVAL if the underlying
> > >> file system was using ext4 (for passthrough_hp the
> > >> 'passthrough' feature has to be disabled).
> > >>
> > >> Given this needs server side changes as new feature flag is
> > >> introduced.
> > >>
> > >> Disadvantage of aligned writes is that server side needs
> > >> needs another splice syscall (when splice is used) to seek
> > >> over the unaligned area - i.e. syscall and memory copy overhead.
> > >>
> > >> Signed-off-by: Bernd Schubert <bschubert@xxxxxxx>
> > >>
> > >> ---
> > >> From implementation point of view 'struct fuse_in_arg' /
> > >> 'struct fuse_arg' gets another parameter 'align_size', which has to
> > >> be set by fuse_write_args_fill. For all other fuse operations this
> > >> parameter has to be 0, which is guranteed by the existing
> > >> initialization via FUSE_ARGS and C99 style
> > >> initialization { .size = 0, .value = NULL }, i.e. other members are
> > >> zero.
> > >> Another choice would have been to extend fuse_write_in to
> > >> PAGE_SIZE - sizeof(fuse_in_header), but then would be an
> > >> arch/PAGE_SIZE depending struct size and would also require
> > >> lots of stack usage.
> > >
> > > Can I see the libfuse side of this? I'm confused why we need the align_size at
> > > all? Is it enough to just say that this connection is aligned, negotiate what
> > > the alignment is up front, and then avoid sending it along on every write?
> >
> > Sure, I had forgotten to post it
> > https://github.com/bsbernd/libfuse/commit/89049d066efade047a72bcd1af8ad68061b11e7c
> >
> > We could also just act on fc->align_writes / FUSE_ALIGN_WRITES and always use
> > sizeof(struct fuse_in_header) + sizeof(struct fuse_write_in) in libfuse and would
> > avoid to send it inside of fuse_write_in. We still need to add it to struct fuse_in_arg,
> > unless you want to check the request type within fuse_copy_args().
>
> I think I like this approach better, at the very least it allows us to use the
> padding for other silly things in the future.
>
This approach seems cleaner to me as well.
I also like the idea of having callers pass in whether alignment
should be done or not to fuse_copy_args() instead of adding
"align_writes" to struct fuse_in_arg.
Thanks,
Joanne
> >
> > The part I don't like in general about current fuse header handling (besides alignment)
> > is that any header size changes will break fuse server and therefore need to be very
> > carefully handled. See for example libfuse commit 681a0c1178fa.
> >
>
> Agreed, if we could have the length of the control struct in the header then
> then things would be a lot simpler to extend later on, but here we are. Thanks,
>
> Josef
>
