xfs
[Top] [All Lists]

Re: [PATCH 1/4] xfs: remove xfs_tosspages

To: Dave Chinner <david@xxxxxxxxxxxxx>
Subject: Re: [PATCH 1/4] xfs: remove xfs_tosspages
From: Ben Myers <bpm@xxxxxxx>
Date: Tue, 13 Nov 2012 15:29:03 -0600
Cc: Andrew Dahl <adahl@xxxxxxx>, xfs@xxxxxxxxxxx
In-reply-to: <20121112230029.GW24575@dastard>
References: <1352455804-17045-1-git-send-email-david@xxxxxxxxxxxxx> <1352455804-17045-2-git-send-email-david@xxxxxxxxxxxxx> <50A15F57.3080900@xxxxxxx> <20121112230029.GW24575@dastard>
User-agent: Mutt/1.5.20 (2009-06-14)
Hi Dave,

On Tue, Nov 13, 2012 at 10:00:29AM +1100, Dave Chinner wrote:
> On Mon, Nov 12, 2012 at 02:43:03PM -0600, Andrew Dahl wrote:
> > On 11/09/2012 04:10 AM, Dave Chinner wrote:
> > > From: Dave Chinner <dchinner@xxxxxxxxxx>
> > > 
> > > It's a buggy, unnecessary wrapper that is duplicating
> > > truncate_pagecache_range().
> > > 
> > > When replacing the call in xfs_change_file_space(), also ensure that
> > > the length being allocated/freed is always positive before making
> > > any changes. These checks are done in the lower extent manipulation
> > > functions, too, but we need to do them before any page cache
> > > operations.
> > > 
> > > Reported-by: Andrew Dahl <adahl@xxxxxxx>
> > > Signed-off-by: Dave Chinner <dchinner@xxxxxxxxxx>
> ....
> > > @@ -2169,7 +2186,8 @@ 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);
> > > +         truncate_pagecache_range(VFS_I(ip), startoffset,
> > > +                  round_down(startoffset + bf->l_len, PAGE_SIZE) - 1);
> > 
> > When calling XFS_IOC_ZERO_RANGE with a range [0, 4095] or [0,1] it's
> > tossing pages because the call to round_down() is returning 0 and
> > passing -1 in for the end will toss all pages.  So, we need to make sure
> > round_down() isn't going to return a 0, or that (startoffset +
> > bf->l_len) > PAGE_SIZE.
> 
> Right, which was the original bug. I didn't think that through
> fully....
> 
> > So, something like...
> > 
> > 
> > xfs_off_t       end;
> > 
> > [...]
> > 
> > if((end = round_down(startoffset + bf->l_len, PAGE_SIZE)) > 0) {
> >     truncate_pagecache_range(VFS_I(ip), startoffset, end - 1);
> > }
> 
> Actually, it is more complex than that. Think of the range [4096,
> 4097]. That would result in passing 4096, 4095 to
> truncate_pagecache_range(), which is also wrong but implicitly
> handled correctly in truncate_inode_pages_range(). Hence it should
> be:
> 
>               end = round_down(startoffset + bf->l_len, PAGE_SIZE) - 1;
>               if (startoffset > end)
>                       truncate_pagecache_range(VFS_I(ip), startoffset, end);
> 
> That's similar to your original fix, which had a last < first check
> in it. I'll post a fix in a few minutes...

I'd like to pull this in tomorrow.  Could you get this one sorted out today?
If not, I think we could also enlist Andrew to fix it.

Regards,
        Ben

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