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

["Darrick J. Wong" <darrick.wong@oracle.com>]

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.

--D

> > 
> > > diff --git a/fs/xfs/libxfs/xfs_da_btree.c b/fs/xfs/libxfs/xfs_da_btree.c
> > > index 897749c41f36e..325c0ae2033d8 100644
> > > --- a/fs/xfs/libxfs/xfs_da_btree.c
> > > +++ b/fs/xfs/libxfs/xfs_da_btree.c
> > > @@ -77,11 +77,13 @@ kmem_zone_t *xfs_da_state_zone;	/* anchor for state struct zone */
> > >  /*
> > >   * Allocate a dir-state structure.
> > >   * We don't put them on the stack since they're large.
> > > + *
> > > + * We can remove this wrapper
> > >   */
> > >  xfs_da_state_t *
> > >  xfs_da_state_alloc(void)
> > >  {
> > > -	return kmem_zone_zalloc(xfs_da_state_zone, KM_NOFS);
> > > +	return kmem_cache_zalloc(xfs_da_state_zone, GFP_NOFS | __GFP_NOFAIL);
> > >  }
> > 
> > Rather than add a comment that everyone will promptly forget about,
> > add another patch to the end of the patchset that removes the
> > wrapper.
> 
> That comment was supposed to be removed before sending the patches, but looks
> like the author forgot about it.
> 
> > >  	*bpp = NULL;
> > > -	bp = kmem_zone_zalloc(xfs_buf_zone, KM_NOFS);
> > > +	bp = kmem_cache_zalloc(xfs_buf_zone, GFP_NOFS);
> > >  	if (unlikely(!bp))
> > >  		return -ENOMEM;
> > 
> > That's a change of behaviour. The existing call does not set
> > KM_MAYFAIL so this allocation will never fail, even though the code
> > is set up to handle a failure. This probably should retain
> > __GFP_NOFAIL semantics and the -ENOMEM handling removed in this
> > patch as the failure code path here has most likely never been
> > tested.
> 
> Thanks, I thought we could attempt an allocation here without NOFAIL, but the
> testability of the fail path here really didn't come to my mind.
> 
> Thanks for the comments, I"ll update the patches and submit a V2.
> 
> Cheers
> 
> -- 
> Carlos
>