| Message ID | 156685444520.2839773.6764652190281485485.stgit@magnolia (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | xfs_spaceman: use runtime support library | expand |
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" > @@ -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.
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
diff --git a/include/xfrog.h b/include/xfrog.h index 6b605dcf..5748e967 100644 --- a/include/xfrog.h +++ b/include/xfrog.h @@ -45,6 +45,9 @@ struct xfs_fd { /* bits for agino in inum */ unsigned int aginolog; + + /* log2 of sb_blocksize / sb_sectsize */ + unsigned int blkbb_log; }; /* Static initializers */ @@ -100,6 +103,69 @@ xfrog_b_to_fsbt( return bytes >> xfd->blocklog; } +/* Convert sector number to bytes. */ +static inline uint64_t +xfrog_bbtob( + uint64_t daddr) +{ + return daddr << BBSHIFT; +} + +/* Convert bytes to sector number, rounding down. */ +static inline uint64_t +xfrog_btobbt( + uint64_t bytes) +{ + return bytes >> BBSHIFT; +} + +/* 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; +} + +/* Convert AG number and AG block to fs block number */ +static inline uint64_t +xfrog_agb_to_daddr( + struct xfs_fd *xfd, + uint32_t agno, + uint32_t agbno) +{ + return xfrog_fsb_to_bb(xfd, + (uint64_t)agno * xfd->fsgeom.agblocks + agbno); +} + +/* Convert sector number to AG number. */ +static inline uint32_t +xfrog_daddr_to_agno( + struct xfs_fd *xfd, + uint64_t daddr) +{ + return xfrog_bb_to_fsbt(xfd, daddr) / xfd->fsgeom.agblocks; +} + +/* Convert sector number to AG block number. */ +static inline uint32_t +xfrog_daddr_to_agbno( + struct xfs_fd *xfd, + uint64_t daddr) +{ + return xfrog_bb_to_fsbt(xfd, daddr) % xfd->fsgeom.agblocks; +} + /* Bulkstat wrappers */ struct xfs_bstat; int xfrog_bulkstat_single(struct xfs_fd *xfd, uint64_t ino, diff --git a/libfrog/fsgeom.c b/libfrog/fsgeom.c index 159738c5..17479e4a 100644 --- a/libfrog/fsgeom.c +++ b/libfrog/fsgeom.c @@ -114,6 +114,7 @@ xfrog_prepare_geometry( xfd->inodelog = highbit32(xfd->fsgeom.inodesize); xfd->inopblog = xfd->blocklog - xfd->inodelog; xfd->aginolog = xfd->agblklog + xfd->inopblog; + xfd->blkbb_log = xfd->blocklog - BBSHIFT; return 0; } diff --git a/spaceman/freesp.c b/spaceman/freesp.c index 83cbecbd..9e9f32a2 100644 --- a/spaceman/freesp.c +++ b/spaceman/freesp.c @@ -150,9 +150,7 @@ scan_ag( struct fsmap *extent; struct fsmap *l, *h; struct fsmap *p; - struct xfs_fsop_geom *fsgeom = &file->xfd.fsgeom; - off64_t blocksize = fsgeom->blocksize; - off64_t bperag; + struct xfs_fd *xfd = &file->xfd; off64_t aglen; xfs_agblock_t agbno; unsigned long long freeblks = 0; @@ -160,8 +158,6 @@ scan_ag( int ret; int i; - bperag = (off64_t)fsgeom->agblocks * blocksize; - fsmap = malloc(fsmap_sizeof(NR_EXTENTS)); if (!fsmap) { fprintf(stderr, _("%s: fsmap malloc failed.\n"), progname); @@ -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; @@ -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)); + 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); } else { offset = 0; - length = fsgeom->datablocks * fsgeom->blocksize; + length = xfrog_fsb_to_b(xfd, fsgeom->datablocks); } trim.start = offset;