Re: [PATCH] xfs: get rid of unnecessary xfs_perag_{get,put} pairs

[Gao Xiang <hsiangkao@redhat.com>]

On Wed, Jun 03, 2020 at 01:02:41PM +1000, Dave Chinner wrote:
> On Wed, Jun 03, 2020 at 09:40:39AM +0800, Gao Xiang wrote:
> > On Wed, Jun 03, 2020 at 11:27:34AM +1000, Dave Chinner wrote:
> > > On Tue, Jun 02, 2020 at 10:52:38PM +0800, Gao Xiang wrote:
> > > >  fs/xfs/libxfs/xfs_ag.c             |  4 ++--
> > > >  fs/xfs/libxfs/xfs_alloc.c          | 22 ++++++-----------
> > > >  fs/xfs/libxfs/xfs_alloc_btree.c    | 10 ++++----
> > > >  fs/xfs/libxfs/xfs_ialloc.c         | 28 ++++++----------------
> > > >  fs/xfs/libxfs/xfs_refcount_btree.c |  5 ++--
> > > >  fs/xfs/libxfs/xfs_rmap_btree.c     |  5 ++--
> > > >  fs/xfs/xfs_inode.c                 | 38 +++++++++---------------------
> > > >  7 files changed, 35 insertions(+), 77 deletions(-)
> > > 
> > > There were more places using this pattern than I thought. :)
> > > 
> > > With an updated commit message,
> > > 
> > > Reviewed-by: Dave Chinner <dchinner@redhat.com>
> > 
> > Thanks for your review. b.t.w, would you tend to drop all extra ASSERTs
> > or leave these ASSERTs for a while to catch potential issues on this
> > patch?...
> 
> We typically use ASSERT() statements to document assumptions the
> function implementation makes. e.g. if we expect that the inode is
> locked on entry to a function, rather than adding that as a comment
> we'll do:
> 
> 	ASSERT(xfs_isilocked(ip, XFS_ILOCK_EXCL));

Yes, that's the typical use for most filesystems.

> 
> That way our debug builds validate that all the callers of the
> function are doing the right thing.
> 
> I frequently add ASSERT()s when debugging my code, but then remove
> once I've found the issue. Typically I'm adding asserts to cover
> conditions I know shouldn't occur, but could be caused by a bug and
> I try to place the asserts to catch the issue earlier than what I'm
> currently seeing. Depending on which debug assert fires first, I'll
> change/add/remove asserts to further narrow down the problem.
> 
> Hence the ASSERTs I tend to leave in the code are either documenting
> assumptions or were the ones that were most helpful in debugging the
> changes I was making.
> 
> I did think about the asserts you added, wondering if they were
> necessary. But then I noticed they were replicating a pattern in
> other parts of the code so they seemed like a reasonable addition.

Okay... I will follow your suggestion and fold in all remaining
ASSERTs (was not in this version) about this pattern. Will sort
out the next version later...

> 
> > And in addition I will try to find more potential cases, if
> > not, I will just send out with updated commit messages (maybe without
> > iunlink orphan inode related part, just to confirm?).
> 
> Your original patch is fine including those iunlink bits. I was was
> simply pointing out that spending more time cleaning up the iunlink
> code wasn't worth spending time on because I've got much more
> substantial changes that address those issues already...

Okay...

Thanks,
Gao Xiang

> 
> Cheers,
> 
> Dave.
> -- 
> Dave Chinner
> david@fromorbit.com
>