Adding an ugliness in __read_cache_page()?

Adding an ugliness in __read_cache_page()?

[Hugh Dickins]

Linus, Christoph,

Could you bear a patch something like the one below?

I have a series aimed at 2.6.41 to remove mm/shmem.c's peculiar radix
tree of swap entries, using slots in the file's standard radix_tree
instead - prompted in part by https://lkml.org/lkml/2011/1/22/110

In isolating the occurrence of swap entries in radix_trees, it becomes
convenient to remove shmem's ->readpage at last, giving it instead its
own ->splice_read.

But drivers/gpu/drm i915 and ttm are using read_cache_page_gfp() or
read_mapping_page() on tmpfs: on objects created by shmem_file_setup().

Nothing else uses read_cache_page_gfp().  I cannot find anything else
using read_mapping_page() on tmpfs, but wonder if something might be
out there.  Stacked filesystems appear not to go that way nowadays.

Would it be better to make i915 and ttm call shmem_read_cache_page()
directly?  Perhaps removing the then unused read_cache_page_gfp(), or
perhaps not: may still be needed for i915 and ttm on tiny !SHMEM ramfs.

I find both ways ugly, but no nice alternative: introducing a new method
when the known callers are already tied to tmpfs/ramfs seems over the top.

Thanks,
Hugh

---

 include/linux/mm.h |    1 +
 mm/filemap.c       |   13 +++++++++++++
 mm/shmem.c         |   26 +++++++++++++++-----------
 3 files changed, 29 insertions(+), 11 deletions(-)

