xfs
[Top] [All Lists]

Re: [PATCH 13/25] xfs: introduce xfs_bmap_last_extent

To: Christoph Hellwig <hch@xxxxxxxxxxxxx>
Subject: Re: [PATCH 13/25] xfs: introduce xfs_bmap_last_extent
From: Alex Elder <aelder@xxxxxxx>
Date: Fri, 9 Sep 2011 15:23:46 -0500
Cc: <xfs@xxxxxxxxxxx>
In-reply-to: <20110824060643.154462765@xxxxxxxxxxxxxxxxxxxxxx>
References: <20110824060428.789245205@xxxxxxxxxxxxxxxxxxxxxx> <20110824060643.154462765@xxxxxxxxxxxxxxxxxxxxxx>
Reply-to: <aelder@xxxxxxx>
On Wed, 2011-08-24 at 02:04 -0400, Christoph Hellwig wrote:
> plain text document attachment (xfs-simplify-xfs_bmap_isaeof)
> Add a common helper for finding the last extent in a file.
> 
> Largely based on a patch from Dave Chinner.


The new version of xfs_bmap_isaeof() no longer asserts
that the data fork is the one being operated on.  Why?

I have a couple of suggested comment clarifications,
and a suggested logic simplification below.

Reviewed-by: Alex Elder <aelder@xxxxxxx>

(Still reviewing the rest, but thought I'd fire off
a batch of the ones that I'm done with.)


> Signed-off-by: Christoph Hellwig <hch@xxxxxx>
> 
> Index: xfs/fs/xfs/xfs_bmap.c

. . .

> +
> +/*
> + * Check the last inode extent to determine whether this allocation will 
> result
> + * in blocks being allocated at the end of the file. When we allocate new 
> data
> + * blocks at the end of the file which do not start at the previous data 
> block,
> + * we will try to align the new blocks at stripe unit boundaries.

It is not obvious what value will be returned when the
fork is empty, so mention that case in the comment header:
 
    Returns 0 in *aeof if the file (fork) is empty.

(Maybe you could explain why this is true at a more
abstract level though.)

> + */
> +STATIC int
> +xfs_bmap_isaeof(
> +     struct xfs_inode        *ip,
> +     xfs_fileoff_t           off,
> +     int                     whichfork,
> +     char                    *aeof)
> +{
> +     struct xfs_bmbt_irec    rec;
> +     int                     is_empty;
> +     int                     error;
> +
> +     *aeof = 0;
> +     error = xfs_bmap_last_extent(NULL, ip, whichfork, &rec, &is_empty);
> +     if (error || is_empty)
> +             return error;
> +
> +     /*
> +      * Check we are allocating in the last extent (for delayed allocations)
> +      * or past the last extent for non-delayed allocations.
> +      */
> +     *aeof = (off >= rec.br_startoff &&
> +              off < rec.br_startoff + rec.br_blockcount &&
> +              isnullstartblock(rec.br_startblock)) ||
> +             off >= rec.br_startoff + rec.br_blockcount;

This logic could be shortened:

                *aeof = off >= rec.br_startoff + rec.br_blockcount ||
                        (off >= rec.br_startoff &&
                                 isnullstartblock(rec.br_startblock);

> +     return 0;
> +}
> +
> +/*
> + * Check if the endoff is outside the last extent. If so the caller will grow
> + * the allocation to a stripe unit boundary.

    All offsets are considered outside the end of file for an
    empty file (fork), so 1 is returned in *eof in that case.

> + */
> +int
> +xfs_bmap_eof(
> +     struct xfs_inode        *ip,
> +     xfs_fileoff_t           endoff,
> +     int                     whichfork,
> +     int                     *eof)
> +{
> +     struct xfs_bmbt_irec    rec;
> +     int                     error;
> +
> +     error = xfs_bmap_last_extent(NULL, ip, whichfork, &rec, eof);
> +     if (error || *eof)
> +             return error;
> +
> +     *eof = endoff >= rec.br_startoff + rec.br_blockcount;
> +     return 0;
> +}
> +
. . .

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