On Tue, Jul 22, 2014 at 12:10:14PM -0500, Eric Sandeen wrote: > The commit > > 83e782e xfs: Remove incore use of XFS_OQUOTA_ENFD and XFS_OQUOTA_CHKD > > added a new function xfs_sb_quota_from_disk() which swaps > on-disk XFS_OQUOTA_* flags for in-core XFS_GQUOTA_* and XFS_PQUOTA_* > flags after the superblock is read. > > However, if log recovery is required, the superblock is read again, > and the modified in-core flags are re-read from disk, so we have > XFS_OQUOTA_* flags in memory again. This causes the > XFS_QM_NEED_QUOTACHECK() test to be true, because the XFS_OQUOTA_CHKD > is still set, and not XFS_GQUOTA_CHKD or XFS_PQUOTA_CHKD. > > The simple one-line fix is to call xfs_sb_quota_from_disk after > we re-read the superblock during log recovery. > First off, this looks reasonable to me given the current code. > Reported-by: Cyril B. <cbay@xxxxxxxxxxxxx> > Signed-off-by: Eric Sandeen <sandeen@xxxxxxxxxx> > --- > > Quick-tested only; it seems to resolve Cyril's testcase, but I've > not done an xfstests run yet as this is somewhat of an RFC: > > It feels like there may be a better/more systemic fix here, though. > We can't call xfs_sb_quota_from_disk it from xfs_sb_from_disk(), > because the sb read verifier wants to know what was really on disk, > not what was fixed up after the fact. > I was wondering why this is the case because the verifier runs in the I/O path, so it would hit before we actually return with the buffer in hand in the mount codepath. Looking a bit further, I see that we call xfs_sb_from_disk() from within the verifier (xfs_sb_verify()) and apparently some flag validation is done on the result of that. So I suspect that's what you mean here and probably why xfs_sb_quota_from_disk() is outside of xfs_sb_from_disk(). FWIW, xfs_sb_quota_to_disk() is called within xfs_sb_to_disk(). IMO, the more clear thing to do is make xfs_sb_to_disk() and xfs_sb_from_disk() consistent in behavior and let the special verifier case deal with the quirk. In other words, rename it to something like __xfs_sb_from_disk(..., bool convert_flags) and conditionally call xfs_sb_quota_from_disk(). Let the verifier call that variant directly with false and then #define xfs_sb_from_disk to the true variant. Just my .02. ;) Brian > We could add an "xlate_quota" arg to xfs_sb_from_disk(), but > that feels a little odd; the function today quite clearly only > does endian conversions, not other more detailed translations. > > But if something like that is preferred, I can send a patch V2. > > diff --git a/fs/xfs/xfs_log_recover.c b/fs/xfs/xfs_log_recover.c > index bce53ac..0a29de7 100644 > --- a/fs/xfs/xfs_log_recover.c > +++ b/fs/xfs/xfs_log_recover.c > @@ -4429,6 +4429,7 @@ xlog_do_recover( > /* Convert superblock from on-disk format */ > sbp = &log->l_mp->m_sb; > xfs_sb_from_disk(sbp, XFS_BUF_TO_SBP(bp)); > + xfs_sb_quota_from_disk(sbp); > ASSERT(sbp->sb_magicnum == XFS_SB_MAGIC); > ASSERT(xfs_sb_good_version(sbp)); > xfs_buf_relse(bp); > > _______________________________________________ > xfs mailing list > xfs@xxxxxxxxxxx > http://oss.sgi.com/mailman/listinfo/xfs _______________________________________________ xfs mailing list xfs@xxxxxxxxxxx http://oss.sgi.com/mailman/listinfo/xfs
