[PATCH RFC] fuse: invalidate inode attr in writeback cache mode

[PATCH RFC] fuse: invalidate inode attr in writeback cache mode

[Eryu Guan]

Under writeback mode, inode->i_blocks is not updated, making utils like
du read st.blocks as 0.

For example, when using virtiofs (cache=always & nondax mode) with
writeback_cache enabled, writing a new file and check its disk usage
with du, du reports 0 usage.

  # uname -r
  5.6.0-rc6+
  # mount -t virtiofs virtiofs /mnt/virtiofs
  # rm -f /mnt/virtiofs/testfile

  # create new file and do extend write
  # xfs_io -fc "pwrite 0 4k" /mnt/virtiofs/testfile
  wrote 4096/4096 bytes at offset 0
  4 KiB, 1 ops; 0.0001 sec (28.103 MiB/sec and 7194.2446 ops/sec)
  # stat -c %s,%b /mnt/virtiofs/testfile
  4096,0	  <==== i_size is correct, but block usage is 0

Fix it by invalidating attr in fuse_write_end(), like other write paths.

Signed-off-by: Eryu Guan <eguan@linux.alibaba.com>
---
 fs/fuse/file.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/fs/fuse/file.c b/fs/fuse/file.c
index 9d67b830fb7a..3c875104ca11 100644
--- a/fs/fuse/file.c
+++ b/fs/fuse/file.c
@@ -2228,6 +2228,7 @@ static int fuse_write_end(struct file *file, struct address_space *mapping,
 	}
 
 	fuse_write_update_size(inode, pos + copied);
+	fuse_invalidate_attr(inode);
 	set_page_dirty(page);
 
 unlock:
-- 
1.8.3.1

Re: [PATCH RFC] fuse: invalidate inode attr in writeback cache mode

[Miklos Szeredi]

On Wed, May 6, 2020 at 5:21 PM Eryu Guan <eguan@linux.alibaba.com> wrote:
>
> Under writeback mode, inode->i_blocks is not updated, making utils like
> du read st.blocks as 0.
>
> For example, when using virtiofs (cache=always & nondax mode) with
> writeback_cache enabled, writing a new file and check its disk usage
> with du, du reports 0 usage.

Hmm... invalidating the attribute might also yield the wrong result as
the server may not have received the WRITE request that modifies the
underlying file.

Invalidating attributes at the end of fuse_flush() definitely makes
sense, though.

If we wanted 100% correct behavior, we'd need to flush WRITE requests
before each GETATTR request.  That might be a performance bottleneck,
though.

So first I'd just try doing the invalidation from fuse_flush().

Thanks,
Miklos

Re: [PATCH RFC] fuse: invalidate inode attr in writeback cache mode

[Eryu Guan]

On Mon, May 11, 2020 at 03:06:08PM +0200, Miklos Szeredi wrote:
> On Wed, May 6, 2020 at 5:21 PM Eryu Guan <eguan@linux.alibaba.com> wrote:
> >
> > Under writeback mode, inode->i_blocks is not updated, making utils like
> > du read st.blocks as 0.
> >
> > For example, when using virtiofs (cache=always & nondax mode) with
> > writeback_cache enabled, writing a new file and check its disk usage
> > with du, du reports 0 usage.
> 
> Hmm... invalidating the attribute might also yield the wrong result as
> the server may not have received the WRITE request that modifies the
> underlying file.

That's true, I open-write a file then sleep without closing it, and
check file's st_blocks in another terminal, the usage is 0, and remains
0 even after the file is closed, because the wrong attr is cached.

> 
> Invalidating attributes at the end of fuse_flush() definitely makes
> sense, though.

du also reports 0 if I didn't close the file, but I got correct
st_blocks when it's closed.

> 
> If we wanted 100% correct behavior, we'd need to flush WRITE requests
> before each GETATTR request.  That might be a performance bottleneck,
> though.
> 
> So first I'd just try doing the invalidation from fuse_flush().

Sure, will send v2 soon.

> 
> Thanks,
> Miklos

Thanks for the review!

Eryu

