Re: [PATCH 1/3] xfs: make xfs_rtalloc_query_range input parameters const

[Chandan Babu R <chandanrlinux@gmail.com>]

On 11 Aug 2021 at 17:58, "Darrick J. Wong" <djwong@kernel.org> wrote:
> From: Darrick J. Wong <djwong@kernel.org>
>
> In commit 8ad560d2565e, we changed xfs_rtalloc_query_range to constrain
> the range of bits in the realtime bitmap file that would actually be
> searched.  In commit a3a374bf1889, we changed the range again
> (incorrectly), leading to the fix in commit d88850bd5516, which finally
> corrected the range check code.  Unfortunately, the author never noticed
> that the function modifies its input parameters, which is a totaly no-no
> since none of the other range query functions change their input
> parameters.
>
> So, fix this function yet again to stash the upper end of the query
> range (i.e. the high key) in a local variable and hope this is the last
> time I have to fix my own function.  While we're at it, mark the key
> inputs const so nobody makes this mistake again. :(
>

Looks good.

Reviewed-by: Chandan Babu R <chandanrlinux@gmail.com>

> Fixes: 8ad560d2565e ("xfs: strengthen rtalloc query range checks")
> Not-fixed-by: a3a374bf1889 ("xfs: fix off-by-one error in xfs_rtalloc_query_range")
> Not-fixed-by: d88850bd5516 ("xfs: fix high key handling in the rt allocator's query_range function")
> Signed-off-by: Darrick J. Wong <djwong@kernel.org>
> ---
>  fs/xfs/libxfs/xfs_rtbitmap.c |   14 +++++++-------
>  fs/xfs/xfs_rtalloc.h         |    7 +++----
>  2 files changed, 10 insertions(+), 11 deletions(-)
>
>
> diff --git a/fs/xfs/libxfs/xfs_rtbitmap.c b/fs/xfs/libxfs/xfs_rtbitmap.c
> index 483375c6a735..5740ba664867 100644
> --- a/fs/xfs/libxfs/xfs_rtbitmap.c
> +++ b/fs/xfs/libxfs/xfs_rtbitmap.c
> @@ -1009,8 +1009,8 @@ xfs_rtfree_extent(
>  int
>  xfs_rtalloc_query_range(
>  	struct xfs_trans		*tp,
> -	struct xfs_rtalloc_rec		*low_rec,
> -	struct xfs_rtalloc_rec		*high_rec,
> +	const struct xfs_rtalloc_rec	*low_rec,
> +	const struct xfs_rtalloc_rec	*high_rec,
>  	xfs_rtalloc_query_range_fn	fn,
>  	void				*priv)
>  {
> @@ -1018,6 +1018,7 @@ xfs_rtalloc_query_range(
>  	struct xfs_mount		*mp = tp->t_mountp;
>  	xfs_rtblock_t			rtstart;
>  	xfs_rtblock_t			rtend;
> +	xfs_rtblock_t			high_key;
>  	int				is_free;
>  	int				error = 0;
>  
> @@ -1026,12 +1027,12 @@ xfs_rtalloc_query_range(
>  	if (low_rec->ar_startext >= mp->m_sb.sb_rextents ||
>  	    low_rec->ar_startext == high_rec->ar_startext)
>  		return 0;
> -	high_rec->ar_startext = min(high_rec->ar_startext,
> -			mp->m_sb.sb_rextents - 1);
> +
> +	high_key = min(high_rec->ar_startext, mp->m_sb.sb_rextents - 1);
>  
>  	/* Iterate the bitmap, looking for discrepancies. */
>  	rtstart = low_rec->ar_startext;
> -	while (rtstart <= high_rec->ar_startext) {
> +	while (rtstart <= high_key) {
>  		/* Is the first block free? */
>  		error = xfs_rtcheck_range(mp, tp, rtstart, 1, 1, &rtend,
>  				&is_free);
> @@ -1039,8 +1040,7 @@ xfs_rtalloc_query_range(
>  			break;
>  
>  		/* How long does the extent go for? */
> -		error = xfs_rtfind_forw(mp, tp, rtstart,
> -				high_rec->ar_startext, &rtend);
> +		error = xfs_rtfind_forw(mp, tp, rtstart, high_key, &rtend);
>  		if (error)
>  			break;
>  
> diff --git a/fs/xfs/xfs_rtalloc.h b/fs/xfs/xfs_rtalloc.h
> index ed885620589c..51097cb24311 100644
> --- a/fs/xfs/xfs_rtalloc.h
> +++ b/fs/xfs/xfs_rtalloc.h
> @@ -124,10 +124,9 @@ int xfs_rtfree_range(struct xfs_mount *mp, struct xfs_trans *tp,
>  		     xfs_rtblock_t start, xfs_extlen_t len,
>  		     struct xfs_buf **rbpp, xfs_fsblock_t *rsb);
>  int xfs_rtalloc_query_range(struct xfs_trans *tp,
> -			    struct xfs_rtalloc_rec *low_rec,
> -			    struct xfs_rtalloc_rec *high_rec,
> -			    xfs_rtalloc_query_range_fn fn,
> -			    void *priv);
> +		const struct xfs_rtalloc_rec *low_rec,
> +		const struct xfs_rtalloc_rec *high_rec,
> +		xfs_rtalloc_query_range_fn fn, void *priv);
>  int xfs_rtalloc_query_all(struct xfs_trans *tp,
>  			  xfs_rtalloc_query_range_fn fn,
>  			  void *priv);


-- 
chandan