xfs
[Top] [All Lists]

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

To: Christoph Hellwig <hch@xxxxxxxxxxxxx>
Subject: Re: [PATCH 09/27] xfs: split xfs_itruncate_finish
From: Dave Chinner <david@xxxxxxxxxxxxx>
Date: Thu, 30 Jun 2011 12:44:28 +1000
Cc: xfs@xxxxxxxxxxx
In-reply-to: <20110629140338.286808024@xxxxxxxxxxxxxxxxxxxxxx>
References: <20110629140109.003209430@xxxxxxxxxxxxxxxxxxxxxx> <20110629140338.286808024@xxxxxxxxxxxxxxxxxxxxxx>
User-agent: Mutt/1.5.20 (2009-06-14)
On Wed, Jun 29, 2011 at 10:01:18AM -0400, Christoph Hellwig wrote:
> Split the guts of xfs_itruncate_finish that loop over the existing extents
> and calls xfs_bunmapi on them into a new helper, xfs_itruncate_externs.
> Make xfs_attr_inactive call it directly instead of xfs_itruncate_finish,
> which allows to simplify the latter a lot, by only letting it deal with
> the data fork.  As a result xfs_itruncate_finish is renamed to
> xfs_itruncate_data to make its use case more obvious.
> 
> Also remove the sync parameter from xfs_itruncate_data, which has been
> unessecary since the introduction of the busy extent list in 2002, and
> completely dead code since 2003 when the XFS_BMAPI_ASYNC parameter was
> made a no-op.
> 
> I can't actually see why the xfs_attr_inactive needs to set the transaction
> sync, but let's keep this patch simple and without changes in behaviour.
> 
> Signed-off-by: Christoph Hellwig <hch@xxxxxx>

Overall, looks good. A few minor comments in line, but consider it

Reviewed-by: Dave Chinner <dchinner@xxxxxxxxxx>

> +     struct xfs_mount        *mp = ip->i_mount;
> +     struct xfs_trans        *ntp = *tpp;
> +     xfs_bmap_free_t         free_list;
> +     xfs_fsblock_t           first_block;
> +     xfs_fileoff_t           first_unmap_block;
> +     xfs_fileoff_t           last_block;
> +     xfs_filblks_t           unmap_len;
> +     int                     committed;
> +     int                     error = 0;
> +     int                     done = 0;
>  
>       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);


> @@ -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.....

Cheers,

Dave.
-- 
Dave Chinner
david@xxxxxxxxxxxxx

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