Re: [PATCH v16 08/15] netfs: Change netfs_writeback_single() to not look inside the iterator

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential
issues to consider:
- [Medium] netfs: netfs_write_collection() wakes waiter before
  netfs_wb_end()
- [Medium] netfs: set wreq->i_size to len in netfs_writeback_single()
--

commit 4b137787e59ae6eefb0ddaed87ce3b7cc4ab57b8
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 data iterator
directly to the cache as a single monolithic write and wait synchronously
for the write to complete.

> 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;

[Severity: Medium]
Should netfs_writeback_single() also set wreq->i_size = len here?

netfs_create_write_req() initializes wreq->i_size via a separate
i_size_read(inode) call in netfs_alloc_request().  In afs_dir_writepages(),
the caller only holds down_read(&dvnode->validate_lock) when reading
i_size to pass as len, while concurrent RPC completions in
afs_apply_status() can update inode->i_size under cb_lock without holding
validate_lock.

If wreq->i_size does not match len and len is not a multiple of
cache->bsize, cachefiles_issue_write() skips tail padding because
start + len == wreq->i_size is false:

fs/cachefiles/io.c:cachefiles_issue_write() {
	...
	/* We also need to end on the cache granularity boundary */
	if (start + len == wreq->i_size) {
		size_t part = len & (cache->bsize - 1);
		size_t need = cache->bsize - part;

		if (part && stream->submit_extendable_to >= need) {
			len += need;
			subreq->len += need;
			subreq->io_iter.count += need;
		}
	}
	...
}

cachefiles_issue_write() then strips the unaligned tail, causing
cachefiles_collect_write() to hit WARN_ON(len & (cache->bsize - 1)) and
invalidate the cache object.

>  	__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 to its caller before the collector
worker releases NETFS_ICTX_WB_LOCK?

Because NETFS_RREQ_OFFLOAD_COLLECTION is set, netfs_wait_for_write() sleeps
until the collector worker clears NETFS_RREQ_IN_PROGRESS, which happens in
netfs_write_collection() before netfs_wb_end() is called:

CPU0                              kworker
netfs_writeback_single()
  netfs_wb_begin(ictx, ...)
  ...
  netfs_wait_for_write(wreq)
                                  netfs_write_collection()
                                    netfs_wake_rreq_flag(...,
                                      NETFS_RREQ_IN_PROGRESS, ...)
  /* returns to caller */
  /* next writeback on inode */
  netfs_wb_begin(ictx, true)
    /* sees NETFS_ICTX_WB_LOCK */ <-- false lock conflict
                                    netfs_wb_end(ictx)

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

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