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

[Dave Chinner <david@fromorbit.com>]

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));

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.

> 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...

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com