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