Re: [PATCH v17 09/17] netfs: Change netfs_writeback_single() to not look inside the iterator

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential
issue to consider:
- [Medium] netfs: netfs_write_collection() wakes waiter before
  netfs_wb_end()
--

commit 6578826ffb6d0041f876eafa90bcd2bd1b096881
Author: David Howells <dhowells@redhat.com>

netfs: Change netfs_writeback_single() to not look inside the iterator

This commit changes netfs_writeback_single() to pass the caller's iterator
directly to the write streams as a single monolithic write and wait
synchronously for completion instead of iterating over the folios
individually.

> diff --git a/fs/netfs/write_issue.c b/fs/netfs/write_issue.c
> index e041023811ed0..f084a6796667f 100644
> --- a/fs/netfs/write_issue.c
> +++ b/fs/netfs/write_issue.c
[ ... ]
> @@ -787,36 +665,42 @@ int netfs_writeback_single(struct address_space *mapping,
>  		goto couldnt_start;
>  	}
>  
> +	wreq->buffer.iter = *iter;
> +	wreq->len = len;
> +	wreq->submitted = wreq->len;
> +
>  	__set_bit(NETFS_RREQ_OFFLOAD_COLLECTION, &wreq->flags);
>  	trace_netfs_write(wreq, netfs_write_trace_writeback_single);
>  	netfs_stat(&netfs_n_wh_writepages);
>  
> -	if (__test_and_set_bit(NETFS_RREQ_UPLOAD_TO_SERVER, &wreq->flags))
> -		wreq->netfs_ops->begin_writeback(wreq);
> +	for (int s = 0; s < NR_IO_STREAMS; s++) {
> +		struct netfs_io_subrequest *subreq;
> +		struct netfs_io_stream *stream = &wreq->io_streams[s];
> +
> +		if (!stream->avail)
> +			continue;
>  
> -	for (fq = (struct folio_queue *)iter->folioq; fq; fq = fq->next) {
> -		for (int slot = 0; slot < folioq_count(fq); slot++) {
> -			struct folio *folio = folioq_folio(fq, slot);
> -			size_t part = umin(folioq_folio_size(fq, slot), size);
> +		netfs_prepare_write(wreq, stream, 0);
> +		/* Allocation not allowed to fail in writeback. */
>  
> -			_debug("wbiter %lx %llx", folio->index, atomic64_read(&wreq->issued_to));
> +		subreq = stream->construct;
> +		subreq->len = wreq->len;
> +		stream->submit_len = subreq->len;
> +		stream->submit_extendable_to = iov_iter_count(iter) - len;
>  
> -			ret = netfs_write_folio_single(wreq, folio);
> -			if (ret < 0)
> -				goto stop;
> -			size -= part;
> -			if (size <= 0)
> -				goto stop;
> -		}
> +		netfs_issue_write(wreq, stream);
>  	}
>  
> -stop:
> -	for (int s = 0; s < NR_IO_STREAMS; s++)
> -		netfs_issue_write(wreq, &wreq->io_streams[s]);
>  	netfs_all_subreqs_queued(wreq);
> -
>  	netfs_wake_collector(wreq);
>  
> +	/* TODO: Might want to be async here if WB_SYNC_NONE, but then need to
> +	 * wait before modifying.
> +	 */
> +	ret = netfs_wait_for_write(wreq);
> +	if (ret > 0)
> +		ret = 0;

[Severity: Medium]
Can netfs_writeback_single() return while NETFS_ICTX_WB_LOCK is still held
by the collector worker?

Because NETFS_RREQ_OFFLOAD_COLLECTION is set, netfs_wait_for_write() sleeps
on wreq->waitq until NETFS_RREQ_IN_PROGRESS is cleared by the collector
workqueue, which happens before netfs_wb_end() releases NETFS_ICTX_WB_LOCK:

fs/netfs/write_collect.c:netfs_write_collection() {
	...
	netfs_wake_rreq_flag(wreq, NETFS_RREQ_IN_PROGRESS, netfs_rreq_trace_wake_ip);
	/* As we cleared NETFS_RREQ_IN_PROGRESS, we acquired its ref. */

	switch (wreq->origin) {
	case NETFS_WRITEBACK:
	case NETFS_WRITEBACK_SINGLE:
		netfs_wb_end(ictx);
		break;
	...
}

CPU0                                  kworker
netfs_writeback_single()
  netfs_wb_begin(ictx, ...)
  ...
  netfs_wake_collector(wreq)
  netfs_wait_for_write(wreq)
                                      netfs_write_collection()
                                        netfs_wake_rreq_flag(wreq,
                                          NETFS_RREQ_IN_PROGRESS, ...)
  return 0;
netfs_writeback_single()
  netfs_wb_begin(ictx, true)  <-- sees NETFS_ICTX_WB_LOCK still set
                                        netfs_wb_end(ictx)

An immediate subsequent WB_SYNC_NONE writeback on the same inode will see
NETFS_ICTX_WB_LOCK still held, re-dirty the inode via
netfs_single_mark_inode_dirty(), and abort the writeback pass.

Should netfs_write_collection() call netfs_wb_end() before waking
NETFS_RREQ_IN_PROGRESS, or should netfs_writeback_single() omit
NETFS_RREQ_OFFLOAD_COLLECTION while it waits synchronously?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261111203034.2126314-1-dhowells@redhat.com?part=9