xfs
[Top] [All Lists]

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

To: Christoph Hellwig <hch@xxxxxxxxxxxxx>
Subject: Re: [PATCH 1/2] xfs: split xfs_setattr
From: Dave Chinner <david@xxxxxxxxxxxxx>
Date: Tue, 21 Jun 2011 11:21:50 +1000
Cc: xfs@xxxxxxxxxxx
In-reply-to: <20110617131519.GA2822@xxxxxxxxxxxxx>
References: <20110617131519.GA2822@xxxxxxxxxxxxx>
User-agent: Mutt/1.5.20 (2009-06-14)
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

<Prev in Thread] Current Thread [Next in Thread>