linux-kernel - Re: [PATCH 19/19] writeback: simplify writeback iteration

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date: Tue, 30 Jan 2024 15:16:01 +0100
From: Christoph Hellwig <hch@....de>
To: Jan Kara <jack@...e.cz>
Cc: Christoph Hellwig <hch@....de>, linux-mm@...ck.org,
	Matthew Wilcox <willy@...radead.org>, Jan Kara <jack@...e.com>,
	David Howells <dhowells@...hat.com>,
	Brian Foster <bfoster@...hat.com>,
	Christian Brauner <brauner@...nel.org>,
	"Darrick J. Wong" <djwong@...nel.org>, linux-xfs@...r.kernel.org,
	linux-fsdevel@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH 19/19] writeback: simplify writeback iteration

On Tue, Jan 30, 2024 at 11:46:05AM +0100, Jan Kara wrote:
> Looking at it now I'm thinking whether we would not be better off to
> completely dump the 'error' argument of writeback_iter() /
> writeback_iter_next() and just make all .writepage implementations set
> wbc->err directly. But that means touching all the ~20 writepage
> implementations we still have...

Heh.  I actually had an earlier version that looked at wbc->err in
the ->writepages callers.  But it felt a bit too ugly.

> > +		 */
> > +		if (wbc->sync_mode == WB_SYNC_NONE &&
> > +		    (wbc->err || wbc->nr_to_write <= 0))
> > +			goto finish;
> 
> I think it would be a bit more comprehensible if we replace the goto with:
> 			folio_batch_release(&wbc->fbatch);
> 			if (wbc->range_cyclic)
> 				mapping->writeback_index =
> 					folio->index + folio_nr_pages(folio);
> 			*error = wbc->err;
> 			return NULL;

I agree that keeping the logic on when to break and when to set the
writeback_index is good, but duplicating the batch release and error
assignment seems a bit suboptimal.  Let me know what you think of the
alternatіve variant below.

> > +	struct folio *folio = 0;
> 			     ^^ NULL please

Fixed.

> >  			ret = writeback_use_writepage(mapping, wbc);
> > +			if (!ret)
> > +				ret = wbc->err;
> 
> AFAICT this should not be needed as writeback_iter() made sure wbc->err is
> returned when set?

Heh.  That's a leftover from my above mentioned different attempt at
error handling and shouldn't have stayed in.