Re: [PATCH] Give a transaction to xfs_attr_set_int

[Dave Chinner <david@xxxxxxxxxxxxx>]

On Mon, Jun 23, 2008 at 02:42:30PM +1000, Niv Sardi wrote:
> From: Niv Sardi <xaiki@xxxxxxxxxx>
> 
> We introduce xfs_trans_attr_set_int that takes a transaction pointer
> as an argument (or creates one if NULL) and only finishes the
> transaction if it has created it. We use xfs_attr_rolltrans to do the
> tranS_dup dance.
> 
> xfs_attr_set_int is changed to a wrapper that will only call
> xfs_trans_attr_set_int with a NULL transaction.

As a general comment to the entire patch set, I dislike the
namespace pollution caused by changing xfs_attr_...  to
xfs_trans_attr...

The xfs_trans_... namespace is used for stuff inside xfs_trans*[ch],
not for attr code. I'd suggest making the "trans" a suffix rather
than a prefix, or something similar, so that these attr functions
are not easily confused with core transaction code....

BTW:

> @@ -356,6 +381,8 @@ xfs_attr_set_int(xfs_inode_t *dp, const char *name, int 
> namelen,
>                       if (!error && (flags & ATTR_KERNOTIME) == 0) {
>                               xfs_ichgtime(dp, XFS_ICHGTIME_CHG);
>                       }
> +                     if (tpp)
> +                             tpp = &args.trans;

That's busted too. Can you please review all the places where you
return transactio pointers to the caller via a function parameterrr
for this bug as you've made in at least a couple of places.

> +     if (tpp)
> +             tpp = &args.trans;

Here as well.

>       return(error);
>  
>  out:
> @@ -434,10 +467,25 @@ out:
>               xfs_trans_cancel(args.trans,
>                       XFS_TRANS_RELEASE_LOG_RES|XFS_TRANS_ABORT);
>       xfs_iunlock(dp, XFS_ILOCK_EXCL);
> +     if (tpp)
> +             tpp = &args.trans;

And again.

Cheers,

Dave.
-- 
Dave Chinner
david@xxxxxxxxxxxxx