xfs
[Top] [All Lists]

Re: [PATCH 09/27] xfs: split xfs_itruncate_finish

To: Dave Chinner <david@xxxxxxxxxxxxx>
Subject: Re: [PATCH 09/27] xfs: split xfs_itruncate_finish
From: Christoph Hellwig <hch@xxxxxxxxxxxxx>
Date: Thu, 30 Jun 2011 03:18:33 -0400
Cc: Christoph Hellwig <hch@xxxxxxxxxxxxx>, xfs@xxxxxxxxxxx
In-reply-to: <20110630024428.GB561@dastard>
References: <20110629140109.003209430@xxxxxxxxxxxxxxxxxxxxxx> <20110629140338.286808024@xxxxxxxxxxxxxxxxxxxxxx> <20110630024428.GB561@dastard>
User-agent: Mutt/1.5.21 (2010-09-15)
On Thu, Jun 30, 2011 at 12:44:28PM +1000, Dave Chinner wrote:
> >  
> >     ASSERT(xfs_isilocked(ip, XFS_ILOCK_EXCL|XFS_IOLOCK_EXCL));
> > -   ASSERT((new_size == 0) || (new_size <= ip->i_size));
> > -   ASSERT(*tp != NULL);
> > -   ASSERT((*tp)->t_flags & XFS_TRANS_PERM_LOG_RES);
> > -   ASSERT(ip->i_transp == *tp);
> > +   ASSERT(new_size == 0 || new_size <= ip->i_size);
> 
> If new_size == 0, then it will always be <= ip->i_size, so that's
> kind of a redundant check. I think this really should be two
> different asserts, one that validates the data fork new_size range,
> and one that validates the attr fork truncate to zero length only
> condition:
> 
>       ASSERT(new_size <= ip->i_size);
>       ASSERT(whichfork != XFS_ATTR_FORK || new_size == 0);

For now I was just keeping the existing assert, but changing this one
sounds ok.  OTOH I kept the whole routine fork agnostic, so I think
I'll rather just make the assert read:

        ASSERT(new_size <= ip->i_size);

and assume the one and only attr fork caller does the right thing.

> > @@ -1464,15 +1311,16 @@ xfs_itruncate_finish(
> >             }
> >  
> >             ntp = xfs_trans_dup(ntp);
> > -           error = xfs_trans_commit(*tp, 0);
> > -           *tp = ntp;
> > +           error = xfs_trans_commit(*tpp, 0);
> > +           *tpp = ntp;
> 
> I've always found this a mess to follow which transaction is which
> because of the rewriting of ntp. This is easier to follow:
> 
>               ntp = xfs_trans_dup(*tpp);
>               error = xfs_trans_commit(*tpp, 0);
>               *tpp = ntp;
> 
> Now it's clear that we are duplicating *tpp, then committing it, and
> then setting it to the duplicated transaction. Now I don't have to
> go look at all the surrounding code to remind myself what ntp
> contains to validate that the fragment of code is doing the right
> thing.....

I've cleaned this up even further and added a local tp variable that
has the current transaction as a normal pointer.  *tpp is only assigned
back to in a single place after goto out, and ntp is only used for
the switching around to the duplicated transaction.

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