Forced quotacheck after unclean unmount since 3.11, bisected

Forced quotacheck after unclean unmount since 3.11, bisected

[Cyril B.]

Hello,

I've noticed that since Linux 3.11, quotacheck always happen after 
unclean unmounts. Is this expected? It didn't happen on previous Linux 
versions.

A bisect shows that this commit is responsible:
xfs: Remove incore use of XFS_OQUOTA_ENFD and XFS_OQUOTA_CHKD
id: 83e782e1a1cc0159888e58e14dfc8f3289663338

Relevant dmesg:
[    8.844063] XFS (md4): Mounting Filesystem
[    9.054023] XFS (md4): Starting recovery (logdev: internal)
[    9.331785] XFS (md4): Ending recovery (logdev: internal)
[    9.383856] XFS (md4): Quotacheck needed: Please wait.
[   48.427732] XFS (md4): Quotacheck: Done.

More details:
   * vanilla kernel
   * on top of mdadm (RAID1)
   * xfsprogs 3.1.5
   * mount options: noatime,nosuid,nodev,grpquota,inode64
   * xfs_info:
meta-data=/dev/md4               isize=256    agcount=4, 
agsize=119997280 blks
          =                       sectsz=512   attr=2
data     =                       bsize=4096   blocks=479989120, imaxpct=5
          =                       sunit=0      swidth=0 blks
naming   =version 2              bsize=4096   ascii-ci=0
log      =internal               bsize=4096   blocks=234369, version=2
          =                       sectsz=512   sunit=0 blks, lazy-count=1
realtime =none                   extsz=4096   blocks=0, rtextents=0

Thanks,

-- 
Cyril B.

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

Re: Forced quotacheck after unclean unmount since 3.11, bisected

[Eric Sandeen]

On 7/22/14, 8:16 AM, Cyril B. wrote:
> Hello,
> 
> I've noticed that since Linux 3.11, quotacheck always happen after
> unclean unmounts. Is this expected? It didn't happen on previous
> Linux versions.

I can reproduce this, I'll take a look - thanks for bisecting!

-Eric

> A bisect shows that this commit is responsible:
> xfs: Remove incore use of XFS_OQUOTA_ENFD and XFS_OQUOTA_CHKD
> id: 83e782e1a1cc0159888e58e14dfc8f3289663338
> 
> Relevant dmesg:
> [    8.844063] XFS (md4): Mounting Filesystem
> [    9.054023] XFS (md4): Starting recovery (logdev: internal)
> [    9.331785] XFS (md4): Ending recovery (logdev: internal)
> [    9.383856] XFS (md4): Quotacheck needed: Please wait.
> [   48.427732] XFS (md4): Quotacheck: Done.
> 
> More details:
>   * vanilla kernel
>   * on top of mdadm (RAID1)
>   * xfsprogs 3.1.5
>   * mount options: noatime,nosuid,nodev,grpquota,inode64
>   * xfs_info:
> meta-data=/dev/md4               isize=256    agcount=4, agsize=119997280 blks
>          =                       sectsz=512   attr=2
> data     =                       bsize=4096   blocks=479989120, imaxpct=5
>          =                       sunit=0      swidth=0 blks
> naming   =version 2              bsize=4096   ascii-ci=0
> log      =internal               bsize=4096   blocks=234369, version=2
>          =                       sectsz=512   sunit=0 blks, lazy-count=1
> realtime =none                   extsz=4096   blocks=0, rtextents=0
> 
> Thanks,
> 

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

[PATCH] xfs: avoid false quotacheck after unclean shutdown

[Eric Sandeen]

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.

Reported-by: Cyril B. <cbay@excellency.fr>
Signed-off-by: Eric Sandeen <sandeen@redhat.com>
---

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.

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@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCH] xfs: avoid false quotacheck after unclean shutdown

[Brian Foster]

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@excellency.fr>
> Signed-off-by: Eric Sandeen <sandeen@redhat.com>
> ---
> 
> 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@oss.sgi.com
> http://oss.sgi.com/mailman/listinfo/xfs

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

[PATCH V2] xfs: avoid false quotacheck after unclean shutdown

[Eric Sandeen]

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.

Change xfs_sb_from_disk to call xfs_sb_quota_from disk and always
convert the disk flags to in-memory flags.

Add a lower-level function which can be called with "false" to
not convert the flags, so that the sb verifier can verify
exactly what was on disk, per Brian Foster's suggestion.

Reported-by: Cyril B. <cbay@excellency.fr>
Signed-off-by: Eric Sandeen <sandeen@redhat.com>
---

V2: Call xfs_sb_quota_from_disk from xfs_sb_from_disk,
unless we're the verifier, which wants to see what is
actually on-disk to verify it.  Thanks Brian!


diff --git a/fs/xfs/xfs_mount.c b/fs/xfs/xfs_mount.c
index 3507cd0..16d1671 100644
--- a/fs/xfs/xfs_mount.c
+++ b/fs/xfs/xfs_mount.c
@@ -324,7 +324,6 @@ reread:
 	 * Initialize the mount structure from the superblock.
 	 */
 	xfs_sb_from_disk(sbp, XFS_BUF_TO_SBP(bp));
