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