xfs
[Top] [All Lists]

Re: [patch 1/2] xfs: xfs_tosspages() bug

To: Andrew Dahl <adahl@xxxxxxx>
Subject: Re: [patch 1/2] xfs: xfs_tosspages() bug
From: Dave Chinner <david@xxxxxxxxxxxxx>
Date: Fri, 9 Nov 2012 10:06:49 +1100
Cc: xfs@xxxxxxxxxxx
In-reply-to: <20121108222315.626928496@xxxxxxx>
References: <20121108222315.505370321@xxxxxxx> <20121108222315.626928496@xxxxxxx>
User-agent: Mutt/1.5.21 (2010-09-15)
On Thu, Nov 08, 2012 at 04:23:16PM -0600, Andrew Dahl wrote:
> xfs_tosspages() takes a closed interval as an argument, take 
> this into account when rounding down to the last byte of the
> last complete page. If the request consists of a single 
> partial page, there will be nothing to toss. 
> 
> Signed-off-by: Andrew Dahl <adahl@xxxxxxx>
> 
> ---
> 
> Index: xfs/fs/xfs/xfs_fs_subr.c
> ===================================================================
> --- xfs.orig/fs/xfs/xfs_fs_subr.c
> +++ xfs/fs/xfs/xfs_fs_subr.c
> @@ -32,9 +32,17 @@ xfs_tosspages(
>       xfs_off_t       last,
>       int             fiopt)
>  {
> -     /* can't toss partial tail pages, so mask them out */
> -     last &= ~(PAGE_SIZE - 1);
> -     truncate_inode_pages_range(VFS_I(ip)->i_mapping, first, last - 1);
> +     /*
> +      * Can't toss partial tail pages, so mask them out.  If the only
> +      * page to toss was a partial tail, there will be nothing left
> +      * to do.
> +      */
> +     if (last != -1) {
> +             last = ((last + 1) & PAGE_MASK) - 1;
> +             if (last < first)
> +                     return;
> +     }
> +     truncate_inode_pages_range(VFS_I(ip)->i_mapping, first, last);

Ok, lets look at critical ranges:

                passed to truncate_inode_pages_range
first,last      current         patched
0,4095          0,0xffffffff    0,4095
0,4096          0,4095          0,4095
0,4097          0,4095          0,4095

Yup, that's needed.

0,1             0,0xffffffff    aborts (0,0xffffffff)

Big assumption: xfs_off_t is signed.

0xfffffffe      0xffffefff      0xfffeffff
0xffffffff      0xffffefff      0xffffffff

So the change is good.

However, there's a bigger issue here. We've planned to remove these
wrappers for a long time, just never got around to doing it. Seeing
as there is a bug in this wrapper and it needs to be fixed, now
seems like the right time to remove it.

Hence I'd suggest that fixing this particular bug should just
remove xfs_tosspages() and call truncate_inode_pages_range()
directly. There are only two calls to this function, so it should be
a simple conversion.  That can then be followed up with more patches
to remove the other wrappers in xfs_fs_subr.c and hence remove the
file completely...

>  int
> Index: xfs/fs/xfs/xfs_vnodeops.c
> ===================================================================
> --- xfs.orig/fs/xfs/xfs_vnodeops.c
> +++ xfs/fs/xfs/xfs_vnodeops.c
> @@ -2172,7 +2172,7 @@ xfs_change_file_space(
>       switch (cmd) {
>       case XFS_IOC_ZERO_RANGE:
>               prealloc_type |= XFS_BMAPI_CONVERT;
> -             xfs_tosspages(ip, startoffset, startoffset + bf->l_len, 0);
> +             xfs_tosspages(ip, startoffset, bf->l_len ? startoffset + llen : 
> -1, 0);
>               /* FALLTHRU */
>       case XFS_IOC_RESVSP:
>       case XFS_IOC_RESVSP64:

What's this hunk for? Indeed, one of the first things that the
xfs_alloc_file_space() checks is this:

        if (len <= 0)
                return XFS_ERROR(EINVAL);

xfs_free_file_space() does the same check, so it is invalid to pass
a bf_len <= 0 for any of these specific functions. Hence this change
is wrong regardless of what the comment on the struct xfs_flock64_t
says - preallocation and hole punch operations must have a positive
length associated with them.

Cheers,

Dave.
-- 
Dave Chinner
david@xxxxxxxxxxxxx

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