-	xfs_sb_quota_from_disk(sbp);
 
 	/*
 	 * If we haven't validated the superblock, do so now before we try
diff --git a/fs/xfs/xfs_sb.c b/fs/xfs/xfs_sb.c
index 7703fa6..0f631d2 100644
--- a/fs/xfs/xfs_sb.c
+++ b/fs/xfs/xfs_sb.c
@@ -386,10 +386,11 @@ xfs_sb_quota_from_disk(struct xfs_sb *sbp)
 	}
 }
 
-void
-xfs_sb_from_disk(
+static void
+__xfs_sb_from_disk(
 	struct xfs_sb	*to,
-	xfs_dsb_t	*from)
+	xfs_dsb_t	*from,
+	bool		convert_xquota)
 {
 	to->sb_magicnum = be32_to_cpu(from->sb_magicnum);
 	to->sb_blocksize = be32_to_cpu(from->sb_blocksize);
@@ -445,6 +446,17 @@ xfs_sb_from_disk(
 	to->sb_pad = 0;
 	to->sb_pquotino = be64_to_cpu(from->sb_pquotino);
 	to->sb_lsn = be64_to_cpu(from->sb_lsn);
+	/* Convert on-disk flags to in-memory flags? */
+	if (convert_xquota)
+		xfs_sb_quota_from_disk(to);
+}
+
+void
+xfs_sb_from_disk(
+	struct xfs_sb	*to,
+	xfs_dsb_t	*from)
+{
+	__xfs_sb_from_disk(to, from, true);
 }
 
 static inline void
@@ -577,7 +589,11 @@ xfs_sb_verify(
 	struct xfs_mount *mp = bp->b_target->bt_mount;
 	struct xfs_sb	sb;
 
-	xfs_sb_from_disk(&sb, XFS_BUF_TO_SBP(bp));
+	/*
+	 * Use call variant which doesn't convert quota flags from disk 
+	 * format, because xfs_mount_validate_sb checks the on-disk flags.
+	 */
+	__xfs_sb_from_disk(&sb, XFS_BUF_TO_SBP(bp), false);
 
 	/*
 	 * Only check the in progress field for the primary superblock as


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

Re: [PATCH V2] xfs: avoid false quotacheck after unclean shutdown

[Brian Foster]

On Thu, Jul 24, 2014 at 06:29:34PM -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.
> 
> Change xfs_sb_from_disk to call xfs_sb_quota_from disk and always
> convert the disk flags to in-memory flags.
> 
> Add a lower-level function which can be called with "false" to
> not convert the flags, so that the sb verifier can verify
> exactly what was on disk, per Brian Foster's suggestion.
> 
> Reported-by: Cyril B. <cbay@excellency.fr>
> Signed-off-by: Eric Sandeen <sandeen@redhat.com>
> ---

Looks good to me, thanks!

Reviewed-by: Brian Foster <bfoster@redhat.com>

> 
> V2: Call xfs_sb_quota_from_disk from xfs_sb_from_disk,
> unless we're the verifier, which wants to see what is
> actually on-disk to verify it.  Thanks Brian!
> 
> 
> diff --git a/fs/xfs/xfs_mount.c b/fs/xfs/xfs_mount.c
> index 3507cd0..16d1671 100644
> --- a/fs/xfs/xfs_mount.c
> +++ b/fs/xfs/xfs_mount.c
> @@ -324,7 +324,6 @@ reread:
>  	 * Initialize the mount structure from the superblock.
>  	 */
>  	xfs_sb_from_disk(sbp, XFS_BUF_TO_SBP(bp));
> -	xfs_sb_quota_from_disk(sbp);
>  
>  	/*
>  	 * If we haven't validated the superblock, do so now before we try
> diff --git a/fs/xfs/xfs_sb.c b/fs/xfs/xfs_sb.c
> index 7703fa6..0f631d2 100644
> --- a/fs/xfs/xfs_sb.c
> +++ b/fs/xfs/xfs_sb.c
> @@ -386,10 +386,11 @@ xfs_sb_quota_from_disk(struct xfs_sb *sbp)
>  	}
>  }
>  
> -void
> -xfs_sb_from_disk(
> +static void
> +__xfs_sb_from_disk(
>  	struct xfs_sb	*to,
> -	xfs_dsb_t	*from)
> +	xfs_dsb_t	*from,
> +	bool		convert_xquota)
>  {
>  	to->sb_magicnum = be32_to_cpu(from->sb_magicnum);
>  	to->sb_blocksize = be32_to_cpu(from->sb_blocksize);
> @@ -445,6 +446,17 @@ xfs_sb_from_disk(
>  	to->sb_pad = 0;
>  	to->sb_pquotino = be64_to_cpu(from->sb_pquotino);
>  	to->sb_lsn = be64_to_cpu(from->sb_lsn);
> +	/* Convert on-disk flags to in-memory flags? */
> +	if (convert_xquota)
> +		xfs_sb_quota_from_disk(to);
> +}
> +
> +void
> +xfs_sb_from_disk(
> +	struct xfs_sb	*to,
> +	xfs_dsb_t	*from)
> +{
> +	__xfs_sb_from_disk(to, from, true);
>  }
>  
>  static inline void
> @@ -577,7 +589,11 @@ xfs_sb_verify(
>  	struct xfs_mount *mp = bp->b_target->bt_mount;
>  	struct xfs_sb	sb;
>  
> -	xfs_sb_from_disk(&sb, XFS_BUF_TO_SBP(bp));
> +	/*
> +	 * Use call variant which doesn't convert quota flags from disk 
> +	 * format, because xfs_mount_validate_sb checks the on-disk flags.
> +	 */
> +	__xfs_sb_from_disk(&sb, XFS_BUF_TO_SBP(bp), false);
>  
>  	/*
>  	 * Only check the in progress field for the primary superblock as
> 
> 
> _______________________________________________
> xfs mailing list
> xfs@oss.sgi.com
> http://oss.sgi.com/mailman/listinfo/xfs

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