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. ;)
> 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);
> xfs mailing list