On Wed, Jun 03, 2020 at 01:02:41PM +1000, Dave Chinner wrote: > 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)); Yes, that's the typical use for most filesystems. > > 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. Okay... I will follow your suggestion and fold in all remaining ASSERTs (was not in this version) about this pattern. Will sort out the next version later... > > > 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... Okay... Thanks, Gao Xiang > > Cheers, > > Dave. > -- > Dave Chinner > david@xxxxxxxxxxxxx >
