Linux-Fsdevel Archive on lore.kernel.org
help / color / mirror / Atom feed
* [PATCH RFC] fuse: invalidate inode attr in writeback cache mode
@ 2020-05-06 15:20 Eryu Guan
  2020-05-11 13:06 ` Miklos Szeredi
  2020-05-12  2:29 ` [PATCH v2] " Eryu Guan
  0 siblings, 2 replies; 5+ messages in thread
From: Eryu Guan @ 2020-05-06 15:20 UTC (permalink / raw)
  To: linux-fsdevel; +Cc: virtio-fs, miklos, Liu Bo, 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


^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH RFC] fuse: invalidate inode attr in writeback cache mode
  2020-05-06 15:20 [PATCH RFC] fuse: invalidate inode attr in writeback cache mode Eryu Guan
@ 2020-05-11 13:06 ` Miklos Szeredi
  2020-05-12  2:06   ` Eryu Guan
  2020-05-12  2:29 ` [PATCH v2] " Eryu Guan
  1 sibling, 1 reply; 5+ messages in thread
From: Miklos Szeredi @ 2020-05-11 13:06 UTC (permalink / raw)
  To: Eryu Guan; +Cc: linux-fsdevel, virtio-fs-list, Liu Bo

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

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH RFC] fuse: invalidate inode attr in writeback cache mode
  2020-05-11 13:06 ` Miklos Szeredi
@ 2020-05-12  2:06   ` Eryu Guan
  0 siblings, 0 replies; 5+ messages in thread
From: Eryu Guan @ 2020-05-12  2:06 UTC (permalink / raw)
  To: Miklos Szeredi; +Cc: linux-fsdevel, virtio-fs-list, Liu Bo

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

^ permalink raw reply	[flat|nested] 5+ messages in thread

* [PATCH v2] fuse: invalidate inode attr in writeback cache mode
  2020-05-06 15:20 [PATCH RFC] fuse: invalidate inode attr in writeback cache mode Eryu Guan
  2020-05-11 13:06 ` Miklos Szeredi
@ 2020-05-12  2:29 ` Eryu Guan
  2020-05-13  9:39   ` Miklos Szeredi
  1 sibling, 1 reply; 5+ messages in thread
From: Eryu Guan @ 2020-05-12  2:29 UTC (permalink / raw)
  To: linux-fsdevel; +Cc: virtio-fs, miklos, Liu Bo, 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


^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH v2] fuse: invalidate inode attr in writeback cache mode
  2020-05-12  2:29 ` [PATCH v2] " Eryu Guan
@ 2020-05-13  9:39   ` Miklos Szeredi
  0 siblings, 0 replies; 5+ messages in thread
From: Miklos Szeredi @ 2020-05-13  9:39 UTC (permalink / raw)
  To: Eryu Guan; +Cc: linux-fsdevel, virtio-fs-list, Liu Bo

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

^ permalink raw reply	[flat|nested] 5+ messages in thread

end of thread, other threads:[~2020-05-13  9:39 UTC | newest]

Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-05-06 15:20 [PATCH RFC] fuse: invalidate inode attr in writeback cache mode Eryu Guan
2020-05-11 13:06 ` Miklos Szeredi
2020-05-12  2:06   ` Eryu Guan
2020-05-12  2:29 ` [PATCH v2] " Eryu Guan
2020-05-13  9:39   ` Miklos Szeredi

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).