[PATCH v2] fuse: invalidate inode attr in writeback cache mode

[Eryu Guan]

Under writeback mode, inode->i_blocks is not updated, making utils du
read st.blocks as 0.

For example, when using virtiofs (cache=always & nondax mode) with
writeback_cache enabled, writing a new file and check its disk usage
with du, du reports 0 usage.

  # uname -r
  5.6.0-rc6+
  # mount -t virtiofs virtiofs /mnt/virtiofs
  # rm -f /mnt/virtiofs/testfile

  # create new file and do extend write
  # xfs_io -fc "pwrite 0 4k" /mnt/virtiofs/testfile
  wrote 4096/4096 bytes at offset 0
  4 KiB, 1 ops; 0.0001 sec (28.103 MiB/sec and 7194.2446 ops/sec)
  # du -k /mnt/virtiofs/testfile
  0               <==== disk usage is 0
  # stat -c %s,%b /mnt/virtiofs/testfile
  4096,0          <==== i_size is correct, but st_blocks is 0

Fix it by invalidating attr in fuse_flush(), so we get up-to-date attr
from server on next getattr.

Signed-off-by: Eryu Guan <eguan@linux.alibaba.com>
---
v2:
- do fuse_invalidate_attr() in fuse_flush() as Miklos suggested

 fs/fuse/file.c | 11 ++++++++++-
 1 file changed, 10 insertions(+), 1 deletion(-)

diff --git a/fs/fuse/file.c b/fs/fuse/file.c
index 9d67b830fb7a..4bb399caec01 100644
--- a/fs/fuse/file.c
+++ b/fs/fuse/file.c
@@ -445,8 +445,9 @@ static int fuse_flush(struct file *file, fl_owner_t id)
 	if (is_bad_inode(inode))
 		return -EIO;
 
+	err = 0;
 	if (fc->no_flush)
-		return 0;
+		goto inval_attr_out;
 
 	err = write_inode_now(inode, 1);
 	if (err)
@@ -475,6 +476,14 @@ static int fuse_flush(struct file *file, fl_owner_t id)
 		fc->no_flush = 1;
 		err = 0;
 	}
+
+inval_attr_out:
+	/*
+	 * In memory i_blocks is not maintained by fuse, if writeback cache is
+	 * enabled, i_blocks from cached attr may not be accurate.
+	 */
+	if (!err && fc->writeback_cache)
+		fuse_invalidate_attr(inode);
 	return err;
 }
 
-- 
2.26.1.107.gefe3874

Re: [PATCH v2] fuse: invalidate inode attr in writeback cache mode

[Miklos Szeredi]

On Tue, May 12, 2020 at 4:29 AM Eryu Guan <eguan@linux.alibaba.com> wrote:
>
> Under writeback mode, inode->i_blocks is not updated, making utils du
> read st.blocks as 0.
>
> For example, when using virtiofs (cache=always & nondax mode) with
> writeback_cache enabled, writing a new file and check its disk usage
> with du, du reports 0 usage.
>
>   # uname -r
>   5.6.0-rc6+
>   # mount -t virtiofs virtiofs /mnt/virtiofs
>   # rm -f /mnt/virtiofs/testfile
>
>   # create new file and do extend write
>   # xfs_io -fc "pwrite 0 4k" /mnt/virtiofs/testfile
>   wrote 4096/4096 bytes at offset 0
>   4 KiB, 1 ops; 0.0001 sec (28.103 MiB/sec and 7194.2446 ops/sec)
>   # du -k /mnt/virtiofs/testfile
>   0               <==== disk usage is 0
>   # stat -c %s,%b /mnt/virtiofs/testfile
>   4096,0          <==== i_size is correct, but st_blocks is 0
>
> Fix it by invalidating attr in fuse_flush(), so we get up-to-date attr
> from server on next getattr.

Thanks, applied.

I started thinking: why is fuse_flush() only writing out dirty pages
if fc->no_flush is false?   It just doesn't make sense...  But that's
an independent bug, and I'll do a separate patch for that.

Thanks,
Miklos