Is writeback_iter() missing some error handling? -- was Re: [PATCH] netfs: fix ENOMEM handling in netfs_writepages() to drain all dirty folios

[David Howells]

Yun Zhou <yun.zhou@xxxxxxxxxxxxx> wrote:

> When netfs_create_write_req() fails with -ENOMEM in netfs_writepages(),
> the couldnt_start error path redirties and unlocks the first folio, then
> calls writeback_iter() expecting it to return NULL. However, if the
> mapping contains multiple dirty folios, writeback_iter() returns the
> next one, triggering WARN_ON_ONCE(folio != NULL).
> 
> This can be reproduced via 9p (cache=loose) with shared mmap writes and
> fault injection (fail_nth), where v9fs_mmap_vm_close() triggers
> writeback on a mapping with multiple dirty folios during mmap overlap.
> 
> Fix this by looping over all remaining dirty folios in the ENOMEM path,
> redirtying and unlocking each one. This ensures all folios taken by the
> writeback iterator are properly released, and they will be retried on
> the next writeback cycle when memory is available.
>
> ...
> --- a/fs/netfs/write_issue.c
> +++ b/fs/netfs/write_issue.c
> @@ -597,10 +597,11 @@ int netfs_writepages(struct address_space *mapping,
>  
>  couldnt_start:
>  	if (error == -ENOMEM) {
> -		folio_redirty_for_writepage(wbc, folio);
> -		folio_unlock(folio);
> -		folio = writeback_iter(mapping, wbc, folio, &error);
> -		WARN_ON_ONCE(folio != NULL);
> +		/* Redirty all dirty folios and let writeback retry later. */
> +		do {
> +			folio_redirty_for_writepage(wbc, folio);
> +			folio_unlock(folio);
> +		} while ((folio = writeback_iter(mapping, wbc, folio, &error)));
>  	} else {
>  		netfs_kill_dirty_pages(mapping, wbc, folio);
>  	}

This seems like the wrong thing to do - or, at least, a bug in the
writeback_iter() API.  Getting something like ENOMEM would seem to indicate
that all subsequent writeback_iter() calls in this loop are pointless as it
looks like the sequence will just go { lock, undirty, dirty, unlock } for each
folio.

David