From: Dave Chinner <david@fromorbit.com>
To: "Darrick J. Wong" <darrick.wong@oracle.com>
Cc: sandeen@sandeen.net, linux-xfs@vger.kernel.org
Subject: Re: [PATCH 4/4] xfs_spaceman: convert open-coded unit conversions to helpers
Date: Tue, 27 Aug 2019 18:15:28 +1000 [thread overview]
Message-ID: <20190827081528.GH1119@dread.disaster.area> (raw)
In-Reply-To: <156685444520.2839773.6764652190281485485.stgit@magnolia>
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.
--
Dave Chinner
david@fromorbit.com
next prev parent reply other threads:[~2019-08-27 8:15 UTC|newest]
Thread overview: 14+ messages / expand[flat|nested] mbox.gz Atom feed top
2019-08-26 21:20 [PATCH 0/4] xfs_spaceman: use runtime support library Darrick J. Wong
2019-08-26 21:20 ` [PATCH 1/4] xfs_spaceman: remove typedef usage Darrick J. Wong
2019-08-27 5:01 ` Dave Chinner
2019-08-26 21:20 ` [PATCH 2/4] xfs_spaceman: remove unnecessary test in openfile() Darrick J. Wong
2019-08-27 5:02 ` Dave Chinner
2019-08-26 21:20 ` [PATCH 3/4] xfs_spaceman: embed struct xfs_fd in struct fileio Darrick J. Wong
2019-08-27 5:06 ` Dave Chinner
2019-08-27 5:12 ` Darrick J. Wong
2019-08-27 7:51 ` Dave Chinner
2019-08-26 21:20 ` [PATCH 4/4] xfs_spaceman: convert open-coded unit conversions to helpers Darrick J. Wong
2019-08-27 8:15 ` Dave Chinner [this message]
2019-08-29 3:48 ` Darrick J. Wong
2019-09-04 4:38 [PATCH v2 0/4] xfs_spaceman: use runtime support library Darrick J. Wong
2019-09-04 4:38 ` [PATCH 4/4] xfs_spaceman: convert open-coded unit conversions to helpers Darrick J. Wong
2019-09-05 0:01 ` Dave Chinner
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20190827081528.GH1119@dread.disaster.area \
--to=david@fromorbit.com \
--cc=darrick.wong@oracle.com \
--cc=linux-xfs@vger.kernel.org \
--cc=sandeen@sandeen.net \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).