On Mon, Aug 19, 2024 at 3:36 PM Bernd Schubert
<bernd.schubert@xxxxxxxxxxx> wrote:
>
>
>
> 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 for the update, Bernd!
>
>
> Thanks,
> Bernd
