Re: [PATCH 2/4] xfs: Remove kmem_zone_zalloc() usage

[Dave Chinner <david@fromorbit.com>]

On Thu, Jul 09, 2020 at 09:42:29AM -0700, Darrick J. Wong wrote:
> On Thu, Jul 09, 2020 at 10:55:00AM +0200, Carlos Maiolino wrote:
> > On Thu, Jul 09, 2020 at 12:55:23PM +1000, Dave Chinner wrote:
> > > On Wed, Jul 08, 2020 at 02:56:06PM +0200, Carlos Maiolino wrote:
> > > > Use kmem_cache_zalloc() directly.
> > > > 
> > > > With the exception of xlog_ticket_alloc() which will be dealt on the
> > > > next patch for readability.
> > > > 
> > > > Most users of kmem_zone_zalloc() were converted to either
> > > > "GFP_KERNEL | __GFP_NOFAIL" or "GFP_NOFS | __GFP_NOFAIL", with the
> > > > exception of _xfs_buf_alloc(), which is allowed to fail, so __GFP_NOFAIL
> > > > is not used there.
> > > > 
> > > > Signed-off-by: Carlos Maiolino <cmaiolino@redhat.com>
> > > > ---
> > > >  fs/xfs/libxfs/xfs_alloc_btree.c    | 3 ++-
> > > >  fs/xfs/libxfs/xfs_bmap.c           | 5 ++++-
> > > >  fs/xfs/libxfs/xfs_bmap_btree.c     | 3 ++-
> > > >  fs/xfs/libxfs/xfs_da_btree.c       | 4 +++-
> > > >  fs/xfs/libxfs/xfs_ialloc_btree.c   | 2 +-
> > > >  fs/xfs/libxfs/xfs_inode_fork.c     | 6 +++---
> > > >  fs/xfs/libxfs/xfs_refcount_btree.c | 2 +-
> > > >  fs/xfs/libxfs/xfs_rmap_btree.c     | 2 +-
> > > >  fs/xfs/xfs_bmap_item.c             | 4 ++--
> > > >  fs/xfs/xfs_buf.c                   | 2 +-
> > > >  fs/xfs/xfs_buf_item.c              | 2 +-
> > > >  fs/xfs/xfs_dquot.c                 | 2 +-
> > > >  fs/xfs/xfs_extfree_item.c          | 6 ++++--
> > > >  fs/xfs/xfs_icreate_item.c          | 2 +-
> > > >  fs/xfs/xfs_inode_item.c            | 3 ++-
> > > >  fs/xfs/xfs_refcount_item.c         | 5 +++--
> > > >  fs/xfs/xfs_rmap_item.c             | 6 ++++--
> > > >  fs/xfs/xfs_trans.c                 | 5 +++--
> > > >  fs/xfs/xfs_trans_dquot.c           | 3 ++-
> > > >  19 files changed, 41 insertions(+), 26 deletions(-)
> > > > 
> > > > diff --git a/fs/xfs/libxfs/xfs_alloc_btree.c b/fs/xfs/libxfs/xfs_alloc_btree.c
> > > > index 60c453cb3ee37..9cc1a4af40180 100644
> > > > --- a/fs/xfs/libxfs/xfs_alloc_btree.c
> > > > +++ b/fs/xfs/libxfs/xfs_alloc_btree.c
> > > > @@ -484,7 +484,8 @@ xfs_allocbt_init_common(
> > > >  
> > > >  	ASSERT(btnum == XFS_BTNUM_BNO || btnum == XFS_BTNUM_CNT);
> > > >  
> > > > -	cur = kmem_zone_zalloc(xfs_btree_cur_zone, KM_NOFS);
> > > > +	cur = kmem_cache_zalloc(xfs_btree_cur_zone,
> > > > +				GFP_NOFS | __GFP_NOFAIL);
> > > 
> > > This still fits on one line....
> > > 
> > > Hmmm - many of the other conversions are similar, but not all of
> > > them. Any particular reason why these are split over multiple lines
> > > and not kept as a single line of code? My preference is that they
> > > are a single line if it doesn't overrun 80 columns....
> > 
> > Hmmm, I have my vim set to warn me on 80 column limit, and it warned me here (or
> > maybe I just went in auto mode), I'll double check it, thanks.
> 
> That was increased to 100 lines as of 5.7.0.

Please don't. Leave things at 80 columns in XFS as that's where all
the code is right now. Single random long lines is not going to
improve things, just introduce inconsistency and line wrapping,
especially when it comes to 80 column terminals used for email and
patch review....

IMO, when we stop saying "wrap commits and email at 68-72 columns"
then maybe we can think about longer lines in the code we write...

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com