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!
>
> Thanks,
> Bernd
