Re: [PATCH] xfs_repair: don't call xfs_sb_quota_from_disk twice

From: Carlos Maiolino <cmaiolino@redhat.com>
To: Eric Sandeen <sandeen@sandeen.net>
Cc: xfs@oss.sgi.com
Subject: Re: [PATCH] xfs_repair: don't call xfs_sb_quota_from_disk twice
Date: Tue, 28 Jun 2016 10:57:26 +0200	[thread overview]
Message-ID: <20160628085726.GA4397@redhat.com> (raw)
In-Reply-To: <5c408cf1-e9fa-8bee-106e-93e553fc98a7@sandeen.net>

On Mon, Jun 27, 2016 at 10:34:39AM -0500, Eric Sandeen wrote:
> 
> 
> On 6/27/16 4:48 AM, Carlos Maiolino wrote:
> > On Fri, Jun 24, 2016 at 04:24:58PM -0500, Eric Sandeen wrote:
> >> kernel commit 5ef828c4
> >> xfs: avoid false quotacheck after unclean shutdown
> >>
> >> made xfs_sb_from_disk() also call xfs_sb_quota_from_disk
> >> by default.
> >>
> >> However, when this was merged to libxfs, existing separate
> >> calls to libxfs_sb_quota_from_disk remained, and calling it
> >> twice in a row on a V4 superblock leads to issues, because:
> >>
> >>
> >>         if (sbp->sb_qflags & XFS_PQUOTA_ACCT)  {
> >> ...
> >>                 sbp->sb_pquotino = sbp->sb_gquotino;
> >>                 sbp->sb_gquotino = NULLFSINO;
> >>
> >> and after the second call, we have set both pquotino and gquotino
> >> to NULLFSINO.
> >>
> >> Fix this by making it safe to call twice, and also remove the extra
> >> calls to libxfs_sb_quota_from_disk.
> >>
> >> This is only spotted when running xfstests with "-m crc=0" because
> >> the sb_from_disk change came about after V5 became default, and
> >> the above behavior only exists on a V4 superblock.
> >>
> >> Reported-by: Eryu Guan <eguan@redhat.com>
> >> Signed-off-by: Eric Sandeen <sandeen@redhat.com>
> >> ---
> >>
> >>
> >> diff --git a/libxfs/xfs_sb.c b/libxfs/xfs_sb.c
> >> index 45db6ae..44f3e3e 100644
> >> --- a/libxfs/xfs_sb.c
> >> +++ b/libxfs/xfs_sb.c
> >> @@ -316,13 +316,16 @@ xfs_sb_quota_from_disk(struct xfs_sb *sbp)
> >>  					XFS_PQUOTA_CHKD : XFS_GQUOTA_CHKD;
> >>  	sbp->sb_qflags &= ~(XFS_OQUOTA_ENFD | XFS_OQUOTA_CHKD);
> >>  
> >> -	if (sbp->sb_qflags & XFS_PQUOTA_ACCT)  {
> >> +	if (sbp->sb_qflags & XFS_PQUOTA_ACCT &&
> >> +	    sbp->sb_gquotino != NULLFSINO)  {
> > 
> > Although I agree with this check, shouldn't we report some sort of error when it
> > happens? Once, it's not supposed to happen, and, might be a sign of corruption?
> 
> I dunno, it would also happen if it gets called twice, which is intentionally
> made harmless by this change.  We don't warn on free(NULL) for example...
> 

Well, I don't 100% agree with not having a warning here, but it doesn't make the
patch less valuable.

Reviewed-by: Carlos Maiolino <cmaiolino@redhat.com>

> I don't think it needs a warning.
> 
> -Eric
>  
> 
> _______________________________________________
> xfs mailing list
> xfs@oss.sgi.com
> http://oss.sgi.com/mailman/listinfo/xfs

-- 
Carlos

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