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

[Dave Chinner <david@fromorbit.com>]

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

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

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

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com