[PATCH] mm for fs: add truncate_pagecache_range

[PATCH] mm for fs: add truncate_pagecache_range

[Hugh Dickins]

Holepunching filesystems ext4 and xfs are using truncate_inode_pages_range
but forgetting to unmap pages first (ocfs2 remembers).  This is not really
a bug, since races already require truncate_inode_page() to handle that
case once the page is locked; but it can be very inefficient if the file
being punched happens to be mapped into many vmas.

Provide a drop-in replacement truncate_pagecache_range() which does the
unmapping pass first, handling the awkward mismatch between arguments
to truncate_inode_pages_range() and arguments to unmap_mapping_range().

Note that holepunching does not unmap privately COWed pages in the range:
POSIX requires that we do so when truncating, but it's hard to justify,
difficult to implement without an i_size cutoff, and no filesystem is
attempting to implement it.

Signed-off-by: Hugh Dickins <hughd@google.com>
---
I do have patches for ext4, ocfs2 and xfs to use this, but they're too
late now for v3.4.  However, it would be helpful if this function could
go ahead into v3.4, so filesystems can convert to it at leisure afterwards.

 include/linux/mm.h |    2 +-
 mm/truncate.c      |   40 ++++++++++++++++++++++++++++++++++++++++
 2 files changed, 41 insertions(+), 1 deletion(-)

--- linux.git/include/linux/mm.h	2012-03-23 10:19:53.364051630 -0700
+++ linux/include/linux/mm.h	2012-03-23 10:40:06.036080706 -0700
@@ -953,7 +953,7 @@ extern void truncate_pagecache(struct in
 extern void truncate_setsize(struct inode *inode, loff_t newsize);
 extern int vmtruncate(struct inode *inode, loff_t offset);
 extern int vmtruncate_range(struct inode *inode, loff_t offset, loff_t end);
-
+void truncate_pagecache_range(struct inode *inode, loff_t offset, loff_t end);
 int truncate_inode_page(struct address_space *mapping, struct page *page);
 int generic_error_remove_page(struct address_space *mapping, struct page *page);
 
--- linux.git/mm/truncate.c	2012-03-23 10:19:53.588051635 -0700
+++ linux/mm/truncate.c	2012-03-23 10:40:06.036080706 -0700
@@ -626,3 +626,43 @@ int vmtruncate_range(struct inode *inode
 
 	return 0;
 }
+
+/**
+ * truncate_pagecache_range - unmap and remove pagecache that is hole-punched
+ * @inode: inode
+ * @lstart: offset of beginning of hole
+ * @lend: offset of last byte of hole
+ *
+ * This function should typically be called before the filesystem
+ * releases resources associated with the freed range (eg. deallocates
+ * blocks). This way, pagecache will always stay logically coherent
+ * with on-disk format, and the filesystem would not have to deal with
+ * situations such as writepage being called for a page that has already
+ * had its underlying blocks deallocated.
+ */
+void truncate_pagecache_range(struct inode *inode, loff_t lstart, loff_t lend)
+{
+	struct address_space *mapping = inode->i_mapping;
+	loff_t unmap_start = round_up(lstart, PAGE_SIZE);
+	loff_t unmap_end = round_down(1 + lend, PAGE_SIZE) - 1;
+	/*
+	 * This rounding is currently just for example: unmap_mapping_range
+	 * expands its hole outwards, whereas we want it to contract the hole
+	 * inwards.  However, existing callers of truncate_pagecache_range are
+	 * doing their own page rounding first; and truncate_inode_pages_range
+	 * currently BUGs if lend is not pagealigned-1 (it handles partial
+	 * page at start of hole, but not partial page at end of hole).  Note
+	 * unmap_mapping_range allows holelen 0 for all, and we allow lend -1.
+	 */
+
+	/*
+	 * Unlike in truncate_pagecache, unmap_mapping_range is called only
+	 * once (before truncating pagecache), and without "even_cows" flag:
+	 * hole-punching should not remove private COWed pages from the hole.
+	 */
+	if ((u64)unmap_end > (u64)unmap_start)
+		unmap_mapping_range(mapping, unmap_start,
+				    1 + unmap_end - unmap_start, 0);
+	truncate_inode_pages_range(mapping, lstart, lend);
+}
+EXPORT_SYMBOL(truncate_pagecache_range);

--
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/ .
Fight unfair telecom internet charges in Canada: sign http://stopthemeter.ca/
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH] mm for fs: add truncate_pagecache_range

[Andrew Morton]

On Fri, 23 Mar 2012 13:46:35 -0700 (PDT)
Hugh Dickins <hughd@google.com> wrote:

