Re: [PATCH V10 12/14] xfs: Compute bmap extent alignments in a separate function

[Chandan Babu R <chandanrlinux@gmail.com>]

On Wednesday 4 November 2020 1:21:14 AM IST Darrick J. Wong wrote:
> On Tue, Nov 03, 2020 at 08:36:40PM +0530, Chandan Babu R wrote:
> > This commit moves over the code which computes stripe alignment and
> > extent size hint alignment into a separate function. Apart from
> > xfs_bmap_btalloc(), the new function will be used by another function
> > introduced in a future commit.
> > 
> > Signed-off-by: Chandan Babu R <chandanrlinux@gmail.com>
> 
> Looks fine at last :)

Thanks for the review comments.

> Reviewed-by: Darrick J. Wong <darrick.wong@oracle.com>
> 
> Could you please add a fsstress style test that sets the errortag to see
> what kinds of, uh, testing artifacts fall out of that mode?

Sure. I will work on that.

> 
> --D
> 
> > ---
> >  fs/xfs/libxfs/xfs_bmap.c | 89 +++++++++++++++++++++++-----------------
> >  1 file changed, 52 insertions(+), 37 deletions(-)
> > 
> > diff --git a/fs/xfs/libxfs/xfs_bmap.c b/fs/xfs/libxfs/xfs_bmap.c
> > index 64c4d0e384a5..5032539d5e85 100644
> > --- a/fs/xfs/libxfs/xfs_bmap.c
> > +++ b/fs/xfs/libxfs/xfs_bmap.c
> > @@ -3463,13 +3463,59 @@ xfs_bmap_btalloc_accounting(
> >  		args->len);
> >  }
> >  
> > +static int
> > +xfs_bmap_compute_alignments(
> > +	struct xfs_bmalloca	*ap,
> > +	struct xfs_alloc_arg	*args)
> > +{
> > +	struct xfs_mount	*mp = args->mp;
> > +	xfs_extlen_t		align = 0; /* minimum allocation alignment */
> > +	int			stripe_align = 0;
> > +	int			error;
> > +
> > +	/* stripe alignment for allocation is determined by mount parameters */
> > +	if (mp->m_swidth && (mp->m_flags & XFS_MOUNT_SWALLOC))
> > +		stripe_align = mp->m_swidth;
> > +	else if (mp->m_dalign)
> > +		stripe_align = mp->m_dalign;
> > +
> > +	if (ap->flags & XFS_BMAPI_COWFORK)
> > +		align = xfs_get_cowextsz_hint(ap->ip);
> > +	else if (ap->datatype & XFS_ALLOC_USERDATA)
> > +		align = xfs_get_extsz_hint(ap->ip);
> > +	if (align) {
> > +		error = xfs_bmap_extsize_align(mp, &ap->got, &ap->prev,
> > +						align, 0, ap->eof, 0, ap->conv,
> > +						&ap->offset, &ap->length);
> > +		ASSERT(!error);
> > +		ASSERT(ap->length);
> > +	}
> > +
> > +	/* apply extent size hints if obtained earlier */
> > +	if (align) {
> > +		args->prod = align;
> > +		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;
> > +		div_u64_rem(ap->offset, args->prod, &args->mod);
> > +		if (args->mod)
> > +			args->mod = args->prod - args->mod;
> > +	}
> > +
> > +	return stripe_align;
> > +}
> > +
> >  STATIC int
> >  xfs_bmap_btalloc(
> >  	struct xfs_bmalloca	*ap)	/* bmap alloc argument struct */
> >  {
> >  	xfs_mount_t	*mp;		/* mount point structure */
> >  	xfs_alloctype_t	atype = 0;	/* type for allocation routines */
> > -	xfs_extlen_t	align = 0;	/* minimum allocation alignment */
> >  	xfs_agnumber_t	fb_agno;	/* ag number of ap->firstblock */
> >  	xfs_agnumber_t	ag;
> >  	xfs_alloc_arg_t	args;
> > @@ -3489,25 +3535,11 @@ xfs_bmap_btalloc(
> >  
> >  	mp = ap->ip->i_mount;
> >  
> > -	/* stripe alignment for allocation is determined by mount parameters */
> > -	stripe_align = 0;
> > -	if (mp->m_swidth && (mp->m_flags & XFS_MOUNT_SWALLOC))
> > -		stripe_align = mp->m_swidth;
> > -	else if (mp->m_dalign)
> > -		stripe_align = mp->m_dalign;
> > -
> > -	if (ap->flags & XFS_BMAPI_COWFORK)
> > -		align = xfs_get_cowextsz_hint(ap->ip);
> > -	else if (ap->datatype & XFS_ALLOC_USERDATA)
> > -		align = xfs_get_extsz_hint(ap->ip);
> > -	if (align) {
> > -		error = xfs_bmap_extsize_align(mp, &ap->got, &ap->prev,
> > -						align, 0, ap->eof, 0, ap->conv,
> > -						&ap->offset, &ap->length);
> > -		ASSERT(!error);
> > -		ASSERT(ap->length);
> > -	}
> > +	memset(&args, 0, sizeof(args));
> > +	args.tp = ap->tp;
> > +	args.mp = mp;
> >  
> > +	stripe_align = xfs_bmap_compute_alignments(ap, &args);
> >  
> >  	nullfb = ap->tp->t_firstblock == NULLFSBLOCK;
> >  	fb_agno = nullfb ? NULLAGNUMBER : XFS_FSB_TO_AGNO(mp,
> > @@ -3538,9 +3570,6 @@ xfs_bmap_btalloc(
> >  	 * Normal allocation, done through xfs_alloc_vextent.
> >  	 */
> >  	tryagain = isaligned = 0;
> > -	memset(&args, 0, sizeof(args));
> > -	args.tp = ap->tp;
> > -	args.mp = mp;
> >  	args.fsbno = ap->blkno;
> >  	args.oinfo = XFS_RMAP_OINFO_SKIP_UPDATE;
> >  
> > @@ -3571,21 +3600,7 @@ xfs_bmap_btalloc(
> >  		args.total = ap->total;
> >  		args.minlen = ap->minlen;
> >  	}
> > -	/* apply extent size hints if obtained earlier */
> > -	if (align) {
> > -		args.prod = align;
> > -		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;
> > -		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 underlying
> >  	 * logical volume manager is a stripe, and the file offset is zero then
> 

-- 
chandan