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, 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. 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.... > --- > 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> -- Dave Chinner david@xxxxxxxxxxxxx
