[Virtio-fs] [PATCH] overlayfs: Pass O_TRUNC flag to underlying filesystem

[Miklos Szeredi]

On Tue, Apr 21, 2020 at 11:04 PM Vivek Goyal <vgoyal at 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 at 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