[PATCH] overlayfs: Pass O_TRUNC flag to underlying filesystem

[PATCH] overlayfs: Pass O_TRUNC flag to underlying filesystem

[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);

Fix it by passing O_TRUNC to underlying filesystem.

I found this problem while running unionmount-testsuite. It fails.

***
*** ./run --ov --samefs --ts=0 open-trunc
***
TEST open-trunc.py:10: Open O_TRUNC|O_RDONLY
 ./run --open-file /mnt/virtiofs/mnt/a/foo100 -r -t -R 
 ./run --open-file /mnt/virtiofs/mnt/a/foo100 -r -t -R 
TEST open-trunc.py:18: Open O_TRUNC|O_WRONLY
 ./run --open-file /mnt/virtiofs/mnt/a/foo101 -w -t -W q
 ./run --open-file /mnt/virtiofs/mnt/a/foo101 -r -R q
 ./run --open-file /mnt/virtiofs/mnt/a/foo101 -w -t -W p
 ./run --open-file /mnt/virtiofs/mnt/a/foo101 -r -R p
TEST open-trunc.py:28: Open O_TRUNC|O_APPEND|O_WRONLY
 ./run --open-file /mnt/virtiofs/mnt/a/foo102 -a -t -W q
 ./run --open-file /mnt/virtiofs/mnt/a/foo102 -r -R q
 ./run --open-file /mnt/virtiofs/mnt/a/foo102 -a -t -W p
 ./run --open-file /mnt/virtiofs/mnt/a/foo102 -r -R p
/mnt/virtiofs/mnt/a/foo102: File size wrong (got 2, want 1)

After this patch, unionmount-testsuite passes with overlayfs on top of
virtiofs.
 
Signed-off-by: Vivek Goyal <vgoyal@redhat.com>
---
 fs/overlayfs/file.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Index: redhat-linux/fs/overlayfs/file.c
===================================================================
--- redhat-linux.orig/fs/overlayfs/file.c	2020-04-21 13:33:40.777125594 -0400
+++ redhat-linux/fs/overlayfs/file.c	2020-04-21 13:53:30.317125594 -0400
@@ -134,7 +134,7 @@ static int ovl_open(struct inode *inode,
 		return err;
 
 	/* No longer need these flags, so don't pass them on to underlying fs */
-	file->f_flags &= ~(O_CREAT | O_EXCL | O_NOCTTY | O_TRUNC);
+	file->f_flags &= ~(O_CREAT | O_EXCL | O_NOCTTY);
 
 	realfile = ovl_open_realfile(file, ovl_inode_realdata(inode));
 	if (IS_ERR(realfile))

Re: [PATCH] overlayfs: Pass O_TRUNC flag to underlying filesystem

[Miklos Szeredi]

On Tue, Apr 21, 2020 at 8:41 PM Vivek Goyal <vgoyal@redhat.com> wrote:
>
> 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);
>
> Fix it by passing O_TRUNC to underlying filesystem.


Or clear ATTR_OPEN in ovl_setattr()

Need to think about side effects of passing O_TRUNC down to underlying
fs.   Clearing ATTR_OPEN seems obviously safe, so as a quick fix I'd
rather go with that for now.

Thanks,
Miklos

Re: [PATCH] overlayfs: Pass O_TRUNC flag to underlying filesystem

[Vivek Goyal]

On Tue, Apr 21, 2020 at 08:59:24PM +0200, Miklos Szeredi wrote:
> On Tue, Apr 21, 2020 at 8:41 PM Vivek Goyal <vgoyal@redhat.com> wrote:
> >
> > 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);
> >
> > Fix it by passing O_TRUNC to underlying filesystem.
> 
> 
> Or clear ATTR_OPEN in ovl_setattr()
> 
> Need to think about side effects of passing O_TRUNC down to underlying
> fs.   Clearing ATTR_OPEN seems obviously safe, so as a quick fix I'd
> rather go with that for now.

Ok, I will send another patch which clears ATTR_OPEN in ovl_setattr().

Thanks
Vivek

Re: [PATCH] overlayfs: Pass O_TRUNC flag to underlying filesystem

[Vivek Goyal]

On Tue, Apr 21, 2020 at 08:59:24PM +0200, Miklos Szeredi wrote:
> On Tue, Apr 21, 2020 at 8:41 PM Vivek Goyal <vgoyal@redhat.com> wrote:
> >
> > 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);
> >
> > Fix it by passing O_TRUNC to underlying filesystem.
> 
> 
> Or clear ATTR_OPEN in ovl_setattr()
> 
> Need to think about side effects of passing O_TRUNC down to underlying
> fs.   Clearing ATTR_OPEN seems obviously safe, so as a quick fix I'd
> rather go with that for now.

Found another interesting problem while I cleared ATTR_OPEN. VFS also
sets ATTR_FILE and attr->ia_file has ovl file pointer. ovl_setattr() does not
look at this attribute and passes it to underlying layer as it is. Fuse
thinks it got a valid file object and passes file handle to server and
server complains -EBADF.

ext4/xfs don't seem to look at ATTR_FILE, so it did not create problems
so far. 

For now, I will simply reset ATTR_FILE, indicating to lower layers
don't use attr->ia_file.

Thanks
Vivek

Re: [PATCH] overlayfs: Pass O_TRUNC flag to underlying filesystem

[Miklos Szeredi]

On Tue, Apr 21, 2020 at 11:04 PM Vivek Goyal <vgoyal@redhat.com> wrote:
>
> On Tue, Apr 21, 2020 at 08:59:24PM +0200, Miklos Szeredi wrote:
> > On Tue, Apr 21, 2020 at 8:41 PM Vivek Goyal <vgoyal@redhat.com> wrote:
> > >
> > > 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);
> > >
> > > Fix it by passing O_TRUNC to underlying filesystem.
> >
> >
> > Or clear ATTR_OPEN in ovl_setattr()
> >
> > Need to think about side effects of passing O_TRUNC down to underlying
> > fs.   Clearing ATTR_OPEN seems obviously safe, so as a quick fix I'd
> > rather go with that for now.
>
> Found another interesting problem while I cleared ATTR_OPEN. VFS also
> sets ATTR_FILE and attr->ia_file has ovl file pointer. ovl_setattr() does not
> look at this attribute and passes it to underlying layer as it is. Fuse
> thinks it got a valid file object and passes file handle to server and
> server complains -EBADF.
>
> ext4/xfs don't seem to look at ATTR_FILE, so it did not create problems
> so far.
>
> For now, I will simply reset ATTR_FILE, indicating to lower layers
> don't use attr->ia_file.

Right.

The solution to that would be to replace attr->ia_file with the real
file, but that can wait until we see any use case for that.

Thanks,
Miklos