--- 2.6.39/include/linux/mm.h	2011-05-18 21:06:34.000000000 -0700
+++ linux/include/linux/mm.h	2011-05-22 11:50:49.332431949 -0700
@@ -873,6 +873,7 @@ extern void __show_free_areas(unsigned i
 int shmem_lock(struct file *file, int lock, struct user_struct *user);
 struct file *shmem_file_setup(const char *name, loff_t size, unsigned long flags);
 int shmem_zero_setup(struct vm_area_struct *);
+struct page *shmem_read_cache_page(struct address_space *, pgoff_t, gfp_t);
 
 #ifndef CONFIG_MMU
 extern unsigned long shmem_get_unmapped_area(struct file *file,
--- 2.6.39/mm/filemap.c	2011-05-18 21:06:34.000000000 -0700
+++ linux/mm/filemap.c	2011-05-22 11:50:49.332431949 -0700
@@ -1762,6 +1762,19 @@ static struct page *__read_cache_page(st
 {
 	struct page *page;
 	int err;
+
+#ifdef CONFIG_TMPFS
+	/*
+	 * The ->readpage() interface does not suit tmpfs at all, since it
+	 * may have pages in swapcache, and needs to find those for itself;
+	 * but gpu/drm/i915 and gpu/drm/ttm need it to support this function.
+	 */
+	if (!filler) {
+		BUG_ON(!mapping_cap_swap_backed(mapping));
+		return shmem_read_cache_page(mapping, index, gfp);
+	}
+#endif
+
 repeat:
 	page = find_get_page(mapping, index);
 	if (!page) {
--- 2.6.39/mm/shmem.c	2011-05-18 21:06:34.000000000 -0700
+++ linux/mm/shmem.c	2011-05-22 11:50:49.332431949 -0700
@@ -1652,17 +1652,22 @@ static struct inode *shmem_get_inode(str
 static const struct inode_operations shmem_symlink_inode_operations;
 static const struct inode_operations shmem_symlink_inline_operations;
 
-/*
- * Normally tmpfs avoids the use of shmem_readpage and shmem_write_begin;
- * but providing them allows a tmpfs file to be used for splice, sendfile, and
- * below the loop driver, in the generic fashion that many filesystems support.
- */
-static int shmem_readpage(struct file *file, struct page *page)
+struct page *
+shmem_read_cache_page(struct address_space *mapping, pgoff_t index, gfp_t gfp)
 {
-	struct inode *inode = page->mapping->host;
-	int error = shmem_getpage(inode, page->index, &page, SGP_CACHE, NULL);
-	unlock_page(page);
-	return error;
+	struct page *page;
+	int error;
+
+	/*
+	 * Not shown: addition of gfp arg to shmem_getpage(), passed down here.
+	 * Not shown: addition of shmem_file_splice_read() avoiding ->readpage.
+	 */
+	error = shmem_getpage(mapping->host, index, &page, SGP_CACHE, NULL);
+	if (error)
+		page = ERR_PTR(error);
+	else
+		unlock_page(page);
+	return page;
 }
 
 static int
@@ -2475,7 +2480,6 @@ static const struct address_space_operat
 	.writepage	= shmem_writepage,
 	.set_page_dirty	= __set_page_dirty_no_writeback,
 #ifdef CONFIG_TMPFS
-	.readpage	= shmem_readpage,
 	.write_begin	= shmem_write_begin,
 	.write_end	= shmem_write_end,
 #endif

--
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: Adding an ugliness in __read_cache_page()?

[Linus Torvalds]

On Sun, May 22, 2011 at 3:25 PM, Hugh Dickins <hughd@google.com> wrote:
>
> But drivers/gpu/drm i915 and ttm are using read_cache_page_gfp() or
> read_mapping_page() on tmpfs: on objects created by shmem_file_setup().
>
> Nothing else uses read_cache_page_gfp().  I cannot find anything else
> using read_mapping_page() on tmpfs, but wonder if something might be
> out there.  Stacked filesystems appear not to go that way nowadays.
>
> Would it be better to make i915 and ttm call shmem_read_cache_page()
> directly?  Perhaps removing the then unused read_cache_page_gfp(), or
> perhaps not: may still be needed for i915 and ttm on tiny !SHMEM ramfs.

I would certainly prefer the "make i915 and ttm call
shmem_read_cache_page directly" approach over putting some nasty hack
in __read_cache_page.

                         Linus

--
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: Adding an ugliness in __read_cache_page()?

[Hugh Dickins]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 546 bytes --]

On Sun, 22 May 2011, Linus Torvalds wrote:
> On Sun, May 22, 2011 at 3:25 PM, Hugh Dickins <hughd@google.com> wrote:
> >
> > Would it be better to make i915 and ttm call shmem_read_cache_page()
> > directly?  Perhaps removing the then unused read_cache_page_gfp(), or
> > perhaps not: may still be needed for i915 and ttm on tiny !SHMEM ramfs.
> 
> I would certainly prefer the "make i915 and ttm call
> shmem_read_cache_page directly" approach over putting some nasty hack
> in __read_cache_page.

Thank you: I'll go that way.

Hugh

Re: Adding an ugliness in __read_cache_page()?

[Christoph Hellwig]

On Sun, May 22, 2011 at 03:25:31PM -0700, Hugh Dickins wrote:
> I find both ways ugly, but no nice alternative: introducing a new method
> when the known callers are already tied to tmpfs/ramfs seems over the top.

Calling into shmem directly is the less ugly variant.  Long term killing
that tmpfs abuse would be even better, but I already lost that fight
when it was initially added.

--
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: Adding an ugliness in __read_cache_page()?

[Hugh Dickins]

On Mon, 23 May 2011, Christoph Hellwig wrote:
> On Sun, May 22, 2011 at 03:25:31PM -0700, Hugh Dickins wrote:
> > I find both ways ugly, but no nice alternative: introducing a new method
> > when the known callers are already tied to tmpfs/ramfs seems over the top.
> 
> Calling into shmem directly is the less ugly variant.

Okay, that's good, thanks.

> Long term killing
> that tmpfs abuse would be even better, but I already lost that fight
> when it was initially added.

I'd better match your restraint and not fan the flames now -
I believe we're on opposite sides, or at least orthogonal on that.

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>