Re: [PATCH 1/2] xfs: split xfs_setattr

[Christoph Hellwig <hch@infradead.org>]

On Tue, Jun 21, 2011 at 11:21:50AM +1000, Dave Chinner wrote:
> I'm not sure that xfs_setattr_simple() is the best name for this.
> It's not really a simple setattr case, it's all the "all except size"
> changes. Perhaps xfs_setattr_nonsize() and xfs_setattr_size()
> would be a better name pair...

Ok.

> > +	tp = xfs_trans_alloc(mp, XFS_TRANS_SETATTR_NOT_SIZE);
> > +	error = xfs_trans_reserve(tp, 0, XFS_ICHANGE_LOG_RES(mp), 0, 0, 0);
> > +	if (error)
> > +		goto error_return;
> > +
> > +	lock_flags = XFS_ILOCK_EXCL;
> > +	xfs_ilock(ip, lock_flags);
> 
> With a slight change to the error unwind stack, you can kill the
> lock_flags variable altogether - it never gets changed in the code
> except for here. 

> If you change it to:
> 
> error_return:
> 	xfs_iunlock(ip, XFS_ILOCK_EXCL);
> error_free_tp:
> 	xfs_trans_cancel(tp, 0);
> 	xfs_qm_dqrele(udqp);
> 	xfs_qm_dqrele(gdqp);
> 	return error;
> 
> And jump to error_free_tp when xfs_trans_reserve() fails above,
> lock_flags and the conditionals in the unwind stack go away.

We'll actually need a third label for just the dqrele calls, as we
get references to the dqouts before allocating the transaction.  And
even with that we change the order of cleanup even if that seems
harmless from a quick audit.

I've prepared a version with the suggestions for QA, but I have a bit
of an uneasy feeling about mixing up such subtile changes with the
simple split.

> > +	if (mp->m_flags & XFS_MOUNT_WSYNC)
> > +		xfs_trans_set_sync(tp);
> > +
> > +	error = xfs_trans_commit(tp, XFS_TRANS_RELEASE_LOG_RES);
> > +	goto out_unlock;
> > +
> > +out_trans_abort:
> > +	commit_flags |= XFS_TRANS_ABORT;
> > +out_trans_cancel:
> > +	xfs_trans_cancel(tp, commit_flags);
> > +out_unlock:
> > +	if (lock_flags)
> > +		xfs_iunlock(ip, lock_flags);
> > +	return error;
> > +}
> 
> And here we never get to out_unlock without lock_flags being set, so
> the conditional can be removed.

We can.  E.g. the zero length truncate case (which is removed in the
next patch) does it, as  does the xfs_flush_pages case if XFS_ATTR_NOLOCK
is set.  So I'd rather leave it in instead of hacking around those cases.

> I also think that the goto after the
> commit call is a bit ugly. I'd prefer the none-failure case is
> straight line code so it is easy to follow, and the unwind stack has
> an extra jump in it. i.e.:

Ok.

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs