Re: [PATCH v9 6/6] 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: cut misaligned segments at min folio size in nfsd_write_dio_iters_init()
--

commit 539f478b663513be1ed1736c065becca6e5a5016
Author: Mike Snitzer <snitzer@kernel.org>

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

This commit defers dropping shared boundary units during buffered
segments of direct-mode writes by passing IOCB_DONTCACHE_DEFER and
calling filemap_dontcache_edge_completes() and
filemap_dontcache_edge_complete() around the write and fsync.

> diff --git a/fs/nfsd/vfs.c b/fs/nfsd/vfs.c
> index 9219f8eb386d0..0b62c4659e547 100644
> --- a/fs/nfsd/vfs.c
> +++ b/fs/nfsd/vfs.c
[ ... ]
> @@ -1406,6 +1407,82 @@ nfsd_write_dio_seg_init(struct nfsd_write_dio_seg *segment,
>  		iov_iter_advance(&segment->iter, start);
>  	iov_iter_truncate(&segment->iter, len);
>  	segment->flags = iocb->ki_flags;
> +	segment->claim = false;
> +}
> +
> +/* The shared pages a segment completes, and how much of it was written. */
> +struct nfsd_write_dio_hold {
> +	loff_t	first, last;
> +	size_t	first_len;	/* the segment's bytes in its first unit */
> +	bool	complete_first, complete_last;
> +	size_t	written;
> +};
> +
> +/*
> + * A page that a buffered DONTCACHE segment covers only partly is shared with
> + * the neighbouring WRITE.  The page cache keeps it, unmarked, for the write
> + * that completes it; the segment is issued IOCB_DONTCACHE_DEFER so that this
> + * WRITE, not the page cache, marks it, once the whole WRITE, its fsync
> + * included, is complete.  Ask whether the neighbour has already written its
> + * part immediately before writing @seg: asking earlier gives the neighbour
> + * time to write its part and find this WRITE's part missing.
> + */
> +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;
> +	size_t unit, last_len;
> +	loff_t mask, last_unit;
> +	bool one_unit, head, tail;
> +
> +	hold->first = pos;
> +	hold->last = pos + len - 1;
> +	hold->complete_first = false;
> +	hold->complete_last = false;
> +	hold->written = 0;
> +	if (!seg->claim || !len)
> +		return;
> +
> +	/*
> +	 * The unit two writes share is the smallest folio the mapping allows:
> +	 * a page, or a block where the block is larger.  Only a unit the
> +	 * segment covers partly is shared.
> +	 */
> +	unit = mapping_min_folio_nrbytes(mapping);
> +	mask = ~(loff_t)(unit - 1);
> +	one_unit = (hold->first & mask) == (hold->last & mask);
> +	head = hold->first & (unit - 1);
> +	tail = (hold->last + 1) & (unit - 1);
> +	last_unit = hold->last & mask;
> +	last_len = (hold->last & (unit - 1)) + 1;
> +
> +	hold->first_len = min_t(size_t, len,
> +				unit - (hold->first & (unit - 1)));
> +	if (head || (one_unit && tail))
> +		hold->complete_first =
> +			filemap_dontcache_edge_completes(mapping, hold->first,
> +							 hold->first_len);
> +	if (!one_unit && tail)
> +		hold->complete_last =
> +			filemap_dontcache_edge_completes(mapping, last_unit,
> +							 last_len);
> +}

[Severity: Medium]
Here in nfsd_write_dio_seg_hold(), the shared page-cache unit size is
computed via mapping_min_folio_nrbytes(mapping) to account for large
block size filesystems where the block size exceeds the base page size.

However, in fs/nfsd/vfs.c:nfsd_write_dio_iters_init(), the boundary
between the buffered prefix or suffix and the direct middle segment is
still cut at max_t(u32, offset_align, PAGE_SIZE):

	cut = max_t(u32, offset_align, PAGE_SIZE);
	prefix_end = offset;
	if (!IS_ALIGNED(offset, offset_align))
		prefix_end += min_t(loff_t, cut - (offset & (cut - 1)), total);
	middle_end = orig_end;
	if (!IS_ALIGNED(orig_end, offset_align))
		middle_end = max(round_down(orig_end, cut), prefix_end);

On filesystems with a block size larger than the base page size,
nf->nf_dio_offset_align can still be the underlying block device logical
sector size (for example, 512 or 4096 bytes), such as for non-cow XFS
inodes in fs/xfs/xfs_iops.c:xfs_report_dioalign():

	stat->dio_read_offset_align = bdev_logical_block_size(bdev);
	if (xfs_is_cow_inode(ip))
		stat->dio_offset_align = xfs_inode_alloc_unitsize(ip);
	else
		stat->dio_offset_align = stat->dio_read_offset_align;

When offset_align is smaller than mapping_min_folio_nrbytes(mapping),
nfsd_write_dio_iters_init() cuts prefix_end and middle_end on a base page
boundary inside the minimum-size folio rather than on a unit boundary:

1. The direct middle segment shares the minimum folio with the buffered
   prefix or suffix, so kiocb_invalidate_pages() synchronously writes back
   and evicts the entire minimum folio just written by the prefix (or left
   cached by a neighbouring write for the suffix), and the buffered suffix
   starts at an interior page offset within the unit, forcing iomap to
   read back from disk the bytes just written by the middle segment.

2. If both the buffered prefix and suffix fall within the same minimum
   folio (for example, on a 64 KiB block size filesystem with 4 KiB pages)
   and an earlier write already wrote the leading bytes before the prefix,
   nfsd_write_dio_seg_hold() sets holds[0].complete_first = true for the
   prefix. After the middle segment evicts the folio and the suffix
   re-populates only the prefix-through-suffix range for the next
   neighbour, nfsd_write_dio_seg_release() for holds[0] calls
   filemap_dontcache_edge_complete() on that same unit and drops the
   incomplete folio before the following write arrives.

Should cut in nfsd_write_dio_iters_init() also take
mapping_min_folio_nrbytes(nf->nf_file->f_mapping) into account so that
buffered prefix and suffix segments are cut on unit boundaries?

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