Re: [PATCH v8 7/7] NFSD: drop a shared boundary page only once both WRITEs have written it

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] nfsd: nfsd_write_dio_seg_hold() uses PAGE_SIZE instead of min folio size
--

commit 49f26acfa03e4ed2bb366e82b2cb9296ee952e69
Author: Mike Snitzer <snitzer@kernel.org>

NFSD: drop a shared boundary page only once both WRITEs have written it

This commit issues buffered DONTCACHE segments of direct-mode WRITEs with
IOCB_DONTCACHE_DEFER so that NFSD can defer marking shared boundary pages
drop-behind until the entire WRITE and its fsync complete. Before writing a
buffered segment, NFSD checks filemap_dontcache_edge_completes() for partly
covered boundary pages and calls filemap_dontcache_edge_complete() once the
WRITE finishes.

> diff --git a/fs/nfsd/vfs.c b/fs/nfsd/vfs.c
> index ade00dabef187..f151289345c92 100644
> --- a/fs/nfsd/vfs.c
> +++ b/fs/nfsd/vfs.c
[ ... ]
> @@ -1406,6 +1407,65 @@ nfsd_write_dio_seg_init(struct nfsd_write_dio_seg *segment,
[ ... ]
> +static void
> +nfsd_write_dio_seg_hold(struct file *file, const struct nfsd_write_dio_seg *seg,
> +			loff_t pos, size_t len, struct nfsd_write_dio_hold *hold)
> +{
> +	struct address_space *mapping = file->f_mapping;
> +
> +	hold->first = pos;
> +	hold->last = pos + len - 1;
> +	hold->complete_first = false;
> +	hold->complete_last = false;
> +	hold->written = 0;
> +	if (seg->claim && len) {
> +		bool one_page = (hold->first >> PAGE_SHIFT) ==
> +				(hold->last >> PAGE_SHIFT);
> +		bool head = hold->first & ~PAGE_MASK;
> +		bool tail = (hold->last + 1) & ~PAGE_MASK;
> +
> +		/* Only a page the segment covers partly is shared. */
> +		if (head || (one_page && tail))
> +			hold->complete_first =
> +				filemap_dontcache_edge_completes(mapping, hold->first);
> +		if (!one_page && tail)
> +			hold->complete_last =
> +				filemap_dontcache_edge_completes(mapping, hold->last);
> +	}
> +}

[Severity: Medium]
Should nfsd_write_dio_seg_hold() compute one_page, head, and tail using the
mapping's minimum folio size rather than PAGE_SHIFT and PAGE_MASK?

In nfsd_write_dio_iters_init(), misaligned direct WRITEs are split at
mapping_min_folio_nrbytes(nf->nf_file->f_mapping) boundaries:

	cut = max3(offset_align, PAGE_SIZE,
		   mapping_min_folio_nrbytes(nf->nf_file->f_mapping));

And during vfs_iocb_iter_write(), iomap_get_folio() treats a folio as a
partly covered edge unit whenever the write does not cover a full
mapping_min_folio_nrbytes() unit, setting FGP_DONTCACHE_EDGE and
FGP_DONTCACHE_DEFER so the page cache leaves the folio unmarked for the
caller to complete:

	if (iter->flags & IOMAP_DONTCACHE) {
		size_t unit = mapping_min_folio_nrbytes(iter->inode->i_mapping);
		size_t off = pos & (unit - 1);
		...
		if (off || len < unit) {
			fgp |= FGP_DONTCACHE_EDGE;
			if (iter->flags & IOMAP_DONTCACHE_DEFER)
				fgp |= FGP_DONTCACHE_DEFER;
			len = min_t(size_t, len, unit - off);
		}
	}

Likewise, filemap_dontcache_edge_completes() and
filemap_dontcache_edge_complete() check filemap_dontcache_edge_unit(), which
tests folio_nr_pages(folio) == mapping_min_folio_nrpages(mapping).

On a large-block-size filesystem where mapping_min_folio_nrbytes(mapping) is
larger than PAGE_SIZE (for example, 16 KiB blocks with 4 KiB pages), if two
neighbouring WRITEs split a 16 KiB minimum-order folio on a 4 KiB PAGE_SIZE
boundary, iomap_get_folio() defers dropbehind marking on that folio via
FGP_DONTCACHE_DEFER, but head and tail in nfsd_write_dio_seg_hold() both
evaluate to false. Wouldn't that leave the shared folio unmarked in the page
cache until reclaim, or cause a partial segment spanning multiple base pages
within the same minimum-order folio to treat hold->first and hold->last as
two separate pages?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261023024548.71123-1-snitzer@kernel.org?part=7