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