> +/**
> + * truncate_pagecache_range - unmap and remove pagecache that is hole-punched
> + * @inode: inode
> + * @lstart: offset of beginning of hole
> + * @lend: offset of last byte of hole
> + *
> + * This function should typically be called before the filesystem
> + * releases resources associated with the freed range (eg. deallocates
> + * blocks). This way, pagecache will always stay logically coherent
> + * with on-disk format, and the filesystem would not have to deal with
> + * situations such as writepage being called for a page that has already
> + * had its underlying blocks deallocated.
> + */

--- a/mm/truncate.c~mm-for-fs-add-truncate_pagecache_range-fix
+++ a/mm/truncate.c
@@ -639,6 +639,9 @@ int vmtruncate_range(struct inode *inode
  * with on-disk format, and the filesystem would not have to deal with
  * situations such as writepage being called for a page that has already
  * had its underlying blocks deallocated.
+ *
+ * Must be called with inode->i_mapping->i_mutex held.
+ * Takes inode->i_mapping->i_mmap_mutex.
  */
 void truncate_pagecache_range(struct inode *inode, loff_t lstart, loff_t lend)
 {

yes?

--
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/ .
Fight unfair telecom internet charges in Canada: sign http://stopthemeter.ca/
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH] mm for fs: add truncate_pagecache_range

[Hugh Dickins]

On Fri, 23 Mar 2012, Andrew Morton wrote:
> 
> --- a/mm/truncate.c~mm-for-fs-add-truncate_pagecache_range-fix
> +++ a/mm/truncate.c
> @@ -639,6 +639,9 @@ int vmtruncate_range(struct inode *inode
>   * with on-disk format, and the filesystem would not have to deal with
>   * situations such as writepage being called for a page that has already
>   * had its underlying blocks deallocated.
> + *
> + * Must be called with inode->i_mapping->i_mutex held.

You catch me offguard: I forget whether that's an absolute requirement or
just commonly the case.  What do the other interfaces in truncate.c say ?-)

> + * Takes inode->i_mapping->i_mmap_mutex.

Yes, and inode->i_mapping->tree_lock.

>   */
>  void truncate_pagecache_range(struct inode *inode, loff_t lstart, loff_t lend)
>  {
> 
> yes?

Probably!

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/ .
Fight unfair telecom internet charges in Canada: sign http://stopthemeter.ca/
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH] mm for fs: add truncate_pagecache_range

[Andrew Morton]

On Fri, 23 Mar 2012 14:14:54 -0700 (PDT)
Hugh Dickins <hughd@google.com> wrote:

> On Fri, 23 Mar 2012, Andrew Morton wrote:
> > 
> > --- a/mm/truncate.c~mm-for-fs-add-truncate_pagecache_range-fix
> > +++ a/mm/truncate.c
> > @@ -639,6 +639,9 @@ int vmtruncate_range(struct inode *inode
> >   * with on-disk format, and the filesystem would not have to deal with
> >   * situations such as writepage being called for a page that has already
> >   * had its underlying blocks deallocated.
> > + *
> > + * Must be called with inode->i_mapping->i_mutex held.
> 
> You catch me offguard: I forget whether that's an absolute requirement or
> just commonly the case.  What do the other interfaces in truncate.c say ?-)

i_mutex is generally required, to stabilise i_size.

> > + * Takes inode->i_mapping->i_mmap_mutex.
> 
> Yes, and inode->i_mapping->tree_lock.

I don't think it's necessary to get into the spinning locks for a
high-level function which clearly does sleeping things.


--
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/ .
Fight unfair telecom internet charges in Canada: sign http://stopthemeter.ca/
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH] mm for fs: add truncate_pagecache_range

[Hugh Dickins]

On Fri, 23 Mar 2012, Andrew Morton wrote:
> On Fri, 23 Mar 2012 14:14:54 -0700 (PDT)
> Hugh Dickins <hughd@google.com> wrote:
> > On Fri, 23 Mar 2012, Andrew Morton wrote:
> > > 
> > > --- a/mm/truncate.c~mm-for-fs-add-truncate_pagecache_range-fix
> > > +++ a/mm/truncate.c
> > > @@ -639,6 +639,9 @@ int vmtruncate_range(struct inode *inode
> > >   * with on-disk format, and the filesystem would not have to deal with
> > >   * situations such as writepage being called for a page that has already
> > >   * had its underlying blocks deallocated.
> > > + *
> > > + * Must be called with inode->i_mapping->i_mutex held.
> > 
> > You catch me offguard: I forget whether that's an absolute requirement or
> > just commonly the case.  What do the other interfaces in truncate.c say ?-)
> 
> i_mutex is generally required, to stabilise i_size.

Sorry for being quarrelsome, but I do want to Nak your followup "fix".

