On 8/14/24 19:18, Joanne Koong wrote:
> On Tue, Aug 13, 2024 at 3:41 PM Bernd Schubert
> <bernd.schubert@xxxxxxxxxxx> wrote:
>>
>> On August 13, 2024 11:57:44 PM GMT+02:00, Joanne Koong <joannelkoong@xxxxxxxxx> wrote:
>>> On Tue, Aug 13, 2024 at 2:44 PM Bernd Schubert
>>> <bernd.schubert@xxxxxxxxxxx> wrote:
>>>>
>>>> On 8/13/24 23:21, Joanne Koong wrote:
>>>>> Add FOPEN_FETCH_ATTR flag to indicate that attributes should be
>>>>> fetched from the server after an open.
>>>>>
>>>>> For fuse servers that are backed by network filesystems, this is
>>>>> needed to ensure that file attributes are up to date between
>>>>> consecutive open calls.
>>>>>
>>>>> For example, if there is a file that is opened on two fuse mounts,
>>>>> in the following scenario:
>>>>>
>>>>> on mount A, open file.txt w/ O_APPEND, write "hi", close file
>>>>> on mount B, open file.txt w/ O_APPEND, write "world", close file
>>>>> on mount A, open file.txt w/ O_APPEND, write "123", close file
>>>>>
>>>>> when the file is reopened on mount A, the file inode contains the old
>>>>> size and the last append will overwrite the data that was written when
>>>>> the file was opened/written on mount B.
>>>>>
>>>>> (This corruption can be reproduced on the example libfuse passthrough_hp
>>>>> server with writeback caching disabled and nopassthrough)
>>>>>
>>>>> Having this flag as an option enables parity with NFS's close-to-open
>>>>> consistency.
>>>>>
>>>>> Signed-off-by: Joanne Koong <joannelkoong@xxxxxxxxx>
>>>>> ---
>>>>> fs/fuse/file.c | 7 ++++++-
>>>>> include/uapi/linux/fuse.h | 7 ++++++-
>>>>> 2 files changed, 12 insertions(+), 2 deletions(-)
>>>>>
>>>>> diff --git a/fs/fuse/file.c b/fs/fuse/file.c
>>>>> index f39456c65ed7..437487ce413d 100644
>>>>> --- a/fs/fuse/file.c
>>>>> +++ b/fs/fuse/file.c
>>>>> @@ -264,7 +264,12 @@ static int fuse_open(struct inode *inode, struct file *file)
>>>>> err = fuse_do_open(fm, get_node_id(inode), file, false);
>>>>> if (!err) {
>>>>> ff = file->private_data;
>>>>> - err = fuse_finish_open(inode, file);
>>>>> + if (ff->open_flags & FOPEN_FETCH_ATTR) {
>>>>> + fuse_invalidate_attr(inode);
>>>>> + err = fuse_update_attributes(inode, file, STATX_BASIC_STATS);
>>>>> + }
>>>>> + if (!err)
>>>>> + err = fuse_finish_open(inode, file);
>>>>> if (err)
>>>>> fuse_sync_release(fi, ff, file->f_flags);
>>>>> else if (is_truncate)
>>>>
>>>> I didn't come to it yet, but I actually wanted to update Dharmendras/my
>>>> atomic open patches - giving up all the vfs changes (for now) and then
>>>> always use atomic open if available, for FUSE_OPEN and FUSE_CREATE. And
>>>> then update attributes through that.
>>>> Would that be an alternative for you? Would basically require to add an
>>>> atomic_open method into your file system.
>>>>
>>>> Definitely more complex than your solution, but avoids a another
>>>> kernel/userspace transition.
>>>
>>> Hi Bernd,
>>>
>>> Unfortunately I don't think this is an alternative for my use case. I
>>> haven't looked closely at the implementation details of your atomic
>>> open patchset yet but if I'm understanding the gist of it correctly,
>>> it bundles the lookup with the open into 1 request, where the
>>> attributes can be passed from server -> kernel through the reply to
>>> that request. I think in the case I'm working on, the file open call
>>> does not require a lookup so it can't take advantage of your feature.
>>> I just tested it on libfuse on the passthrough_hp server (with no
>>> writeback caching and nopassthrough) on the example in the commit
>>> message and I'm not seeing any lookup request being sent for that last
>>> open call (for writing "123").
>>>
>>
>>
>> Hi Joanne,
>>
>> gets late here and I'm typing on my phone. I hope formatting is ok.
>>
>> what I meant is that we use the atomic open op code for both, lookup-open and plain open - i.e. we always update attributes on open. Past atomic open patches did not do that yet, but I later realized that always using atomic open op
>>
>> - avoids the data corruption you run into
>> - probably no need for atomic-revalidate-open vfs patches anymore as we can now safely set a high attr timeout
>>
>>
>> Kind of the same as your patch, just through a new op code.
>
> Awesome, thanks for the context Bernd. I think this works for our use
> case then. To confirm the "we will always update attributes on open"
> part, this will only send the FUSE_GETATTR request to the server if
> the server has invalidated the inode (eg through the
> fuse_lowlevel_notify_inval_inode() api), otherwise this will not send
> an extra FUSE_GETATTR request, correct? Other than the attribute
If we send FUSE_OPEN_ATOMIC (or whatever we name it) in
fuse_file_open(), it would always ask server side for attributes.
I.e. we assume that a server that has atomic open implemented can easily
provide attributes or asks for close-to-open coherency.
I'm not sure if I correctly understood your questions about
notifications and FUSE_GETATTR - from my point of view that that is
entirely independent from open. And personally I try to reduce
kernel/userspace transitions - additional notifications and FUSE_GETATTR
are not helpful here :)
> updating, would there be any other differences from using plain open
> vs the atomic open version of plain open?
Just the additional file attributes and complexity that brings.
>
> Do you have a tentative timeline in mind for when the next iteration
> of the atomic open patchset would be out?
I wanted to have new fuse-uring patches ready by last week, but I'm
still refactoring things - changing things on top of the existing series
is easy, rebasing it is painful... I can _try_ to make a raw new
atomic-open patch set during the next days (till Sunday), but not promised.
Thanks,
Bernd
