Re: [PATCH 08/16] btrfs: stop setting PageError in the data I/O path

From: Matthew Wilcox <willy@infradead.org>
To: Christoph Hellwig <hch@lst.de>
Cc: David Sterba <dsterba@suse.cz>, Chris Mason <clm@fb.com>,
	Josef Bacik <josef@toxicpanda.com>,
	David Sterba <dsterba@suse.com>,
	linux-btrfs@vger.kernel.org, linux-mm@kvack.org
Subject: Re: [PATCH 08/16] btrfs: stop setting PageError in the data I/O path
Date: Tue, 30 May 2023 07:08:38 +0100	[thread overview]
Message-ID: <ZHWS5nOm++6zCNkE@casper.infradead.org> (raw)
In-Reply-To: <20230530054547.GA5825@lst.de>

On Tue, May 30, 2023 at 07:45:47AM +0200, Christoph Hellwig wrote:
> On Mon, May 29, 2023 at 07:52:10PM +0200, David Sterba wrote:
> > On Tue, May 23, 2023 at 10:13:14AM +0200, Christoph Hellwig wrote:
> > > PageError is not used by the VFS/MM and deprecated.
> > 
> > Is there some reference other than code? I remember LSF/MM talks about
> > writeback, reducing page flags but I can't find anything that would say
> > that PageError is not used anymore. For such core changes in the
> > neighboring subysystems I kind of rely on lwn.net, hearsay or accidental
> > notice on fsdevel.
> > 
> > Removing the Error bit handling looks overall good but I'd also need to
> > refresh my understanding of the interactions with writeback.
> 
> willy is the driving force behind the PageErro removal, so maybe he
> has a coherent writeup.  But the basic idea is:
> 
>  - page flag space availability is limited, and killing any one we can
>    easily is a good thing
>  - PageError is not well defined and not part of any VM or VFS contract,
>    so in addition to freeing up space it also generally tends to remove
>    confusion.

I don't think I have a coherent writeup.  Basically:

 - The VFS and VM do not check the error flag

   $ git grep folio_test_error
   fs/gfs2/lops.c: if (folio_test_error(folio))
   mm/migrate.c:   if (folio_test_error(folio))

   (the use in mm/migrate.c replicates the error flag on migration)

   A similar grep -w PageError finds only tests in filesystems and
   comments.

 - We do not document clearly under what circumstances the error flag
   is set or cleared

If we can remove all uses of testing the error flag, then we can remove
everywhere that sets or clears the flag.  This is usually a simple
matter of checking folio_test_uptodate() instead of !PageError(),
since the folio will not be marked uptodate if there is a read error.
Write errors are not normally tracked on a per-folio basis.  Instead they
are tracked through mapping_set_error().

I did a number of these conversions about a year ago; I'm happy Christoph
is making progress with btrfs here.  See 'git log 6e8e79fc8443' for many
of them.