| Message ID | 20200710091536.95828-2-cmaiolino@redhat.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Continue xfs kmem cleanup - V2 | expand |
> 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.
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.
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 >
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 >
> > 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.
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 >
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 --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;
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(-)