On Fri, Jun 17, 2011 at 09:15:20AM -0400, Christoph Hellwig wrote:
> Split up xfs_setattr into two functions, one for the complex truncate
> handling, and one for the trivial attribute updates. Also move both
> new routines to xfs_iops.c as they are fairly Linux-specific.
>
> Signed-off-by: Christoph Hellwig <hch@xxxxxx>
Looks good. A few minor comments below.
>
> Index: xfs/fs/xfs/linux-2.6/xfs_iops.c
> ===================================================================
> --- xfs.orig/fs/xfs/linux-2.6/xfs_iops.c 2011-06-17 14:07:57.059091534 +0200
> +++ xfs/fs/xfs/linux-2.6/xfs_iops.c 2011-06-17 14:18:42.495725522 +0200
> @@ -39,6 +39,7 @@
> #include "xfs_buf_item.h"
> #include "xfs_utils.h"
> #include "xfs_vnodeops.h"
> +#include "xfs_inode_item.h"
> #include "xfs_trace.h"
>
> #include <linux/capability.h>
> @@ -497,12 +498,452 @@ xfs_vn_getattr(
> return 0;
> }
>
> +int
> +xfs_setattr_simple(
> + struct xfs_inode *ip,
> + struct iattr *iattr,
> + int flags)
> +{
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...
.....
> +
> + 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.
....
> +error_return:
> + xfs_qm_dqrele(udqp);
> + xfs_qm_dqrele(gdqp);
> + if (tp)
> + xfs_trans_cancel(tp, 0);
> + if (lock_flags != 0)
> + xfs_iunlock(ip, lock_flags);
> + return error;
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.
> + xfs_trans_log_inode(tp, ip, XFS_ILOG_CORE);
> +
> + XFS_STATS_INC(xs_ig_attrchg);
> +
> + 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. 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.:
if (mp->m_flags & XFS_MOUNT_WSYNC)
xfs_trans_set_sync(tp);
error = xfs_trans_commit(tp, XFS_TRANS_RELEASE_LOG_RES);
out_unlock:
xfs_iunlock(ip, lock_flags);
return error;
out_trans_abort:
commit_flags |= XFS_TRANS_ABORT;
out_trans_cancel:
xfs_trans_cancel(tp, commit_flags);
goto out_unlock;
}
Otherwise this looks like a good improvement - the setattr path has
always been a mess and difficult to follow....
Cheers,
Dave.
--
Dave Chinner
david@xxxxxxxxxxxxx
_______________________________________________
xfs mailing list
xfs@xxxxxxxxxxx
http://oss.sgi.com/mailman/listinfo/xfs
