Re: [PATCH 3/7] xfs_repair: enforce that inode btree chunks can't point to AG headers

From: "Darrick J. Wong" <darrick.wong@oracle.com>
To: Eric Sandeen <sandeen@sandeen.net>
Cc: linux-xfs@vger.kernel.org, alex@zadara.com
Subject: Re: [PATCH 3/7] xfs_repair: enforce that inode btree chunks can't point to AG headers
Date: Wed, 26 Feb 2020 09:24:54 -0800	[thread overview]
Message-ID: <20200226172454.GF8045@magnolia> (raw)
In-Reply-To: <dd15d451-c745-0318-91b0-36864c0052f7@sandeen.net>

On Wed, Feb 26, 2020 at 09:09:42AM -0800, Eric Sandeen wrote:
> On 2/4/20 4:46 PM, Darrick J. Wong wrote:
> > From: Darrick J. Wong <darrick.wong@oracle.com>
> > 
> > xfs_repair has a very old check that evidently excuses the AG 0 inode
> > btrees pointing to blocks that are already marked XR_E_INUSE_FS* (e.g.
> > AG headers).  mkfs never formats filesystems that way and it looks like
> > an error, so purge the check.  After this, we always complain if inodes
> > overlap with AG headers because that should never happen.
> > 
> > Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> 
> I know it's hard to keep track, but it'd be nice if
> 
> > -	ASSERT(M_IGEO(mp)->ialloc_blks > 0);
> 
> this line had been kept per the feedback on the last patchset...

I've added that back.  For real this time.

> This also lost my feedback the first time, re:
> 
> @@ -1782,18 +1775,6 @@ _("inode chunk claims untracked block, finobt block - agno %d, bno %d, inopb %d\
>   				break;
>   			case XR_E_INUSE_FS:
>   			case XR_E_INUSE_FS1:
> 
> "I guess there's no real reason to list a couple cases that all fall through
> to default:, I'd just remove them as well since they aren't any more special
> than the other unmentioned cases."
> 
> -				if (agno == 0 &&
> -				    ino + j >= first_prealloc_ino &&
> -				    ino + j < last_prealloc_ino) {
> -					do_warn(
> -_("inode chunk claims untracked block, finobt block - agno %d, bno %d, inopb %d\n"),
> -						agno, agbno, mp->m_sb.sb_inopblock);
> -
> -					set_bmap(agno, agbno, XR_E_INO);
> -					suspect++;
> -					break;
> -				}
> -				/* fall through */
>  			default:
> 
> I guess I should stop saying "I'll do that on the way in" if 2 more
> versions are going to hit the list, maybe it takes the feedback off
> your radar.

I (almost) always make the changes to my local tree even if you say
you'll do it on the way in, because that makes it easier to compare the
for-next tree vs. my about-to-be-rebased dev tree.

Unfortunately, I do occasionally slip up and forget to make the changes,
even if I've sent email assenting to the changes, because there's not
anything linking "I will make this change" in the email thread to
actually scribbling in the git tree.

Add to that the fact that email clients don't maintain spatial locality
between v3->v4->v5 of a patchset and that just makes it more difficult
to stay on top of reviews as a developer, because I can't even
self-check without having to scroll through hundreds of emails.

So yeah, I guess I'll go review my reviews...  sorry for the crap.

--D

> -Eric