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@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"
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@xxxxxxxxxxxxx
