[RFC v6 PATCH 3/5] xfs: Add pquotaino to on-disk super block

[Dave Chinner]

On Wed, Aug 22, 2012 at 06:30:48PM -0500, Chandra Seetharaman wrote:
> On Wed, 2012-08-15 at 12:09 +1000, Dave Chinner wrote:
> > On Fri, Jul 20, 2012 at 06:02:41PM -0500, Chandra Seetharaman wrote:
.....
> > > +		if (to->sb_qflags & (XFS_OQUOTA_ENFD | XFS_OQUOTA_CHKD)) {
> > > +			xfs_notice(mp, "Super block has XFS_OQUOTA bits with "
> > > +			"version NO_OQUOTA. Fixing it.\n");
> > > +			to->sb_qflags &= ~(XFS_OQUOTA_ENFD | XFS_OQUOTA_CHKD);
> > > +		}
> > > +		to->sb_pquotino = be64_to_cpu(from->sb_pquotino);
> > 
> > So we have a feature bit set, but old quota bits set. How can that
> > happen?
> > 
> > If it does occur, doesn't that mean we cannot trust the group or
> > project quotas to be correct, so at minimum this case needs to
> > trigger a quotacheck for both group and project quotas?
> 
> Sure, will do a quotacheck here. I just call xfs_qm_quotacheck() and
> fail if it fails ?

The quotacheck occurs later in the mount process. IIRC, just
clearing the relevant XFS_[UGP]QUOTA_CHKD flag will cause a quota
check to be done at the appropriate time.

> > > +		}
> > > +		if (to->sb_qflags & XFS_OQUOTA_ENFD)
> > > +			to->sb_qflags |= (to->sb_qflags & XFS_PQUOTA_ACCT) ?
> > > +					XFS_PQUOTA_ENFD : XFS_GQUOTA_ENFD;
> > > +		if (to->sb_qflags & XFS_OQUOTA_CHKD)
> > > +			to->sb_qflags |= (to->sb_qflags & XFS_PQUOTA_ACCT) ?
> > > +					XFS_PQUOTA_CHKD : XFS_GQUOTA_CHKD;
> > 
> > what do you do if both XFS_PQUOTA_ACCT and XFS_GQUOTA_ACCT are set?
> > i.e. both quotas were active even though the feature bit wasn't set?
> 
> I will do a check on both flags being set and do a quotacheck ?

Yes, I think that will be sufficient with an appropriate warning.

> > >  		} else {
> > >  			switch (size) {
> > >  			case 2:
> > > @@ -759,6 +803,12 @@ reread:
> > >  		goto reread;
> > >  	}
> > >  
> > > +	if (!xfs_sb_version_has_no_oquota(&mp->m_sb) &&
> > > +			XFS_IS_PQUOTA_ON(mp)) {
> > > +		mp->m_sb.sb_pquotino = mp->m_sb.sb_gquotino;
> > > +		mp->m_sb.sb_gquotino = NULLFSINO;
> > > +	}
> > 
> > why this is necessary? Didn't the earlier xfs_sb_from_disk() call
> > deal with this?
> 
> The call in xfs_sb_from_disk() only sets if the superblock has pquota
> already. 
> 
> This sets up the fields when superblock didn't have it, but the user
> specified pquota as a mount option.

Ah, so it is the same as the previous case I mentioned needs a
comment. Can you document this here as well?

> > > @@ -838,19 +840,22 @@ xfs_qm_qino_alloc(
> > >  		ASSERT((sbfields & (XFS_SB_VERSIONNUM | XFS_SB_UQUOTINO |
> > >  				   XFS_SB_GQUOTINO | XFS_SB_QFLAGS)) ==
> > >  		       (XFS_SB_VERSIONNUM | XFS_SB_UQUOTINO |
> > > -			XFS_SB_GQUOTINO | XFS_SB_QFLAGS));
> > > +			XFS_SB_GQUOTINO | XFS_SB_PQUOTINO | XFS_SB_QFLAGS));
> > 
> > Did you test this with CONFIG_XFS_DEBUG=y? It will always fail
> > because you didn't add XFS_SB_PQUOTINO to the sbfields mask....
> 
> In my box, I always had problems with DEBUG :(... So, I stopped testing
> with it.

Hmmm. DEBUG shouldn't cause you any extra problems unless there
really is something wrong - I run almost exclusively with DEBUG
enabled and I rarely have problems with spurious ASSERT()s
triggering. It's better to report spurious/unrelated ASSERT()
failures than to ignore them. 

[ FWIW, the only time I turnoff DEBUG is when I'm doing performance
benchmarking to get maximum numbers - DEBUG drops metadata
throughput by about 25% and changes allocation patterns to improve
code coverage, so the results of performance testing with DEBUG are
not really representative. ]

In the mean time, run with DEBUG without your patches and exclude
all the tests that trigger problems on a vanilla kernel (e.g. via
'echo 068 >> expunged') and then run with you patches. Any new
failures are likely to be caused by your patches and need to be
analysed.

Cheers,

Dave.
-- 
Dave Chinner
david at fromorbit.com