Building a test kernel quickly told me that inode->i_mapping->i_mutex
doesn't exist, of course it's inode->i_mutex.

Then running the test kernel quickly told me that neither ext4 nor xfs
(I didn't try ocfs2) holds inode->i_mutex where holepunching calls
truncate_inode_pages_range().

Now, there might or might not be reasons why ext4 or xfs ought to hold
i_mutex there for its own consistency, but it's beyond me to determine
that: let's assume they're correct without evidence to the contrary.

Stabilizing i_size is not a reason: holepunching does not affect i_size
and is not affected by i_size (okay, ext4 still has the bug I reported
a couple of months ago, whereby its holepunching stops at i_size,
forgetting blocks fallocated beyond; but no doubt that will get fixed).

And nothing that truncate_pagecache_range() does needs i_mutex:
neither the unmap_mapping_range() nor the truncate_inode_pages_range()
needs i_mutex.  A year ago, yes, Miklos showed how unmap_mapping_range()
was relying on mutex serialization, and added an additional mutex for
that, which Peter was able to remove once he mutified i_mmap_lock.

truncate_pagecache_range() is just a drop-in replacement for
truncate_inode_pages_range(), and has no different locking needs.

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/ .
Fight unfair telecom internet charges in Canada: sign http://stopthemeter.ca/
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH] mm for fs: add truncate_pagecache_range

[Andrew Morton]

On Sun, 25 Mar 2012 13:26:10 -0700 (PDT) Hugh Dickins <hughd@google.com> wrote:

> Building a test kernel quickly told me that inode->i_mapping->i_mutex
> doesn't exist, of course it's inode->i_mutex.
> 
> Then running the test kernel quickly told me that neither ext4 nor xfs
> (I didn't try ocfs2) holds inode->i_mutex where holepunching calls
> truncate_inode_pages_range().
> 
> Now, there might or might not be reasons why ext4 or xfs ought to hold
> i_mutex there for its own consistency, but it's beyond me to determine
> that: let's assume they're correct without evidence to the contrary.
> 
> Stabilizing i_size is not a reason: holepunching does not affect i_size
> and is not affected by i_size (okay, ext4 still has the bug I reported
> a couple of months ago, whereby its holepunching stops at i_size,
> forgetting blocks fallocated beyond; but no doubt that will get fixed).
> 
> And nothing that truncate_pagecache_range() does needs i_mutex:
> neither the unmap_mapping_range() nor the truncate_inode_pages_range()
> needs i_mutex.  A year ago, yes, Miklos showed how unmap_mapping_range()
> was relying on mutex serialization, and added an additional mutex for
> that, which Peter was able to remove once he mutified i_mmap_lock.
> 
> truncate_pagecache_range() is just a drop-in replacement for
> truncate_inode_pages_range(), and has no different locking needs.

Does anything prevent new pages from getting added to pagecache and
perhaps faulted into VMAs after or during the execution of these
functions?

Also, I wonder what prevents pages in the range from being dirtied
between ext4_ext_punch_hole()'s filemap_write_and_wait_range() and
truncate_inode_pages_range().

--
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/ .
Fight unfair telecom internet charges in Canada: sign http://stopthemeter.ca/
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH] mm for fs: add truncate_pagecache_range

[Hugh Dickins]

On Sun, 25 Mar 2012, Andrew Morton wrote:
> On Sun, 25 Mar 2012 13:26:10 -0700 (PDT) Hugh Dickins <hughd@google.com> wrote:
> > truncate_pagecache_range() is just a drop-in replacement for
> > truncate_inode_pages_range(), and has no different locking needs.
> 
> Does anything prevent new pages from getting added to pagecache and
> perhaps faulted into VMAs after or during the execution of these
> functions?

If a page is faulted into a vma after the unmap_mapping_range() but
before truncate_inode_pages_range() reaches it, then it gets unmapped
by the fallback unmap_mapping_range(), called from truncate_inode_page()
while holding page lock.

A new page could be faulted in a moment after; but last year I did
change truncate_inode_pages_range() slightly, pinching down on the range
instead of just the ascending linear scan, so it doesn't return until
the range is empty of pages (excepting rcu races, which I think mean
there's no exact instant of return which all cpus would agree upon).

A new page could be faulted in a moment after that, and then it survives:
unlike in the truncation case, there's no equivalent of i_size to
determine whether to SIGBUS.  (But even in the truncation case, a
truncate or write to increase i_size may follow an instant later.)

Individual filesystems may impose additional constraints to guarantee
their own internal consistency; and tmpfs certainly finds inode->i_mutex
useful for that, to serialize between holepunch and truncate and write.
I wouldn't be surprised if other filesystems found it useful too,
but that's up to them - truncate_pagecache_range() doesn't need it.

> 
> Also, I wonder what prevents pages in the range from being dirtied
> between ext4_ext_punch_hole()'s filemap_write_and_wait_range() and
> truncate_inode_pages_range().

I'm not going to guess on that, or whether it matters: Ted?

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/ .
Fight unfair telecom internet charges in Canada: sign http://stopthemeter.ca/
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH] mm for fs: add truncate_pagecache_range

