On Fri, Jun 24, 2022 at 3:26 AM Vivek Goyal <vgoyal@xxxxxxxxxx> wrote: > > On Tue, Jun 21, 2022 at 08:56:51PM +0800, wubo wrote: > > From: Wu Bo <bo.wu@xxxxxxxx> > > > > Now the fuse driver only trust it's local inode size when > > writeback_cache is enabled. Even the userspace server tell the driver > > the inode cache is invalidated, the size attrabute will not update. And > > will keep it's out-of-date size till the inode cache is dropped. This is > > not reasonable. > > BTW, can you give more details about what's the use case. With > writeback_cache, writes can be cached in fuse and not sent to > file server immediately. And I think that's why fuse trusts > local i_size. > > With writeback_cache enabled, I don't think file should be modified > externally (outside the fuse client). > > So what's that use case where file size cached in fuse is out of > date. You probably should not use writeback_cache if you are > modifying files outside the fuse client. > > Having said that I am not sure why FUSE_NOTIFY_INVAL_INODE was added to > begin with. If files are not supposed to be modifed outside the fuse > client, why are we dropping acls and invalidating attrs. If intent is > just to drop page cache, then it should have been just that nothing > else. > > So up to some extent, FUSE_NOTIFY_INVAL_INODE is somewhat confusing. Would > have been good if there was some documentation for it. > > Thanks > Vivek > Hi Wu and Vivek, Recently, we have had some discussions about the writeback_cache revalidation on the mailing list [1][2]. Miklos gave his initial patchset about writeback_cache v2, which supports c/mtime and size updates [1]. However, those methods do not make use of reverse messages, as virtio-fs does not support reverse notification yet. I'm going to send out a new version of that patch based on the discussion and with more considerations. I also agree that, semantically, FUSE_NOTIFY_INVAL_INODE should invalidate i_size as well. So I think this patch is a good supplement for FUSE_NOTIFY_INVAL_INODE. But we need to be more careful as the size can be updated from server to kernel, and from kernel to server. I will leave some comments about such issues in the following code. For the use case, writeback_cache is superb over write-through mode in write-intensive scenarios, but its consistency among multiple clients is too bad (almost no consistency). I think it's good to give a little more consistency to writeback_cache. [1] https://lore.kernel.org/linux-fsdevel/20220325132126.61949-1-zhangjiachen.jaycee@xxxxxxxxxxxxx/ [2] https://lore.kernel.org/linux-fsdevel/20220608104202.19461-1-zhangjiachen.jaycee@xxxxxxxxxxxxx/ > > > > Signed-off-by: Wu Bo <bo.wu@xxxxxxxx> > > --- > > fs/fuse/inode.c | 10 +++++++++- > > 1 file changed, 9 insertions(+), 1 deletion(-) > > > > diff --git a/fs/fuse/inode.c b/fs/fuse/inode.c > > index 8c0665c5dff8..a4e62c7f2b83 100644 > > --- a/fs/fuse/inode.c > > +++ b/fs/fuse/inode.c > > @@ -162,6 +162,11 @@ static ino_t fuse_squash_ino(u64 ino64) > > return ino; > > } > > > > +static bool fuse_force_sync(struct fuse_inode *fi) > > +{ > > + return fi->i_time == 0; > > +} > > + > > void fuse_change_attributes_common(struct inode *inode, struct fuse_attr *attr, > > u64 attr_valid, u32 cache_mask) > > { > > @@ -222,8 +227,10 @@ void fuse_change_attributes_common(struct inode *inode, struct fuse_attr *attr, > > u32 fuse_get_cache_mask(struct inode *inode) > > { > > struct fuse_conn *fc = get_fuse_conn(inode); > > + struct fuse_inode *fi = get_fuse_inode(inode); > > + bool is_force_sync = fuse_force_sync(fi); > > > > - if (!fc->writeback_cache || !S_ISREG(inode->i_mode)) > > + if (!fc->writeback_cache || !S_ISREG(inode->i_mode) || is_force_sync) > > return 0; > > > > return STATX_MTIME | STATX_CTIME | STATX_SIZE; > > @@ -437,6 +444,7 @@ int fuse_reverse_inval_inode(struct fuse_conn *fc, u64 nodeid, > > fi = get_fuse_inode(inode); > > spin_lock(&fi->lock); > > fi->attr_version = atomic64_inc_return(&fc->attr_version); > > + fi->i_time = 0; > > spin_unlock(&fi->lock); Seems fuse_reverse_inval_inode() only drops page cache from offset to offset+len, should we only invalidate i_time on a full cache drop? Otherwise, as the server size is stale, the users may see a file is truncated. Also, what if a FUSE_GETATTR request gets the attr_version after fuse_reverse_inval_inode() increases it, but tries to update i_size after the invalidate_inode_pages2_range() in fuse_reverse_inval_inode()? In this case, server_size can be updated by invalidate_inode_pages2_range(), and FUSE_GETATTR might gets a stale server_size. Meanwhile, as FUSE_GETATTR has got the newest attr_version, the kernel_size will still be updated. This can cause false truncation even for a single FUSE client. So we may need to do more about the attr_version in writeback mode. Thanks, Jiachen > > > > fuse_invalidate_attr(inode); > > -- > > 2.35.1 > > >
