[RFC] v9fs: readpage support

[RFC] v9fs: readpage support

[Eric Van Hensbergen]

v9fs mmap support was originally removed from v9fs at Al Viro's request, but
recently there have been requests from folks who want readpage
functionality (primarily to enable execution of files mounted via 9P).
This patch adds readpage support (but not writepage which contained most of
the objectionable code).  It passes FSX (and other regressions) so it
should be relatively safe, but I'm a little worried that I have to do more 
to invalidate anything cached by readpage.

Signed-off-by: Eric Van Hensbergen <ericvh@gmail.com>

---

 fs/9p/Makefile    |    1 
 fs/9p/v9fs_vfs.h  |    1 
 fs/9p/vfs_addr.c  |  118 +++++++++++++++++++++++++++++++++++++++++++++++++++++
 fs/9p/vfs_file.c  |    4 ++
 fs/9p/vfs_inode.c |    1 
 5 files changed, 125 insertions(+), 0 deletions(-)
 create mode 100644 fs/9p/vfs_addr.c

applies-to: 531356e3b004da443c01dac74420f4f28b91679f
4dea4edce8d34325c023c51cb693a690c972e043
diff --git a/fs/9p/Makefile b/fs/9p/Makefile
index e4e4ffe..1253f3b 100644
--- a/fs/9p/Makefile
+++ b/fs/9p/Makefile
@@ -3,6 +3,7 @@ obj-$(CONFIG_9P_FS) := 9p2000.o
 9p2000-objs := \
 	vfs_super.o \
 	vfs_inode.o \
+	vfs_addr.o \
 	vfs_file.o \
 	vfs_dir.o \
 	vfs_dentry.o \
diff --git a/fs/9p/v9fs_vfs.h b/fs/9p/v9fs_vfs.h
index 2f2cea7..27d0d5b 100644
--- a/fs/9p/v9fs_vfs.h
+++ b/fs/9p/v9fs_vfs.h
@@ -39,6 +39,7 @@
  */
 
 extern struct file_system_type v9fs_fs_type;
+extern struct address_space_operations v9fs_addr_operations;
 extern struct file_operations v9fs_file_operations;
 extern struct file_operations v9fs_dir_operations;
 extern struct dentry_operations v9fs_dentry_operations;
