diff mbox series

[V2,1/5] xfs: Remove kmem_zone_alloc() usage

Message ID 20200710091536.95828-2-cmaiolino@redhat.com (mailing list archive)
State Superseded, archived
Headers show
Series Continue xfs kmem cleanup - V2 | expand

Commit Message

Carlos Maiolino July 10, 2020, 9:15 a.m. UTC
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.

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

Changelog:
	V2:	- Wire up xfs_inode_alloc to use __GFP_NOFAIL
		  if it's called inside a transaction
		- Rewrite changelog in a more decent way.

 fs/xfs/libxfs/xfs_alloc.c |  3 ++-
 fs/xfs/libxfs/xfs_bmap.c  |  3 ++-
 fs/xfs/xfs_icache.c       | 13 +++++++++----
 3 files changed, 13 insertions(+), 6 deletions(-)

Comments

Christoph Hellwig July 10, 2020, 4:08 p.m. UTC | #1
> diff --git a/fs/xfs/xfs_icache.c b/fs/xfs/xfs_icache.c
> index 5daef654956cb..8c3fe7ef56e27 100644
> --- a/fs/xfs/xfs_icache.c
> +++ b/fs/xfs/xfs_icache.c
> @@ -35,15 +35,20 @@ xfs_inode_alloc(
>  	xfs_ino_t		ino)
>  {
>  	struct xfs_inode	*ip;
> +	gfp_t			gfp_mask = GFP_KERNEL;
>  
>  	/*
> -	 * 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.
> +	 * If this is inside a transaction, we can not fail here,
> +	 * otherwise we can return NULL on ENOMEM.
>  	 */
> -	ip = kmem_zone_alloc(xfs_inode_zone, 0);
> +
> +	if (current->flags & PF_MEMALLOC_NOFS)
> +		gfp_mask |= __GFP_NOFAIL;

I'm a little worried about this change in beavior here.  Can we
just keep the unconditional __GFP_NOFAIL and if we really care do the
change separately after the series?  At that point it should probably
use the re-added PF_FSTRANS flag as well.
Dave Chinner July 10, 2020, 10:21 p.m. UTC | #2
On Fri, Jul 10, 2020 at 05:08:04PM +0100, Christoph Hellwig wrote:
> > diff --git a/fs/xfs/xfs_icache.c b/fs/xfs/xfs_icache.c
> > index 5daef654956cb..8c3fe7ef56e27 100644
> > --- a/fs/xfs/xfs_icache.c
> > +++ b/fs/xfs/xfs_icache.c
> > @@ -35,15 +35,20 @@ xfs_inode_alloc(
> >  	xfs_ino_t		ino)
> >  {
> >  	struct xfs_inode	*ip;
> > +	gfp_t			gfp_mask = GFP_KERNEL;
> >  
> >  	/*
> > -	 * 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.
> > +	 * If this is inside a transaction, we can not fail here,
> > +	 * otherwise we can return NULL on ENOMEM.
> >  	 */
> > -	ip = kmem_zone_alloc(xfs_inode_zone, 0);
> > +
> > +	if (current->flags & PF_MEMALLOC_NOFS)
> > +		gfp_mask |= __GFP_NOFAIL;
> 
> I'm a little worried about this change in beavior here.  Can we
> just keep the unconditional __GFP_NOFAIL and if we really care do the
> change separately after the series?  At that point it should probably
> use the re-added PF_FSTRANS flag as well.

Checking PF_FSTRANS was what I suggested should be done here, not
PF_MEMALLOC_NOFS...

Cheers,

Dave.
Carlos Maiolino July 13, 2020, 9:16 a.m. UTC | #3
Hi Dave, Christoph.

> > > -	ip = kmem_zone_alloc(xfs_inode_zone, 0);
> > > +
> > > +	if (current->flags & PF_MEMALLOC_NOFS)
> > > +		gfp_mask |= __GFP_NOFAIL;
> > 
> > I'm a little worried about this change in beavior here.  Can we
> > just keep the unconditional __GFP_NOFAIL and if we really care do the
> > change separately after the series?  At that point it should probably
> > use the re-added PF_FSTRANS flag as well.

> Checking PF_FSTRANS was what I suggested should be done here, not
> PF_MEMALLOC_NOFS...


No problem in splitting this change into 2 patches, 1 by unconditionally use
__GFP_NOFAIL, and another changing the behavior to use NOFAIL only inside a
transaction.

Regarding the PF_FSTRANS flag, I opted by PF_MEMALLOC_NOFS after reading the
commit which removed PF_FSTRANS initially (didn't mean to ignore your suggestion
Dave, my apologies if I sounded like that), but I actually didn't find any commit
re-adding PF_FSTRANS back. I searched most trees but couldn't find any commit
re-adding it back, could you guys please point me out where is the commit adding
it back?

> 
> Cheers,
> 
> Dave.
> -- 
> Dave Chinner
> david@fromorbit.com
>
Darrick J. Wong July 13, 2020, 4:17 p.m. UTC | #4
On Mon, Jul 13, 2020 at 11:16:10AM +0200, Carlos Maiolino wrote:
> Hi Dave, Christoph.
> 
> > > > -	ip = kmem_zone_alloc(xfs_inode_zone, 0);
> > > > +
> > > > +	if (current->flags & PF_MEMALLOC_NOFS)
> > > > +		gfp_mask |= __GFP_NOFAIL;
> > > 
> > > I'm a little worried about this change in beavior here.  Can we
> > > just keep the unconditional __GFP_NOFAIL and if we really care do the
> > > change separately after the series?  At that point it should probably
> > > use the re-added PF_FSTRANS flag as well.
> 
> > Checking PF_FSTRANS was what I suggested should be done here, not
> > PF_MEMALLOC_NOFS...
> 
> 
> No problem in splitting this change into 2 patches, 1 by unconditionally use
> __GFP_NOFAIL, and another changing the behavior to use NOFAIL only inside a
> transaction.
> 
> Regarding the PF_FSTRANS flag, I opted by PF_MEMALLOC_NOFS after reading the
> commit which removed PF_FSTRANS initially (didn't mean to ignore your suggestion
> Dave, my apologies if I sounded like that), but I actually didn't find any commit
> re-adding PF_FSTRANS back. I searched most trees but couldn't find any commit
> re-adding it back, could you guys please point me out where is the commit adding
> it back?

I suspect Dave is referring to:

"xfs: reintroduce PF_FSTRANS for transaction reservation recursion
protection" by Yang Shao.

AFAICT it hasn't cleared akpm yet, so it's not in his quiltpile, and as
he doesn't use git there won't be a commit until it ends up in
mainline...

--D

> > 
> > Cheers,
> > 
> > Dave.
> > -- 
> > Dave Chinner
> > david@fromorbit.com
> > 
> 
> -- 
> Carlos
>
Carlos Maiolino July 15, 2020, 3:06 p.m. UTC | #5
> > No problem in splitting this change into 2 patches, 1 by unconditionally use
> > __GFP_NOFAIL, and another changing the behavior to use NOFAIL only inside a
> > transaction.
> > 
> > Regarding the PF_FSTRANS flag, I opted by PF_MEMALLOC_NOFS after reading the
> > commit which removed PF_FSTRANS initially (didn't mean to ignore your suggestion
> > Dave, my apologies if I sounded like that), but I actually didn't find any commit
> > re-adding PF_FSTRANS back. I searched most trees but couldn't find any commit
> > re-adding it back, could you guys please point me out where is the commit adding
> > it back?
> 
> I suspect Dave is referring to:
> 
> "xfs: reintroduce PF_FSTRANS for transaction reservation recursion
> protection" by Yang Shao.
> 
> AFAICT it hasn't cleared akpm yet, so it's not in his quiltpile, and as
> he doesn't use git there won't be a commit until it ends up in
> mainline...
> 

Thanks, I think I'll wait until it hits the mainline before trying to push this
series then.
Darrick J. Wong July 15, 2020, 3:37 p.m. UTC | #6
On Wed, Jul 15, 2020 at 05:06:59PM +0200, Carlos Maiolino wrote:
> > > No problem in splitting this change into 2 patches, 1 by unconditionally use
> > > __GFP_NOFAIL, and another changing the behavior to use NOFAIL only inside a
> > > transaction.
> > > 
> > > Regarding the PF_FSTRANS flag, I opted by PF_MEMALLOC_NOFS after reading the
> > > commit which removed PF_FSTRANS initially (didn't mean to ignore your suggestion
> > > Dave, my apologies if I sounded like that), but I actually didn't find any commit
> > > re-adding PF_FSTRANS back. I searched most trees but couldn't find any commit
> > > re-adding it back, could you guys please point me out where is the commit adding
> > > it back?
> > 
> > I suspect Dave is referring to:
> > 
> > "xfs: reintroduce PF_FSTRANS for transaction reservation recursion
> > protection" by Yang Shao.
> > 
> > AFAICT it hasn't cleared akpm yet, so it's not in his quiltpile, and as
> > he doesn't use git there won't be a commit until it ends up in
> > mainline...
> > 
> 
> Thanks, I think I'll wait until it hits the mainline before trying to push this
> series then.

FWIW I could be persuaded to take that one via one of the xfs/iomap
trees if the author puts out a revised patch.

--D

> 
> -- 
> Carlos
>
Christoph Hellwig July 15, 2020, 5:32 p.m. UTC | #7
On Wed, Jul 15, 2020 at 08:37:21AM -0700, Darrick J. Wong wrote:
> On Wed, Jul 15, 2020 at 05:06:59PM +0200, Carlos Maiolino wrote:
> > > > No problem in splitting this change into 2 patches, 1 by unconditionally use
> > > > __GFP_NOFAIL, and another changing the behavior to use NOFAIL only inside a
> > > > transaction.
> > > > 
> > > > Regarding the PF_FSTRANS flag, I opted by PF_MEMALLOC_NOFS after reading the
> > > > commit which removed PF_FSTRANS initially (didn't mean to ignore your suggestion
> > > > Dave, my apologies if I sounded like that), but I actually didn't find any commit
> > > > re-adding PF_FSTRANS back. I searched most trees but couldn't find any commit
> > > > re-adding it back, could you guys please point me out where is the commit adding
> > > > it back?
> > > 
> > > I suspect Dave is referring to:
> > > 
> > > "xfs: reintroduce PF_FSTRANS for transaction reservation recursion
> > > protection" by Yang Shao.
> > > 
> > > AFAICT it hasn't cleared akpm yet, so it's not in his quiltpile, and as
> > > he doesn't use git there won't be a commit until it ends up in
> > > mainline...
> > > 
> > 
> > Thanks, I think I'll wait until it hits the mainline before trying to push this
> > series then.
> 
> FWIW I could be persuaded to take that one via one of the xfs/iomap
> trees if the author puts out a revised patch.

Let's just defer that part of the patch.  It really shouldn't be mixed
with an API cleanup as it is significant behavior change.
diff mbox series

Patch

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..8c3fe7ef56e27 100644
--- a/fs/xfs/xfs_icache.c
+++ b/fs/xfs/xfs_icache.c
@@ -35,15 +35,20 @@  xfs_inode_alloc(
 	xfs_ino_t		ino)
 {
 	struct xfs_inode	*ip;
+	gfp_t			gfp_mask = GFP_KERNEL;
 
 	/*
-	 * 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.
+	 * If this is inside a transaction, we can not fail here,
+	 * otherwise we can return NULL on ENOMEM.
 	 */
-	ip = kmem_zone_alloc(xfs_inode_zone, 0);
+
+	if (current->flags & PF_MEMALLOC_NOFS)
+		gfp_mask |= __GFP_NOFAIL;
+
+	ip = kmem_cache_alloc(xfs_inode_zone, gfp_mask);
 	if (!ip)
 		return NULL;
+
 	if (inode_init_always(mp->m_super, VFS_I(ip))) {
 		kmem_cache_free(xfs_inode_zone, ip);
 		return NULL;