On Tue, Aug 13, 2024 at 7:46 PM Jingbo Xu <jefflexu@xxxxxxxxxxxxxxxxx> wrote: > > On 8/14/24 5:21 AM, 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. > > It seems a general demand for close-to-open consistency similar to NFS > when the backend store for FUSE is a NFS-like filesystem. We have a > similar private implementation for close-to-open consistency in our > internal distribution. Also FYI there was a similar proposal for this: > > https://lore.kernel.org/linux-fsdevel/20220608104202.19461-1-zhangjiachen.jaycee@xxxxxxxxxxxxx/ Thanks for the link. I think that proposal though only invalidates the attributes on file open, but the use case we have needs the attributes to be updated on open (so that the subsequent write call uses the updated file size). > > > > > 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) > > diff --git a/include/uapi/linux/fuse.h b/include/uapi/linux/fuse.h > > index d08b99d60f6f..f5d1af6fe352 100644 > > --- a/include/uapi/linux/fuse.h > > +++ b/include/uapi/linux/fuse.h > > @@ -217,6 +217,9 @@ > > * - add backing_id to fuse_open_out, add FOPEN_PASSTHROUGH open flag > > * - add FUSE_NO_EXPORT_SUPPORT init flag > > * - add FUSE_NOTIFY_RESEND, add FUSE_HAS_RESEND init flag > > + * > > + * 7.41 > > + * - add FOPEN_FETCH_ATTR > > */ > > > > #ifndef _LINUX_FUSE_H > > @@ -252,7 +255,7 @@ > > #define FUSE_KERNEL_VERSION 7 > > > > /** Minor version number of this interface */ > > -#define FUSE_KERNEL_MINOR_VERSION 40 > > +#define FUSE_KERNEL_MINOR_VERSION 41 > > > > /** The node ID of the root inode */ > > #define FUSE_ROOT_ID 1 > > @@ -360,6 +363,7 @@ struct fuse_file_lock { > > * FOPEN_NOFLUSH: don't flush data cache on close (unless FUSE_WRITEBACK_CACHE) > > * FOPEN_PARALLEL_DIRECT_WRITES: Allow concurrent direct writes on the same inode > > * FOPEN_PASSTHROUGH: passthrough read/write io for this open file > > + * FOPEN_FETCH_ATTR: attributes are fetched after file is opened > > */ > > #define FOPEN_DIRECT_IO (1 << 0) > > #define FOPEN_KEEP_CACHE (1 << 1) > > @@ -369,6 +373,7 @@ struct fuse_file_lock { > > #define FOPEN_NOFLUSH (1 << 5) > > #define FOPEN_PARALLEL_DIRECT_WRITES (1 << 6) > > #define FOPEN_PASSTHROUGH (1 << 7) > > +#define FOPEN_FETCH_ATTR (1 << 8) > > > > /** > > * INIT request/reply flags > > Does this close-to-open consistency support writeback mode? AFAIK, the > cached ctime/mtime/size at the kernel side are always trusted while > these attributes from the server are dropped, see: No, the use case we're running into doesn't have the writeback cache enabled, so we haven't looked into what would be needed for full CTO consistency on writeback mode. > > ``` > fuse_update_attributes > fuse_update_get_attr > cache_mask = fuse_get_cache_mask(inode) > if writeback mode: > return STATX_MTIME | STATX_CTIME | STATX_SIZE > ``` > > Also FYI there's a similar proposal for enhancing the close-to-open > consistency in writeback mode to fix the above issue: > > https://lore.kernel.org/linux-fsdevel/20220624055825.29183-1-zhangjiachen.jaycee@xxxxxxxxxxxxx/ > > Besides, IIUC this patch only implements the revalidate-on-open semantic > for metadata. To fulfill the full close-to-open consistency, do you > need to disable FOPEN_KEEP_CACHE to fulfill the revalidate-on-open > semantic for data? (Though the revalidate-on-open semantic for data is > not needed in your append-only case.) This patch is aimed at addressing the overwrite/corruption issue we've been seeing rather than adding full support for close-to-open consistency. We haven't so far had a need for revalidate-on-open for data. I should have phrased the last sentence of the commit message as "enables better parity with NFS's close-to-open consistency" :) Thanks, Joanne > > -- > Thanks, > Jingbo
