Re: [PATCH] fuse: Allow to align reads/writes

[Josef Bacik <josef@toxicpanda.com>]

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@ddn.com>
> >>
> >> ---
> >> 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.

> 
> 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