Re: [PATCH 06/15] mm: teach truncate_inode_pages_range() to handle non page aligned ranges

[Hugh Dickins <hughd@google.com>]

Urrgh, now I messed up trying to correct linux-mm: resend to fix.

On Mon, 20 Aug 2012, Lukas Czerner wrote:
> On Sun, 19 Aug 2012, Hugh Dickins wrote:
> > 
> > This looks good to me.  I like the way you provide the same args
> > to do_invalidatepage_range() as to zero_user_segment():
> > 
> > 		zero_user_segment(page, partial_start, top);
> > 		if (page_has_private(page))
> > 			do_invalidatepage_range(page, partial_start, top);
> > 
> > Unfortunately, that is not what patches 01-05 are expecting...
> 
> Thank for the review Hugh. The fact is that the third argument of
> the invalidatepage_range() was meant to be length and the problem is
> actually in this patch, where I am passing end offset as the third
> argument.
> 
> But you've made it clear that you would like better the semantics
> where the third argument is actually the end offset. Is that right ?
> If so, I'll change it accordingly, otherwise I'll just fix this
> patch.

I do get irritated by gratuitous differences between function calling
conventions, so yes, I liked that you (appeared to) follow
zero_user_segment() here.

However, I don't think my opinion and that precedent are very important
in this case.  What do the VFS people think makes the most sensible
interface for ->invalidatepage_range()?  page, startoffset-within-page,
length-within-page or page, startoffset-within-page, endoffset-within-page?
(where "within" may actually take you to the end of the page).

If they think 3rd arg should be length (and I'd still suggest unsigned
int for both 2nd and 3rd argument, to make it clearer that it's inside
the page, not an erroneous use of unsigned long for ssize_t or loff_t),
that's okay by me.

I can see advantages to length, actually: it's often unclear
whether "end" is of the "last-of-this" or "start-of-next" variety;
in most of mm we are consistent in using end in the start-of-next
sense, but here truncate_inode_pages_range() itself has gone for
the last-of-this meaning.

But even you keep to length, you still need to go through patches 01-05,
changing block_invalidatepage() etc. to
	block_invalidatepage_range(page, offset, PAGE_CACHE_SIZE - offset);
and removing (or more probably replacing by some BUG_ONs for now) the
strange "(stop < length)" stuff in the invalidatatepage_range()s.

I do not think it's a good idea to be lenient about out-of-range args
there: that approach has already wasted time.

Hugh

--
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>