Re: [PATCH 19/18] xfs: can't use kmem_zalloc() for attribute buffers

["Darrick J. Wong" <djwong@kernel.org>]

On Wed, May 11, 2022 at 10:54:20AM +1000, Dave Chinner wrote:
> On Tue, May 10, 2022 at 04:59:31PM -0700, Darrick J. Wong wrote:
> > On Wed, May 11, 2022 at 08:27:16AM +1000, Dave Chinner wrote:
> > > From: Dave Chinner <dchinner@redhat.com>
> > > 
> > > Because when running fsmark workloads with 64kB xattrs, heap
> > > allocation of >64kB buffers for the attr item name/value buffer
> > > will fail and deadlock.
> .....
> > > @@ -119,11 +119,11 @@ xfs_attri_item_format(
> > >  			sizeof(struct xfs_attri_log_format));
> > >  	xlog_copy_iovec(lv, &vecp, XLOG_REG_TYPE_ATTR_NAME,
> > >  			attrip->attri_name,
> > > -			xlog_calc_iovec_len(attrip->attri_name_len));
> > > +			attrip->attri_name_len);
> > 
> > Are we fixing these because the xlog_{copy,finish}_iovec functions do
> > the rounding themselves now?
> 
> Yes, I forgot that I cleaned this up here when I noticed it.
> Probably should mention it in the commit log:
> 
> "We also clean up the attribute name and value lengths as they no
> longer need to be rounded out to sizes compatible with log vectors."

Ok.

> > 
> > >  	if (attrip->attri_value_len > 0)
> > >  		xlog_copy_iovec(lv, &vecp, XLOG_REG_TYPE_ATTR_VALUE,
> > >  				attrip->attri_value,
> > > -				xlog_calc_iovec_len(attrip->attri_value_len));
> > > +				attrip->attri_value_len);
> > >  }
> > >  
> > >  /*
> > > @@ -163,26 +163,21 @@ xfs_attri_init(
> > >  
> > >  {
> > >  	struct xfs_attri_log_item	*attrip;
> > > -	uint32_t			name_vec_len = 0;
> > > -	uint32_t			value_vec_len = 0;
> > > -	uint32_t			buffer_size;
> > > -
> > > -	if (name_len)
> > > -		name_vec_len = xlog_calc_iovec_len(name_len);
> > > -	if (value_len)
> > > -		value_vec_len = xlog_calc_iovec_len(value_len);
> > 
> > ...and we don't need to bloat up the internal structures anymore either,
> > right?
> 
> Yup, because we only copy out the exact length of the name/val now.

<nod>

> > 
> > > -
> > > -	buffer_size = name_vec_len + value_vec_len;
> > > +	uint32_t			buffer_size = name_len + value_len;
> > >  
> > >  	if (buffer_size) {
> > > -		attrip = kmem_zalloc(sizeof(struct xfs_attri_log_item) +
> > > -				    buffer_size, KM_NOFS);
> > > -		if (attrip == NULL)
> > > -			return NULL;
> > > +		/*
> > > +		 * This could be over 64kB in length, so we have to use
> > > +		 * kvmalloc() for this. But kvmalloc() utterly sucks, so we
> > > +		 * use own version.
> > > +		 */
> > > +		attrip = xlog_kvmalloc(sizeof(struct xfs_attri_log_item) +
> > > +					buffer_size);
> > >  	} else {
> > > -		attrip = kmem_cache_zalloc(xfs_attri_cache,
> > > -					  GFP_NOFS | __GFP_NOFAIL);
> > > +		attrip = kmem_cache_alloc(xfs_attri_cache,
> > > +					GFP_NOFS | __GFP_NOFAIL);
> > >  	}
> > > +	memset(attrip, 0, sizeof(struct xfs_attri_log_item));
> > 
> > I wonder if this memset should be right after the xlog_kvmalloc and
> > leave the kmem_cache_zalloc alone?
> 
> Then we memset the header twice in the common small attr case, and
> the xfs_attri_log_item header is not exactly what you'd call small
> (224 bytes).

Eh, ok, NBD.
Reviewed-by: Darrick J. Wong <djwong@kernel.org>

--D

> Cheers,
> 
> Dave.
> -- 
> Dave Chinner
> david@fromorbit.com