On Thu, May 19, 2022 at 11:08:16AM -0700, Darrick J. Wong wrote:
> On Thu, May 19, 2022 at 10:27:27AM +1000, Dave Chinner wrote:
> > On Wed, May 18, 2022 at 11:55:13AM -0700, Darrick J. Wong wrote:
> > > @@ -158,41 +239,17 @@ xfs_attri_item_release(
> > > STATIC struct xfs_attri_log_item *
> > > xfs_attri_init(
> > > struct xfs_mount *mp,
> > > - uint32_t name_len,
> > > - uint32_t value_len)
> > > -
> > > + struct xfs_attri_log_nameval *anvl)
> > > {
> > > struct xfs_attri_log_item *attrip;
> > > - uint32_t buffer_size = name_len + value_len;
> > >
> > > - if (buffer_size) {
> > > - /*
> > > - * 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_alloc(xfs_attri_cache,
> > > - GFP_NOFS | __GFP_NOFAIL);
> > > - }
> > > - memset(attrip, 0, sizeof(struct xfs_attri_log_item));
> > > + attrip = kmem_cache_zalloc(xfs_attri_cache, GFP_NOFS | __GFP_NOFAIL);
> > >
> > > - attrip->attri_name_len = name_len;
> > > - if (name_len)
> > > - attrip->attri_name = ((char *)attrip) +
> > > - sizeof(struct xfs_attri_log_item);
> > > - else
> > > - attrip->attri_name = NULL;
> > > -
> > > - attrip->attri_value_len = value_len;
> > > - if (value_len)
> > > - attrip->attri_value = ((char *)attrip) +
> > > - sizeof(struct xfs_attri_log_item) +
> > > - name_len;
> > > - else
> > > - attrip->attri_value = NULL;
> > > + /*
> > > + * Grab an extra reference to the name/value buffer for this log item.
> > > + * The caller retains its own reference!
> > > + */
> > > + attrip->attri_nameval = xfs_attri_log_nameval_get(anvl);
> >
> > Handle _get failure here, or better, pass in an already referenced
> > nv because the caller should always have a reference to begin
> > with. Caller can probably handle allocation failure, too, because
> > this should be called before we've dirtied the transaction, right?
>
> _nameval_get merely bumps the refcount, so the caller should already
> have a valid reference to begin with. So I think the _get function can
> become:
>
> if (!refcount_inc_not_zero(anvl..))
> return NULL;
> return val;
>
> and then this callsite can add a simple ASSERT to ensure that we never
> pass around a zero-refcount object (in theory the refcount code will
> also fail loudly):
>
> attrip->attri_nameval = xfs_attri_log_nameval_get(anvl);
> ASSERT(attrip->attri_nameval);
I'm fine with that - it documents that the get should not fail at
this point.
>
> > .....
> >
> > > @@ -385,16 +435,33 @@ xfs_attr_create_intent(
> > > * Each attr item only performs one attribute operation at a time, so
> > > * this is a list of one
> > > */
> > > - list_for_each_entry(attr, items, xattri_list) {
> > > - attrip = xfs_attri_init(mp, attr->xattri_da_args->namelen,
> > > - attr->xattri_da_args->valuelen);
> > > - if (attrip == NULL)
> > > - return NULL;
> > > + attr = list_first_entry_or_null(items, struct xfs_attr_item,
> > > + xattri_list);
> > >
> > > - xfs_trans_add_item(tp, &attrip->attri_item);
> > > - xfs_attr_log_item(tp, attrip, attr);
> > > + /*
> > > + * Create a buffer to store the attribute name and value. This buffer
> > > + * will be shared between the higher level deferred xattr work state
> > > + * and the lower level xattr log items.
> > > + */
> > > + if (!attr->xattri_nameval) {
> > > + struct xfs_da_args *args = attr->xattri_da_args;
> > > +
> > > + /*
> > > + * Transfer our reference to the name/value buffer to the
> > > + * deferred work state structure.
> > > + */
> > > + attr->xattri_nameval = xfs_attri_log_nameval_alloc(args->name,
> > > + args->namelen, args->value, args->valuelen);
> > > + }
> > > + if (!attr->xattri_nameval) {
> > > + xlog_force_shutdown(mp->m_log, SHUTDOWN_LOG_IO_ERROR);
> > > + return NULL;
> > > }
> >
> > Why shutdown on ENOMEM? I would have expects that we return to the
> > caller so it can cancel the transaction. That way we only shut down
> > if the transaction is dirty or the caller context can't handle
> > errors....
>
> ->create_intent can return NULL if they don't need to log any intent
> items to restart a piece of deferred work. xfs_defer_create_intent*()
> currently have no means to convey an errno up to their callers, but that
> could be a preparatory cleanup.
Ok, if you add a brief comment then I'm fine with it. The cleanup so
we have error paths here can be done later.
Cheers,
Dave.
--
Dave Chinner
david@xxxxxxxxxxxxx
