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

[Carlos Maiolino <cmaiolino@redhat.com>]

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.

> 
> > 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