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

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

[Goldwyn Rodrigues]

Filesystems such as btrfs are unable to guarantee page invalidation
because pages could be locked as a part of the extent. Return zero
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;
+	}
 
 	if (iov_iter_rw(iter) == WRITE && !wait_for_completion &&
 	    !inode->i_sb->s_dio_done_wq) {

-- 
Goldwyn

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

[Darrick J. Wong]

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...

--D

> +	}
>  
>  	if (iov_iter_rw(iter) == WRITE && !wait_for_completion &&
>  	    !inode->i_sb->s_dio_done_wq) {
> 
> -- 
> Goldwyn

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

[Filipe Manana]

On Fri, May 29, 2020 at 1:23 AM Darrick J. Wong <darrick.wong@oracle.com> 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.

Yes, it will be confusing even for someone more familiar with btrfs.
The changelog could be more detailed to make it clear what's happening and why.

So what happens:

1) iomap_dio_rw() calls filemap_write_and_wait_range().
    That starts delalloc for all dirty pages in the range and then
waits for writeback to complete.
    This is enough for most filesystems at least (if not all except btrfs).

2) However, in btrfs once writeback finishes, a job is queued to run
on a dedicated workqueue, to execute the function
btrfs_finish_ordered_io().
    So that job will be run after filemap_write_and_wait_range() returns.
    That function locks the file range (using a btrfs specific data
structure), does a bunch of things (updating several btrees), and then
unlocks the file range.

3) While iomap calls invalidate_inode_pages2_range(), which ends up
calling the btrfs callback btfs_releasepage(),
    btrfs_finish_ordered_io() is running and has the file range locked
(this is what Goldwyn means by "pages could be locked", which is
confusing because it's not about any locked struct page).

4) Because the file range is locked, btrfs_releasepage() returns 0
(page can't be released), this happens in the helper function
try_release_extent_state().
    Any page in that range is not dirty nor under writeback anymore
and, in fact, btrfs_finished_ordered_io() doesn't do anything with the
pages, it's only updating metadata.

    btrfs_releasepage() in this case could release the pages, but
there are other contextes where the file range is locked, the pages
are still not dirty and not under writeback, where this would not be
safe to do.

5) So because of that invalidate_inode_pages2_range() returns
non-zero, the iomap code prints that warning message and then proceeds
with doing a direct IO write anyway.

What happens currently in btrfs, before Goldwyn's patchset:

1) generic_file_direct_write() also calls filemap_write_and_wait_range().
2) After that it calls invalidate_inode_pages2_range() too, but if
that returns non-zero, it doesn't print any warning and falls back to
a buffered write.

So Goldwyn here is effectively adding that behaviour from
generic_file_direct_write() to iomap.

Thanks.

