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

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

[Hugh Dickins]

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>

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

[Hugh Dickins]

On Tue, 21 Aug 2012, Lukas Czerner wrote:
> On Mon, 20 Aug 2012, Hugh Dickins wrote:
> > 
> > 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.
> 
> I really do agree with this paragraph and this is why I like the "length"
> argument better. So if there is no objections I'll stick with it and
> fix the other things you've pointed out.

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

Re: [PATCH 01/15] mm: add invalidatepage_range address space operation

[Hugh Dickins]

On Tue, 21 Aug 2012, Lukas Czerner wrote:
> On Sun, 19 Aug 2012, Hugh Dickins wrote:
> > > --- a/include/linux/fs.h
> > > +++ b/include/linux/fs.h
> > > @@ -620,6 +620,8 @@ struct address_space_operations {
> > >  	/* Unfortunately this kludge is needed for FIBMAP. Don't use it */
> > >  	sector_t (*bmap)(struct address_space *, sector_t);
> > >  	void (*invalidatepage) (struct page *, unsigned long);
> > > +	void (*invalidatepage_range) (struct page *, unsigned long,
> > > +				      unsigned long);
> > 
> > It may turn out to be bad advice, given how invalidatepage() already
> > takes an unsigned long, but I'd be tempted to make both of these args
> > unsigned int, since that helps to make it clearer that they're intended
> > to be offsets within a page, in the range 0..PAGE_CACHE_SIZE.
> > 
> > (partial_start, partial_end and top in truncate_inode_pages_range()
> > are all unsigned int.)
> 
> Hmm, this does not seem right. I can see that PAGE_CACHE_SIZE
> (PAGE_SIZE) can be defined as unsigned long, or am I missing
> something ?

They would be defined as unsigned long so that they can be used in
masks like ~(PAGE_SIZE - 1), and behave as expected on addresses,
without needing casts to be added all over.

We do not (currently!) expect PAGE_SIZE or PAGE_CACHE_SIZE to grow
beyond an unsigned int - but indeed they can be larger than what's
held in an unsigned short (look no further than ia64 or ppc64).

For more reassurance, see include/linux/highmem.h, which declares
zero_user_segments() and others: unsigned int (well, unsigned with
the int implicit) for offsets within a page.

Hugh

> > 
> > Andrew is very keen on naming arguments in prototypes,
> > and I think there is an especially strong case for it here.

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