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:

quoted

On Fri, Jul 20, 2012 at 06:02:41PM -0500, Chandra Seetharaman wrote:

.....

quoted

quoted

+		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.

quoted

quoted

+		}
+		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.

quoted

quoted

 		} 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?

quoted

quoted

@@ -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@fromorbit.com

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