Re: [PATCH] xfs: split default quota limits by quota type V2

[Carlos Maiolino <cmaiolino@redhat.com>]

On Wed, Jan 27, 2016 at 01:04:27PM -0600, Eric Sandeen wrote:
> On 1/27/16 10:57 AM, Carlos Maiolino wrote:
> > Default quotas are globally set due historical reasons. IRIX only supported user
> > and project quotas, and default quota was only applied to user quotas.
> > 
> > In Linux, when a default quota is set, all different quota types inherits the
> > same default value.
> > 
> > An user with a quota limit larger than the default quota value, will still be
> > limited to the default value because the group quotas also inherits the default
> > quotas. Unless the group which the user belongs to have a custom quota limit
> > set.
> > 
> > This patch aims to split the default quota value by quota type. Allowing each
> > quota type having different default values.
> > 
> > Default time limits are still set globally. XFS does not set a per-user/group
> > timer, but a single global timer, so, there is no reason to split time limits
> > too.
> > 
> 
> Hohum, found something else, sorry:
> 
Yep, I noticed it today's morning while playing with the time limits, but I
think I know how to fix it already, I'll test it and let you know.

Thanks for the heads up though :)


> > @@ -606,55 +653,15 @@ xfs_qm_init_quotainfo(
> >  	 * We try to get the limits from the superuser's limits fields.
> >  	 * This is quite hacky, but it is standard quota practice.
> >  	 *
> > -	 * We look at the USR dquot with id == 0 first, but if user quotas
> > -	 * are not enabled we goto the GRP dquot with id == 0.
> > -	 * We don't really care to keep separate default limits for user
> > -	 * and group quotas, at least not at this point.
> > -	 *
> >  	 * Since we may not have done a quotacheck by this point, just read
> >  	 * the dquot without attaching it to any hashtables or lists.
> >  	 */
> > -	error = xfs_qm_dqread(mp, 0,
> > -			XFS_IS_UQUOTA_RUNNING(mp) ? XFS_DQ_USER :
> > -			 (XFS_IS_GQUOTA_RUNNING(mp) ? XFS_DQ_GROUP :
> > -			  XFS_DQ_PROJ),
> > -			XFS_QMOPT_DOWARN, &dqp);
> 
> So here, we grab the *first* one that is running, and only that, to
> populate the defaults ...
> 
> > -	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;
> 
> ... including the time limits.
> 
> > -		qinf->qi_bhardlimit = be64_to_cpu(ddqp->d_blk_hardlimit);
> > -		qinf->qi_bsoftlimit = be64_to_cpu(ddqp->d_blk_softlimit);
> > -		qinf->qi_ihardlimit = be64_to_cpu(ddqp->d_ino_hardlimit);
> > -		qinf->qi_isoftlimit = be64_to_cpu(ddqp->d_ino_softlimit);
> > -		qinf->qi_rtbhardlimit = be64_to_cpu(ddqp->d_rtb_hardlimit);
> > -		qinf->qi_rtbsoftlimit = be64_to_cpu(ddqp->d_rtb_softlimit);
> > -
> > -		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;
> > -	}
> > +	if (XFS_IS_UQUOTA_RUNNING(mp))
> > +		error = xfs_qm_set_defquota(mp, XFS_DQ_USER, qinf);
> > +	if (XFS_IS_GQUOTA_RUNNING(mp))
> > +		error = xfs_qm_set_defquota(mp, XFS_DQ_GROUP, qinf);
> > +	if (XFS_IS_PQUOTA_RUNNING(mp))
> > +		error = xfs_qm_set_defquota(mp, XFS_DQ_PROJ, qinf);
> 
> Here, you set the defaults for *each* one that is running, in succession.
> But xfs_qm_set_defquota() sets global time limits, so you may overwrite
> what you set earlier, for the previous type.  i.e. it's last one wins.
> 
> So before, the global grace period was set by the first quota found running;
> now it will be set by the last quota found running.  That's probably a difference
> in behavior you didn't intend.
> 
> (It's crazy behavior to start with, though; see my other email in this thread,
> but we probably shouldn't trade crazy(1) for crazy(1);  we should keep crazy(1)
> and fix it properly at some point)
> 
> -Eric
> 
> _______________________________________________
> xfs mailing list
> xfs@oss.sgi.com
> http://oss.sgi.com/mailman/listinfo/xfs

-- 
Carlos

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs