Re: [PATCH 2/3] xfs: replace do_mod with native operations

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

On Thu, Jun 07, 2018 at 03:27:50PM +1000, Dave Chinner wrote:
> From: Dave Chinner <dchinner@redhat.com>
> 
> do_mod() is a hold-over from when we have different sizes for file
> offsets and and other internal values for 40 bit XFS filesystems.
> Hence depending on build flags variables passed to do_mod() could
> change size. We no longer support those small format filesystems and
> hence everything is of fixed size theses days, even on 32 bit
> platforms.
> 
> As such, we can convert all the do_mod() callers to platform
> optimised modulus operations as defined by linux/math64.h.
> Individual conversions depend on the types of variables being used.
> 
> Signed-Off-By: Dave Chinner <dchinner@redhat.com>
> ---
>  fs/xfs/libxfs/xfs_bmap.c | 37 +++++++++++++++++++++++--------------
>  fs/xfs/xfs_bmap_util.c   | 14 +++++++++-----
>  fs/xfs/xfs_inode.c       |  2 +-
>  fs/xfs/xfs_iomap.h       |  4 ++--
>  fs/xfs/xfs_linux.h       | 19 -------------------
>  fs/xfs/xfs_log_recover.c | 32 +++++++++++++++++++++++++-------
>  fs/xfs/xfs_rtalloc.c     | 15 +++++++++++----
>  7 files changed, 71 insertions(+), 52 deletions(-)
> 
> diff --git a/fs/xfs/libxfs/xfs_bmap.c b/fs/xfs/libxfs/xfs_bmap.c
> index 3de047eb8209..4d8fd37ec7ae 100644
> --- a/fs/xfs/libxfs/xfs_bmap.c
> +++ b/fs/xfs/libxfs/xfs_bmap.c
> @@ -2923,7 +2923,7 @@ xfs_bmap_extsize_align(
>  	 * perform this alignment, or if a truncate shot us in the
>  	 * foot.
>  	 */
> -	temp = do_mod(orig_off, extsz);
> +	div_u64_rem(orig_off, extsz, &temp);
>  	if (temp) {
>  		align_alen += temp;
>  		align_off -= temp;
> @@ -3497,15 +3497,17 @@ xfs_bmap_btalloc(
>  	/* apply extent size hints if obtained earlier */
>  	if (align) {
>  		args.prod = align;
> -		if ((args.mod = (xfs_extlen_t)do_mod(ap->offset, args.prod)))
> -			args.mod = (xfs_extlen_t)(args.prod - args.mod);
> +		div_u64_rem(ap->offset, args.prod, &args.mod);
> +		if (args.mod)
> +			args.mod = args.prod - args.mod;
>  	} else if (mp->m_sb.sb_blocksize >= PAGE_SIZE) {
>  		args.prod = 1;
>  		args.mod = 0;
>  	} else {
>  		args.prod = PAGE_SIZE >> mp->m_sb.sb_blocklog;
> -		if ((args.mod = (xfs_extlen_t)(do_mod(ap->offset, args.prod))))
> -			args.mod = (xfs_extlen_t)(args.prod - args.mod);
> +		div_u64_rem(ap->offset, args.prod, &args.mod);
> +		if (args.mod)
> +			args.mod = args.prod - args.mod;
>  	}
>  	/*
>  	 * If we are not low on available data blocks, and the
> @@ -4953,13 +4955,15 @@ xfs_bmap_del_extent_real(
>  	if (whichfork == XFS_DATA_FORK && XFS_IS_REALTIME_INODE(ip)) {
>  		xfs_fsblock_t	bno;
>  		xfs_filblks_t	len;
> +		xfs_extlen_t	mod;
> +
> +		bno = div_u64_rem(del->br_startblock, mp->m_sb.sb_rextsize,
> +				  &mod);
> +		ASSERT(mod == 0);
> +		len = div_u64_rem(del->br_blockcount, mp->m_sb.sb_rextsize,
> +				  &mod);
> +		ASSERT(mod == 0);
>  
> -		ASSERT(do_mod(del->br_blockcount, mp->m_sb.sb_rextsize) == 0);
> -		ASSERT(do_mod(del->br_startblock, mp->m_sb.sb_rextsize) == 0);
> -		bno = del->br_startblock;
> -		len = del->br_blockcount;
> -		do_div(bno, mp->m_sb.sb_rextsize);
> -		do_div(len, mp->m_sb.sb_rextsize);
>  		error = xfs_rtfree_extent(tp, bno, (xfs_extlen_t)len);
>  		if (error)
>  			goto done;
> @@ -5296,9 +5300,12 @@ __xfs_bunmapi(
>  			del.br_blockcount = max_len;
>  		}
>  
> +		if (!isrt)
> +			goto delete;
> +
>  		sum = del.br_startblock + del.br_blockcount;
> -		if (isrt &&
> -		    (mod = do_mod(sum, mp->m_sb.sb_rextsize))) {
> +		div_u64_rem(sum, mp->m_sb.sb_rextsize, &mod);
> +		if (mod) {
>  			/*
>  			 * Realtime extent not lined up at the end.
>  			 * The extent could have been split into written
> @@ -5345,7 +5352,8 @@ __xfs_bunmapi(
>  				goto error0;
>  			goto nodelete;
>  		}
> -		if (isrt && (mod = do_mod(del.br_startblock, mp->m_sb.sb_rextsize))) {
> +		div_u64_rem(del.br_startblock, mp->m_sb.sb_rextsize, &mod);
> +		if (mod) {
>  			/*
>  			 * Realtime extent is lined up at the end but not
>  			 * at the front.  We'll get rid of full extents if
> @@ -5414,6 +5422,7 @@ __xfs_bunmapi(
>  			}
>  		}
>  
> +delete:
>  		if (wasdel) {
>  			error = xfs_bmap_del_extent_delay(ip, whichfork, &icur,
>  					&got, &del);
> diff --git a/fs/xfs/xfs_bmap_util.c b/fs/xfs/xfs_bmap_util.c
> index 7d26933a542f..13e6caf8b801 100644
> --- a/fs/xfs/xfs_bmap_util.c
> +++ b/fs/xfs/xfs_bmap_util.c
> @@ -80,6 +80,7 @@ xfs_bmap_rtalloc(
>  	int		error;		/* error return value */
>  	xfs_mount_t	*mp;		/* mount point structure */
>  	xfs_extlen_t	prod = 0;	/* product factor for allocators */
> +	xfs_extlen_t	mod = 0;	/* product factor for allocators */

I don't think we need the initial value or the comment.

>  	xfs_extlen_t	ralen = 0;	/* realtime allocation length */
>  	xfs_extlen_t	align;		/* minimum allocation alignment */
>  	xfs_rtblock_t	rtb;
> @@ -99,7 +100,8 @@ xfs_bmap_rtalloc(
>  	 * If the offset & length are not perfectly aligned
>  	 * then kill prod, it will just get us in trouble.
>  	 */
> -	if (do_mod(ap->offset, align) || ap->length % align)
> +	div_u64_rem(ap->offset, align, &mod);
> +	if (mod || ap->length % align)
>  		prod = 1;
>  	/*
>  	 * Set ralen to be the actual requested length in rtextents.
> @@ -936,9 +938,11 @@ xfs_alloc_file_space(
>  			do_div(s, extsz);
>  			s *= extsz;
>  			e = startoffset_fsb + allocatesize_fsb;
> -			if ((temp = do_mod(startoffset_fsb, extsz)))
> +			div_u64_rem(startoffset_fsb, extsz, &temp);
> +			if (temp)
>  				e += temp;
> -			if ((temp = do_mod(e, extsz)))
> +			div_u64_rem(e, extsz, &temp);
> +			if (temp)
>  				e += extsz - temp;
>  		} else {
>  			s = 0;
> @@ -1090,7 +1094,7 @@ xfs_adjust_extent_unmap_boundaries(
>  	struct xfs_mount	*mp = ip->i_mount;
>  	struct xfs_bmbt_irec	imap;
>  	int			nimap, error;
> -	xfs_extlen_t		mod = 0;
> +	xfs_rtblock_t		mod = 0;
>  
>  	nimap = 1;
>  	error = xfs_bmapi_read(ip, *startoffset_fsb, 1, &imap, &nimap, 0);
> @@ -1099,7 +1103,7 @@ xfs_adjust_extent_unmap_boundaries(
>  
>  	if (nimap && imap.br_startblock != HOLESTARTBLOCK) {
>  		ASSERT(imap.br_startblock != DELAYSTARTBLOCK);
> -		mod = do_mod(imap.br_startblock, mp->m_sb.sb_rextsize);
> +		div64_u64_rem(imap.br_startblock, mp->m_sb.sb_rextsize, &mod);

Why does this need to be a div64_u64_rem?  sb_rextsize is 32-bit, so the
remainder won't exceed 2^32-1, right?

>  		if (mod)
>  			*startoffset_fsb += mp->m_sb.sb_rextsize - mod;

Indeed if it ever did that would screw up this logic, wouldn't it?

--D

>  	}
> diff --git a/fs/xfs/xfs_inode.c b/fs/xfs/xfs_inode.c
> index 6cda0f08b045..4a2e5e13c569 100644
> --- a/fs/xfs/xfs_inode.c
> +++ b/fs/xfs/xfs_inode.c
> @@ -2258,7 +2258,7 @@ xfs_ifree_cluster(
>  		 */
>  		ioffset = inum - xic->first_ino;
>  		if ((xic->alloc & XFS_INOBT_MASK(ioffset)) == 0) {
> -			ASSERT(do_mod(ioffset, inodes_per_cluster) == 0);
> +			ASSERT(ioffset % inodes_per_cluster == 0);
>  			continue;
>  		}
>  
> diff --git a/fs/xfs/xfs_iomap.h b/fs/xfs/xfs_iomap.h
> index b0c98d4faa5b..83474c9cede9 100644
> --- a/fs/xfs/xfs_iomap.h
> +++ b/fs/xfs/xfs_iomap.h
> @@ -30,10 +30,10 @@ xfs_aligned_fsb_count(
>  	if (extsz) {
>  		xfs_extlen_t	align;
>  
> -		align = do_mod(offset_fsb, extsz);
> +		div_u64_rem(offset_fsb, extsz, &align);
>  		if (align)
>  			count_fsb += align;
> -		align = do_mod(count_fsb, extsz);
> +		div_u64_rem(count_fsb, extsz, &align);
>  		if (align)
>  			count_fsb += extsz - align;
>  	}
> diff --git a/fs/xfs/xfs_linux.h b/fs/xfs/xfs_linux.h
> index 1631cf4546f2..2c800ffbcffd 100644
> --- a/fs/xfs/xfs_linux.h
> +++ b/fs/xfs/xfs_linux.h
> @@ -209,25 +209,6 @@ static inline xfs_dev_t linux_to_xfs_dev_t(dev_t dev)
>  #define xfs_sort(a,n,s,fn)	sort(a,n,s,fn,NULL)
>  #define xfs_stack_trace()	dump_stack()
>  
> -/* Side effect free 64 bit mod operation */
> -static inline __u32 xfs_do_mod(void *a, __u32 b, int n)
> -{
> -	switch (n) {
> -		case 4:
> -			return *(__u32 *)a % b;
> -		case 8:
> -			{
> -			__u64	c = *(__u64 *)a;
> -			return do_div(c, b);
> -			}
> -	}
> -
> -	/* NOTREACHED */
> -	return 0;
> -}
> -
> -#define do_mod(a, b)	xfs_do_mod(&(a), (b), sizeof(a))
> -
>  static inline uint64_t roundup_64(uint64_t x, uint32_t y)
>  {
>  	x += y - 1;
> diff --git a/fs/xfs/xfs_log_recover.c b/fs/xfs/xfs_log_recover.c
> index 7d897c58b0c8..4405ff21f9a9 100644
> --- a/fs/xfs/xfs_log_recover.c
> +++ b/fs/xfs/xfs_log_recover.c
> @@ -1235,6 +1235,25 @@ xlog_verify_head(
>  				be32_to_cpu((*rhead)->h_size));
>  }
>  
> +/*
> + * We need to make sure we handle log wrapping properly, so we can't use teh
> + * calculated logbno directly. Make sure it wraps to teh correct bno inside teh
> + * log.
> + *
> + * The log is limited to 32 bit sizes, so we use the appropriate modulus
> + * operation here and cast it back to a 64 bit daddr on return.
> + */
> +static inline xfs_daddr_t
> +xlog_wrap_logbno(
> +	struct xlog		*log,
> +	xfs_daddr_t		bno)
> +{
> +	int			mod;
> +
> +	div_s64_rem(bno, log->l_logBBsize, &mod);
> +	return mod;
> +}
> +
>  /*
>   * Check whether the head of the log points to an unmount record. In other
>   * words, determine whether the log is clean. If so, update the in-core state
> @@ -1283,12 +1302,13 @@ xlog_check_unmount_rec(
>  	} else {
>  		hblks = 1;
>  	}
> -	after_umount_blk = rhead_blk + hblks + BTOBB(be32_to_cpu(rhead->h_len));
> -	after_umount_blk = do_mod(after_umount_blk, log->l_logBBsize);
> +
> +	after_umount_blk = xlog_wrap_logbno(log,
> +			rhead_blk + hblks + BTOBB(be32_to_cpu(rhead->h_len)));
> +
>  	if (*head_blk == after_umount_blk &&
>  	    be32_to_cpu(rhead->h_num_logops) == 1) {
> -		umount_data_blk = rhead_blk + hblks;
> -		umount_data_blk = do_mod(umount_data_blk, log->l_logBBsize);
> +		umount_data_blk = xlog_wrap_logbno(log, rhead_blk + hblks);
>  		error = xlog_bread(log, umount_data_blk, 1, bp, &offset);
>  		if (error)
>  			return error;
> @@ -5459,9 +5479,7 @@ xlog_do_recovery_pass(
>  			 */
>  			if (blk_no + bblks <= log->l_logBBsize ||
>  			    blk_no >= log->l_logBBsize) {
> -				/* mod blk_no in case the header wrapped and
> -				 * pushed it beyond the end of the log */
> -				rblk_no = do_mod(blk_no, log->l_logBBsize);
> +				rblk_no = xlog_wrap_logbno(log, blk_no);
>  				error = xlog_bread(log, rblk_no, bblks, dbp,
>  						   &offset);
>  				if (error)
> diff --git a/fs/xfs/xfs_rtalloc.c b/fs/xfs/xfs_rtalloc.c
> index 80bbfe604ce0..776502a5dcb7 100644
> --- a/fs/xfs/xfs_rtalloc.c
> +++ b/fs/xfs/xfs_rtalloc.c
> @@ -301,8 +301,12 @@ xfs_rtallocate_extent_block(
>  		/*
>  		 * If size should be a multiple of prod, make that so.
>  		 */
> -		if (prod > 1 && (p = do_mod(bestlen, prod)))
> -			bestlen -= p;
> +		if (prod > 1) {
> +			div_u64_rem(bestlen, prod, &p);
> +			if (p)
> +				bestlen -= p;
> +		}
> +
>  		/*
>  		 * Allocate besti for bestlen & return that.
>  		 */
> @@ -1262,8 +1266,11 @@ xfs_rtpick_extent(
>  		resid = seq - (1ULL << log2);
>  		b = (mp->m_sb.sb_rextents * ((resid << 1) + 1ULL)) >>
>  		    (log2 + 1);
> -		if (b >= mp->m_sb.sb_rextents)
> -			b = do_mod(b, mp->m_sb.sb_rextents);
> +		if (b >= mp->m_sb.sb_rextents) {
> +			xfs_rtblock_t mod;
> +			div64_u64_rem(b, mp->m_sb.sb_rextents, &mod);
> +			b = mod;
> +		}
>  		if (b + len > mp->m_sb.sb_rextents)
>  			b = mp->m_sb.sb_rextents - len;
>  	}
> -- 
> 2.17.0
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html