diff --git a/fs/9p/vfs_addr.c b/fs/9p/vfs_addr.c
new file mode 100644
index 0000000..d30601a
--- /dev/null
+++ b/fs/9p/vfs_addr.c
@@ -0,0 +1,118 @@
+/*
+ *  linux/fs/9p/vfs_addr.c
+ *
+ * This file contians vfs address (mmap) ops for 9P2000. 
+ *
+ *  Copyright (C) 2005 by Eric Van Hensbergen <ericvh@gmail.com>
+ *  Copyright (C) 2002 by Ron Minnich <rminnich@lanl.gov>
+ *
+ *  This program is free software; you can redistribute it and/or modify
+ *  it under the terms of the GNU General Public License as published by
+ *  the Free Software Foundation; either version 2 of the License, or
+ *  (at your option) any later version.
+ *
+ *  This program is distributed in the hope that it will be useful,
+ *  but WITHOUT ANY WARRANTY; without even the implied warranty of
+ *  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ *  GNU General Public License for more details.
+ *
+*  You should have received a copy of the GNU General Public License
+ *  along with this program; if not, write to:
+ *  Free Software Foundation
+ *  51 Franklin Street, Fifth Floor
+ *  Boston, MA  02111-1301  USA
+ *
+ */
+
+#include <linux/module.h>
+#include <linux/errno.h>
+#include <linux/fs.h>
+#include <linux/file.h>
+#include <linux/stat.h>
+#include <linux/string.h>
+#include <linux/smp_lock.h>
+#include <linux/inet.h>
+#include <linux/version.h>
+#include <linux/pagemap.h>
+#include <linux/idr.h>
+
+#include "debug.h"
+#include "v9fs.h"
+#include "9p.h"
+#include "v9fs_vfs.h"
+#include "fid.h"
+
+/*
+ * v9fs_vfs_readpage - read an entire page in from 9P
+ *
+ * @file: file being read 
+ * @page: structure to page
+ *
+ */
+
+static int v9fs_vfs_readpage(struct file *filp, struct page *page)
+{
+	char *buffer = NULL;
+	int retval = -EIO;
+	loff_t offset = ((loff_t) page->index) << PAGE_CACHE_SHIFT;
+	int count = PAGE_CACHE_SIZE;
+	struct inode *inode = filp->f_dentry->d_inode;
+	struct v9fs_session_info *v9ses = v9fs_inode2v9ses(inode);
+	int rsize = v9ses->maxdata - V9FS_IOHDRSZ;
+	struct v9fs_fid *v9f = filp->private_data;
+	struct v9fs_fcall *fcall = NULL;
+	int fid = v9f->fid;
+	int total = 0;
+	int result = 0;
+
+	page_cache_get(page);
+	buffer = kmap(page);
+
+	do {
+		if (count < rsize)
+			rsize = count;
+
+		result = v9fs_t_read(v9ses, fid, offset, rsize, &fcall);
+
+		if (result < 0) {
+			printk(KERN_ERR "v9fs_t_read returned %d\n",
+			       result);
+
+			kfree(fcall);
+			goto Error;
+		} else
+			offset += result;
+
+		memcpy(buffer, fcall->params.rread.data, result);
+
+		count -= result;
+		buffer += result;
+		total += result;
+
+		kfree(fcall);
+
+		if (result < rsize)
+			break;
+	} while (count);
+
+	if (total < count) {
+		count = count - total;
+		memset(buffer + total, 0, count);
+	}
+
+	flush_dcache_page(page);
+	SetPageUptodate(page);
+	if (PageError(page))
+		ClearPageError(page);
+	retval = 0;
+
+      Error:
+	kunmap(page);
+	unlock_page(page);
+	page_cache_release(page);
+	return retval;
+}
+
+struct address_space_operations v9fs_addr_operations = {
+      .readpage = v9fs_vfs_readpage,
+};
diff --git a/fs/9p/vfs_file.c b/fs/9p/vfs_file.c
index 89c849d..f332f65 100644
--- a/fs/9p/vfs_file.c
+++ b/fs/9p/vfs_file.c
@@ -306,6 +306,9 @@ v9fs_file_write(struct file *filp, const
 		total += result;
 	} while (count);
 
+	if ((inode->i_mapping) && (inode->i_mapping->nrpages))
+		invalidate_inode_pages2(inode->i_mapping);
+
 	kfree(buf);
 	return total;
 }
@@ -317,4 +320,5 @@ struct file_operations v9fs_file_operati
 	.open = v9fs_file_open,
 	.release = v9fs_dir_release,
 	.lock = v9fs_file_lock,
+	.mmap = generic_file_mmap,
 };
