Re: [PATCH] xfs: add per-mount read/write I/O completion counters

[Eric Sandeen]

On 8/27/26 10:34 PM, Eric Peterson wrote:
> From: Eric Peterson <eric.peterson@xxxxxxx>
> 
> Add two per-mount statistics counters, xs_read_completions and
> xs_write_completions, to complement the existing xs_read_calls and
> xs_write_calls counters. The existing counters count I/O submissions
> (entries); the new counters count I/O completions. The pair (calls,
> completions) lets a consumer compute outstanding I/O as a queue depth
> (calls - completions) and, via Little's law, derive an approximate
> response time in userspace without any hot-path timestamping.

After reading through this thread, it occurs to me that it might
be worth asking my favorite question: what problem are you trying
to solve?

Please don't answer with LLM output.

Thanks,
-Eric

> The counters are plain monotonic increments (no clock reads), so they
> add negligible cost to the read/write path. Per-op timestamping was
> deliberately not used: a clock read on the hot path costs ~20-30 ns on
> TSC but hundreds of ns to ~1 us on HPET, which would be a regression for
> general users. Queue depth from completion counters is an approximation
> (instantaneous depth); this is a deliberate design choice, not
> a placeholder.
> 
> Completions are accounted at exactly the same sites where XFS already
> accounts the xs_*_bytes counters, so their semantics match the existing
> byte counters per path:
> 
>   - Reads are counted at the frame in xfs_file_read_iter and
>     xfs_file_splice_read.
>   - Buffered writes are counted at the frame, i.e. when data reaches
>     the page cache, mirroring how xs_write_bytes is accounted for
>     buffered writes -- not at physical writeback.
>   - DAX writes are counted at the frame after the synchronous
>     dax_iomap_rw copy returns, mirroring xs_write_bytes for DAX.
>   - Direct I/O writes are counted at true completion in
>     xfs_dio_write_end_io, which is async-safe and fires for both sync
>     and async DIO, mirroring xs_write_bytes for DIO.
> 
> Caveat: async O_DIRECT reads are counted at submission, not completion,
> because XFS has no read end_io today (iomap_dio_rw is called with NULL
> ops for reads). This matches the existing read-byte semantics.
> 
> The counters are uint32_t and wrap like the existing xs_*_calls
> counters; userspace diffs handle wrap.
> 
> The per-mount stats file gains a new appended "rwcmpl" line printing
> write and read completions. The existing "rw" line is unchanged, so
> positional parsers of "rw" are unaffected:
> 
>   rw     <write_calls>       <read_calls>
>   rwcmpl <write_completions> <read_completions>
> 
> Signed-off-by: Eric Peterson <eric.peterson@xxxxxxx>
> ---
> 
> Notes for reviewers (not part of the commit log):
> 
> * Placement: the new "rwcmpl" group is inserted between "rw" and
>   "attr" in the xstats[] table. The "rw" line itself is unchanged,
>   and "rwcmpl" is appended after it, but lines below "rw" in
>   /proc/fs/xfs/stat shift by one for strictly positional parsers. I
>   can instead append the group at the END of the table if preferred.
> 
> * checkpatch --strict reports two CHECKs preferring u32 over uint32_t
>   for the new fields. They are kept as uint32_t to match struct
>   __xfsstats, whose every field is uint32_t; changing only these two
>   would break local consistency.
> 
> * Testing: fstests -g auto on v6.12.74 shows baseline and patched
>   fail the identical 6/1277 tests -- zero regressions. The rwcmpl
>   interface was verified on hardware (rw >= rwcmpl, counters
>   advance under load). This for-next port applies cleanly with no
>   drift and compiles clean; a runtime -g quick smoke on for-next was
>   omitted as the logic is identical to the tested v6.12.74 patch.
>  fs/xfs/xfs_file.c  | 11 +++++++++--
>  fs/xfs/xfs_stats.c |  3 ++-
>  fs/xfs/xfs_stats.h |  2 ++
>  3 files changed, 13 insertions(+), 3 deletions(-)
> 
> diff --git a/fs/xfs/xfs_file.c b/fs/xfs/xfs_file.c
> index 426a67b813..3ecd4ed534 100644
> --- a/fs/xfs/xfs_file.c
> +++ b/fs/xfs/xfs_file.c
> @@ -347,8 +347,10 @@ xfs_file_read_iter(
>  	else
>  		ret = xfs_file_buffered_read(iocb, to);
>  
> -	if (ret > 0)
> +	if (ret > 0) {
>  		XFS_STATS_ADD(mp, xs_read_bytes, ret);
> +		XFS_STATS_INC(mp, xs_read_completions);
> +	}
>  	return ret;
>  }
>  
> @@ -375,8 +377,10 @@ xfs_file_splice_read(
>  	xfs_ilock(ip, XFS_IOLOCK_SHARED);
>  	ret = filemap_splice_read(in, ppos, pipe, len, flags);
>  	xfs_iunlock(ip, XFS_IOLOCK_SHARED);
> -	if (ret > 0)
> +	if (ret > 0) {
>  		XFS_STATS_ADD(mp, xs_read_bytes, ret);
> +		XFS_STATS_INC(mp, xs_read_completions);
> +	}
>  	return ret;
>  }
>  
> @@ -663,6 +667,7 @@ xfs_dio_write_end_io(
>  	 * for it on submission.
>  	 */
>  	XFS_STATS_ADD(ip->i_mount, xs_write_bytes, size);
> +	XFS_STATS_INC(ip->i_mount, xs_write_completions);
>  
>  	/*
>  	 * We can allocate memory here while doing writeback on behalf of
> @@ -1032,6 +1037,7 @@ xfs_file_dax_write(
>  
>  	if (ret > 0) {
>  		XFS_STATS_ADD(ip->i_mount, xs_write_bytes, ret);
> +		XFS_STATS_INC(ip->i_mount, xs_write_completions);
>  
>  		/* Handle various SYNC-type writes */
>  		ret = generic_write_sync(iocb, ret);
> @@ -1098,6 +1104,7 @@ xfs_file_buffered_write(
>  
>  	if (ret > 0) {
>  		XFS_STATS_ADD(ip->i_mount, xs_write_bytes, ret);
> +		XFS_STATS_INC(ip->i_mount, xs_write_completions);
>  		/* Handle various SYNC-type writes */
>  		ret = generic_write_sync(iocb, ret);
>  	}
> diff --git a/fs/xfs/xfs_stats.c b/fs/xfs/xfs_stats.c
> index c13d600732..5b276666b6 100644
> --- a/fs/xfs/xfs_stats.c
> +++ b/fs/xfs/xfs_stats.c
> @@ -40,7 +40,8 @@ int xfs_stats_format(struct xfsstats __percpu *stats, char *buf)
>  		{ "log",		xfsstats_offset(xs_try_logspace)},
>  		{ "push_ail",		xfsstats_offset(xs_xstrat_quick)},
>  		{ "xstrat",		xfsstats_offset(xs_write_calls)	},
> -		{ "rw",			xfsstats_offset(xs_attr_get)	},
> +		{ "rw",			xfsstats_offset(xs_write_completions)	},
> +		{ "rwcmpl",		xfsstats_offset(xs_attr_get)	},
>  		{ "attr",		xfsstats_offset(xs_iflush_count)},
>  		{ "icluster",		xfsstats_offset(xs_inodes_active) },
>  		{ "vnodes",		xfsstats_offset(xb_get)		},
> diff --git a/fs/xfs/xfs_stats.h b/fs/xfs/xfs_stats.h
> index 57c32b86c3..608d12d0c6 100644
> --- a/fs/xfs/xfs_stats.h
> +++ b/fs/xfs/xfs_stats.h
> @@ -93,6 +93,8 @@ struct __xfsstats {
>  	uint32_t		xs_xstrat_split;
>  	uint32_t		xs_write_calls;
>  	uint32_t		xs_read_calls;
> +	uint32_t		xs_write_completions;
> +	uint32_t		xs_read_completions;
>  	uint32_t		xs_attr_get;
>  	uint32_t		xs_attr_set;
>  	uint32_t		xs_attr_remove;