Re: [PATCH 1/4] holepunch: fix shmem_truncate_range punching too far

[Miklos Szeredi <miklos@szeredi.hu>]

> > >  	} else {
> > > -		limit = (end + PAGE_CACHE_SIZE - 1) >> PAGE_CACHE_SHIFT;
> > > -		if (limit > info->next_index)
> > > -			limit = info->next_index;
> > > +		if (end + 1 >= inode->i_size) {	/* we may free a little more */
> > 
> > Why end + 1?  If the hole end is at 4096 and the file size is 4097 we
> > surely don't want to truncate that second page also?
> 
> I spent ages over that!  It's the fix to one of those silly little bugs
> that were in the interim Not-Yet-Signed-off-by version I sent you earlier.
> 
> In that interim version indeed I had just "end": I thought the original
> (see the first - line above) was wrong to be rounding up "end" to a page
> boundary, since "start" has been rounded up to a page boundary - the
> right thing would be to round up start and round down end (though it's
> academic so long as the only way here is through sys_madvise, which
> enforces page alignment of start and rounds up len).
> 
> For a long time I was mystified why my final page's swap entry wasn't
> getting punched out.  Eventually I printk'ed the arguments coming in,
> then found this line in madvise_remove:
> 
> 	endoff = (loff_t)(end - vma->vm_start - 1)
> 			+ ((loff_t)vma->vm_pgoff << PAGE_SHIFT);
> 
> It subtracts 1 from the "end" I'm used to dealing with (offset of
> byte beyond the area in question), to make it the offset of the
> last byte of the hole, then passes that down as "lend" or "end"
> to truncate_inode_pages_range and shmem_truncate_range.
> 
> Personally, I'd call that "last"; or rather, I wouldn't do it like
> that at all - I find it terribly confusing to deal with; but I
> guess Badari himself found it more natural that way, or it fitted
> better with the convention of -1 he was using for full truncation.

Oh, it _is_ confusing.

> Of course, you can then question the args I gave to the additional
> unmap_mapping_range: I dithered over that for a long time too (and
> again got it wrong in the interim version), in the end decided to say
> the same as vmtruncate_range (to avoid questions!), though strictly
> unmap_mapping_range(mapping, offset, 1 + end - offset, 1)
> would be more correct - though again, it's academic since
> unmap_mapping_range does its own rounding.

I think we should at least have a

  BUG_ON((end + 1) % PAGE_CACHE_SIZE);

or something, to remind us about this wart.

Miklos

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>