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
updating, would there be any other differences from using plain open
vs the atomic open version of plain open?
Do you have a tentative timeline in mind for when the next iteration
of the atomic open patchset would be out?
Thanks,
Joanne
>
> Thanks,
> Bernd
>