>
> > 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...
>
> --D
>
> > +     }
> >
> >       if (iov_iter_rw(iter) == WRITE && !wait_for_completion &&
> >           !inode->i_sb->s_dio_done_wq) {
> >
> > --
> > Goldwyn



-- 
Filipe David Manana,

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

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

[Matthew Wilcox]

On Fri, May 29, 2020 at 11:55:33AM +0100, Filipe Manana wrote:
> On Fri, May 29, 2020 at 1:23 AM Darrick J. Wong <darrick.wong@oracle.com> 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.
> 
> Yes, it will be confusing even for someone more familiar with btrfs.
> The changelog could be more detailed to make it clear what's happening and why.
> 
> So what happens:
> 
> 1) iomap_dio_rw() calls filemap_write_and_wait_range().
>     That starts delalloc for all dirty pages in the range and then
> waits for writeback to complete.
>     This is enough for most filesystems at least (if not all except btrfs).
> 
> 2) However, in btrfs once writeback finishes, a job is queued to run
> on a dedicated workqueue, to execute the function
> btrfs_finish_ordered_io().
>     So that job will be run after filemap_write_and_wait_range() returns.
>     That function locks the file range (using a btrfs specific data
> structure), does a bunch of things (updating several btrees), and then
> unlocks the file range.
> 
> 3) While iomap calls invalidate_inode_pages2_range(), which ends up
> calling the btrfs callback btfs_releasepage(),
>     btrfs_finish_ordered_io() is running and has the file range locked
> (this is what Goldwyn means by "pages could be locked", which is
> confusing because it's not about any locked struct page).
> 
> 4) Because the file range is locked, btrfs_releasepage() returns 0
> (page can't be released), this happens in the helper function
> try_release_extent_state().
>     Any page in that range is not dirty nor under writeback anymore
> and, in fact, btrfs_finished_ordered_io() doesn't do anything with the
> pages, it's only updating metadata.
> 
>     btrfs_releasepage() in this case could release the pages, but
> there are other contextes where the file range is locked, the pages
> are still not dirty and not under writeback, where this would not be
> safe to do.

Isn't this the bug, though?  Rather than returning "page can't be
released", shouldn't ->releasepage sleep on the extent state, at least
if the GFP mask indicates you can sleep?

> 5) So because of that invalidate_inode_pages2_range() returns
> non-zero, the iomap code prints that warning message and then proceeds
> with doing a direct IO write anyway.
> 
> What happens currently in btrfs, before Goldwyn's patchset:
> 
> 1) generic_file_direct_write() also calls filemap_write_and_wait_range().
> 2) After that it calls invalidate_inode_pages2_range() too, but if
> that returns non-zero, it doesn't print any warning and falls back to
> a buffered write.
> 
> So Goldwyn here is effectively adding that behaviour from
> generic_file_direct_write() to iomap.
> 
> Thanks.
> 
> >
> > > 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...
> >
> > --D
> >
> > > +     }
> > >
> > >       if (iov_iter_rw(iter) == WRITE && !wait_for_completion &&
> > >           !inode->i_sb->s_dio_done_wq) {
> > >
> > > --
> > > Goldwyn
> 
> 
> 
> -- 
> Filipe David Manana,
> 
> “Whether you think you can, or you think you can't — you're right.”

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

[Filipe Manana]

On Fri, May 29, 2020 at 12:31 PM Matthew Wilcox <willy@infradead.org> wrote:
>
> On Fri, May 29, 2020 at 11:55:33AM +0100, Filipe Manana wrote:
> > On Fri, May 29, 2020 at 1:23 AM Darrick J. Wong <darrick.wong@oracle.com> 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.
> >
> > Yes, it will be confusing even for someone more familiar with btrfs.
> > The changelog could be more detailed to make it clear what's happening and why.
> >
> > So what happens:
> >
> > 1) iomap_dio_rw() calls filemap_write_and_wait_range().
> >     That starts delalloc for all dirty pages in the range and then
> > waits for writeback to complete.
> >     This is enough for most filesystems at least (if not all except btrfs).
> >
> > 2) However, in btrfs once writeback finishes, a job is queued to run
> > on a dedicated workqueue, to execute the function
> > btrfs_finish_ordered_io().
> >     So that job will be run after filemap_write_and_wait_range() returns.
> >     That function locks the file range (using a btrfs specific data
> > structure), does a bunch of things (updating several btrees), and then
> > unlocks the file range.
> >
> > 3) While iomap calls invalidate_inode_pages2_range(), which ends up
> > calling the btrfs callback btfs_releasepage(),
> >     btrfs_finish_ordered_io() is running and has the file range locked
> > (this is what Goldwyn means by "pages could be locked", which is
> > confusing because it's not about any locked struct page).
> >
> > 4) Because the file range is locked, btrfs_releasepage() returns 0
> > (page can't be released), this happens in the helper function
> > try_release_extent_state().
> >     Any page in that range is not dirty nor under writeback anymore
> > and, in fact, btrfs_finished_ordered_io() doesn't do anything with the
> > pages, it's only updating metadata.
> >
> >     btrfs_releasepage() in this case could release the pages, but
> > there are other contextes where the file range is locked, the pages
> > are still not dirty and not under writeback, where this would not be
> > safe to do.
>
> Isn't this the bug, though?  Rather than returning "page can't be
> released", shouldn't ->releasepage sleep on the extent state, at least
> if the GFP mask indicates you can sleep?

Goldwyn mentioned in another thread that he had tried that with the
following patch:

https://patchwork.kernel.org/patch/11275063/

But he mentioned it didn't work though, caused some locking problems.
I don't know the details and I haven't tried the patchset yet.
Goldwyn?

>
> > 5) So because of that invalidate_inode_pages2_range() returns
> > non-zero, the iomap code prints that warning message and then proceeds
> > with doing a direct IO write anyway.
> >
> > What happens currently in btrfs, before Goldwyn's patchset:
> >
> > 1) generic_file_direct_write() also calls filemap_write_and_wait_range().
> > 2) After that it calls invalidate_inode_pages2_range() too, but if
> > that returns non-zero, it doesn't print any warning and falls back to
> > a buffered write.
> >
> > So Goldwyn here is effectively adding that behaviour from
> > generic_file_direct_write() to iomap.
> >
> > Thanks.
> >
> > >
> > > > 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...
> > >
> > > --D
> > >
> > > > +     }
> > > >
> > > >       if (iov_iter_rw(iter) == WRITE && !wait_for_completion &&
> > > >           !inode->i_sb->s_dio_done_wq) {
> > > >
> > > > --
> > > > 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.”

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

[Goldwyn Rodrigues]

On 12:50 29/05, Filipe Manana wrote:
> On Fri, May 29, 2020 at 12:31 PM Matthew Wilcox <willy@infradead.org> wrote:
> >
> > On Fri, May 29, 2020 at 11:55:33AM +0100, Filipe Manana wrote:
> > > On Fri, May 29, 2020 at 1:23 AM Darrick J. Wong <darrick.wong@oracle.com> 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.
> > >
> > > Yes, it will be confusing even for someone more familiar with btrfs.
> > > The changelog could be more detailed to make it clear what's happening and why.
> > >
> > > So what happens:
> > >
> > > 1) iomap_dio_rw() calls filemap_write_and_wait_range().
> > >     That starts delalloc for all dirty pages in the range and then
> > > waits for writeback to complete.
> > >     This is enough for most filesystems at least (if not all except btrfs).
> > >
> > > 2) However, in btrfs once writeback finishes, a job is queued to run
> > > on a dedicated workqueue, to execute the function
> > > btrfs_finish_ordered_io().
> > >     So that job will be run after filemap_write_and_wait_range() returns.
> > >     That function locks the file range (using a btrfs specific data
> > > structure), does a bunch of things (updating several btrees), and then
> > > unlocks the file range.
> > >
> > > 3) While iomap calls invalidate_inode_pages2_range(), which ends up
> > > calling the btrfs callback btfs_releasepage(),
> > >     btrfs_finish_ordered_io() is running and has the file range locked
> > > (this is what Goldwyn means by "pages could be locked", which is
> > > confusing because it's not about any locked struct page).
> > >
> > > 4) Because the file range is locked, btrfs_releasepage() returns 0
> > > (page can't be released), this happens in the helper function
> > > try_release_extent_state().
> > >     Any page in that range is not dirty nor under writeback anymore
> > > and, in fact, btrfs_finished_ordered_io() doesn't do anything with the
> > > pages, it's only updating metadata.
> > >
> > >     btrfs_releasepage() in this case could release the pages, but
> > > there are other contextes where the file range is locked, the pages
> > > are still not dirty and not under writeback, where this would not be
> > > safe to do.
> >
> > Isn't this the bug, though?  Rather than returning "page can't be
> > released", shouldn't ->releasepage sleep on the extent state, at least
> > if the GFP mask indicates you can sleep?
> 
> Goldwyn mentioned in another thread that he had tried that with the
> following patch:
> 
> https://patchwork.kernel.org/patch/11275063/
> 
> But he mentioned it didn't work though, caused some locking problems.
> I don't know the details and I haven't tried the patchset yet.
> Goldwyn?
> 

Yes, direct I/O would wait for extent bits to be unlocked forever and hang.
I think it was against an fsync call, but I don't remember. In the light
of new developments, I would pursue this further. This should be valid
even in the current (before iomap patches) source.

-- 
Goldwyn

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

[Goldwyn Rodrigues]

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?

-- 
Goldwyn

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

[Filipe Manana]

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.

Thanks.

>
> --
> Goldwyn



-- 
Filipe David Manana,

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

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

[Filipe Manana]

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.”

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

[Darrick J. Wong]

On Wed, Jun 03, 2020 at 12:32:15PM +0100, Filipe Manana wrote:
> 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

What happens if you try to dirty an mmap page at the same time as
issuing a directio?

> > > happen hypothetically if they do occur? Carry on with the direct I/O?
> > > Would an error return like -ENOTBLK be better?

In the old days, we would only WARN when we encountered collisions like
this.  How about only setting EIO in the mapping if we fail the
pagecache invalidation in directio completion?  If a buffered write
dirties the page after the direct write thread flushes the dirty pages
but before invalidation, we can argue that we didn't lose anything; the
direct write simply happened after the buffered write.

XFS doesn't implement buffered write fallback, and it never has.  Either
the entire directio succeeds, or it returns a negative error code.  Some
of the iomap_dio_rw callers (ext4, jfs2) will notice a short direct
write and try to finish the rest with buffered io, but xfs and zonefs do
not.

The net effect of this (on xfs anyway) is that when buffered and direct
writes collide, before we'd make the buffered writer lose, now we make
the direct writer lose.

You also /could/ propose teaching xfs how to fall back to an
invalidating synchronous buffered write like ext4 does, but that's not
part of this patch set, and that's not a behavior I want to introduce
suddenly during the merge window.

> > 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.

Y'all /just/ sent the pull request containing that conversion 2 days
ago.  Why did you move forward with that when you knew there were
unresolved fstests failures?

> > 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.

Now I'm annoyed because I feel like you're trying to strong-arm me into
making last minute changes to iomap when you could have held off for
another cycle.

While I'm pretty sure your analysis is correct that we could /probably/
get away with only setting EIO if we can't invalidate the cache after
we've already written the disk blocks directly (because then we really
did lose something), I /really/ want these kinds of behavioral changes
to shared code to soak in for-next for long enough that we can work out
the kinks without exposing the upstream kernel to unnecessary risk.

