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

[Gao Xiang <hsiangkao@redhat.com>]

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:
> > Sometimes no need to play with perag_tree since for many
> > cases perag can also be accessed by agbp reliably.
> > 
> > Signed-off-by: Gao Xiang <hsiangkao@redhat.com>
> 
> Hi Xiang,

Hi Dave,

> 
> One of the quirks of XFS is that we tend towards commit messages
> that explain the reason for the change than the actual change being
> made in the commit message. That means we'll put iinformation about
> how to reproduce bugs, the problem that needed to be solved,
> assumptions that are being made, etc into the commit message rather
> than describe what the change being made is. We can see what the
> change is from the code, but we may not be able to understand why
> the change is being made from reading the code.
> 
> Hence we try to put the "why?" into the commit message so that
> everyone reviewing the code knows this information without having to
> ask. This also means that we capture the reasons/thinking/issues
> that the commit address in the code repository and hence when we
> look up a change (e.g. when considering if we need to back port it
> to another kernel), we have a good idea of what problem that change
> is addressing. It also means that in a few months/years time when
> we've forgotten exactly why a specific change was made, the commit
> message should contain enough detail to remind us.

Okay, I understood. I'm a newbie to XFS here. I'll try to add more
reasons/thoughts from now on with my current limited knowledage
about XFS.

> 
> Perhaps something like this?
> 
> 	In the course of some operations, we look up the perag from
> 	the mount multiple times to get or change perag information.
> 	These are often very short pieces of code, so while the
> 	lookup cost is generally low, the cost of the lookup is far
> 	higher than the cost of the operation we are doing on the
> 	perag.
> 
> 	Since we changed buffers to hold references to the perag
> 	they are cached in, many modification contexts already hold
> 	active references to the perag that are held across these
> 	operations. This is especially true for any operation that
> 	is serialised by an allocation group header buffer.
> 
> 	In these cases, we can just use the buffer's reference to
> 	the perag to avoid needing to do lookups to access the
> 	perag. This means that many operations don't need to do
> 	perag lookups at all to access the perag because they've
> 	already looked up objects that own persistent references
> 	and hence can use that reference instead.
> 
> The first paragraph explains the problem. The second paragraph
> explains the underlying assumption the change depends on. And the
> third paragraph defines the scope we can apply the general pattern
> to.
> 
> It takes a while to get used to doing this - for any major change I
> tend to write the series description first (the requirements and
> design doc), then for each patch I write the commit message before
> I start modifying the code (detailed design). Treating the commit
> messages as design documentation really helps other people
> understand the changes being made....

Yeah, I saw many patchsets of you.. Partially due to my limited
knowledge and somewhat limited English skill though... But I will
write more as much as possible to get myself better...

> 
> > ---
> > Not sure addressing all the cases, but seems mostly.
> > Kindly correct me if something wrong somewhere...
> > 
> >  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?... 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?).

Thanks,
Gao Xiang

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