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

[Carlos Maiolino <cmaiolino@redhat.com>]

On Tue, Jun 28, 2016 at 10:40:31AM -0500, Eric Sandeen wrote:
> 
> 
> On 6/28/16 3:57 AM, Carlos Maiolino wrote:
> > 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.
> 
> Thanks Carlos - 
> 
> Maybe I don't understand what you want to warn about.
> 
> If we get here with:
> 
> 	if (sbp->sb_qflags & XFS_PQUOTA_ACCT &&
> 	    sbp->sb_gquotino != NULLFSINO)  {
> 
> that means we have an on-disk super without the pquotino field,
> the XFS_PQUOTA_ACCT flag is set, and so the gquotino field was
> used for the project quota; this is valid, and there is
> nothing to warn about in this case.
> 
> If we get here with:
> 
> 	if (sbp->sb_qflags & XFS_PQUOTA_ACCT &&
> 	    sbp->sb_gquotino == NULLFSINO)  {
> 
> that means we have an on-disk super without the pquotino field,
> the XFS_PQUOTA_ACCT flag is set, and the gquotino was not set
> to a valid value.  This could happen either from a bad on-disk
> value, or it could mean that we called the function twice in a
> row.  Without maintaining more state, we can't know which, and
> warning the user about a programming error wouldn't be helpful.
> 
> Actually, repair already handles this case elsewhere:
> 
> quota_sb_check(xfs_mount_t *mp)
> {
>         /*
>          * if the sb says we have quotas and we lost both,
>          * signal a superblock downgrade.  that will cause
>          * the quota flags to get zeroed.  (if we only lost
>          * one quota inode, do nothing and complain later.)
>          *
>          * if the sb says we have quotas but we didn't start out
>          * with any quota inodes, signal a superblock downgrade.
> 
> In the case where quota flags are on but all quota inodes are
> zero, it silently clears the quota flags.  Whether or not that
> should be silent I'm not sure, but I think that is separate
> from this patch.
> 
> Thanks,
> -Eric

Thanks for the great and detailed explanation Eric, I think I was just being too
careful about not having a warning there, without completely understand why a
warning isn't not necessary there. :)

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

-- 
Carlos

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