[prev in list] [next in list] [prev in thread] [next in thread] 

List:       linux-btrfs
Subject:    Re: [PATCH 4/7] btrfs: Switch to iomap_dio_rw() for dio
From:       Goldwyn Rodrigues <rgoldwyn () suse ! de>
Date:       2020-05-28 18:38:48
Message-ID: 20200528183848.siuljkvqmxbqa436 () fiona
[Download RAW message or body]

On 17:45 28/05, Filipe Manana wrote:
> On Thu, May 28, 2020 at 5:34 PM Goldwyn Rodrigues <rgoldwyn@suse.de> wrote:
> > > And who locked the extent range before?
> >
> > This is usually locked by a previous buffered write or read.
> 
> A previous buffered write/read that has already finished or is still
> in progress?
> 
> Because if it has finished we're not supposed to have the file range
> locked anymore.

In progress. Mixing buffered I/O with direct writes.

> 
> >
> > >
> > > That seems alarming to me, specially if it's a direct IO write failing
> > > to invalidate the page cache, since a subsequent buffered read could
> > > get stale data (what's in the page cache), and not what the direct IO
> > > write wrote.
> > >
> > > Can you elaborate more on all those details?
> >
> > The origin of the message is when iomap_dio_rw() tries to invalidate the
> > inode pages, but fails and calls dio_warn_stale_pagecache().
> >
> > In the vanilla code, generic_file_direct_write() aborts direct writes
> > and returns 0 so that it may fallback to buffered I/O. Perhaps this
> > should be changed in iomap_dio_rw() as well. I will write a patch to
> > accomodate that.
> 
> On vanilla we have no problems of mixing buffered and direct
> operations as long as they are done sequentially at least.
> And even if done concurrently we take several measures to ensure that
> are no surprises (locking ranges, waiting for any ordered extents in
> progress, etc).

Yes, it is because of the code in generic_file_direct_write(). Anyways,
I did some tests with the following patch, and it seems to work. I will
send a formal patch to so that it gets incorporated in iomap sequence as
well.

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
[prev in list] [next in list] [prev in thread] [next in thread]