xfs
[Top] [All Lists]

Re: [PATCH 02.5/32] xfs: remove xfs_tosspages

To: Dave Chinner <david@xxxxxxxxxxxxx>
Subject: Re: [PATCH 02.5/32] xfs: remove xfs_tosspages
From: Andrew Dahl <adahl@xxxxxxx>
Date: Wed, 21 Nov 2012 23:10:02 -0600
Cc: Mark Tinguely <tinguely@xxxxxxx>, <xfs@xxxxxxxxxxx>
In-reply-to: <20121121080502.GP2591@dastard>
References: <1352721264-3700-1-git-send-email-david@xxxxxxxxxxxxx> <1352721264-3700-3-git-send-email-david@xxxxxxxxxxxxx> <20121114064247.GC1710@dastard> <50A3E807.5010403@xxxxxxx> <50A3E86A.2060402@xxxxxxx> <50A3F80C.7050502@xxxxxxx> <20121121080502.GP2591@dastard>
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:16.0) Gecko/20121029 Thunderbird/16.0.2
On 11/21/2012 02:05 AM, Dave Chinner wrote:
...
> 
> [ Here's a tip for the future: anything that changes allocation
> corner cases needs to be run through the entire of xfstests suite
> because they have a nasty habit of causing secondary problems.... ]
> 
Makes sense -- I'll keep that in mind for the future. (Thanks!)

...

> +
> +STATIC int
> +xfs_zero_file_space(
> +     struct xfs_inode        *ip,
> +     xfs_off_t               offset,
> +     xfs_off_t               len,
> +     int                     attr_flags)
> +{
> +     struct xfs_mount        *mp = ip->i_mount;
> +     uint                    rounding;
> +     xfs_off_t               start;
> +     xfs_off_t               end;
> +     int                     error;
> +
> +     rounding = max_t(uint, 1 << mp->m_sb.sb_blocklog, PAGE_CACHE_SIZE);
Let's say rounding is 4K
> +
> +     /* round the range iof extents we are going to convert inwards */
> +     start = round_up(offset, rounding);
> +     end = round_down(offset + len, rounding);
Now, let's say we pass in (4K-1) for the offset and (4K-1).

Then start would be 4K and the end would be 4K, right?

> +
> +     ASSERT(start >= offset);
> +     ASSERT(end <= offset + len);
These are both true, so this is good.
> +
> +     if (!(attr_flags & XFS_ATTR_NOLOCK))
> +             xfs_ilock(ip, XFS_IOLOCK_EXCL);
> +
> +     if (start < end - 1) {
This is false, as expected.
> +             /* punch out the page cache over the conversion range */
> +             truncate_pagecache_range(VFS_I(ip), start, end - 1);
> +             /* convert the blocks */
> +             error = xfs_alloc_file_space(ip, start, end - start - 1,
> +                                 XFS_BMAPI_PREALLOC | XFS_BMAPI_CONVERT,
> +                                 attr_flags);
> +             if (error)
> +                     goto out_unlock;
> +     } else {
> +             /* it's a sub-rounding range */
> +             ASSERT(offset + len <= rounding);
This is false. (8K - 2) <= 4K -- Not so good.

Maybe (2*rounding) would be better, as offset + len could never be
greater than 2rounding (but can be greater than 1rounding). Or removing
this assert altogether.

> +             error = xfs_iozero(ip, offset, len);
> +             goto out_unlock;
> +     }
> +
> +     /* now we've handled the interior of the range, handle the edges */
> +     if (start != offset)
> +             error = xfs_iozero(ip, offset, start - offset);
> +     if (!error && end != offset + len)
> +             error = xfs_iozero(ip, end, offset + len - end);
This looks good.
> +
> +out_unlock:
> +     if (!(attr_flags & XFS_ATTR_NOLOCK))
> +             xfs_iunlock(ip, XFS_IOLOCK_EXCL);
> +     return error;
...

Beyond that, I think it all looks good and like what you've done!

Thanks,

Andrew

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