[Joel Becker]

On Sun, Mar 25, 2012 at 01:26:10PM -0700, Hugh Dickins wrote:
> On Fri, 23 Mar 2012, Andrew Morton wrote:
> > On Fri, 23 Mar 2012 14:14:54 -0700 (PDT)
> > Hugh Dickins <hughd@google.com> wrote:
> > > On Fri, 23 Mar 2012, Andrew Morton wrote:
> > > > 
> > > > --- a/mm/truncate.c~mm-for-fs-add-truncate_pagecache_range-fix
> > > > +++ a/mm/truncate.c
> > > > @@ -639,6 +639,9 @@ int vmtruncate_range(struct inode *inode
> > > >   * with on-disk format, and the filesystem would not have to deal with
> > > >   * situations such as writepage being called for a page that has already
> > > >   * had its underlying blocks deallocated.
> > > > + *
> > > > + * Must be called with inode->i_mapping->i_mutex held.
> > > 
> > > You catch me offguard: I forget whether that's an absolute requirement or
> > > just commonly the case.  What do the other interfaces in truncate.c say ?-)
> > 
> > i_mutex is generally required, to stabilise i_size.
> 
> Sorry for being quarrelsome, but I do want to Nak your followup "fix".
> 
> Building a test kernel quickly told me that inode->i_mapping->i_mutex
> doesn't exist, of course it's inode->i_mutex.
> 
> Then running the test kernel quickly told me that neither ext4 nor xfs
> (I didn't try ocfs2) holds inode->i_mutex where holepunching calls
> truncate_inode_pages_range().

Just for completeness:

ocfs2 holds i_mutex around the entire ocfs2_change_file_space() call,
which can do hole punching and unwritten extent allocation (it is a
clone of xfs_change_file_space()).  xfs itself seems hold its own idea
of a shared lock while doing the work and an exclusive lock around the
transaction join.  I'm not clear enough about xfs to say how this
compares or even if I read it right.

But ocfs2 uses an allocation sem to protect allocation changes,
including i_size, so perhaps i_mutex isn't strictly necessary.  I don't
think we contend enough here to try hard to remove it :-)

Joel

-- 

"The question of whether computers can think is just like the question
 of whether submarines can swim."
	- Edsger W. Dijkstra

			http://www.jlbec.org/
			jlbec@evilplan.org

--
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/ .
Fight unfair telecom internet charges in Canada: sign http://stopthemeter.ca/
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH] mm for fs: add truncate_pagecache_range

[Hugh Dickins]

On Fri, 23 Mar 2012, Hugh Dickins wrote:
> I do have patches for ext4, ocfs2 and xfs to use this, but they're too
> late now for v3.4.  However, it would be helpful if this function could
> go ahead into v3.4, so filesystems can convert to it at leisure afterwards.

I just sent out the little ext4 and xfs patches, but decided not
to bother you with the ocfs2 one.  ocfs2 is already doing it right with
unmap_mapping_range; and since file.c is using unmap_mapping_range with
truncate_inode_pages in other places, it seemed wrong to force a different
convention upon you in this one place (perhaps they can all be converted
to truncate_pagecache_range, but if it ain't broke...)

Hugh

Re: [PATCH] mm for fs: add truncate_pagecache_range

[Joel Becker]

On Sun, May 13, 2012 at 02:03:03PM -0700, Hugh Dickins wrote:
> On Fri, 23 Mar 2012, Hugh Dickins wrote:
> > I do have patches for ext4, ocfs2 and xfs to use this, but they're too
> > late now for v3.4.  However, it would be helpful if this function could
> > go ahead into v3.4, so filesystems can convert to it at leisure afterwards.
> 
> I just sent out the little ext4 and xfs patches, but decided not
> to bother you with the ocfs2 one.  ocfs2 is already doing it right with
> unmap_mapping_range; and since file.c is using unmap_mapping_range with
> truncate_inode_pages in other places, it seemed wrong to force a different
> convention upon you in this one place (perhaps they can all be converted
> to truncate_pagecache_range, but if it ain't broke...)

Works for me.  Thanks.

Joel

> 
> Hugh

-- 

"There are only two ways to live your life. One is as though nothing
 is a miracle. The other is as though everything is a miracle."
        - Albert Einstein

			http://www.jlbec.org/
			jlbec@evilplan.org