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@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?... 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@xxxxxxxxxxxxx
