[PATCH 0/2] overlayfs: Fix overlayfs on virtiofs open(O_TRUNC) issue

[PATCH 0/2] overlayfs: Fix overlayfs on virtiofs open(O_TRUNC) issue

[Vivek Goyal]

Hi Miklos,

Here are couple of fixes solving open(O_TRUNC) issue while overlayfs
is layered on top of virtiofs.

These are also available here.

https://github.com/rhvgoyal/linux/commits/ovl-setattr-fix

Thanks
Vivek

Vivek Goyal (2):
  overlayfs: ovl_setattr() should clear ATTR_FILE from attr->ia_valid
  overlayfs: ovl_setattr() should clear ATTR_OPEN

 fs/overlayfs/inode.c | 16 ++++++++++++++++
 1 file changed, 16 insertions(+)

-- 
2.25.3

[PATCH 1/2] overlayfs: ovl_setattr() should clear ATTR_FILE from attr->ia_valid

[Vivek Goyal]

ovl_setattr() can be passed an attr which has ATTR_FILE set and
attr->ia_file is a file pointer to overlay file. This is done
in open(O_TRUNC) path.

We should either replace with attr->ia_file with underlying file object
or clear ATTR_FILE so that underlying filesystem does not end up using
overlayfs file object pointer.

There are no good use cases yet so for now clear ATTR_FILE. fuse seems
to be one user which can use this. But it can work even without this.
So it is not mandatory to pass ATTR_FILE to fuse.

Signed-off-by: Vivek Goyal <vgoyal@redhat.com>
---
 fs/overlayfs/inode.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/fs/overlayfs/inode.c b/fs/overlayfs/inode.c
index b0d42ece4d7c..8d147bc70f0b 100644
--- a/fs/overlayfs/inode.c
+++ b/fs/overlayfs/inode.c
@@ -58,6 +58,11 @@ int ovl_setattr(struct dentry *dentry, struct iattr *attr)
 		if (attr->ia_valid & (ATTR_KILL_SUID|ATTR_KILL_SGID))
 			attr->ia_valid &= ~ATTR_MODE;
 
+		/* We might have to translate ovl file into underlying file
+		 * object once some use cases are there. For now, simply
+		 * don't let underlying filesystem rely on attr->ia_file */
+		attr->ia_valid &= ~ATTR_FILE;
+
 		inode_lock(upperdentry->d_inode);
 		old_cred = ovl_override_creds(dentry->d_sb);
 		err = notify_change(upperdentry, attr, NULL);
-- 
2.25.3

[PATCH 2/2] overlayfs: ovl_setattr() should clear ATTR_OPEN

[Vivek Goyal]

As of now during open(), we don't pass bunch of flags to underlying
filesystem. O_TRUNC is one of these. Normally this is not a problem as VFS
calls ->setattr() with zero size and underlying filesystem sets file size
to 0.

But when overlayfs is running on top of virtiofs, it has an optimization
where it does not send setattr request to server if dectects that
truncation is part of open(O_TRUNC). It assumes that server already zeroed
file size as part of open(O_TRUNC).

fuse_do_setattr() {
        if (attr->ia_valid & ATTR_OPEN) {
                /*
                 * No need to send request to userspace, since actual
                 * truncation has already been done by OPEN.  But still
                 * need to truncate page cache.
                 */
        }
}

IOW, fuse expects O_TRUNC to be passed to it as part of open flags.

But currently overlayfs does not pass O_TRUNC to underlying filesystem
hence fuse/virtiofs breaks. Setup overlayfs on top of virtiofs and
following does not zero the file size of a file is either upper only
or has already been copied up.

fd = open(foo.txt, O_TRUNC | O_WRONLY);

There are two ways to fix this. Either pass O_TRUNC to underlying filesystem
or clear ATTR_OPEN from attr->ia_valid so that fuse ends up sending a
SETATTR request to server. Miklos is concerned that O_TRUNC might have
side affects so it is better to clear ATTR_OPEN for now. Hence this patch
clears ATTR_OPEN from attr->ia_valid.

I found this problem while running unionmount-testsuite. With this patch,
unionmount-testsuite passes with overlayfs on top of virtiofs.

Signed-off-by: Vivek Goyal <vgoyal@redhat.com>
---
 fs/overlayfs/inode.c | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/fs/overlayfs/inode.c b/fs/overlayfs/inode.c
index 8d147bc70f0b..08ae88b72d9a 100644
--- a/fs/overlayfs/inode.c
+++ b/fs/overlayfs/inode.c
@@ -37,6 +37,17 @@ int ovl_setattr(struct dentry *dentry, struct iattr *attr)
 
 		/* Truncate should trigger data copy up as well */
 		full_copy_up = true;
+
+		/* If open(O_TRUNC) is done, VFS calls ->setattr with
+		 * ATTR_OPEN set. Overlayfs does not pass O_TRUNC flag
+		 * to underlying filesystem during open. Do not pass
+		 * ATTR_OPEN. This disables optimization in fuse which
+		 * assumes open(O_TRUNC) already set file size to 0. But
+		 * we never passed O_TRUNC to fuse. So by clearing ATTR_OPEN,
+		 * fuse will be forced to set ->setattr() request to
+		 * server.
+		 */
+		attr->ia_valid &= ~ATTR_OPEN;
 	}
 
 	if (!full_copy_up)
-- 
2.25.3