This conversation would be /much/ different had you not just merged the
btrfs directio iomap conversion yesterday.

--D

> 
> >
> > 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.”

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

[Filipe Manana]

On Wed, Jun 3, 2020 at 8:02 PM Darrick J. Wong <darrick.wong@oracle.com> wrote:
>
> On Wed, Jun 03, 2020 at 12:32:15PM +0100, Filipe Manana wrote:
> > 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
>
> What happens if you try to dirty an mmap page at the same time as
> issuing a directio?
>
> > > > happen hypothetically if they do occur? Carry on with the direct I/O?
> > > > Would an error return like -ENOTBLK be better?
>
> In the old days, we would only WARN when we encountered collisions like
> this.  How about only setting EIO in the mapping if we fail the
> pagecache invalidation in directio completion?  If a buffered write
> dirties the page after the direct write thread flushes the dirty pages
> but before invalidation, we can argue that we didn't lose anything; the
> direct write simply happened after the buffered write.
>
> XFS doesn't implement buffered write fallback, and it never has.  Either
> the entire directio succeeds, or it returns a negative error code.  Some
> of the iomap_dio_rw callers (ext4, jfs2) will notice a short direct
> write and try to finish the rest with buffered io, but xfs and zonefs do
> not.
>
> The net effect of this (on xfs anyway) is that when buffered and direct
> writes collide, before we'd make the buffered writer lose, now we make
> the direct writer lose.
>
> You also /could/ propose teaching xfs how to fall back to an
> invalidating synchronous buffered write like ext4 does, but that's not
> part of this patch set, and that's not a behavior I want to introduce
> suddenly during the merge window.
>
> > > 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.
>
> Y'all /just/ sent the pull request containing that conversion 2 days
> ago.  Why did you move forward with that when you knew there were
> unresolved fstests failures?
>
> > > 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.
>
> Now I'm annoyed because I feel like you're trying to strong-arm me into
> making last minute changes to iomap when you could have held off for
> another cycle.

