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@xxxxxxxxxx>
> >
> > 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@xxxxxxxxxxxxx
