xfs
[Top] [All Lists]

Re: [PATCH v2 3/8] xfs: create helper to check whether to free eofblocks

To: Brian Foster <bfoster@xxxxxxxxxx>
Subject: Re: [PATCH v2 3/8] xfs: create helper to check whether to free eofblocks on inode
From: Dave Chinner <david@xxxxxxxxxxxxx>
Date: Tue, 11 Sep 2012 15:27:43 +1000
Cc: xfs@xxxxxxxxxxx
In-reply-to: <1347302246-52710-4-git-send-email-bfoster@xxxxxxxxxx>
References: <1347302246-52710-1-git-send-email-bfoster@xxxxxxxxxx> <1347302246-52710-4-git-send-email-bfoster@xxxxxxxxxx>
User-agent: Mutt/1.5.21 (2010-09-15)
On Mon, Sep 10, 2012 at 02:37:21PM -0400, Brian Foster wrote:
> This check is used in multiple places to determine whether we
> should check for (and potentially free) post EOF blocks on an
> inode. Add a helper to consolidate the check.
> 
> Note that when we remove an inode from the cache (xfs_inactive()),
> we are required to trim post-EOF blocks even if the inode is marked
> preallocated or append-only to maintain correct space accounting.
> The 'force' parameter to xfs_can_free_eofblocks() specifies whether
> we should ignore the prealloc/append-only status of the inode.
> 
> Signed-off-by: Brian Foster <bfoster@xxxxxxxxxx>

Just quickly, I'd rewrite this convoluted logic:

> +/*
> + * Test whether it is appropriate to check an inode for and free post EOF
> + * blocks. The 'force' parameter determines whether we should also consider
> + * regular files that are marked preallocated or append-only.
> + */
> +static inline bool
> +xfs_can_free_eofblocks(struct xfs_inode *ip, bool force)
> +{
> +     if (S_ISREG(ip->i_d.di_mode) &&
> +         (VFS_I(ip)->i_size > 0 ||
> +          (VN_CACHED(VFS_I(ip)) > 0 || ip->i_delayed_blks > 0)) &&
> +         (ip->i_df.if_flags & XFS_IFEXTENTS)  &&
> +         (!(ip->i_d.di_flags & (XFS_DIFLAG_PREALLOC | XFS_DIFLAG_APPEND)) ||
> +          (force && ip->i_delayed_blks != 0)))
> +             return true;
> +
> +     return false;
> +}

in a manner that can means it no longer has to be analysed from first principles
every time it is read. e.g.:

static inline bool
xfs_can_free_eofblocks(struct xfs_inode *ip, bool force)
{
        /* prealloc/delalloc exists only on regular files */
        if (!S_ISREG(ip->i_d.di_mode))
                return false;
        /*
         * Zero sized files with no cached page or delalloc blocks will not have
         * any speculative prealloc/delalloc to remove.
         */
        if (VFS_I(ip)->i_size == 0 &&
            VFS_I(ip)->i_mapping->nr_pages == 0 &&
            ip->i_delayed_blks == 0)
                return false;

        /* If we haven't read in the extent list, then don't do it now. */
        if (!(ip->i_df.if_flags & XFS_IFEXTENTS))
                return false;

        /*
         * Real preallocation or append only files do not get EOF blocks freed
         * unless there are also delalloc blocks and we are forced to remove
         * them.
         */
        if ((ip->i_d.di_flags & (XFS_DIFLAG_PREALLOC | XFS_DIFLAG_APPEND)) {
                if (!ip->i_delayed_blks)
                        return false;
                if (!force)
                        return false;
        }

        return true;
}

-- 
Dave Chinner
david@xxxxxxxxxxxxx

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