If you are talking to me, I'm not trying to strong-arm anyone nor
point a fingers.
I'm just reporting a problem that I found earlier today while testing
some work I was doing.

Thanks.

>
> While I'm pretty sure your analysis is correct that we could /probably/
> get away with only setting EIO if we can't invalidate the cache after
> we've already written the disk blocks directly (because then we really
> did lose something), I /really/ want these kinds of behavioral changes
> to shared code to soak in for-next for long enough that we can work out
> the kinks without exposing the upstream kernel to unnecessary risk.
>
> This conversation would be /much/ different had you not just merged the
> btrfs directio iomap conversion yesterday.
>
> --D
>
> >
> > >
> > > 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.”



-- 
Filipe David Manana,

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

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

[Matthew Wilcox]

On Wed, Jun 03, 2020 at 08:10:50PM +0100, Filipe Manana wrote:
> On Wed, Jun 3, 2020 at 8:02 PM Darrick J. Wong <darrick.wong@oracle.com> wrote:
> > On Wed, Jun 03, 2020 at 12:32:15PM +0100, Filipe Manana wrote:
> > > On Wed, Jun 3, 2020 at 12:23 PM Filipe Manana <fdmanana@gmail.com> wrote:
> > > > 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.
> >
> > Y'all /just/ sent the pull request containing that conversion 2 days
> > ago.  Why did you move forward with that when you knew there were
> > unresolved fstests failures?
> >
> > Now I'm annoyed because I feel like you're trying to strong-arm me into
> > making last minute changes to iomap when you could have held off for
> > another cycle.
> 
> If you are talking to me, I'm not trying to strong-arm anyone nor
> point a fingers.
> I'm just reporting a problem that I found earlier today while testing
> some work I was doing.

