Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Thu, 23 Jun 2022 14:38:36 -0400
From:   Vivek Goyal <vgoyal@redhat.com>
To:     wubo <11123156@vivo.com>
Cc:     miklos@szeredi.hu, linux-fsdevel@vger.kernel.org,
        linux-kernel@vger.kernel.org, Wu Bo <bo.wu@vivo.com>
Subject: Re: [PATCH] fuse: force sync attr when inode is invalidated
Message-ID: <YrSzLNCSg/8fWZ1j@redhat.com>
References: <20220621125651.14954-1-11123156@vivo.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20220621125651.14954-1-11123156@vivo.com>
Precedence: bulk

On Tue, Jun 21, 2022 at 08:56:51PM +0800, wubo wrote:
> From: Wu Bo <bo.wu@vivo.com>
> 
> 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

> 
> Signed-off-by: Wu Bo <bo.wu@vivo.com>
> ---
>  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);
>  
>  	fuse_invalidate_attr(inode);
> -- 
> 2.35.1
>