Re: [PATCH] iomap: Return zero in case of unsuccessful pagecache invalidation before DIO

[Filipe Manana <fdmanana@gmail.com>]

On Wed, Jun 3, 2020 at 12:23 PM Filipe Manana <fdmanana@gmail.com> wrote:
>
> On Mon, Jun 1, 2020 at 4:16 PM Goldwyn Rodrigues <rgoldwyn@suse.de> wrote:
> >
> > On 17:23 28/05, Darrick J. Wong wrote:
> > > On Thu, May 28, 2020 at 02:21:03PM -0500, Goldwyn Rodrigues wrote:
> > > >
> > > > Filesystems such as btrfs are unable to guarantee page invalidation
> > > > because pages could be locked as a part of the extent. Return zero
> > >
> > > Locked for what?  filemap_write_and_wait_range should have just cleaned
> > > them off.
> > >
> > > > in case a page cache invalidation is unsuccessful so filesystems can
> > > > fallback to buffered I/O. This is similar to
> > > > generic_file_direct_write().
> > > >
> > > > This takes care of the following invalidation warning during btrfs
> > > > mixed buffered and direct I/O using iomap_dio_rw():
> > > >
> > > > Page cache invalidation failure on direct I/O.  Possible data
> > > > corruption due to collision with buffered I/O!
> > > >
> > > > Signed-off-by: Goldwyn Rodrigues <rgoldwyn@suse.com>
> > > >
> > > > diff --git a/fs/iomap/direct-io.c b/fs/iomap/direct-io.c
> > > > index e4addfc58107..215315be6233 100644
> > > > --- a/fs/iomap/direct-io.c
> > > > +++ b/fs/iomap/direct-io.c
> > > > @@ -483,9 +483,15 @@ iomap_dio_rw(struct kiocb *iocb, struct iov_iter *iter,
> > > >      */
> > > >     ret = invalidate_inode_pages2_range(mapping,
> > > >                     pos >> PAGE_SHIFT, end >> PAGE_SHIFT);
> > > > -   if (ret)
> > > > -           dio_warn_stale_pagecache(iocb->ki_filp);
> > > > -   ret = 0;
> > > > +   /*
> > > > +    * If a page can not be invalidated, return 0 to fall back
> > > > +    * to buffered write.
> > > > +    */
> > > > +   if (ret) {
> > > > +           if (ret == -EBUSY)
> > > > +                   ret = 0;
> > > > +           goto out_free_dio;
> > >
> > > XFS doesn't fall back to buffered io when directio fails, which means
> > > this will cause a regression there.
> > >
> > > Granted mixing write types is bogus...
> > >
> >
> > I have not seen page invalidation failure errors on XFS, but what should
> > happen hypothetically if they do occur? Carry on with the direct I/O?
> > Would an error return like -ENOTBLK be better?
>
> It doesn't make much to me to emit the warning and then proceed to the
> direct IO write path anyway, as if nothing happened.
> If we are concerned about possible corruption, we should either return
> an error or fallback to buffered IO just like
> generic_file_direct_write() did, and not allow the possibility for
> corruptions.
>
> Btw, this is causing a regression in Btrfs now. The problem is that
> dio_warn_stale_pagecache() sets an EIO error in the inode's mapping:
>
> errseq_set(&inode->i_mapping->wb_err, -EIO);
>
> So the next fsync on the file will return that error, despite the
> fsync having completed successfully with any errors.
>
> Since patchset to make btrfs direct IO use iomap is already in Linus'
> tree, we need to fix this somehow.
> This makes generic/547 fail often for example - buffered write against
> file + direct IO write + fsync - the later returns -EIO.

Just to make it clear, despite the -EIO error, there was actually no
data loss or corruption (generic/547 checks that),
since the direct IO write path in btrfs figures out there's a buffered
write still ongoing and waits for it to complete before proceeding
with the dio write.

Nevertheless, it's still a regression, -EIO shouldn't be returned as
everything went fine.

>
> Thanks.
>
> >
> > --
> > Goldwyn
>
>
>
> --
> Filipe David Manana,
>
> “Whether you think you can, or you think you can't — you're right.”



-- 
Filipe David Manana,

“Whether you think you can, or you think you can't — you're right.”