diff --git a/fs/9p/vfs_inode.c b/fs/9p/vfs_inode.c
index be72881..4571290 100644
--- a/fs/9p/vfs_inode.c
+++ b/fs/9p/vfs_inode.c
@@ -246,6 +246,7 @@ struct inode *v9fs_get_inode(struct supe
 		inode->i_blocks = 0;
 		inode->i_rdev = 0;
 		inode->i_atime = inode->i_mtime = inode->i_ctime = CURRENT_TIME;
+		inode->i_mapping->a_ops = &v9fs_addr_operations;
 
 		switch (mode & S_IFMT) {
 		case S_IFIFO:
---
0.99.9j

Re: [RFC] v9fs: readpage support

[Christoph Hellwig]

> +/*
> + * v9fs_vfs_readpage - read an entire page in from 9P
> + *
> + * @file: file being read 
> + * @page: structure to page
> + *
> + */

this isn't actually a valid kerneldoc comment, they start with /**
e.g.

/**
 * v9fs_vfs_readpage  -  read an entire page in from 9P
 * @file: file being read
 * @page: structure to page
 */

not that I think kerneldoc comments on implementations driver / fs API
methods are very useful in general..
 
> +	loff_t offset = ((loff_t) page->index) << PAGE_CACHE_SHIFT;

please use the page_offset() helper.

> +	struct v9fs_fcall *fcall = NULL;
> +	int fid = v9f->fid;
> +	int total = 0;
> +	int result = 0;
> +
> +	page_cache_get(page);

->readpage is synchronous, so you shouldn't need to grab a reference.

> +	if (PageError(page))
> +		ClearPageError(page);

this doesn't make a lot of sense.

> +	retval = 0;
> +
> +      Error:

labels always at positions zero or one please.  I'd call this out
instead of erorr aswell as it's used for the successfull completion
case aswell.

> diff --git a/fs/9p/vfs_file.c b/fs/9p/vfs_file.c
> index 89c849d..f332f65 100644
> --- a/fs/9p/vfs_file.c
> +++ b/fs/9p/vfs_file.c
> @@ -306,6 +306,9 @@ v9fs_file_write(struct file *filp, const
>  		total += result;
>  	} while (count);
>  
> +	if ((inode->i_mapping) && (inode->i_mapping->nrpages))
> +		invalidate_inode_pages2(inode->i_mapping);
> +

how could inode->i_mapping be zero here?  Also the parenthes around
those values are superflous.

Re: [RFC] v9fs: readpage support

[Eric Van Hensbergen]

On 11/18/05, Christoph Hellwig <hch@infradead.org> wrote:
>
> > +     struct v9fs_fcall *fcall = NULL;
> > +     int fid = v9f->fid;
> > +     int total = 0;
> > +     int result = 0;
> > +
> > +     page_cache_get(page);
>
> ->readpage is synchronous, so you shouldn't need to grab a reference.
>

Weird things happen if I don't have page_cache_get - perhaps there's
something else I need to call if I don't call that?

bash-2.05b# /root/fsx/fsx -N 1000 -W -d testfile
mapped writes DISABLED
1 write 0x1f7e6 thru    0x2250e (0x2d29 bytes)
2 write 0x10f13 thru    0x16b11 (0x5bff bytes)
3 mapread       0x1f48a thru    0x2250e (0x3085 bytes)
Bad page state at free_hot_cold_page (in process 'fsx', page c0000000006175e0)
flags:0x0000700000000008 mapping:c0000000073606b0 mapcount:0 count:0
Backtrace:
Call Trace:
[C0000000072274B0] [C000000000025890] .show_stack+0x50/0x1cc (unreliable)
[C000000007227560] [C00000000007D0E4] .bad_page+0x90/0xec
[C0000000072275E0] [C00000000007DAA4] .free_hot_cold_page+0x80/0x1ac
[C000000007227680] [C00000000007DC1C] .__pagevec_free+0x4c/0x78
[C000000007227710] [C000000000088B3C] .release_pages+0x1fc/0x2c0
[C000000007227840] [C000000000088F64] .__pagevec_lru_add+0x138/0x170
[C0000000072278F0] [C000000000081C48] .__do_page_cache_readahead+0x27c/0x338
[C000000007227A50] [C00000000007981C] .filemap_nopage+0x308/0x460
[C000000007227B20] [C000000000091C30] .__handle_mm_fault+0x51c/0xe48
[C000000007227C20] [C0000000002BD5E8] .do_page_fault+0x524/0x7b4
[C000000007227E30] [C000000000004760] .handle_page_fault+0x20/0x54
Trying to fix it up, but a reboot is needed



I've incorporated the rest of your suggestions and found a bug in the
meanwhile.  I'll be submitting an amended patch to Andrew sometime
this week unless I hear back from you on the above issue.

Thanks for the comments Christoph.


          -eric