[Top] [All Lists]

Re: [PATCH 7/8] xfs: fix fstrim offset calculations

To: Dave Chinner <david@xxxxxxxxxxxxx>
Subject: Re: [PATCH 7/8] xfs: fix fstrim offset calculations
From: Christoph Hellwig <hch@xxxxxxxxxxxxx>
Date: Sat, 24 Mar 2012 09:36:59 -0400
Cc: xfs@xxxxxxxxxxx
In-reply-to: <1332393313-1955-8-git-send-email-david@xxxxxxxxxxxxx>
References: <1332393313-1955-1-git-send-email-david@xxxxxxxxxxxxx> <1332393313-1955-8-git-send-email-david@xxxxxxxxxxxxx>
User-agent: Mutt/1.5.21 (2010-09-15)
On Thu, Mar 22, 2012 at 04:15:12PM +1100, Dave Chinner wrote:
> From: Dave Chinner <dchinner@xxxxxxxxxx>
> xfs_ioc_fstrim() doesn't treat the incoming offset and length
> correctly. It treats them as a filesystem block address, rather than
> a disk address. This is wrong because the range passed in is a
> linear representation , while the filesystem block address notiation
> is a sparse representation. Hence we cannot convert the range direct
> to filesystem block units and then use that for calculating the
> range to trim.
> While this sounds dangerous, the problem is limited to calculting
> what AGs need to be trimmed. The code that calcuates the actual
> ranges to trim gets the right result (i.e. only ever discards free
> space), even though it uses the wrong ranges to limit what is
> trimmed. Hence this is not a bug that endangers user data.
> Fix this by treating the range as a disk address range and use the
> appropriate functions to convert the range into the desired formats
> for calculations.
> Further, fix the first free extent lookup (the longest) to actually
> find the largest free extent. Currently this lookup uses a <=
> lookup, which results in finding the extent to the left of the
> largest because we can never get an exact match on the largest
> extent. This is due to the fact that while we know it's size, we
> don't know it's location and so the exact match fails and we move
> one record to the left to get the next largest extent. Instead, use
> a >= search so that the lookup returns the largest extent regardless
> of the fact we don't get an exact match on it.
> Signed-off-by: Dave Chinner <dchinner@xxxxxxxxxx>

Looks good with the minor nitpick below.

Reviewed-by: Christoph Hellwig <hch@xxxxxx>

> -     error = xfs_alloc_lookup_le(cur, 0,
> +     error = xfs_alloc_lookup_ge(cur, 0,
>                           be32_to_cpu(XFS_BUF_TO_AGF(agbp)->agf_longest), &i);

After this change xfs_alloc_lookup_le can be marked static in

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