[PATCH 0/4] Continue xfs kmem cleanup

[PATCH 0/4] Continue xfs kmem cleanup

[Carlos Maiolino]

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

[PATCH 1/4] xfs: Remove kmem_zone_alloc() usage

[Carlos Maiolino]

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

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

[Carlos Maiolino]

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

[PATCH 3/4] xfs: Modify xlog_ticket_alloc() to use kernel's MM API

[Carlos Maiolino]

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

[PATCH 4/4] xfs: remove xfs_zone_{alloc,zalloc} helpers

[Carlos Maiolino]

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

Re: [PATCH 1/4] xfs: Remove kmem_zone_alloc() usage

[Dave Chinner]

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

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

[Dave Chinner]

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

Re: [PATCH 3/4] xfs: Modify xlog_ticket_alloc() to use kernel's MM API

[Dave Chinner]

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

Re: [PATCH 4/4] xfs: remove xfs_zone_{alloc,zalloc} helpers

[Dave Chinner]

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

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

[Carlos Maiolino]

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

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

[Darrick J. Wong]

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
> 

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

[Dave Chinner]

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