Re: [PATCH 11/11] xfs: Remove kmem_alloc_{io, large} and kmem_zalloc_large

[Dave Chinner <david@fromorbit.com>]

On Wed, Nov 13, 2019 at 10:23:43AM -0800, Darrick J. Wong wrote:
> On Wed, Nov 13, 2019 at 03:23:35PM +0100, Carlos Maiolino wrote:
> > Getting rid of these functions, is a bit more complicated, giving the
> > fact they use a vmalloc fallback, and (in case of _io version) uses an
> > alignment check, so they have their useness.
> > 
> > Instead of keeping both of them, I think sharing the same function for
> > both cases is a more interesting idea, giving the fact they both have
> > the same purpose, with the only difference being the alignment check,
> > which can be selected by using a flag.
> > 
> > Signed-off-by: Carlos Maiolino <cmaiolino@redhat.com>
> > ---
> >  fs/xfs/kmem.c                 | 39 +++++++++++------------------------
> >  fs/xfs/kmem.h                 | 10 +--------
> >  fs/xfs/libxfs/xfs_attr_leaf.c |  2 +-
> >  fs/xfs/scrub/attr.c           |  2 +-
> >  fs/xfs/scrub/symlink.c        |  3 ++-
> >  fs/xfs/xfs_acl.c              |  3 ++-
> >  fs/xfs/xfs_buf.c              |  4 ++--
> >  fs/xfs/xfs_ioctl.c            |  8 ++++---
> >  fs/xfs/xfs_ioctl32.c          |  3 ++-
> >  fs/xfs/xfs_log.c              |  5 +++--
> >  fs/xfs/xfs_log_cil.c          |  2 +-
> >  fs/xfs/xfs_log_recover.c      |  4 ++--
> >  fs/xfs/xfs_rtalloc.c          |  3 ++-
> >  13 files changed, 36 insertions(+), 52 deletions(-)
> > 
> > diff --git a/fs/xfs/kmem.c b/fs/xfs/kmem.c
> > index 44145293cfc9..bb4990970647 100644
> > --- a/fs/xfs/kmem.c
> > +++ b/fs/xfs/kmem.c
> > @@ -8,40 +8,25 @@
> >  #include "xfs_message.h"
> >  #include "xfs_trace.h"
> >  
> > -/*
> > - * Same as kmem_alloc_large, except we guarantee the buffer returned is aligned
> > - * to the @align_mask. We only guarantee alignment up to page size, we'll clamp
> > - * alignment at page size if it is larger. vmalloc always returns a PAGE_SIZE
> > - * aligned region.
> > - */
> >  void *
> > -kmem_alloc_io(size_t size, int align_mask, gfp_t flags)
> > +xfs_kmem_alloc(size_t size, gfp_t flags, bool align, int align_mask)
> 
> A boolean for the align /and/ an alignment mask?  Yuck.
> 
> I think I'd rather have:
> 
> void *
> kmem_alloc(
> 	size_t		size,
> 	gfp_t		flags,
> 	unsigned int	align_mask)
> {
> 	... allocation logic ...
> }

If you avoid changing the order of the flags/alignmask parameters,
most of the churn in this patch goes away.

> 
> and in kmem.h:
> 
> static inline void *
> kmem_alloc_io(
> 	size_t		size,
> 	gfp_t		flags,
> 	unsigned int	align_mask)
> {
> 	trace_kmem_alloc_io(size, flags, align_mask, _RET_IP_);
> 	return kmem_alloc(size, flags, align_mask);
> }

This should be able to go away soon, because the heap allocator will
guarantee alignment soon. That means kmem.c is a single function,
and kmem.h is a single function. I'd be looking to move the two
helper functions into some other utility file at that point
(fsops?)...

ANother question: how much work is there to be done on the userspace
side of things?

> >   */
> >  
> > -extern void *kmem_alloc_io(size_t size, int align_mask, gfp_t flags);
> > -extern void *kmem_alloc_large(size_t size, gfp_t);
> > +extern void *xfs_kmem_alloc(size_t, gfp_t, bool, int);
> >  static inline void  kmem_free(const void *ptr)
> >  {
> >  	kvfree(ptr);
> >  }

Didn't an earlier patch get rid of kmem_free(), or am I just
imagining this? Seems silly to leave this behind, now that the
only place that needs kvfree() is the callers to kmem_alloc_io and
kmem_alloc_large...

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com