On 7/3/24 19:49, Joanne Koong wrote:
> 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.
There is no caller for FUSE_WRITE for fuse_copy_args(), but it is called
from fuse_dev_do_read for all request types. I'm going to add in request
parsing within fuse_copy_args, I can't decide myself which of both
versions I like less.
Thanks,
Bernd
