Re: [PATCH 4/7] btrfs: Switch to iomap_dio_rw() for dio

From: Filipe Manana <fdmanana@gmail.com>
To: Goldwyn Rodrigues <rgoldwyn@suse.de>
Cc: Johannes Thumshirn <Johannes.Thumshirn@wdc.com>,
	"linux-btrfs@vger.kernel.org" <linux-btrfs@vger.kernel.org>,
	"hch@infradead.org" <hch@infradead.org>,
	"dsterba@suse.cz" <dsterba@suse.cz>
Subject: Re: [PATCH 4/7] btrfs: Switch to iomap_dio_rw() for dio
Date: Thu, 28 May 2020 17:45:24 +0100	[thread overview]
Message-ID: <CAL3q7H700X4E5-NjTWcWwosBwLuKeFPOPx00f+OVn=2fBfmJbQ@mail.gmail.com> (raw)
In-Reply-To: <20200528163450.uykayisbrn6hfm2z@fiona>

On Thu, May 28, 2020 at 5:34 PM Goldwyn Rodrigues <rgoldwyn@suse.de> wrote:
>
> On 16:13 28/05, Filipe Manana wrote:
> > On Tue, May 26, 2020 at 5:47 PM Goldwyn Rodrigues <rgoldwyn@suse.de> wrote:
> > >
> > > On 15:03 26/05, Johannes Thumshirn wrote:
> > > > Just as a heads up, this one gives me lot's of Page cache invalidation
> > > > failure prints from dio_warn_stale_pagecache() on btrfs/004 with
> > > > current misc-next:
> > > >
> > > <snip>
> > >
> > > > [   23.696400] Page cache invalidation failure on direct I/O.  Possible data corruption due to collision with buffered I/O!
> > > > [   23.698115] File: /mnt/scratch/bgnoise/p0/f0 PID: 6562 Comm: fsstress
> > > >
> > > > I have no idea yet why but I'm investigating.
> > >
> > > This is caused because we are trying to release a page when the extent
> > > has locked the page and release page returns false.
> >
> > By "we" you mean what exaclty, a direct IO read, a direct IO write?
> >
> > 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.

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

Thanks.

>
> --
> Goldwyn

-- 
Filipe David Manana,

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