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

["Darrick J. Wong" <darrick.wong@oracle.com>]

On Tue, Aug 27, 2019 at 06:15:28PM +1000, Dave Chinner wrote:
> On Mon, Aug 26, 2019 at 02:20:45PM -0700, Darrick J. Wong wrote:
> > From: Darrick J. Wong <darrick.wong@oracle.com>
> > 
> > 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@oracle.com>
> > ---
> >  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"

Ok.  prefix conversion done.

> > @@ -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...

Done.

--D

> Cheers,
> 
> Dave.
> 
> -- 
> Dave Chinner
> david@fromorbit.com