Re: [PATCH 3/14] tmpfs: take control of its truncate_range

[Hugh Dickins]

On Fri, 3 Jun 2011, Christoph Hellwig wrote:
> On Wed, Jun 01, 2011 at 09:58:18AM -0700, Hugh Dickins wrote:
> 
> > Fine, I'll add tmpfs PUNCH_HOLE later on.  And wire up madvise MADV_REMOVE
> > to fallocate PUNCH_HOLE, yes?
> 
> Yeah.  One thing I've noticed is that the hole punching doesn't seem
> to do the unmap_mapping_range.  It might be worth to audit that from the
> VM point of view.

I'd noticed that recently too.

At first I was alarmed, but it's actually an inefficiency rather than
a danger: because at some stage a safety unmap_mapping_range() call has
been added into truncate_inode_page().  I don't know what case that was
originally for, but it will cover fallocate() for now.

This is a call to unmap_mapping_range() with 0 for the even_cows arg
i.e. it will not remove COWed copies of the file page from private
mappings.  I think that's good semantics for hole punching (and it's
difficult to enforce the alternative, because we've neither i_size nor
page lock to prevent races); but it does differ from the (odd) POSIX
truncation behaviour, to unmap even the private COWs.

What do you think?  If you think we should unmap COWs, then it ought
to be corrrected sooner.  Otherwise I was inclined not to rush (I'm
also wondering about cleancache in truncation: that should be another
mail thread, but might call for passing down a flag useful here too).

You might notice that the alternate hole-punching's vmtruncate_range()
is passing even_cows 1: doesn't matter in practice, since that one has
been restricted to operating on shared writable mappings.  Oh, I
suppose there could also be a parallel private writable mapping,
whose COWs would get unmapped.  Hmm, worth worrying about if we
choose the opposite with fallocate hole punching?

> 
> > Would you like me to remove the ->truncate_range method from
> > inode_operations completely?
> 
> Doing that would be nice.  Do we always have the required file struct
> for ->fallocate in the callers?

Good point, but yes, no problem.

I'm carrying on using ->truncate_range for the moment, partly because
I don't want to get diverted by testing the ->fallocate alternative yet,
but also because removing ->truncate_range now would force an immediate
change on drm/i915: better use shmem_truncate_range() for the transition.

Hugh
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/