Re: [PATCH 1/8] xfs: refactor quota limits initialization

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

On Fri, May 11, 2018 at 11:19:18AM -0400, Brian Foster wrote:
> On Thu, May 10, 2018 at 12:18:06PM -0700, Darrick J. Wong wrote:
> > From: Darrick J. Wong <darrick.wong@oracle.com>
> > 
> > Replace all the if (!error) weirdness with helper functions that follow
> > our regular coding practices.
> > 
> > Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> > ---
> >  fs/xfs/xfs_qm.c |  141 ++++++++++++++++++++++++++++++-------------------------
> >  1 file changed, 77 insertions(+), 64 deletions(-)
> > 
> > 
> > diff --git a/fs/xfs/xfs_qm.c b/fs/xfs/xfs_qm.c
> > index f927b7d72db1..9ce6dffc25f2 100644
> > --- a/fs/xfs/xfs_qm.c
> > +++ b/fs/xfs/xfs_qm.c
> > @@ -561,26 +561,87 @@ xfs_qm_set_defquota(
> ...
> > +/* Initialize quota time limits from the root dquot. */
> > +static void
> > +xfs_qm_init_timelimits(
> > +	struct xfs_mount	*mp,
> > +	struct xfs_quotainfo	*qinf)
> > +{
> > +	struct xfs_disk_dquot	*ddqp;
> > +	struct xfs_dquot	*dqp;
> > +	uint			type;
> > +	int			error;
> > +
> > +	qinf->qi_btimelimit = XFS_QM_BTIMELIMIT;
> > +	qinf->qi_itimelimit = XFS_QM_ITIMELIMIT;
> > +	qinf->qi_rtbtimelimit = XFS_QM_RTBTIMELIMIT;
> > +	qinf->qi_bwarnlimit = XFS_QM_BWARNLIMIT;
> > +	qinf->qi_iwarnlimit = XFS_QM_IWARNLIMIT;
> > +	qinf->qi_rtbwarnlimit = XFS_QM_RTBWARNLIMIT;
> > +
> > +	/*
> > +	 * We try to get the limits from the superuser's limits fields.
> > +	 * This is quite hacky, but it is standard quota practice.
> > +	 *
> > +	 * Since we may not have done a quotacheck by this point, just read
> > +	 * the dquot without attaching it to any hashtables or lists.
> > +	 *
> > +	 * Timers and warnings are globally set by the first timer found in
> > +	 * user/group/proj quota types, otherwise a default value is used.
> > +	 * This should be split into different fields per quota type.
> > +	 */
> > +	if (XFS_IS_UQUOTA_RUNNING(mp))
> > +		type = XFS_DQ_USER;
> > +	else if (XFS_IS_GQUOTA_RUNNING(mp))
> > +		type = XFS_DQ_GROUP;
> > +	else
> > +		type = XFS_DQ_PROJ;
> > +	error = xfs_qm_dqget_uncached(mp, 0, type, &dqp);
> > +	if (error)
> > +		return;
> > +
> > +	ddqp = &dqp->q_core;
> > +	/*
> > +	 * The warnings and timers set the grace period given to
> > +	 * a user or group before he or she can not perform any
> > +	 * more writing. If it is zero, a default is used.
> > +	 */
> > +	qinf->qi_btimelimit = ddqp->d_btimer ?
> > +		be32_to_cpu(ddqp->d_btimer) : XFS_QM_BTIMELIMIT;
> > +	qinf->qi_itimelimit = ddqp->d_itimer ?
> > +		be32_to_cpu(ddqp->d_itimer) : XFS_QM_ITIMELIMIT;
> > +	qinf->qi_rtbtimelimit = ddqp->d_rtbtimer ?
> > +		be32_to_cpu(ddqp->d_rtbtimer) : XFS_QM_RTBTIMELIMIT;
> > +	qinf->qi_bwarnlimit = ddqp->d_bwarns ?
> > +		be16_to_cpu(ddqp->d_bwarns) : XFS_QM_BWARNLIMIT;
> > +	qinf->qi_iwarnlimit = ddqp->d_iwarns ?
> > +		be16_to_cpu(ddqp->d_iwarns) : XFS_QM_IWARNLIMIT;
> > +	qinf->qi_rtbwarnlimit = ddqp->d_rtbwarns ?
> > +		be16_to_cpu(ddqp->d_rtbwarns) : XFS_QM_RTBWARNLIMIT;
> 
> There's no need for these ternary statements if we initialized default
> values above. How about we just convert these to if checks? I.e.,
> 
> 	if (ddqp->d_btimer)
> 		qinf->qi_btimelimit = be32_to_cpu(ddqp->d_btimer);
> 	if (ddqp->d_itimer)
> 		qinf->qi_itimelimit = be32_to_cpu(ddqp->d_itimer);
> 	...
> 
> Otherwise looks Ok.

Fixed.

--D

> Brian
> 
> > +	xfs_qm_dqdestroy(dqp);
> >  }
> >  
> >  /*
> > @@ -592,8 +653,6 @@ xfs_qm_init_quotainfo(
> >  	struct xfs_mount	*mp)
> >  {
> >  	struct xfs_quotainfo	*qinf;
> > -	struct xfs_dquot	*dqp;
> > -	uint			type;
> >  	int			error;
> >  
> >  	ASSERT(XFS_IS_QUOTA_RUNNING(mp));
> > @@ -626,53 +685,7 @@ xfs_qm_init_quotainfo(
> >  
> >  	mp->m_qflags |= (mp->m_sb.sb_qflags & XFS_ALL_QUOTA_CHKD);
> >  
> > -	/*
> > -	 * We try to get the limits from the superuser's limits fields.
> > -	 * This is quite hacky, but it is standard quota practice.
> > -	 *
> > -	 * Since we may not have done a quotacheck by this point, just read
> > -	 * the dquot without attaching it to any hashtables or lists.
> > -	 *
> > -	 * Timers and warnings are globally set by the first timer found in
> > -	 * user/group/proj quota types, otherwise a default value is used.
> > -	 * This should be split into different fields per quota type.
> > -	 */
> > -	if (XFS_IS_UQUOTA_RUNNING(mp))
> > -		type = XFS_DQ_USER;
> > -	else if (XFS_IS_GQUOTA_RUNNING(mp))
> > -		type = XFS_DQ_GROUP;
> > -	else
> > -		type = XFS_DQ_PROJ;
> > -	error = xfs_qm_dqget_uncached(mp, 0, type, &dqp);
> > -	if (!error) {
> > -		xfs_disk_dquot_t	*ddqp = &dqp->q_core;
> > -
> > -		/*
> > -		 * The warnings and timers set the grace period given to
> > -		 * a user or group before he or she can not perform any
> > -		 * more writing. If it is zero, a default is used.
> > -		 */
> > -		qinf->qi_btimelimit = ddqp->d_btimer ?
> > -			be32_to_cpu(ddqp->d_btimer) : XFS_QM_BTIMELIMIT;
> > -		qinf->qi_itimelimit = ddqp->d_itimer ?
> > -			be32_to_cpu(ddqp->d_itimer) : XFS_QM_ITIMELIMIT;
> > -		qinf->qi_rtbtimelimit = ddqp->d_rtbtimer ?
> > -			be32_to_cpu(ddqp->d_rtbtimer) : XFS_QM_RTBTIMELIMIT;
> > -		qinf->qi_bwarnlimit = ddqp->d_bwarns ?
> > -			be16_to_cpu(ddqp->d_bwarns) : XFS_QM_BWARNLIMIT;
> > -		qinf->qi_iwarnlimit = ddqp->d_iwarns ?
> > -			be16_to_cpu(ddqp->d_iwarns) : XFS_QM_IWARNLIMIT;
> > -		qinf->qi_rtbwarnlimit = ddqp->d_rtbwarns ?
> > -			be16_to_cpu(ddqp->d_rtbwarns) : XFS_QM_RTBWARNLIMIT;
> > -		xfs_qm_dqdestroy(dqp);
> > -	} else {
> > -		qinf->qi_btimelimit = XFS_QM_BTIMELIMIT;
> > -		qinf->qi_itimelimit = XFS_QM_ITIMELIMIT;
> > -		qinf->qi_rtbtimelimit = XFS_QM_RTBTIMELIMIT;
> > -		qinf->qi_bwarnlimit = XFS_QM_BWARNLIMIT;
> > -		qinf->qi_iwarnlimit = XFS_QM_IWARNLIMIT;
> > -		qinf->qi_rtbwarnlimit = XFS_QM_RTBWARNLIMIT;
> > -	}
> > +	xfs_qm_init_timelimits(mp, qinf);
> >  
> >  	if (XFS_IS_UQUOTA_RUNNING(mp))
> >  		xfs_qm_set_defquota(mp, XFS_DQ_USER, qinf);
> > 
> > --
> > 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
> --
> 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