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@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."
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@xxxxxxxxxx>
--D
> Cheers,
>
> Dave.
> --
> Dave Chinner
> david@xxxxxxxxxxxxx
