* [PATCH 0/4] Continue xfs kmem cleanup
@ 2020-07-08 12:56 Carlos Maiolino
2020-07-08 12:56 ` [PATCH 1/4] xfs: Remove kmem_zone_alloc() usage Carlos Maiolino
` (3 more replies)
0 siblings, 4 replies; 12+ messages in thread
From: Carlos Maiolino @ 2020-07-08 12:56 UTC (permalink / raw)
To: linux-xfs
Hi,
a while ago I started to cleanup the kmem helpers we have, and use kernel's MM
API. The discussion has stalled because I've got caugh on other stuff, and I'm
trying to continue that cleanup.
The following series basically removes kmem_zone_alloc() and kmem_zone_zalloc(),
replacing them by kmem_cache_{alloc,zalloc}.
It uses __GFP_NOFAIL where we are not allowed to fail, to replicate the behavior
of kmem_zone_alloc().
Patches have been tested with xfstests, on a 1GiG and a 64GiB RAM systems to
check the patches under memory pressure.
The patches are good as-is, but my main point is to revive the topic, and I
though it would be better to do it with the patches. Which is another reason I
decided to split the series in a few more patches than would be required, I
thought it would be better segmenting the changes in the way I did.
Comments?
Cheers
fs/xfs/kmem.c | 21 ---------------------
fs/xfs/kmem.h | 8 --------
fs/xfs/libxfs/xfs_alloc.c | 3 ++-
fs/xfs/libxfs/xfs_alloc_btree.c | 3 ++-
fs/xfs/libxfs/xfs_bmap.c | 8 ++++++--
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_icache.c | 11 +++--------
fs/xfs/xfs_icreate_item.c | 2 +-
fs/xfs/xfs_inode_item.c | 3 ++-
fs/xfs/xfs_log.c | 7 ++++---
fs/xfs/xfs_log_cil.c | 2 +-
fs/xfs/xfs_log_priv.h | 2 +-
fs/xfs/xfs_refcount_item.c | 5 +++--
fs/xfs/xfs_rmap_item.c | 6 ++++--
fs/xfs/xfs_trace.h | 1 -
fs/xfs/xfs_trans.c | 5 +++--
fs/xfs/xfs_trans_dquot.c | 3 ++-
27 files changed, 54 insertions(+), 71 deletions(-)
--
2.26.2
^ permalink raw reply [flat|nested] 12+ messages in thread* [PATCH 1/4] xfs: Remove kmem_zone_alloc() usage 2020-07-08 12:56 [PATCH 0/4] Continue xfs kmem cleanup Carlos Maiolino @ 2020-07-08 12:56 ` Carlos Maiolino 2020-07-09 2:45 ` Dave Chinner 2020-07-08 12:56 ` [PATCH 2/4] xfs: Remove kmem_zone_zalloc() usage Carlos Maiolino ` (2 subsequent siblings) 3 siblings, 1 reply; 12+ messages in thread From: Carlos Maiolino @ 2020-07-08 12:56 UTC (permalink / raw) To: linux-xfs Use kmem_cache_alloc() directly. All kmem_zone_alloc() users pass 0 as flags, which are translated into: GFP_KERNEL | __GFP_NOWARN, and kmem_zone_alloc() loops forever until the allocation succeeds. So, call kmem_cache_alloc() with __GFP_NOFAIL directly. which will have the same result. Once allocation will never fail, don't bother to add __GFP_NOWARN. Signed-off-by: Carlos Maiolino <cmaiolino@redhat.com> --- fs/xfs/libxfs/xfs_alloc.c | 3 ++- fs/xfs/libxfs/xfs_bmap.c | 3 ++- fs/xfs/xfs_icache.c | 11 +++-------- 3 files changed, 7 insertions(+), 10 deletions(-) diff --git a/fs/xfs/libxfs/xfs_alloc.c b/fs/xfs/libxfs/xfs_alloc.c index 203e74fa64aa6..583242253c027 100644 --- a/fs/xfs/libxfs/xfs_alloc.c +++ b/fs/xfs/libxfs/xfs_alloc.c @@ -2467,7 +2467,8 @@ xfs_defer_agfl_block( ASSERT(xfs_bmap_free_item_zone != NULL); ASSERT(oinfo != NULL); - new = kmem_zone_alloc(xfs_bmap_free_item_zone, 0); + new = kmem_cache_alloc(xfs_bmap_free_item_zone, + GFP_KERNEL | __GFP_NOFAIL); new->xefi_startblock = XFS_AGB_TO_FSB(mp, agno, agbno); new->xefi_blockcount = 1; new->xefi_oinfo = *oinfo; diff --git a/fs/xfs/libxfs/xfs_bmap.c b/fs/xfs/libxfs/xfs_bmap.c index 667cdd0dfdf4a..fd5c0d669d0d7 100644 --- a/fs/xfs/libxfs/xfs_bmap.c +++ b/fs/xfs/libxfs/xfs_bmap.c @@ -553,7 +553,8 @@ __xfs_bmap_add_free( #endif ASSERT(xfs_bmap_free_item_zone != NULL); - new = kmem_zone_alloc(xfs_bmap_free_item_zone, 0); + new = kmem_cache_alloc(xfs_bmap_free_item_zone, + GFP_KERNEL | __GFP_NOFAIL); new->xefi_startblock = bno; new->xefi_blockcount = (xfs_extlen_t)len; if (oinfo) diff --git a/fs/xfs/xfs_icache.c b/fs/xfs/xfs_icache.c index 5daef654956cb..69f887abe4334 100644 --- a/fs/xfs/xfs_icache.c +++ b/fs/xfs/xfs_icache.c @@ -36,14 +36,9 @@ xfs_inode_alloc( { struct xfs_inode *ip; - /* - * if this didn't occur in transactions, we could use - * KM_MAYFAIL and return NULL here on ENOMEM. Set the - * code up to do this anyway. - */ - ip = kmem_zone_alloc(xfs_inode_zone, 0); - if (!ip) - return NULL; + ip = kmem_cache_alloc(xfs_inode_zone, + GFP_KERNEL | __GFP_NOFAIL); + if (inode_init_always(mp->m_super, VFS_I(ip))) { kmem_cache_free(xfs_inode_zone, ip); return NULL; -- 2.26.2 ^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [PATCH 1/4] xfs: Remove kmem_zone_alloc() usage 2020-07-08 12:56 ` [PATCH 1/4] xfs: Remove kmem_zone_alloc() usage Carlos Maiolino @ 2020-07-09 2:45 ` Dave Chinner 0 siblings, 0 replies; 12+ messages in thread From: Dave Chinner @ 2020-07-09 2:45 UTC (permalink / raw) To: Carlos Maiolino; +Cc: linux-xfs On Wed, Jul 08, 2020 at 02:56:05PM +0200, Carlos Maiolino wrote: > Use kmem_cache_alloc() directly. > > All kmem_zone_alloc() users pass 0 as flags, which are translated into: > GFP_KERNEL | __GFP_NOWARN, and kmem_zone_alloc() loops forever until the > allocation succeeds. > > So, call kmem_cache_alloc() with __GFP_NOFAIL directly. which will have > the same result. > > Once allocation will never fail, don't bother to add __GFP_NOWARN. Last two paragraphs are a little odd. Maybe: We can use __GFP_NOFAIL to tell the allocator to loop forever rather than doing it ourself, and because the allocation will never fail we do not need to use __GFP_NOWARN anymore. Hence all callers can be converted to use GFP_KERNEL | __GFP_NOFAIL. > Signed-off-by: Carlos Maiolino <cmaiolino@redhat.com> > --- > fs/xfs/libxfs/xfs_alloc.c | 3 ++- > fs/xfs/libxfs/xfs_bmap.c | 3 ++- > fs/xfs/xfs_icache.c | 11 +++-------- > 3 files changed, 7 insertions(+), 10 deletions(-) > > @@ -36,14 +36,9 @@ xfs_inode_alloc( > { > struct xfs_inode *ip; > > - /* > - * if this didn't occur in transactions, we could use > - * KM_MAYFAIL and return NULL here on ENOMEM. Set the > - * code up to do this anyway. > - */ > - ip = kmem_zone_alloc(xfs_inode_zone, 0); > - if (!ip) > - return NULL; > + ip = kmem_cache_alloc(xfs_inode_zone, > + GFP_KERNEL | __GFP_NOFAIL); > + Hmmmm. We really should check PF_FSTRANS here for the flags we should be setting. Something like: gfp_t gfp_mask = GFP_KERNEL; if (current->flags & PF_FSTRANS) gfp_mask |= __GFP_NOFAIL; ip = kmem_cache_alloc(xfs_inode_zone, gfp_mask); if (!ip) return NULL; Cheers, Dave. -- Dave Chinner david@fromorbit.com ^ permalink raw reply [flat|nested] 12+ messages in thread
* [PATCH 2/4] xfs: Remove kmem_zone_zalloc() usage 2020-07-08 12:56 [PATCH 0/4] Continue xfs kmem cleanup Carlos Maiolino 2020-07-08 12:56 ` [PATCH 1/4] xfs: Remove kmem_zone_alloc() usage Carlos Maiolino @ 2020-07-08 12:56 ` Carlos Maiolino 2020-07-09 2:55 ` Dave Chinner 2020-07-08 12:56 ` [PATCH 3/4] xfs: Modify xlog_ticket_alloc() to use kernel's MM API Carlos Maiolino 2020-07-08 12:56 ` [PATCH 4/4] xfs: remove xfs_zone_{alloc,zalloc} helpers Carlos Maiolino 3 siblings, 1 reply; 12+ messages in thread From: Carlos Maiolino @ 2020-07-08 12:56 UTC (permalink / raw) To: linux-xfs 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); cur->bc_tp = tp; cur->bc_mp = mp; diff --git a/fs/xfs/libxfs/xfs_bmap.c b/fs/xfs/libxfs/xfs_bmap.c index fd5c0d669d0d7..9c40d59710357 100644 --- a/fs/xfs/libxfs/xfs_bmap.c +++ b/fs/xfs/libxfs/xfs_bmap.c @@ -1099,7 +1099,10 @@ xfs_bmap_add_attrfork( if (error) goto trans_cancel; ASSERT(ip->i_afp == NULL); - ip->i_afp = kmem_zone_zalloc(xfs_ifork_zone, 0); + + ip->i_afp = kmem_cache_zalloc(xfs_ifork_zone, + GFP_KERNEL | __GFP_NOFAIL); + ip->i_afp->if_format = XFS_DINODE_FMT_EXTENTS; ip->i_afp->if_flags = XFS_IFEXTENTS; logflags = 0; diff --git a/fs/xfs/libxfs/xfs_bmap_btree.c b/fs/xfs/libxfs/xfs_bmap_btree.c index d9c63f17d2dec..6c23ce7ec1101 100644 --- a/fs/xfs/libxfs/xfs_bmap_btree.c +++ b/fs/xfs/libxfs/xfs_bmap_btree.c @@ -552,7 +552,8 @@ xfs_bmbt_init_cursor( struct xfs_btree_cur *cur; ASSERT(whichfork != XFS_COW_FORK); - cur = kmem_zone_zalloc(xfs_btree_cur_zone, KM_NOFS); + cur = kmem_cache_zalloc(xfs_btree_cur_zone, + GFP_NOFS | __GFP_NOFAIL); cur->bc_tp = tp; cur->bc_mp = mp; 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); } /* diff --git a/fs/xfs/libxfs/xfs_ialloc_btree.c b/fs/xfs/libxfs/xfs_ialloc_btree.c index b2c122ad8f0e9..3c8aebc36e643 100644 --- a/fs/xfs/libxfs/xfs_ialloc_btree.c +++ b/fs/xfs/libxfs/xfs_ialloc_btree.c @@ -411,7 +411,7 @@ xfs_inobt_init_common( { struct xfs_btree_cur *cur; - cur = kmem_zone_zalloc(xfs_btree_cur_zone, KM_NOFS); + cur = kmem_cache_zalloc(xfs_btree_cur_zone, GFP_NOFS | __GFP_NOFAIL); cur->bc_tp = tp; cur->bc_mp = mp; cur->bc_btnum = btnum; diff --git a/fs/xfs/libxfs/xfs_inode_fork.c b/fs/xfs/libxfs/xfs_inode_fork.c index 28b366275ae0e..0cf853d42d622 100644 --- a/fs/xfs/libxfs/xfs_inode_fork.c +++ b/fs/xfs/libxfs/xfs_inode_fork.c @@ -291,7 +291,7 @@ xfs_iformat_attr_fork( * Initialize the extent count early, as the per-format routines may * depend on it. */ - ip->i_afp = kmem_zone_zalloc(xfs_ifork_zone, KM_NOFS); + ip->i_afp = kmem_cache_zalloc(xfs_ifork_zone, GFP_NOFS | __GFP_NOFAIL); ip->i_afp->if_format = dip->di_aformat; if (unlikely(ip->i_afp->if_format == 0)) /* pre IRIX 6.2 file system */ ip->i_afp->if_format = XFS_DINODE_FMT_EXTENTS; @@ -673,8 +673,8 @@ xfs_ifork_init_cow( if (ip->i_cowfp) return; - ip->i_cowfp = kmem_zone_zalloc(xfs_ifork_zone, - KM_NOFS); + ip->i_cowfp = kmem_cache_zalloc(xfs_ifork_zone, + GFP_NOFS | __GFP_NOFAIL); ip->i_cowfp->if_flags = XFS_IFEXTENTS; ip->i_cowfp->if_format = XFS_DINODE_FMT_EXTENTS; } diff --git a/fs/xfs/libxfs/xfs_refcount_btree.c b/fs/xfs/libxfs/xfs_refcount_btree.c index 7fd6044a4f780..fde21b5e82ed0 100644 --- a/fs/xfs/libxfs/xfs_refcount_btree.c +++ b/fs/xfs/libxfs/xfs_refcount_btree.c @@ -325,7 +325,7 @@ xfs_refcountbt_init_common( ASSERT(agno != NULLAGNUMBER); ASSERT(agno < mp->m_sb.sb_agcount); - cur = kmem_zone_zalloc(xfs_btree_cur_zone, KM_NOFS); + cur = kmem_cache_zalloc(xfs_btree_cur_zone, GFP_NOFS | __GFP_NOFAIL); cur->bc_tp = tp; cur->bc_mp = mp; cur->bc_btnum = XFS_BTNUM_REFC; diff --git a/fs/xfs/libxfs/xfs_rmap_btree.c b/fs/xfs/libxfs/xfs_rmap_btree.c index b7c05314d07c9..caf0d799c1f42 100644 --- a/fs/xfs/libxfs/xfs_rmap_btree.c +++ b/fs/xfs/libxfs/xfs_rmap_btree.c @@ -457,7 +457,7 @@ xfs_rmapbt_init_common( { struct xfs_btree_cur *cur; - cur = kmem_zone_zalloc(xfs_btree_cur_zone, KM_NOFS); + cur = kmem_cache_zalloc(xfs_btree_cur_zone, GFP_NOFS | __GFP_NOFAIL); cur->bc_tp = tp; cur->bc_mp = mp; /* Overlapping btree; 2 keys per pointer. */ diff --git a/fs/xfs/xfs_bmap_item.c b/fs/xfs/xfs_bmap_item.c index 6736c5ab188f2..ec3691372e7c0 100644 --- a/fs/xfs/xfs_bmap_item.c +++ b/fs/xfs/xfs_bmap_item.c @@ -138,7 +138,7 @@ xfs_bui_init( { struct xfs_bui_log_item *buip; - buip = kmem_zone_zalloc(xfs_bui_zone, 0); + buip = kmem_cache_zalloc(xfs_bui_zone, GFP_KERNEL | __GFP_NOFAIL); xfs_log_item_init(mp, &buip->bui_item, XFS_LI_BUI, &xfs_bui_item_ops); buip->bui_format.bui_nextents = XFS_BUI_MAX_FAST_EXTENTS; @@ -215,7 +215,7 @@ xfs_trans_get_bud( { struct xfs_bud_log_item *budp; - budp = kmem_zone_zalloc(xfs_bud_zone, 0); + budp = kmem_cache_zalloc(xfs_bud_zone, GFP_KERNEL | __GFP_NOFAIL); xfs_log_item_init(tp->t_mountp, &budp->bud_item, XFS_LI_BUD, &xfs_bud_item_ops); budp->bud_buip = buip; diff --git a/fs/xfs/xfs_buf.c b/fs/xfs/xfs_buf.c index 20b748f7e1862..0ccd0a3c840af 100644 --- a/fs/xfs/xfs_buf.c +++ b/fs/xfs/xfs_buf.c @@ -211,7 +211,7 @@ _xfs_buf_alloc( int i; *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; diff --git a/fs/xfs/xfs_buf_item.c b/fs/xfs/xfs_buf_item.c index 9e75e8d6042ec..ae226292bf903 100644 --- a/fs/xfs/xfs_buf_item.c +++ b/fs/xfs/xfs_buf_item.c @@ -734,7 +734,7 @@ xfs_buf_item_init( return 0; } - bip = kmem_zone_zalloc(xfs_buf_item_zone, 0); + bip = kmem_cache_zalloc(xfs_buf_item_zone, GFP_KERNEL | __GFP_NOFAIL); xfs_log_item_init(mp, &bip->bli_item, XFS_LI_BUF, &xfs_buf_item_ops); bip->bli_buf = bp; diff --git a/fs/xfs/xfs_dquot.c b/fs/xfs/xfs_dquot.c index d5b7f03e93c8d..1be11f51afb56 100644 --- a/fs/xfs/xfs_dquot.c +++ b/fs/xfs/xfs_dquot.c @@ -475,7 +475,7 @@ xfs_dquot_alloc( { struct xfs_dquot *dqp; - dqp = kmem_zone_zalloc(xfs_qm_dqzone, 0); + dqp = kmem_cache_zalloc(xfs_qm_dqzone, GFP_KERNEL | __GFP_NOFAIL); dqp->dq_flags = type; dqp->q_core.d_id = cpu_to_be32(id); diff --git a/fs/xfs/xfs_extfree_item.c b/fs/xfs/xfs_extfree_item.c index b9c333bae0a12..6cb8cd11072a3 100644 --- a/fs/xfs/xfs_extfree_item.c +++ b/fs/xfs/xfs_extfree_item.c @@ -161,7 +161,8 @@ xfs_efi_init( ((nextents - 1) * sizeof(xfs_extent_t))); efip = kmem_zalloc(size, 0); } else { - efip = kmem_zone_zalloc(xfs_efi_zone, 0); + efip = kmem_cache_zalloc(xfs_efi_zone, + GFP_KERNEL | __GFP_NOFAIL); } xfs_log_item_init(mp, &efip->efi_item, XFS_LI_EFI, &xfs_efi_item_ops); @@ -332,7 +333,8 @@ xfs_trans_get_efd( (nextents - 1) * sizeof(struct xfs_extent), 0); } else { - efdp = kmem_zone_zalloc(xfs_efd_zone, 0); + efdp = kmem_cache_zalloc(xfs_efd_zone, + GFP_KERNEL | __GFP_NOFAIL); } xfs_log_item_init(tp->t_mountp, &efdp->efd_item, XFS_LI_EFD, diff --git a/fs/xfs/xfs_icreate_item.c b/fs/xfs/xfs_icreate_item.c index 287a9e5c7d758..9b3994b9c716d 100644 --- a/fs/xfs/xfs_icreate_item.c +++ b/fs/xfs/xfs_icreate_item.c @@ -97,7 +97,7 @@ xfs_icreate_log( { struct xfs_icreate_item *icp; - icp = kmem_zone_zalloc(xfs_icreate_zone, 0); + icp = kmem_cache_zalloc(xfs_icreate_zone, GFP_KERNEL | __GFP_NOFAIL); xfs_log_item_init(tp->t_mountp, &icp->ic_item, XFS_LI_ICREATE, &xfs_icreate_item_ops); diff --git a/fs/xfs/xfs_inode_item.c b/fs/xfs/xfs_inode_item.c index ba47bf65b772b..972a26d056576 100644 --- a/fs/xfs/xfs_inode_item.c +++ b/fs/xfs/xfs_inode_item.c @@ -636,7 +636,8 @@ xfs_inode_item_init( struct xfs_inode_log_item *iip; ASSERT(ip->i_itemp == NULL); - iip = ip->i_itemp = kmem_zone_zalloc(xfs_ili_zone, 0); + iip = ip->i_itemp = kmem_cache_zalloc(xfs_ili_zone, + GFP_KERNEL | __GFP_NOFAIL); iip->ili_inode = ip; xfs_log_item_init(mp, &iip->ili_item, XFS_LI_INODE, diff --git a/fs/xfs/xfs_refcount_item.c b/fs/xfs/xfs_refcount_item.c index c81639891e298..7b2c72bc28582 100644 --- a/fs/xfs/xfs_refcount_item.c +++ b/fs/xfs/xfs_refcount_item.c @@ -143,7 +143,8 @@ xfs_cui_init( cuip = kmem_zalloc(xfs_cui_log_item_sizeof(nextents), 0); else - cuip = kmem_zone_zalloc(xfs_cui_zone, 0); + cuip = kmem_cache_zalloc(xfs_cui_zone, + GFP_KERNEL | __GFP_NOFAIL); xfs_log_item_init(mp, &cuip->cui_item, XFS_LI_CUI, &xfs_cui_item_ops); cuip->cui_format.cui_nextents = nextents; @@ -220,7 +221,7 @@ xfs_trans_get_cud( { struct xfs_cud_log_item *cudp; - cudp = kmem_zone_zalloc(xfs_cud_zone, 0); + cudp = kmem_cache_zalloc(xfs_cud_zone, GFP_KERNEL | __GFP_NOFAIL); xfs_log_item_init(tp->t_mountp, &cudp->cud_item, XFS_LI_CUD, &xfs_cud_item_ops); cudp->cud_cuip = cuip; diff --git a/fs/xfs/xfs_rmap_item.c b/fs/xfs/xfs_rmap_item.c index a86599db20a6f..78e24fe835aca 100644 --- a/fs/xfs/xfs_rmap_item.c +++ b/fs/xfs/xfs_rmap_item.c @@ -141,7 +141,8 @@ xfs_rui_init( if (nextents > XFS_RUI_MAX_FAST_EXTENTS) ruip = kmem_zalloc(xfs_rui_log_item_sizeof(nextents), 0); else - ruip = kmem_zone_zalloc(xfs_rui_zone, 0); + ruip = kmem_cache_zalloc(xfs_rui_zone, + GFP_KERNEL | __GFP_NOFAIL); xfs_log_item_init(mp, &ruip->rui_item, XFS_LI_RUI, &xfs_rui_item_ops); ruip->rui_format.rui_nextents = nextents; @@ -243,7 +244,8 @@ xfs_trans_get_rud( { struct xfs_rud_log_item *rudp; - rudp = kmem_zone_zalloc(xfs_rud_zone, 0); + rudp = kmem_cache_zalloc(xfs_rud_zone, + GFP_KERNEL | __GFP_NOFAIL); xfs_log_item_init(tp->t_mountp, &rudp->rud_item, XFS_LI_RUD, &xfs_rud_item_ops); rudp->rud_ruip = ruip; diff --git a/fs/xfs/xfs_trans.c b/fs/xfs/xfs_trans.c index 3c94e5ff43160..24d241a240147 100644 --- a/fs/xfs/xfs_trans.c +++ b/fs/xfs/xfs_trans.c @@ -90,7 +90,8 @@ xfs_trans_dup( trace_xfs_trans_dup(tp, _RET_IP_); - ntp = kmem_zone_zalloc(xfs_trans_zone, 0); + ntp = kmem_cache_zalloc(xfs_trans_zone, + GFP_KERNEL | __GFP_NOFAIL); /* * Initialize the new transaction structure. @@ -262,7 +263,7 @@ xfs_trans_alloc( * GFP_NOFS allocation context so that we avoid lockdep false positives * by doing GFP_KERNEL allocations inside sb_start_intwrite(). */ - tp = kmem_zone_zalloc(xfs_trans_zone, 0); + tp = kmem_cache_zalloc(xfs_trans_zone, GFP_KERNEL | __GFP_NOFAIL); if (!(flags & XFS_TRANS_NO_WRITECOUNT)) sb_start_intwrite(mp->m_super); diff --git a/fs/xfs/xfs_trans_dquot.c b/fs/xfs/xfs_trans_dquot.c index c0f73b82c0551..394d6a0aa18dc 100644 --- a/fs/xfs/xfs_trans_dquot.c +++ b/fs/xfs/xfs_trans_dquot.c @@ -860,7 +860,8 @@ STATIC void xfs_trans_alloc_dqinfo( xfs_trans_t *tp) { - tp->t_dqinfo = kmem_zone_zalloc(xfs_qm_dqtrxzone, 0); + tp->t_dqinfo = kmem_cache_zalloc(xfs_qm_dqtrxzone, + GFP_KERNEL | __GFP_NOFAIL); } void -- 2.26.2 ^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [PATCH 2/4] xfs: Remove kmem_zone_zalloc() usage 2020-07-08 12:56 ` [PATCH 2/4] xfs: Remove kmem_zone_zalloc() usage Carlos Maiolino @ 2020-07-09 2:55 ` Dave Chinner 2020-07-09 8:55 ` Carlos Maiolino 0 siblings, 1 reply; 12+ messages in thread From: Dave Chinner @ 2020-07-09 2:55 UTC (permalink / raw) To: Carlos Maiolino; +Cc: linux-xfs 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.... > 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. > diff --git a/fs/xfs/xfs_buf.c b/fs/xfs/xfs_buf.c > index 20b748f7e1862..0ccd0a3c840af 100644 > --- a/fs/xfs/xfs_buf.c > +++ b/fs/xfs/xfs_buf.c > @@ -211,7 +211,7 @@ _xfs_buf_alloc( > int i; > > *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. Cheers, Dave. -- Dave Chinner david@fromorbit.com ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 2/4] xfs: Remove kmem_zone_zalloc() usage 2020-07-09 2:55 ` Dave Chinner @ 2020-07-09 8:55 ` Carlos Maiolino 2020-07-09 16:42 ` Darrick J. Wong 0 siblings, 1 reply; 12+ messages in thread From: Carlos Maiolino @ 2020-07-09 8:55 UTC (permalink / raw) To: Dave Chinner; +Cc: linux-xfs 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 ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 2/4] xfs: Remove kmem_zone_zalloc() usage 2020-07-09 8:55 ` Carlos Maiolino @ 2020-07-09 16:42 ` Darrick J. Wong 2020-07-09 21:52 ` Dave Chinner 0 siblings, 1 reply; 12+ messages in thread From: Darrick J. Wong @ 2020-07-09 16:42 UTC (permalink / raw) To: Dave Chinner, linux-xfs 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 > ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 2/4] xfs: Remove kmem_zone_zalloc() usage 2020-07-09 16:42 ` Darrick J. Wong @ 2020-07-09 21:52 ` Dave Chinner 0 siblings, 0 replies; 12+ messages in thread From: Dave Chinner @ 2020-07-09 21:52 UTC (permalink / raw) To: Darrick J. Wong; +Cc: linux-xfs 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 ^ permalink raw reply [flat|nested] 12+ messages in thread
* [PATCH 3/4] xfs: Modify xlog_ticket_alloc() to use kernel's MM API 2020-07-08 12:56 [PATCH 0/4] Continue xfs kmem cleanup Carlos Maiolino 2020-07-08 12:56 ` [PATCH 1/4] xfs: Remove kmem_zone_alloc() usage Carlos Maiolino 2020-07-08 12:56 ` [PATCH 2/4] xfs: Remove kmem_zone_zalloc() usage Carlos Maiolino @ 2020-07-08 12:56 ` Carlos Maiolino 2020-07-09 3:00 ` Dave Chinner 2020-07-08 12:56 ` [PATCH 4/4] xfs: remove xfs_zone_{alloc,zalloc} helpers Carlos Maiolino 3 siblings, 1 reply; 12+ messages in thread From: Carlos Maiolino @ 2020-07-08 12:56 UTC (permalink / raw) To: linux-xfs change xlog_ticket_alloc() to use default kmem_cache_zalloc(), and modify its callers to pass MM flags as arguments. Signed-off-by: Carlos Maiolino <cmaiolino@redhat.com> --- fs/xfs/xfs_log.c | 7 ++++--- fs/xfs/xfs_log_cil.c | 2 +- fs/xfs/xfs_log_priv.h | 2 +- 3 files changed, 6 insertions(+), 5 deletions(-) diff --git a/fs/xfs/xfs_log.c b/fs/xfs/xfs_log.c index 00fda2e8e7380..6d40d479e34a1 100644 --- a/fs/xfs/xfs_log.c +++ b/fs/xfs/xfs_log.c @@ -433,7 +433,8 @@ xfs_log_reserve( XFS_STATS_INC(mp, xs_try_logspace); ASSERT(*ticp == NULL); - tic = xlog_ticket_alloc(log, unit_bytes, cnt, client, permanent, 0); + tic = xlog_ticket_alloc(log, unit_bytes, cnt, client, permanent, + (GFP_KERNEL | __GFP_NOFAIL)); *ticp = tic; xlog_grant_push_ail(log, tic->t_cnt ? tic->t_unit_res * tic->t_cnt @@ -3409,12 +3410,12 @@ xlog_ticket_alloc( int cnt, char client, bool permanent, - xfs_km_flags_t alloc_flags) + gfp_t alloc_flags) { struct xlog_ticket *tic; int unit_res; - tic = kmem_zone_zalloc(xfs_log_ticket_zone, alloc_flags); + tic = kmem_cache_zalloc(xfs_log_ticket_zone, alloc_flags); if (!tic) return NULL; diff --git a/fs/xfs/xfs_log_cil.c b/fs/xfs/xfs_log_cil.c index 9ed90368ab311..636c53a62cd3f 100644 --- a/fs/xfs/xfs_log_cil.c +++ b/fs/xfs/xfs_log_cil.c @@ -38,7 +38,7 @@ xlog_cil_ticket_alloc( struct xlog_ticket *tic; tic = xlog_ticket_alloc(log, 0, 1, XFS_TRANSACTION, 0, - KM_NOFS); + (GFP_NOFS | __GFP_NOFAIL)); /* * set the current reservation to zero so we know to steal the basic diff --git a/fs/xfs/xfs_log_priv.h b/fs/xfs/xfs_log_priv.h index 75a62870b63af..039b5cf6e692e 100644 --- a/fs/xfs/xfs_log_priv.h +++ b/fs/xfs/xfs_log_priv.h @@ -465,7 +465,7 @@ xlog_ticket_alloc( int count, char client, bool permanent, - xfs_km_flags_t alloc_flags); + gfp_t alloc_flags); static inline void -- 2.26.2 ^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [PATCH 3/4] xfs: Modify xlog_ticket_alloc() to use kernel's MM API 2020-07-08 12:56 ` [PATCH 3/4] xfs: Modify xlog_ticket_alloc() to use kernel's MM API Carlos Maiolino @ 2020-07-09 3:00 ` Dave Chinner 0 siblings, 0 replies; 12+ messages in thread From: Dave Chinner @ 2020-07-09 3:00 UTC (permalink / raw) To: Carlos Maiolino; +Cc: linux-xfs On Wed, Jul 08, 2020 at 02:56:07PM +0200, Carlos Maiolino wrote: > change xlog_ticket_alloc() to use default kmem_cache_zalloc(), and > modify its callers to pass MM flags as arguments. > > Signed-off-by: Carlos Maiolino <cmaiolino@redhat.com> > --- > fs/xfs/xfs_log.c | 7 ++++--- > fs/xfs/xfs_log_cil.c | 2 +- > fs/xfs/xfs_log_priv.h | 2 +- > 3 files changed, 6 insertions(+), 5 deletions(-) > > diff --git a/fs/xfs/xfs_log.c b/fs/xfs/xfs_log.c > index 00fda2e8e7380..6d40d479e34a1 100644 > --- a/fs/xfs/xfs_log.c > +++ b/fs/xfs/xfs_log.c > @@ -433,7 +433,8 @@ xfs_log_reserve( > XFS_STATS_INC(mp, xs_try_logspace); > > ASSERT(*ticp == NULL); > - tic = xlog_ticket_alloc(log, unit_bytes, cnt, client, permanent, 0); > + tic = xlog_ticket_alloc(log, unit_bytes, cnt, client, permanent, > + (GFP_KERNEL | __GFP_NOFAIL)); > *ticp = tic; xfs_log_reserve() is called either from transaction reservation which is under memalloc_nofs context, or from the CIL with explicit GFP_NOFS, or from the unmount path which is GFP_KERNEL but is holding various filesystem locks. I suspect that this patch should just remove the gfp flags from xlog_ticket_alloc() and just unconditionally use GFP_NOFS | __GFP_NOFAIL fo allocating the ticket. That would clean this up quite a bit.... Cheers, Dave. -- Dave Chinner david@fromorbit.com ^ permalink raw reply [flat|nested] 12+ messages in thread
* [PATCH 4/4] xfs: remove xfs_zone_{alloc,zalloc} helpers 2020-07-08 12:56 [PATCH 0/4] Continue xfs kmem cleanup Carlos Maiolino ` (2 preceding siblings ...) 2020-07-08 12:56 ` [PATCH 3/4] xfs: Modify xlog_ticket_alloc() to use kernel's MM API Carlos Maiolino @ 2020-07-08 12:56 ` Carlos Maiolino 2020-07-09 3:00 ` Dave Chinner 3 siblings, 1 reply; 12+ messages in thread From: Carlos Maiolino @ 2020-07-08 12:56 UTC (permalink / raw) To: linux-xfs All their users have been converted to use MM API directly, no need to keep them around anymore. Signed-off-by: Carlos Maiolino <cmaiolino@redhat.com> --- fs/xfs/kmem.c | 21 --------------------- fs/xfs/kmem.h | 8 -------- fs/xfs/xfs_trace.h | 1 - 3 files changed, 30 deletions(-) diff --git a/fs/xfs/kmem.c b/fs/xfs/kmem.c index f1366475c389c..e841ed781a257 100644 --- a/fs/xfs/kmem.c +++ b/fs/xfs/kmem.c @@ -115,24 +115,3 @@ kmem_realloc(const void *old, size_t newsize, xfs_km_flags_t flags) congestion_wait(BLK_RW_ASYNC, HZ/50); } while (1); } - -void * -kmem_zone_alloc(kmem_zone_t *zone, xfs_km_flags_t flags) -{ - int retries = 0; - gfp_t lflags = kmem_flags_convert(flags); - void *ptr; - - trace_kmem_zone_alloc(kmem_cache_size(zone), flags, _RET_IP_); - do { - ptr = kmem_cache_alloc(zone, lflags); - if (ptr || (flags & KM_MAYFAIL)) - return ptr; - if (!(++retries % 100)) - xfs_err(NULL, - "%s(%u) possible memory allocation deadlock in %s (mode:0x%x)", - current->comm, current->pid, - __func__, lflags); - congestion_wait(BLK_RW_ASYNC, HZ/50); - } while (1); -} diff --git a/fs/xfs/kmem.h b/fs/xfs/kmem.h index 34cbcfde92281..8e8555817e6d3 100644 --- a/fs/xfs/kmem.h +++ b/fs/xfs/kmem.h @@ -85,14 +85,6 @@ kmem_zalloc_large(size_t size, xfs_km_flags_t flags) #define kmem_zone kmem_cache #define kmem_zone_t struct kmem_cache -extern void *kmem_zone_alloc(kmem_zone_t *, xfs_km_flags_t); - -static inline void * -kmem_zone_zalloc(kmem_zone_t *zone, xfs_km_flags_t flags) -{ - return kmem_zone_alloc(zone, flags | KM_ZERO); -} - static inline struct page * kmem_to_page(void *addr) { diff --git a/fs/xfs/xfs_trace.h b/fs/xfs/xfs_trace.h index 460136628a795..d8f5f3d99bb8f 100644 --- a/fs/xfs/xfs_trace.h +++ b/fs/xfs/xfs_trace.h @@ -3582,7 +3582,6 @@ DEFINE_KMEM_EVENT(kmem_alloc); DEFINE_KMEM_EVENT(kmem_alloc_io); DEFINE_KMEM_EVENT(kmem_alloc_large); DEFINE_KMEM_EVENT(kmem_realloc); -DEFINE_KMEM_EVENT(kmem_zone_alloc); TRACE_EVENT(xfs_check_new_dalign, TP_PROTO(struct xfs_mount *mp, int new_dalign, xfs_ino_t calc_rootino), -- 2.26.2 ^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [PATCH 4/4] xfs: remove xfs_zone_{alloc,zalloc} helpers 2020-07-08 12:56 ` [PATCH 4/4] xfs: remove xfs_zone_{alloc,zalloc} helpers Carlos Maiolino @ 2020-07-09 3:00 ` Dave Chinner 0 siblings, 0 replies; 12+ messages in thread From: Dave Chinner @ 2020-07-09 3:00 UTC (permalink / raw) To: Carlos Maiolino; +Cc: linux-xfs On Wed, Jul 08, 2020 at 02:56:08PM +0200, Carlos Maiolino wrote: > All their users have been converted to use MM API directly, no need to > keep them around anymore. > > Signed-off-by: Carlos Maiolino <cmaiolino@redhat.com> Looks good. :) Reviewed-by: Dave Chinner <dchinner@redhat.com> -- Dave Chinner david@fromorbit.com ^ permalink raw reply [flat|nested] 12+ messages in thread
end of thread, other threads:[~2020-07-09 21:52 UTC | newest]
Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-07-08 12:56 [PATCH 0/4] Continue xfs kmem cleanup Carlos Maiolino
2020-07-08 12:56 ` [PATCH 1/4] xfs: Remove kmem_zone_alloc() usage Carlos Maiolino
2020-07-09 2:45 ` Dave Chinner
2020-07-08 12:56 ` [PATCH 2/4] xfs: Remove kmem_zone_zalloc() usage Carlos Maiolino
2020-07-09 2:55 ` Dave Chinner
2020-07-09 8:55 ` Carlos Maiolino
2020-07-09 16:42 ` Darrick J. Wong
2020-07-09 21:52 ` Dave Chinner
2020-07-08 12:56 ` [PATCH 3/4] xfs: Modify xlog_ticket_alloc() to use kernel's MM API Carlos Maiolino
2020-07-09 3:00 ` Dave Chinner
2020-07-08 12:56 ` [PATCH 4/4] xfs: remove xfs_zone_{alloc,zalloc} helpers Carlos Maiolino
2020-07-09 3:00 ` Dave Chinner
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).