[PATCH v2] fuse: use newer inode info when writeback cache is enabled

[PATCH v2] fuse: use newer inode info when writeback cache is enabled

[Fengnan Chang]

When writeback cache is enabled, the inode information in cached is
considered new by default, and the inode information of lowerfs is
stale.
When a lower fs is mount in a different directory through different
connection, for example PATHA and PATHB, since writeback cache is
enabled by default, when the file is modified through PATHA, viewing the
same file from the PATHB, PATHB will think that cached inode is newer
than lowerfs, resulting in file size and time from under PATHA and PATHB
is inconsistent.
Add a judgment condition to check whether to use the info in the cache
according to mtime.

Signed-off-by: Fengnan Chang <changfengnan@vivo.com>
---
 fs/fuse/inode.c | 11 +++++++++--
 1 file changed, 9 insertions(+), 2 deletions(-)

diff --git a/fs/fuse/inode.c b/fs/fuse/inode.c
index b0e18b470e91..55fdafcaca34 100644
--- a/fs/fuse/inode.c
+++ b/fs/fuse/inode.c
@@ -182,7 +182,10 @@ void fuse_change_attributes_common(struct inode *inode, struct fuse_attr *attr,
 	inode->i_atime.tv_sec   = attr->atime;
 	inode->i_atime.tv_nsec  = attr->atimensec;
 	/* mtime from server may be stale due to local buffered write */
-	if (!fc->writeback_cache || !S_ISREG(inode->i_mode)) {
+	if (!fc->writeback_cache || !S_ISREG(inode->i_mode)
+		|| (attr->mtime > inode->i_mtime.tv_sec)
+		|| ((attr->mtime == inode->i_mtime.tv_sec)
+			&& (attr->mtimensec >= inode->i_mtime.tv_nsec))) {
 		inode->i_mtime.tv_sec   = attr->mtime;
 		inode->i_mtime.tv_nsec  = attr->mtimensec;
 		inode->i_ctime.tv_sec   = attr->ctime;
@@ -241,8 +244,12 @@ void fuse_change_attributes(struct inode *inode, struct fuse_attr *attr,
 	 * extend local i_size without keeping userspace server in sync. So,
 	 * attr->size coming from server can be stale. We cannot trust it.
 	 */
-	if (!is_wb || !S_ISREG(inode->i_mode))
+	if (!is_wb || !S_ISREG(inode->i_mode)
+		|| (attr->mtime > inode->i_mtime.tv_sec)
+		|| ((attr->mtime == inode->i_mtime.tv_sec)
+			&& (attr->mtimensec >= inode->i_mtime.tv_nsec))) {
 		i_size_write(inode, attr->size);
+	}
 	spin_unlock(&fi->lock);

 	if (!is_wb && S_ISREG(inode->i_mode)) {
--
2.29.0

Re: [PATCH v2] fuse: use newer inode info when writeback cache is enabled

[Fengnan Chang]

Re: [PATCH v2] fuse: use newer inode info when writeback cache is enabled

[Miklos Szeredi]

On Sat, 30 Jan 2021 at 09:50, Fengnan Chang <changfengnan@vivo.com> wrote:
>
> When writeback cache is enabled, the inode information in cached is
> considered new by default, and the inode information of lowerfs is
> stale.
> When a lower fs is mount in a different directory through different
> connection, for example PATHA and PATHB, since writeback cache is
> enabled by default, when the file is modified through PATHA, viewing the
> same file from the PATHB, PATHB will think that cached inode is newer
> than lowerfs, resulting in file size and time from under PATHA and PATHB
> is inconsistent.
> Add a judgment condition to check whether to use the info in the cache
> according to mtime.

This seems to break the fsx-linux stress test.

I suspect a better direction would be looking at whether the inode has
any files open for write (i_writecount > 0)...

Thanks,
Miklos

Re: [PATCH v2] fuse: use newer inode info when writeback cache is enabled

[Fengnan Chang]

Unh, it seems i_writecount not work.
If we modify file through lowerfs, i_writecount won't change, but the 
size already changed.
For example:
echo "111" > /lowerfs/test
ls -l /upper/test
echo "2222" >> /lowerfs/test
ls -l /upper/test

So, can you describe your test enviroment? including kernel version and 
fsx parameters, I will check it.

Thanks.

On 2021/6/22 15:59, Miklos Szeredi wrote:
> On Sat, 30 Jan 2021 at 09:50, Fengnan Chang <changfengnan@vivo.com> wrote:
>>
>> When writeback cache is enabled, the inode information in cached is
>> considered new by default, and the inode information of lowerfs is
>> stale.
>> When a lower fs is mount in a different directory through different
>> connection, for example PATHA and PATHB, since writeback cache is
>> enabled by default, when the file is modified through PATHA, viewing the
>> same file from the PATHB, PATHB will think that cached inode is newer
>> than lowerfs, resulting in file size and time from under PATHA and PATHB
>> is inconsistent.
>> Add a judgment condition to check whether to use the info in the cache
>> according to mtime.
> 
> This seems to break the fsx-linux stress test.
> 
> I suspect a better direction would be looking at whether the inode has
> any files open for write (i_writecount > 0)...
> 
> Thanks,
> Miklos
> 

Re: [PATCH v2] fuse: use newer inode info when writeback cache is enabled

[Miklos Szeredi]

On Tue, 22 Jun 2021 at 14:25, Fengnan Chang <changfengnan@vivo.com> wrote:
>
> Unh, it seems i_writecount not work.
> If we modify file through lowerfs, i_writecount won't change, but the
> size already changed.
> For example:
> echo "111" > /lowerfs/test
> ls -l /upper/test
> echo "2222" >> /lowerfs/test
> ls -l /upper/test
>
> So, can you describe your test enviroment? including kernel version and
> fsx parameters, I will check it.

linux-5.13-rc5 + patch
mkdir /tmp/test
libfuse/example/passthrough_ll -ocache=always,writeback /mnt/fuse/
fsx-linux -N 1000000 /mnt/fuse/tmp/test/fsx

Thanks,
Miklos

Re: [PATCH v2] fuse: use newer inode info when writeback cache is enabled

[Fengnan Chang]

Hi Miklos:

Thank you for the information, I have been able to reproduce the problem.

The new version of the patch as below. Previous fsx test is pass now. 
Need do more test, Can you help to test new patch? or send me your test 
case, I will test this.

Here is my test case, and is the problem this patch is trying to solve.
Case A:
mkdir /tmp/test
passthrough_ll -ocache=always,writeback /mnt/test/
echo "11111" > /tmp/test/fsx
ls -l /mnt/test/tmp/test/
echo "2222" >> /tmp/test/fsx
ls -l /mnt/test/tmp/test/

Case B:
mkdir /tmp/test
passthrough_ll -ocache=always,writeback /mnt/test/
passthrough_ll -ocache=always,writeback /mnt/test2/
echo "11111" > /tmp/test/fsx
ls -l /mnt/test/tmp/test/
ls -l /mnt/test2/tmp/test/
echo "222" >> /mnt/test/tmp/test/fsx
ls -l /mnt/test/tmp/test/
ls -l /mnt/test2/tmp/test/


diff --git a/fs/fuse/inode.c b/fs/fuse/inode.c
index b9beb39a4a18..8e22a31b55c4 100644
--- a/fs/fuse/inode.c
+++ b/fs/fuse/inode.c
@@ -60,6 +60,10 @@ MODULE_PARM_DESC(max_user_congthresh,
  /** Congestion starts at 75% of maximum */
  #define FUSE_DEFAULT_CONGESTION_THRESHOLD (FUSE_DEFAULT_MAX_BACKGROUND 
* 3 / 4)

+static inline bool attr_newer_than_local(struct fuse_attr *attr, struct 
inode *inode) {
+    return (attr->mtime > inode->i_mtime.tv_sec)
+               || ((attr->mtime == inode->i_mtime.tv_sec) && 
(attr->mtimensec > inode->i_mtime.tv_nsec));
+}
  #ifdef CONFIG_BLOCK
  static struct file_system_type fuseblk_fs_type;
  #endif
@@ -241,8 +245,10 @@ void fuse_change_attributes(struct inode *inode, 
struct fuse_attr *attr,
          * extend local i_size without keeping userspace server in 
sync. So,
          * attr->size coming from server can be stale. We cannot trust it.
          */
-       if (!is_wb || !S_ISREG(inode->i_mode))
+       if (!is_wb || !S_ISREG(inode->i_mode)
+               || (attr_newer_than_local(attr, inode) && 
!inode_is_open_for_write(inode))) {
                 i_size_write(inode, attr->size);
+       }
         spin_unlock(&fi->lock);

         if (!is_wb && S_ISREG(inode->i_mode)) {

On 2021/6/22 23:19, Miklos Szeredi wrote:
> On Tue, 22 Jun 2021 at 14:25, Fengnan Chang <changfengnan@vivo.com> wrote:
>>
>> Unh, it seems i_writecount not work.
>> If we modify file through lowerfs, i_writecount won't change, but the
>> size already changed.
>> For example:
>> echo "111" > /lowerfs/test
>> ls -l /upper/test
>> echo "2222" >> /lowerfs/test
>> ls -l /upper/test
>>
>> So, can you describe your test enviroment? including kernel version and
>> fsx parameters, I will check it.
> 
> linux-5.13-rc5 + patch
> mkdir /tmp/test
> libfuse/example/passthrough_ll -ocache=always,writeback /mnt/fuse/
> fsx-linux -N 1000000 /mnt/fuse/tmp/test/fsx
> 
> Thanks,
> Miklos
> 

Re: [PATCH v2] fuse: use newer inode info when writeback cache is enabled

[Fengnan Chang]

Hi Miklos:

FYI, I run xfstest on fuse, with linux 5.4.61 + patch, no new failure case.

On 2021/6/24 15:42, Fengnan Chang wrote:
> Hi Miklos:
> 
> Thank you for the information, I have been able to reproduce the problem.
> 
> The new version of the patch as below. Previous fsx test is pass now. 
> Need do more test, Can you help to test new patch? or send me your test 
> case, I will test this.
> 
> Here is my test case, and is the problem this patch is trying to solve.
> Case A:
> mkdir /tmp/test
> passthrough_ll -ocache=always,writeback /mnt/test/
> echo "11111" > /tmp/test/fsx
> ls -l /mnt/test/tmp/test/
> echo "2222" >> /tmp/test/fsx
> ls -l /mnt/test/tmp/test/
> 
> Case B:
> mkdir /tmp/test
> passthrough_ll -ocache=always,writeback /mnt/test/
> passthrough_ll -ocache=always,writeback /mnt/test2/
> echo "11111" > /tmp/test/fsx
> ls -l /mnt/test/tmp/test/
> ls -l /mnt/test2/tmp/test/
> echo "222" >> /mnt/test/tmp/test/fsx
> ls -l /mnt/test/tmp/test/
> ls -l /mnt/test2/tmp/test/
> 
> 
> diff --git a/fs/fuse/inode.c b/fs/fuse/inode.c
> index b9beb39a4a18..8e22a31b55c4 100644
> --- a/fs/fuse/inode.c
> +++ b/fs/fuse/inode.c
> @@ -60,6 +60,10 @@ MODULE_PARM_DESC(max_user_congthresh,
>   /** Congestion starts at 75% of maximum */
>   #define FUSE_DEFAULT_CONGESTION_THRESHOLD (FUSE_DEFAULT_MAX_BACKGROUND 
> * 3 / 4)
> 
> +static inline bool attr_newer_than_local(struct fuse_attr *attr, struct 
> inode *inode) {
> +    return (attr->mtime > inode->i_mtime.tv_sec)
> +               || ((attr->mtime == inode->i_mtime.tv_sec) && 
> (attr->mtimensec > inode->i_mtime.tv_nsec));
> +}
>   #ifdef CONFIG_BLOCK
>   static struct file_system_type fuseblk_fs_type;
>   #endif
> @@ -241,8 +245,10 @@ void fuse_change_attributes(struct inode *inode, 
> struct fuse_attr *attr,
>           * extend local i_size without keeping userspace server in 
> sync. So,
>           * attr->size coming from server can be stale. We cannot trust it.
>           */
> -       if (!is_wb || !S_ISREG(inode->i_mode))
> +       if (!is_wb || !S_ISREG(inode->i_mode)
> +               || (attr_newer_than_local(attr, inode) && 
> !inode_is_open_for_write(inode))) {
>                  i_size_write(inode, attr->size);
> +       }
>          spin_unlock(&fi->lock);
> 
>          if (!is_wb && S_ISREG(inode->i_mode)) {
> 
> On 2021/6/22 23:19, Miklos Szeredi wrote:
>> On Tue, 22 Jun 2021 at 14:25, Fengnan Chang <changfengnan@vivo.com> 
>> wrote:
>>>
>>> Unh, it seems i_writecount not work.
>>> If we modify file through lowerfs, i_writecount won't change, but the
>>> size already changed.
>>> For example:
>>> echo "111" > /lowerfs/test
>>> ls -l /upper/test
>>> echo "2222" >> /lowerfs/test
>>> ls -l /upper/test
>>>
>>> So, can you describe your test enviroment? including kernel version and
>>> fsx parameters, I will check it.
>>
>> linux-5.13-rc5 + patch
>> mkdir /tmp/test
>> libfuse/example/passthrough_ll -ocache=always,writeback /mnt/fuse/
>> fsx-linux -N 1000000 /mnt/fuse/tmp/test/fsx
>>
>> Thanks,
>> Miklos
>>

Re: [PATCH v2] fuse: use newer inode info when writeback cache is enabled

[Miklos Szeredi]

On Thu, 24 Jun 2021 at 09:42, Fengnan Chang <changfengnan@vivo.com> wrote:
>
> Hi Miklos:
>
> Thank you for the information, I have been able to reproduce the problem.
>
> The new version of the patch as below. Previous fsx test is pass now.
> Need do more test, Can you help to test new patch? or send me your test
> case, I will test this.
>
> Here is my test case, and is the problem this patch is trying to solve.
> Case A:
> mkdir /tmp/test
> passthrough_ll -ocache=always,writeback /mnt/test/
> echo "11111" > /tmp/test/fsx
> ls -l /mnt/test/tmp/test/
> echo "2222" >> /tmp/test/fsx
> ls -l /mnt/test/tmp/test/
>
> Case B:
> mkdir /tmp/test
> passthrough_ll -ocache=always,writeback /mnt/test/
> passthrough_ll -ocache=always,writeback /mnt/test2/
> echo "11111" > /tmp/test/fsx
> ls -l /mnt/test/tmp/test/
> ls -l /mnt/test2/tmp/test/
> echo "222" >> /mnt/test/tmp/test/fsx
> ls -l /mnt/test/tmp/test/
> ls -l /mnt/test2/tmp/test/

Both these testcases have the "cache=always" option, which means:
cached values (both data and metadata) are always valid; i.e. changes
will be made only through this client and not through some other
channel (like the backing filesystem or another instance).

Why is "cache=always" used?

Thanks,
Miklos

Re: [PATCH v2] fuse: use newer inode info when writeback cache is enabled

[Fengnan Chang]

Remove cache=always still have this problem, this problem is related 
about FUSE_CAP_WRITEBACK_CACHE.

On 2021/8/6 20:20, Miklos Szeredi wrote:
> On Thu, 24 Jun 2021 at 09:42, Fengnan Chang <changfengnan@vivo.com> wrote:
>>
>> Hi Miklos:
>>
>> Thank you for the information, I have been able to reproduce the problem.
>>
>> The new version of the patch as below. Previous fsx test is pass now.
>> Need do more test, Can you help to test new patch? or send me your test
>> case, I will test this.
>>
>> Here is my test case, and is the problem this patch is trying to solve.
>> Case A:
>> mkdir /tmp/test
>> passthrough_ll -ocache=always,writeback /mnt/test/
>> echo "11111" > /tmp/test/fsx
>> ls -l /mnt/test/tmp/test/
>> echo "2222" >> /tmp/test/fsx
>> ls -l /mnt/test/tmp/test/
>>
>> Case B:
>> mkdir /tmp/test
>> passthrough_ll -ocache=always,writeback /mnt/test/
>> passthrough_ll -ocache=always,writeback /mnt/test2/
>> echo "11111" > /tmp/test/fsx
>> ls -l /mnt/test/tmp/test/
>> ls -l /mnt/test2/tmp/test/
>> echo "222" >> /mnt/test/tmp/test/fsx
>> ls -l /mnt/test/tmp/test/
>> ls -l /mnt/test2/tmp/test/
> 
> Both these testcases have the "cache=always" option, which means:
> cached values (both data and metadata) are always valid; i.e. changes
> will be made only through this client and not through some other
> channel (like the backing filesystem or another instance).
> 
> Why is "cache=always" used?
> 
> Thanks,
> Miklos
> 

Re: [PATCH v2] fuse: use newer inode info when writeback cache is enabled

[Peng Tao]

On Tue, Aug 10, 2021 at 3:12 PM Fengnan Chang <changfengnan@vivo.com> wrote:
>
> Remove cache=always still have this problem, this problem is related
> about FUSE_CAP_WRITEBACK_CACHE.

FUSE_CAP_WRITEBACK_CACHE by definition asks the kernel to trust its
own inode attributes. So I don't think we should fix its semantics.
Instead, in your case, it seems that the two mnts (PATHA and PATHB)
are not sharing the same superblock. I would suggest fixing it at a
higher level:
1. use bind-mount to mount PATHA and PATHB,
2. or add a `tag=xxx` argument to fuse mount option to uniquely
identify a fuse file system (just like we do in the virtiofs case)

Cheers,
Tao
-- 
Into Sth. Rich & Strange

Re: [PATCH v2] fuse: use newer inode info when writeback cache is enabled

[changfengnan]

It's been a long time since this patch was sent, any review comments?
Thanks

答复: [PATCH v2] fuse: use newer inode info when writeback cache is enabled

[changfengnan]

Hi Miklos：

Did you get a chance to review this patch? looking forward to your comments.

Thanks

-----邮件原件-----
发件人: changfengnan@vivo.com <changfengnan@vivo.com> 
发送时间: 2021年5月7日 18:10
收件人: changfengnan@vivo.com
抄送: linux-fsdevel@vger.kernel.org; miklos@szeredi.hu
主题: Re: [PATCH v2] fuse: use newer inode info when writeback cache is
enabled

It's been a long time since this patch was sent, any review comments?
Thanks

Re: [PATCH v2] fuse: use newer inode info when writeback cache is enabled

[Miklos Szeredi]

On Tue, May 11, 2021 at 1:21 PM <changfengnan@vivo.com> wrote:
>
> Hi Miklos：
>
> Did you get a chance to review this patch? looking forward to your comments.

Hi,

The patch looks correct, but this is a complex issue and I want to
make sure I understand it fully before applying.

I've not forgotten about it, it's queued for review.

Thanks,
Miklos