Received-SPF: pass (google.com: domain of linux-kernel+bounces-44748-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45e3:2400::1 as permitted sender) client-ip=2604:1380:45e3:2400::1;
Date: Tue, 30 Jan 2024 09:28:22 -0500
From: Brian Foster <bfoster@redhat.com>
To: Christoph Hellwig <hch@lst.de>
Cc: linux-mm@kvack.org, Matthew Wilcox <willy@infradead.org>,
	Jan Kara <jack@suse.com>, David Howells <dhowells@redhat.com>,
	Christian Brauner <brauner@kernel.org>,
	"Darrick J. Wong" <djwong@kernel.org>, linux-xfs@vger.kernel.org,
	linux-fsdevel@vger.kernel.org, linux-kernel@vger.kernel.org,
	Jan Kara <jack@suse.cz>, Dave Chinner <dchinner@redhat.com>
Subject: Re: [PATCH 06/19] writeback: Factor out writeback_finish()
Message-ID: <ZbkHhos9oxnUZL/1@bfoster>
References: <20240125085758.2393327-1-hch@lst.de>
 <20240125085758.2393327-7-hch@lst.de>
 <ZbgG+7QhHGMz/uMJ@bfoster>
 <20240130140459.GA31126@lst.de>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20240130140459.GA31126@lst.de>

On Tue, Jan 30, 2024 at 03:04:59PM +0100, Christoph Hellwig wrote:
> On Mon, Jan 29, 2024 at 03:13:47PM -0500, Brian Foster wrote:
> > > @@ -2481,6 +2500,9 @@ int write_cache_pages(struct address_space *mapping,
> > >  				folio_unlock(folio);
> > >  				error = 0;
> > >  			}
> > > +		
> > 
> > JFYI: whitespace damage on the above line.
> 
> Thanks, fixed.
> 
> > 
> > > +			if (error && !wbc->err)
> > > +				wbc->err = error;
> > >  
> > 
> > Also what happened to the return of the above "first error encountered"
> > for the WB_SYNC_ALL case? Is that not needed for some reason (and so the
> > comment just below might require an update)?
> 
> No, this got broken during the various rebases (and is fixed again later
> in the series).  We need to return wbc->err from write_cache_pages at
> this stage, I'll fix it.
> 

Ok, I noticed it was added back once I got to more of the iter
abstraction bits and so figured it was a transient/unintentional thing.
The above tweak makes sense to me.

FWIW, I haven't stared at the final patch long enough to have a strong
opinion. I tend to agree with Jan that the error handling logic in the
current series is a little wonky in that it's one of those things I'd
have to go read the implementation every time to remember what it does,
but the broader changes all seem reasonable to me. So for patches 1-18
and with the above tweak:

Reviewed-by: Brian Foster <bfoster@redhat.com>