On 8/14/24 20:06, Joanne Koong wrote:
> On Wed, Aug 14, 2024 at 10:52 AM Bernd Schubert
> <bernd.schubert@xxxxxxxxxxx> wrote:
>>
>>
>>
>> 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.
>
> Oh I see, the FUSE_OPEN_ATOMIC request itself would ask for attributes
> and the attributes would be sent by the server as the reply to the
> FUSE_ATOMIC_OPEN. This sounds great! in my patch, there's an
> additional FUSE_GETATTR request incurred to get the 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
>
> I missed that the attributes would be bundled with FUSE_OPEN_ATOMIC so
> I thought we would need an additional FUSE_GETATTR request to get
> them. Apologies for the confusion!
>
>> 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.
>>
>
> Sounds great. thanks for your work on this!
Here is a totally untested (and probably ugly) version of what I had in my
mind
https://github.com/bsbernd/linux/commits/open-getattr/
https://github.com/libfuse/libfuse/pull/1020
(It builds, but nothing more tested).
Instead of rather complex atomic-open it adds FUSE_OPEN_GETATTR and hooks into
fuse_file_open.
I was considering to hook into fuse_do_open, but that would cause quite some code
dup for fuse_file_open. We need the inode to update attributes and in fuse_do_open
we could use file->f_inode, but I didn't verify if it is reliable at this stage
(do_dentry_open() assignes it, but I didn't verify possible other code paths) - for
now I added the inode parameter to all code paths.
Going to test and clean it up tomorrow.
Thanks,
Bernd
