Re: [PATCH 2/3] xfs: share xattr name and value buffers when logging xattr updates

[Dave Chinner <david@fromorbit.com>]

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@fromorbit.com