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@xxxxxxxxxx> > > 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@xxxxxxxxxx> 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@xxxxxxxxxxxxx >
