diff mbox

fuse: invalidate inode pagecache when atomic_o_trunc flag is enabled

Message ID CAJfpegvdWCC_jy_sjCZJm-x6PU7fA7L6wPezdZG7WTg2FijLWA@mail.gmail.com (mailing list archive)
State New, archived
Headers show

Commit Message

Miklos Szeredi Feb. 8, 2018, 2:17 p.m. UTC 
On Tue, Dec 5, 2017 at 10:12 PM, Chad Austin <chadaustin@fb.com> wrote:
> When the atomic_o_trunc flag is set, fuse_do_setattr skips most of its
> logic, including truncating the page cache.  Run the same invalidation
> logic in fuse_finish_open when truncating.  This fixes a bug where
> open(O_TRUNC) followed by a sequence of writes does not flush the page
> cache, resulting in an mmap longer than the new file's length (from
> another process) not properly zero-filling the last page.
>
> Does fuse_finish_open() need additional locking?  I am not sure of the
> locking requirements of truncate_pagecache or invalidate_inode_pages2.
>
> The following C++ program reproduces the bug.  The FUSE daemon must
> have enabled the ATOMIC_O_TRUNC flag
> https://gist.github.com/chadaustin/8e4ba1e2cd2d023ff6f9eff921eb8bfc

Thanks for the report and the patch.

Here's a simpler patch (we don't actually need the
invalidate_inode_pages2(), because all the pages have already been
truncated).

I tested it with your reproducer and it fixes the issue for me.

Unless you see a problem with it, I'll queue this up for 4.16.

Thanks,
Miklos

Comments

Chad Austin Feb. 9, 2018, 6:05 p.m. UTC | #1 
On Thu, Feb 8, 2018 at 6:17 AM, Miklos Szeredi <miklos@szeredi.hu> wrote:
> Thanks for the report and the patch.
>
> Here's a simpler patch (we don't actually need the
> invalidate_inode_pages2(), because all the pages have already been
> truncated).
>
> I tested it with your reproducer and it fixes the issue for me.
>
> Unless you see a problem with it, I'll queue this up for 4.16.

Thanks!  I verified that your simpler patch also fixes our more
complex internal reproduction.  Looking forward to it landing!
diff mbox

Patch

---
 fs/fuse/dir.c |    6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

--- a/fs/fuse/dir.c
+++ b/fs/fuse/dir.c
@@ -1629,8 +1629,12 @@  int fuse_do_setattr(struct dentry *dentr
 		return err;
 
 	if (attr->ia_valid & ATTR_OPEN) {
-		if (fc->atomic_o_trunc)
+		if (fc->atomic_o_trunc) {
+			WARN_ON(!(attr->ia_valid & ATTR_SIZE));
+			WARN_ON(attr->ia_size != 0);
+			truncate_pagecache(inode, 0);
 			return 0;
+		}
 		file = NULL;
 	}