On Fri, Jun 05, 2020 at 07:59:17AM +1000, Dave Chinner wrote: > On Wed, Jun 03, 2020 at 08:11:56PM +0800, Gao Xiang wrote: ... > > Ok, I think we had a small misunderstanding there. I was trying to > say the asserts that were in the first patch were fine, but we > didn't really need any more because the new asserts mostly matched > an existing pattern. > > I wasn't suggesting that we do this everywhere: > > > diff --git a/fs/xfs/libxfs/xfs_ag.c b/fs/xfs/libxfs/xfs_ag.c > > index 9d84007a5c65..4b8c7cb87b84 100644 > > --- a/fs/xfs/libxfs/xfs_ag.c > > +++ b/fs/xfs/libxfs/xfs_ag.c > > @@ -563,7 +563,9 @@ xfs_ag_get_geometry( > > error = xfs_alloc_read_agf(mp, NULL, agno, 0, &agf_bp); > > if (error) > > goto out_agi; > > - pag = xfs_perag_get(mp, agno); > > + > > + pag = agi_bp->b_pag; > > + ASSERT(pag->pag_agno == agno); > > .... because we've already checked this in xfs_ialloc_read_agi() a > few lines of code back up the function. > > That's the pattern I was refering to - we tend to check > relationships when they are first brought into a context, then we > don't need to check them again in that context. Hence the asserts > in xfs_ialloc_read_agi() and xfs_alloc_read_agf() effectively cover > all the places where we pull the pag from those buffers, and so > there's no need to validate the correct perag is attached to the > buffer every time we access it.... Sorry about that, I folded in ASSERTs of my debugging code at that time. Because that is the straight way to check if somewhere has strange due to my modification, but some are unnecessary really, I didn't check that, sorry about that. I will check again and remove unneeded ASSERTs in the next version. Thanks, Gao Xiang > > Cheers, > > Dave. > -- > Dave Chinner > david@xxxxxxxxxxxxx >