I think the correct response to having just found the bug is to back the
btrfs-to-iomap conversion out of Linus' tree.  I don't think changing
the iomap code at this time is the right approach.

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

[Goldwyn Rodrigues]

On 12:02 03/06, Darrick J. Wong wrote:
> On Wed, Jun 03, 2020 at 12:32:15PM +0100, Filipe Manana wrote:
> > 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
> 
> What happens if you try to dirty an mmap page at the same time as
> issuing a directio?

I did not think of that scenario. But in this case, is mmap working on
stale data? and is it considered a writeback error?

> 
> > > > happen hypothetically if they do occur? Carry on with the direct I/O?
> > > > Would an error return like -ENOTBLK be better?
> 
> In the old days, we would only WARN when we encountered collisions like
> this.  How about only setting EIO in the mapping if we fail the
> pagecache invalidation in directio completion?  If a buffered write
> dirties the page after the direct write thread flushes the dirty pages
> but before invalidation, we can argue that we didn't lose anything; the
> direct write simply happened after the buffered write.

This error will finally be returned by iomap_dio_rw(), and EIO would
mean there is a device error, and not a transient error from which it
can recover. We could return -ENOTBLK, but that is used temporarily for
buffered write fallbacks such as in ext4. iomap still returns zero in
case of such transient errors.

> 
> XFS doesn't implement buffered write fallback, and it never has.  Either
> the entire directio succeeds, or it returns a negative error code.  Some
> of the iomap_dio_rw callers (ext4, jfs2) will notice a short direct
> write and try to finish the rest with buffered io, but xfs and zonefs do
> not.
> 
> The net effect of this (on xfs anyway) is that when buffered and direct
> writes collide, before we'd make the buffered writer lose, now we make
> the direct writer lose.
> 
> You also /could/ propose teaching xfs how to fall back to an
> invalidating synchronous buffered write like ext4 does, but that's not
> part of this patch set, and that's not a behavior I want to introduce
> suddenly during the merge window.

So does that mean XFS would be open to fallback to buffered write?
That would make things much simpler!

-- 
Goldwyn

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

[David Sterba]

On Wed, Jun 03, 2020 at 12:02:52PM -0700, Darrick J. Wong wrote:
> > > 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.
> 
> Y'all /just/ sent the pull request containing that conversion 2 days
> ago.  Why did you move forward with that when you knew there were
> unresolved fstests failures?

Because we didn't know that. And the whole mixed buffered io and dio is
considered obscure, documented as 'do not do that', that tests that
report the warning are more of an annyonance (btrfs/004).

That the test generic/547 sometimes returns EIO on fsync is a
regression, reported after the pull request had been merged, but with a
proposed fix that is not that intrusive, so this all counts as a normal
development.

There is always some risk merging code the like dio-iomap and it was
known but with an ultimate fallback plan to revert it in case we
encounter problems that are not solvable before release. But we're not
there yet.

> > > 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.
> 
> Now I'm annoyed because I feel like you're trying to strong-arm me into
> making last minute changes to iomap when you could have held off for
> another cycle.

The patchset was held off for several releases, gradually making into
state where it can be merged, assuming we will be able to fix potential
regressions. Besides SUSE people involved in the patchset, Christoph
asked why it's not merged and how can he help to move it forward. He's
listed as iomap maintainer so it's not like we were pushing code without
maintainers' involved.

Regarding the last minute change, that's not something we'd ask you to
do without testing first. There are 4 filesystems using iomap for
direct io, making sure it does not regress on them is something I'd
consider necessary before asking you to merge it.

This patchset is lacking that but it started a discussion to understand
the full extent of the bug. We're not in rc5 where calling it 'last
minute' would be appropriate.

The big-hammer option to revert 4 patches is still there. If the fix
turns out to require changes beyond iomap and btrfs code, I'd consider
that as a good reason and I'm ready to do the revert (say rc2 at the
latest).