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().
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.
Thanks,
Bernd
