Re: [PATCH 4/4] xfs_spaceman: convert open-coded unit conversions to helpers

[Dave Chinner <david@xxxxxxxxxxxxx>]

On Mon, Aug 26, 2019 at 02:20:45PM -0700, Darrick J. Wong wrote:
> From: Darrick J. Wong <darrick.wong@xxxxxxxxxx>
> 
> Create xfrog analogues of the libxfs byte/sector/block conversion
> functions and convert spaceman to use them instead of open-coded
> arithmatic we do now.
> 
> Signed-off-by: Darrick J. Wong <darrick.wong@xxxxxxxxxx>
> ---
>  include/xfrog.h   |   66 +++++++++++++++++++++++++++++++++++++++++++++++++++++
>  libfrog/fsgeom.c  |    1 +
>  spaceman/freesp.c |   18 ++++++--------
>  spaceman/trim.c   |    9 ++++---
>  4 files changed, 80 insertions(+), 14 deletions(-)

....
> +/* Convert fs block number to sector number. */
> +static inline uint64_t
> +xfrog_fsb_to_bb(
> +	struct xfs_fd		*xfd,
> +	uint64_t		fsbno)
> +{
> +	return fsbno << xfd->blkbb_log;
> +}
> +
> +/* Convert sector number to fs block number, rounded down. */
> +static inline uint64_t
> +xfrog_bb_to_fsbt(
> +	struct xfs_fd		*xfd,
> +	uint64_t		daddr)
> +{
> +	return daddr >> xfd->blkbb_log;
> +}

Same comment as previous ones about off_fsb_to_<foo> and vice versa.

And the more I see it, the less "xfrog" really means in these unit
conversion functions. How about we prefix them "cvt_"?

Then the name of the function actually does exactly what is says.
i.e. "convert basic blocks to offset in filesystem blocks"

> @@ -174,8 +170,10 @@ scan_ag(
>  	l = fsmap->fmh_keys;
>  	h = fsmap->fmh_keys + 1;
>  	if (agno != NULLAGNUMBER) {
> -		l->fmr_physical = agno * bperag;
> -		h->fmr_physical = ((agno + 1) * bperag) - 1;
> +		l->fmr_physical = xfrog_bbtob(
> +				xfrog_agb_to_daddr(xfd, agno, 0));
> +		h->fmr_physical = xfrog_bbtob(
> +				xfrog_agb_to_daddr(xfd, agno + 1, 0));
>  		l->fmr_device = h->fmr_device = file->fs_path.fs_datadev;
>  	} else {
>  		l->fmr_physical = 0;

This is why - that's quite hard to read. A simple wrapper might be
better:

static inline uint64_t
cvt_agbno_to_off_b(
	struct xfs_fd		*xfd,
	xfs_agnumber_t		agno,
	xfs_agblock_t		agbno)
{
	return cvt_bbtob(cvt_agbno_to_daddr(xfd, agno, agbno));
}

And then we have:

		l->fmr_physical = cvt_agbno_to_off_b(xfd, agno, 0);
		h->fmr_physical = cvt_agbno_to_off_b(xfd, agno + 1, 0);


> @@ -206,9 +204,9 @@ scan_ag(
>  			if (!(extent->fmr_flags & FMR_OF_SPECIAL_OWNER) ||
>  			    extent->fmr_owner != XFS_FMR_OWN_FREE)
>  				continue;
> -			agbno = (extent->fmr_physical - (bperag * agno)) /
> -								blocksize;
> -			aglen = extent->fmr_length / blocksize;
> +			agbno = xfrog_daddr_to_agbno(xfd,
> +					xfrog_btobbt(extent->fmr_physical));

That's the reverse - cvt_off_b_to_agbno().

> +			aglen = xfrog_b_to_fsbt(xfd, extent->fmr_length);
>  			freeblks += aglen;
>  			freeexts++;
>  
> diff --git a/spaceman/trim.c b/spaceman/trim.c
> index ea1308f7..8741bab2 100644
> --- a/spaceman/trim.c
> +++ b/spaceman/trim.c
> @@ -23,7 +23,8 @@ trim_f(
>  	char			**argv)
>  {
>  	struct fstrim_range	trim = {0};
> -	struct xfs_fsop_geom	*fsgeom = &file->xfd.fsgeom;
> +	struct xfs_fd		*xfd = &file->xfd;
> +	struct xfs_fsop_geom	*fsgeom = &xfd->fsgeom;
>  	xfs_agnumber_t		agno = 0;
>  	off64_t			offset = 0;
>  	ssize_t			length = 0;
> @@ -66,11 +67,11 @@ trim_f(
>  		length = cvtnum(fsgeom->blocksize, fsgeom->sectsize,
>  				argv[optind + 1]);
>  	} else if (agno) {
> -		offset = (off64_t)agno * fsgeom->agblocks * fsgeom->blocksize;
> -		length = fsgeom->agblocks * fsgeom->blocksize;
> +		offset = xfrog_bbtob(xfrog_agb_to_daddr(xfd, agno, 0));
> +		length = xfrog_fsb_to_b(xfd, fsgeom->agblocks);

cvt_agbno_to_off_b() again...

Cheers,

Dave.

-- 
Dave Chinner
david@xxxxxxxxxxxxx