LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
* [RESEND][PATCH] 9p: add write-cache support to loose cache mode
@ 2007-02-13 23:55 Eric Van Hensbergen
  2007-02-14  0:16 ` Andrew Morton
                   ` (2 more replies)
  0 siblings, 3 replies; 8+ messages in thread
From: Eric Van Hensbergen @ 2007-02-13 23:55 UTC (permalink / raw)
  To: linux-kernel; +Cc: v9fs-developer, Eric Van Hensbergen

Loose cache mode was added primarily to asssist exclusive, read-only
mounts (like venti) -- however, there is also a case for using loose
write cacheing in support of read/write exclusive mounts.  This feature
is linked to the loose cache option and is disabled by default.

This code adds the necessary code to support writes through the page
cache.  Write caches are not used for synthetic files or for files opened
in APPEND mode.

Signed-of-by: Eric Van Hensbergen <ericvh@gmail.com>
---
 Documentation/filesystems/9p.txt |    2 
 fs/9p/9p.h                       |    2 
 fs/9p/conv.c                     |   19 ++++
 fs/9p/conv.h                     |    2 
 fs/9p/fcall.c                    |    6 +
 fs/9p/fid.c                      |   17 ++--
 fs/9p/fid.h                      |    2 
 fs/9p/v9fs_vfs.h                 |    2 
 fs/9p/vfs_addr.c                 |  180 ++++++++++++++++++++++++++++++++++++--
 fs/9p/vfs_dir.c                  |    2 
 fs/9p/vfs_file.c                 |   58 ++++++++++--
 fs/9p/vfs_inode.c                |   13 ++-
 fs/9p/vfs_super.c                |    3 -
 13 files changed, 264 insertions(+), 44 deletions(-)

diff --git a/Documentation/filesystems/9p.txt b/Documentation/filesystems/9p.txt
index bbd8b28..36ed211 100644
--- a/Documentation/filesystems/9p.txt
+++ b/Documentation/filesystems/9p.txt
@@ -42,7 +42,7 @@ OPTIONS
 
   cache=mode	specifies a cacheing policy.  By default, no caches are used.
 			loose = no attempts are made at consistency,
-                                intended for exclusive, read-only mounts
+                                intended for exclusive mounts
 
   debug=n	specifies debug level.  The debug level is a bitmask.
   			0x01 = display verbose error messages
diff --git a/fs/9p/9p.h b/fs/9p/9p.h
index 94e2f92..6f8edf0 100644
--- a/fs/9p/9p.h
+++ b/fs/9p/9p.h
@@ -370,6 +370,6 @@ int v9fs_t_read(struct v9fs_session_info
 		u64 offset, u32 count, struct v9fs_fcall **rcall);
 
 int v9fs_t_write(struct v9fs_session_info *v9ses, u32 fid, u64 offset,
-		 u32 count, const char __user * data,
+		 u32 count, const char __user * data, char * kdata,
 		 struct v9fs_fcall **rcall);
 int v9fs_printfcall(char *, int, struct v9fs_fcall *, int);
diff --git a/fs/9p/conv.c b/fs/9p/conv.c
index a3ed571..89c3d3c 100644
--- a/fs/9p/conv.c
+++ b/fs/9p/conv.c
@@ -458,6 +458,15 @@ v9fs_put_user_data(struct cbuf *bufp, co
 	return copy_from_user(*pdata, data, count);
 }
 
+static int
+v9fs_put_kernel_data(struct cbuf *bufp, char * kdata, int count,
+		   unsigned char **pdata)
+{
+	*pdata = buf_alloc(bufp, count);
+	memcpy(*pdata, kdata, count);
+	return 0;
+}
+
 static void
 v9fs_put_wstat(struct cbuf *bufp, struct v9fs_wstat *wstat,
 	       struct v9fs_stat *stat, int statsz, int extended)
@@ -723,7 +732,7 @@ struct v9fs_fcall *v9fs_create_tread(u32
 }
 
 struct v9fs_fcall *v9fs_create_twrite(u32 fid, u64 offset, u32 count,
-				      const char __user * data)
+				      const char __user * data, char * kdata)
 {
 	int size, err;
 	struct v9fs_fcall *fc;
@@ -738,7 +747,13 @@ struct v9fs_fcall *v9fs_create_twrite(u3
 	v9fs_put_int32(bufp, fid, &fc->params.twrite.fid);
 	v9fs_put_int64(bufp, offset, &fc->params.twrite.offset);
 	v9fs_put_int32(bufp, count, &fc->params.twrite.count);
-	err = v9fs_put_user_data(bufp, data, count, &fc->params.twrite.data);
+	if(data) {
+		err = v9fs_put_user_data(bufp, data, count, 
+						&fc->params.twrite.data); 
+	} else {
+		err = v9fs_put_kernel_data(bufp, kdata, count, 
+						&fc->params.twrite.data); 
+	}	
 	if (err) {
 		kfree(fc);
 		fc = ERR_PTR(err);
diff --git a/fs/9p/conv.h b/fs/9p/conv.h
index dd5b6b1..8091672 100644
--- a/fs/9p/conv.h
+++ b/fs/9p/conv.h
@@ -42,7 +42,7 @@ struct v9fs_fcall *v9fs_create_tcreate(u
 	char *extension, int extended);
 struct v9fs_fcall *v9fs_create_tread(u32 fid, u64 offset, u32 count);
 struct v9fs_fcall *v9fs_create_twrite(u32 fid, u64 offset, u32 count,
-	const char __user *data);
+	const char __user *data, char *kdata);
 struct v9fs_fcall *v9fs_create_tclunk(u32 fid);
 struct v9fs_fcall *v9fs_create_tremove(u32 fid);
 struct v9fs_fcall *v9fs_create_tstat(u32 fid);
diff --git a/fs/9p/fcall.c b/fs/9p/fcall.c
index dc336a6..671301a 100644
--- a/fs/9p/fcall.c
+++ b/fs/9p/fcall.c
@@ -393,13 +393,15 @@ v9fs_t_read(struct v9fs_session_info *v9
  * @fid: fid to write to
  * @offset: offset to start write at
  * @count: how many bytes to write
+ * @data: userspace data 
+ * @kdata: kernelspace data
  * @fcall: pointer to response fcall
  *
  */
 
 int
 v9fs_t_write(struct v9fs_session_info *v9ses, u32 fid, u64 offset, u32 count,
-	const char __user *data, struct v9fs_fcall **rcp)
+	const char __user *data, char *kdata, struct v9fs_fcall **rcp)
 {
 	int ret;
 	struct v9fs_fcall *tc, *rc;
@@ -407,7 +409,7 @@ v9fs_t_write(struct v9fs_session_info *v
 	dprintk(DEBUG_9P, "fid %d offset 0x%llux count 0x%x\n", fid,
 		(long long unsigned) offset, count);
 
-	tc = v9fs_create_twrite(fid, offset, count, data);
+	tc = v9fs_create_twrite(fid, offset, count, data, kdata);
 	if (!IS_ERR(tc)) {
 		ret = v9fs_mux_rpc(v9ses->mux, tc, &rc);
 
diff --git a/fs/9p/fid.c b/fs/9p/fid.c
index 9041971..f3c5cb8 100644
--- a/fs/9p/fid.c
+++ b/fs/9p/fid.c
@@ -40,19 +40,18 @@ #include "fid.h"
  *
  */
 
-int v9fs_fid_insert(struct v9fs_fid *fid, struct dentry *dentry)
+int v9fs_fid_insert(struct v9fs_fid *fid, void **priv)
 {
-	struct list_head *fid_list = (struct list_head *)dentry->d_fsdata;
-	dprintk(DEBUG_9P, "fid %d (%p) dentry %s (%p)\n", fid->fid, fid,
-		dentry->d_iname, dentry);
-	if (dentry->d_fsdata == NULL) {
-		dentry->d_fsdata =
+	struct list_head *fid_list= (struct list_head *) *priv;
+
+	if (fid_list == NULL) {
+		*priv =
 		    kmalloc(sizeof(struct list_head), GFP_KERNEL);
-		if (dentry->d_fsdata == NULL) {
+		if (*priv == NULL) {
 			dprintk(DEBUG_ERROR, "Out of memory\n");
 			return -ENOMEM;
 		}
-		fid_list = (struct list_head *)dentry->d_fsdata;
+		fid_list = (struct list_head *)*priv;
 		INIT_LIST_HEAD(fid_list);	/* Initialize list head */
 	}
 
@@ -80,6 +79,7 @@ struct v9fs_fid *v9fs_fid_create(struct 
 
 	new->fid = fid;
 	new->v9ses = v9ses;
+	new->filp = NULL;
 	new->fidopen = 0;
 	new->fidclunked = 0;
 	new->iounit = 0;
@@ -192,3 +192,4 @@ void v9fs_fid_clunk(struct v9fs_session_
 	v9fs_t_clunk(v9ses, fid->fid);
 	v9fs_fid_destroy(fid);
 }
+
diff --git a/fs/9p/fid.h b/fs/9p/fid.h
index 48fc170..dd19181 100644
--- a/fs/9p/fid.h
+++ b/fs/9p/fid.h
@@ -56,7 +56,7 @@ struct v9fs_fid *v9fs_fid_lookup(struct 
 struct v9fs_fid *v9fs_fid_get_created(struct dentry *);
 void v9fs_fid_destroy(struct v9fs_fid *fid);
 struct v9fs_fid *v9fs_fid_create(struct v9fs_session_info *, int fid);
-int v9fs_fid_insert(struct v9fs_fid *fid, struct dentry *dentry);
+int v9fs_fid_insert(struct v9fs_fid *fid, void **priv);
 struct v9fs_fid *v9fs_fid_clone(struct dentry *dentry);
 void v9fs_fid_clunk(struct v9fs_session_info *v9ses, struct v9fs_fid *fid);
 
diff --git a/fs/9p/v9fs_vfs.h b/fs/9p/v9fs_vfs.h
index 8ada4c5..27f11eb 100644
--- a/fs/9p/v9fs_vfs.h
+++ b/fs/9p/v9fs_vfs.h
@@ -53,3 +53,5 @@ int v9fs_file_open(struct inode *inode, 
 void v9fs_inode2stat(struct inode *inode, struct v9fs_stat *stat);
 void v9fs_dentry_release(struct dentry *);
 int v9fs_uflags2omode(int uflags);
+ssize_t v9fs_write(struct file *filp, const char __user * data, char * kdata, size_t count, loff_t * offset);
+
diff --git a/fs/9p/vfs_addr.c b/fs/9p/vfs_addr.c
index bed48fa..6cdd6b0 100644
--- a/fs/9p/vfs_addr.c
+++ b/fs/9p/vfs_addr.c
@@ -6,6 +6,9 @@
  *  Copyright (C) 2005 by Eric Van Hensbergen <ericvh@gmail.com>
  *  Copyright (C) 2002 by Ron Minnich <rminnich@lanl.gov>
  *
+ *  Some operations based on code from fs/cifs/file.c
+ *  Copyright (C) International Business Machines  Corp., 2002,2003
+ *
  *  This program is free software; you can redistribute it and/or modify
  *  it under the terms of the GNU General Public License version 2
  *  as published by the Free Software Foundation.
@@ -41,14 +44,14 @@ #include "v9fs_vfs.h"
 #include "fid.h"
 
 /**
- * v9fs_vfs_readpage - read an entire page in from 9P
+ * v9fs_vfs_readpage_worker - 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)
+static int v9fs_vfs_readpage_worker(struct file *filp, struct page *page)
 {
 	char *buffer = NULL;
 	int retval = -EIO;
@@ -63,8 +66,7 @@ static int v9fs_vfs_readpage(struct file
 	int total = 0;
 	int result = 0;
 
-	dprintk(DEBUG_VFS, "\n");
-
+	page_cache_get(page);
 	buffer = kmap(page);
 	do {
 		if (count < rsize)
@@ -73,13 +75,13 @@ static int v9fs_vfs_readpage(struct file
 		result = v9fs_t_read(v9ses, fid, offset, rsize, &fcall);
 
 		if (result < 0) {
-			printk(KERN_ERR "v9fs_t_read returned %d\n",
-			       result);
+			printk(KERN_ERR "v9fs_t_read returned %d\n", result);
 
 			kfree(fcall);
-			goto UnmapAndUnlock;
-		} else
+			goto io_error;
+		} else {
 			offset += result;
+		}
 
 		memcpy(buffer, fcall->params.rread.data, result);
 
@@ -98,12 +100,170 @@ static int v9fs_vfs_readpage(struct file
 	SetPageUptodate(page);
 	retval = 0;
 
-UnmapAndUnlock:
+      io_error:
+	kunmap(page);
+	page_cache_release(page);
+	return retval;
+}
+
+/**
+ * v9fs_prepare_write - prepare for mmap or page-cache I/O
+ *
+ * @file: file being read
+ * @page: structure to page
+ * @from: starting offset 
+ * @to: ending offset
+ *
+ * This is mostly just a 
+ *
+ */
+
+int v9fs_prepare_write(struct file *file, struct page *page,
+		       unsigned from, unsigned to)
+{
+	if (!PageUptodate(page)) {
+		if (to - from != PAGE_CACHE_SIZE) {
+			void *kaddr = kmap_atomic(page, KM_USER0);
+			memset(kaddr, 0, from);
+			memset(kaddr + to, 0, PAGE_CACHE_SIZE - to);
+			flush_dcache_page(page);
+			kunmap_atomic(kaddr, KM_USER0);
+		}
+		SetPageUptodate(page);
+	}
+
+	if ((to == PAGE_CACHE_SIZE) && (from == 0))
+		SetPageUptodate(page);
+
+	if ((file->f_flags & O_ACCMODE) != O_WRONLY) {
+		v9fs_vfs_readpage_worker(file, page);
+	}
+
+	return 0;
+}
+
+/**
+ * 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)
+{
+	int retval;
+
+	dprintk(DEBUG_VFS, "\n");
+
+	retval = v9fs_vfs_readpage_worker(filp, page);
+
+	unlock_page(page);
+	return retval;
+}
+
+/**
+ * v9fs_find_file - find a file pointer based on page
+ * @page: page to lookup file based on
+ * 
+ */
+static struct file *v9fs_find_file(struct page *page)
+{
+	struct inode *inode = page->mapping->host;
+	struct v9fs_fid *fid = NULL;
+	struct list_head *p;
+
+	dprintk(DEBUG_VFS, " page: %p\n", page);
+
+	p = (struct list_head *)inode->i_private;
+	if (!p) {
+		dprintk(DEBUG_ERROR, "No list_head\n");
+		return NULL;
+	}
+
+	fid = list_entry(p->next, struct v9fs_fid, list);
+
+	if (fid)
+		return fid->filp;
+
+	dprintk(DEBUG_VFS, " filp not found for page: %p\n", page);
+	return NULL;
+}
+
+/**
+ * v9fs_vfs_writepage - write a mmaped page to server
+ * @page: page to write
+ * @wbc: writeback control
+ * 
+ */
+
+static int v9fs_vfs_writepage(struct page *page, struct writeback_control *wbc)
+{
+	char *buffer = NULL;
+	struct address_space *mapping = page->mapping;
+	int retval = -EIO;
+	loff_t offset = 0;
+	loff_t pageoffset = 0;
+	unsigned long end_index;
+	int count = PAGE_CACHE_SIZE;
+	struct file *filp = v9fs_find_file(page);
+	struct inode *inode = mapping->host;
+
+	dprintk(DEBUG_VFS, "page: %p\n", page);
+
+	if ((!inode) || (!filp))
+		goto UnlockPage;
+
+	end_index = inode->i_size >> PAGE_CACHE_SHIFT;
+
+	/* complicated case at end of file */
+	if (page->index >= end_index) {
+		/* things got complicated... */
+		count = inode->i_size & (PAGE_CACHE_SIZE - 1);
+		if (page->index >= end_index + 1 || !count)
+			return 0;	/* truncated - don't care */
+	}
+
+	/* get buffer */
+	buffer = kmap(page) + pageoffset;
+	offset = ((loff_t) page->index << PAGE_CACHE_SHIFT) + pageoffset;
+
+	page_cache_get(page);
+
+	retval = v9fs_write(filp, NULL, buffer, count, &offset);
+
+	if (retval < 0) {
+		dprintk(DEBUG_ERROR, "error: %d\n", retval);
+		ClearPageUptodate(page);
+		goto UnmapPage;
+	}
+
+	if (retval < count) {
+		dprintk(DEBUG_ERROR, "Short write\n");
+	}
+
+	if (offset > inode->i_size) {
+		inode->i_size = offset;
+	}
+
+	if (PageError(page))
+		ClearPageError(page);
+	SetPageUptodate(page);
+
+	retval = 0;
+
+      UnmapPage:
 	kunmap(page);
+      UnlockPage:
 	unlock_page(page);
+	page_cache_release(page);
+
 	return retval;
 }
 
 const struct address_space_operations v9fs_addr_operations = {
-      .readpage = v9fs_vfs_readpage,
+	.prepare_write = v9fs_prepare_write,
+	.commit_write = simple_commit_write,
+	.readpage = v9fs_vfs_readpage,
+	.writepage = v9fs_vfs_writepage,
 };
diff --git a/fs/9p/vfs_dir.c b/fs/9p/vfs_dir.c
index 3129688..8559f7a 100644
--- a/fs/9p/vfs_dir.c
+++ b/fs/9p/vfs_dir.c
@@ -197,7 +197,7 @@ int v9fs_dir_release(struct inode *inode
 			dprintk(DEBUG_ERROR, "clunk failed\n");
 
 		kfree(fid->rdir_fcall);
-		kfree(fid);
+ 		v9fs_fid_destroy(fid);
 
 		filp->private_data = NULL;
 	}
diff --git a/fs/9p/vfs_file.c b/fs/9p/vfs_file.c
index 653dfa5..f26f628 100644
--- a/fs/9p/vfs_file.c
+++ b/fs/9p/vfs_file.c
@@ -79,13 +79,14 @@ int v9fs_file_open(struct inode *inode, 
 	vfid->filp = file;
 	kfree(fcall);
 
-	if((vfid->qid.version) && (v9ses->cache)) {
-		dprintk(DEBUG_VFS, "cached");
+	if((vfid->qid.version) && (v9ses->cache) && !(file->f_flags&O_APPEND)) {
 		/* enable cached file options */
 		if(file->f_op == &v9fs_file_operations)
 			file->f_op = &v9fs_cached_file_operations;
 	}
 
+         v9fs_fid_insert(vfid, &inode->i_private);
+
 	return 0;
 
 Clunk_Fid:
@@ -187,17 +188,18 @@ v9fs_file_read(struct file *filp, char _
 }
 
 /**
- * v9fs_file_write - write to a file
+ * v9fs_write - write to a file  (generic)
  * @filep: file pointer to write
- * @data: data buffer to write data from
+ * @data: user data buffer to write data from
+ * @kdata: kernel data buffer to write data from
  * @count: size of buffer
  * @offset: offset at which to write data
  *
  */
 
-static ssize_t
-v9fs_file_write(struct file *filp, const char __user * data,
-		size_t count, loff_t * offset)
+ssize_t
+v9fs_write(struct file *filp, const char __user * data,
+		char* kdata, size_t count, loff_t * offset)
 {
 	struct inode *inode = filp->f_path.dentry->d_inode;
 	struct v9fs_session_info *v9ses = v9fs_inode2v9ses(inode);
@@ -208,8 +210,8 @@ v9fs_file_write(struct file *filp, const
 	int rsize = 0;
 	int total = 0;
 
-	dprintk(DEBUG_VFS, "data %p count %d offset %x\n", data, (int)count,
-		(int)*offset);
+	dprintk(DEBUG_VFS, "count %d offset %x\n", 
+			(int)count, (int)*offset);
 	rsize = v9ses->maxdata - V9FS_IOHDRSZ;
 	if (v9fid->iounit != 0 && rsize > v9fid->iounit)
 		rsize = v9fid->iounit;
@@ -218,7 +220,8 @@ v9fs_file_write(struct file *filp, const
 		if (count < rsize)
 			rsize = count;
 
-		result = v9fs_t_write(v9ses, fid, *offset, rsize, data, &fcall);
+		result = v9fs_t_write(v9ses, fid, *offset, rsize, 
+						data, kdata, &fcall);
 		if (result < 0) {
 			PRINT_FCALL_ERROR("error while writing", fcall);
 			kfree(fcall);
@@ -237,19 +240,48 @@ v9fs_file_write(struct file *filp, const
 		}
 
 		count -= result;
-		data += result;
+		if(data)
+			data += result;
+		else
+			kdata += result;
 		total += result;
 	} while (count);
 
-	invalidate_inode_pages2(inode->i_mapping);
 	return total;
 }
 
+
+/**
+ * v9fs_file_write - write to a file 
+ * @filep: file pointer to write
+ * @data: data buffer to write data from
+ * @count: size of buffer
+ * @offset: offset at which to write data
+ *
+ */
+
+static ssize_t
+v9fs_file_write(struct file *filp, const char __user * data,
+		size_t count, loff_t * offset)
+{
+	struct inode *inode = filp->f_path.dentry->d_inode;
+
+	ssize_t ret;
+
+	dprintk(DEBUG_VFS, "count %d offset %x\n", 
+			(int)count, (int)*offset);
+	ret = v9fs_write(filp, data, NULL, count, offset);
+	invalidate_inode_pages2(inode->i_mapping);
+	return ret;
+}
+
 const struct file_operations v9fs_cached_file_operations = {
 	.llseek = generic_file_llseek,
 	.read = do_sync_read,
 	.aio_read = generic_file_aio_read,
-	.write = v9fs_file_write,
+	.write = do_sync_write,
+	.aio_write = generic_file_aio_write,
+	.fsync	= simple_sync_file,
 	.open = v9fs_file_open,
 	.release = v9fs_dir_release,
 	.lock = v9fs_file_lock,
diff --git a/fs/9p/vfs_inode.c b/fs/9p/vfs_inode.c
index 124a085..4099b0e 100644
--- a/fs/9p/vfs_inode.c
+++ b/fs/9p/vfs_inode.c
@@ -208,6 +208,7 @@ struct inode *v9fs_get_inode(struct supe
 		inode->i_rdev = 0;
 		inode->i_atime = inode->i_mtime = inode->i_ctime = CURRENT_TIME;
 		inode->i_mapping->a_ops = &v9fs_addr_operations;
+		inode->i_private = NULL;
 
 		switch (mode & S_IFMT) {
 		case S_IFIFO:
@@ -340,7 +341,7 @@ v9fs_clone_walk(struct v9fs_session_info
 		goto clunk_fid;
 	}
 
-	err = v9fs_fid_insert(ret, dentry);
+	err = v9fs_fid_insert(ret, &dentry->d_fsdata);
 	if (err < 0) {
 		v9fs_fid_destroy(ret);
 		goto clunk_fid;
@@ -520,13 +521,19 @@ v9fs_vfs_create(struct inode *dir, struc
 			v9fs_fid_destroy(ffid);
 			return PTR_ERR(filp);
 		}
-
 		ffid->rdir_pos = 0;
 		ffid->rdir_fcall = NULL;
 		ffid->fidopen = 1;
 		ffid->iounit = iounit;
 		ffid->filp = filp;
 		filp->private_data = ffid;
+	        if((vfid->qid.version) && (v9ses->cache) && 
+		  !(flags & O_APPEND)) {
+                	/* enable cached file options */
+                	if(filp->f_op == &v9fs_file_operations)
+                        	filp->f_op = &v9fs_cached_file_operations;
+        	}
+		v9fs_fid_insert(ffid, &inode->i_private);
 	}
 
 	return 0;
@@ -696,7 +703,7 @@ static struct dentry *v9fs_vfs_lookup(st
 		goto FreeFcall;
 	}
 
-	result = v9fs_fid_insert(fid, dentry);
+	result = v9fs_fid_insert(fid, &dentry->d_fsdata);
 	if (result < 0)
 		goto FreeFcall;
 
diff --git a/fs/9p/vfs_super.c b/fs/9p/vfs_super.c
index 0ec42f6..7c4c289 100644
--- a/fs/9p/vfs_super.c
+++ b/fs/9p/vfs_super.c
@@ -56,6 +56,7 @@ static const struct super_operations v9f
 static void v9fs_clear_inode(struct inode *inode)
 {
 	filemap_fdatawrite(inode->i_mapping);
+	kfree(inode->i_private);
 }
 
 /**
@@ -167,7 +168,7 @@ static int v9fs_get_sb(struct file_syste
 			goto put_back_sb;
 		}
 
-		retval = v9fs_fid_insert(root_fid, root);
+		retval = v9fs_fid_insert(root_fid, &root->d_fsdata);
 		if (retval < 0) {
 			kfree(fcall);
 			goto put_back_sb;
-- 
1.4.1


^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [RESEND][PATCH] 9p: add write-cache support to loose cache mode
  2007-02-13 23:55 [RESEND][PATCH] 9p: add write-cache support to loose cache mode Eric Van Hensbergen
@ 2007-02-14  0:16 ` Andrew Morton
  2007-02-14  2:07   ` Eric Van Hensbergen
  2007-02-14  0:26 ` Andrew Morton
  2007-02-14  0:27 ` Andrew Morton
  2 siblings, 1 reply; 8+ messages in thread
From: Andrew Morton @ 2007-02-14  0:16 UTC (permalink / raw)
  To: Eric Van Hensbergen; +Cc: linux-kernel, v9fs-developer, ericvh

> On Tue, 13 Feb 2007 17:55:31 -0600 Eric Van Hensbergen <ericvh@gmail.com> wrote:
> +int v9fs_prepare_write(struct file *file, struct page *page,
> +		       unsigned from, unsigned to)
> +{
> +	if (!PageUptodate(page)) {
> +		if (to - from != PAGE_CACHE_SIZE) {
> +			void *kaddr = kmap_atomic(page, KM_USER0);
> +			memset(kaddr, 0, from);
> +			memset(kaddr + to, 0, PAGE_CACHE_SIZE - to);
> +			flush_dcache_page(page);
> +			kunmap_atomic(kaddr, KM_USER0);
> +		}
> +		SetPageUptodate(page);
> +	}

This will mark the page uptodate while the piece between `to' and `from' is
uninitialised.  A concurrent pagefault can come in and permit a read of
that uninitialised data.  Because filemap_nopage() doesn't lock the page if
it is uptodate.

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [RESEND][PATCH] 9p: add write-cache support to loose cache mode
  2007-02-13 23:55 [RESEND][PATCH] 9p: add write-cache support to loose cache mode Eric Van Hensbergen
  2007-02-14  0:16 ` Andrew Morton
@ 2007-02-14  0:26 ` Andrew Morton
  2007-02-14  0:27 ` Andrew Morton
  2 siblings, 0 replies; 8+ messages in thread
From: Andrew Morton @ 2007-02-14  0:26 UTC (permalink / raw)
  To: Eric Van Hensbergen; +Cc: linux-kernel, v9fs-developer, ericvh

> On Tue, 13 Feb 2007 17:55:31 -0600 Eric Van Hensbergen <ericvh@gmail.com> wrote:
> +static int v9fs_vfs_writepage(struct page *page, struct writeback_control *wbc)
> +{
> +	char *buffer = NULL;
> +	struct address_space *mapping = page->mapping;
> +	int retval = -EIO;
> +	loff_t offset = 0;
> +	loff_t pageoffset = 0;
> +	unsigned long end_index;

Cosmetic detail: pgoff_t here.

> +	int count = PAGE_CACHE_SIZE;
> +	struct file *filp = v9fs_find_file(page);
> +	struct inode *inode = mapping->host;
> +
> +	dprintk(DEBUG_VFS, "page: %p\n", page);
> +
> +	if ((!inode) || (!filp))
> +		goto UnlockPage;
> +
> +	end_index = inode->i_size >> PAGE_CACHE_SHIFT;
> +
> +	/* complicated case at end of file */
> +	if (page->index >= end_index) {
> +		/* things got complicated... */
> +		count = inode->i_size & (PAGE_CACHE_SIZE - 1);
> +		if (page->index >= end_index + 1 || !count)
> +			return 0;	/* truncated - don't care */
> +	}
> +
> +	/* get buffer */
> +	buffer = kmap(page) + pageoffset;

kmap_atomic() is faster and less deadlocky.  But presumably v9fs_write()
prevents that.

> +	offset = ((loff_t) page->index << PAGE_CACHE_SHIFT) + pageoffset;
> +
> +	page_cache_get(page);
> +
> +	retval = v9fs_write(filp, NULL, buffer, count, &offset);
> +
> +	if (retval < 0) {
> +		dprintk(DEBUG_ERROR, "error: %d\n", retval);
> +		ClearPageUptodate(page);
> +		goto UnmapPage;
> +	}

I think you'll find that the ->writepage() caller handles all this.  The
callers also set AS_EIO, which this code forgot.

> +	if (retval < count) {
> +		dprintk(DEBUG_ERROR, "Short write\n");
> +	}
> +
> +	if (offset > inode->i_size) {
> +		inode->i_size = offset;
> +	}

Can this happen??

If so, mark_inode_dirty() seems to be missing.

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

Is this needed?

> +	SetPageUptodate(page);

I'd expect that v9fs_writepage() is only ever called against uptodate pages?

> +	retval = 0;
> +
> +      UnmapPage:
>  	kunmap(page);
> +      UnlockPage:
>  	unlock_page(page);
> +	page_cache_release(page);
> +
>  	return retval;
>  }

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [RESEND][PATCH] 9p: add write-cache support to loose cache mode
  2007-02-13 23:55 [RESEND][PATCH] 9p: add write-cache support to loose cache mode Eric Van Hensbergen
  2007-02-14  0:16 ` Andrew Morton
  2007-02-14  0:26 ` Andrew Morton
@ 2007-02-14  0:27 ` Andrew Morton
  2 siblings, 0 replies; 8+ messages in thread
From: Andrew Morton @ 2007-02-14  0:27 UTC (permalink / raw)
  To: Eric Van Hensbergen; +Cc: linux-kernel, v9fs-developer, ericvh

> On Tue, 13 Feb 2007 17:55:31 -0600 Eric Van Hensbergen <ericvh@gmail.com> wrote:
> +static ssize_t
> +v9fs_file_write(struct file *filp, const char __user * data,
> +		size_t count, loff_t * offset)
> +{
> +	struct inode *inode = filp->f_path.dentry->d_inode;
> +
> +	ssize_t ret;
> +
> +	dprintk(DEBUG_VFS, "count %d offset %x\n", 
> +			(int)count, (int)*offset);
> +	ret = v9fs_write(filp, data, NULL, count, offset);
> +	invalidate_inode_pages2(inode->i_mapping);
> +	return ret;
> +}
> +

invalidate_inode_pages2() can fail.  It might be worth at least warning here
if it does.

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [RESEND][PATCH] 9p: add write-cache support to loose cache mode
  2007-02-14  0:16 ` Andrew Morton
@ 2007-02-14  2:07   ` Eric Van Hensbergen
  2007-02-14  2:11     ` Andrew Morton
  0 siblings, 1 reply; 8+ messages in thread
From: Eric Van Hensbergen @ 2007-02-14  2:07 UTC (permalink / raw)
  To: Andrew Morton; +Cc: linux-kernel, v9fs-developer

On 2/13/07, Andrew Morton <akpm@linux-foundation.org> wrote:
> > On Tue, 13 Feb 2007 17:55:31 -0600 Eric Van Hensbergen <ericvh@gmail.com> wrote:
> > +int v9fs_prepare_write(struct file *file, struct page *page,
> > +                    unsigned from, unsigned to)
> > +{
> > +     if (!PageUptodate(page)) {
> > +             if (to - from != PAGE_CACHE_SIZE) {
> > +                     void *kaddr = kmap_atomic(page, KM_USER0);
> > +                     memset(kaddr, 0, from);
> > +                     memset(kaddr + to, 0, PAGE_CACHE_SIZE - to);
> > +                     flush_dcache_page(page);
> > +                     kunmap_atomic(kaddr, KM_USER0);
> > +             }
> > +             SetPageUptodate(page);
> > +     }
>
> This will mark the page uptodate while the piece between `to' and `from' is
> uninitialised.  A concurrent pagefault can come in and permit a read of
> that uninitialised data.  Because filemap_nopage() doesn't lock the page if
> it is uptodate.
>

Okay - I snagged this code from fs/libfs.c (simple_prepare_write) --
is that code also not correct, or am I just using it in the wrong
context?

        -eric

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [RESEND][PATCH] 9p: add write-cache support to loose cache mode
  2007-02-14  2:07   ` Eric Van Hensbergen
@ 2007-02-14  2:11     ` Andrew Morton
  2007-02-14  3:05       ` Nick Piggin
  0 siblings, 1 reply; 8+ messages in thread
From: Andrew Morton @ 2007-02-14  2:11 UTC (permalink / raw)
  To: Eric Van Hensbergen; +Cc: linux-kernel, v9fs-developer

> On Tue, 13 Feb 2007 20:07:44 -0600 "Eric Van Hensbergen" <ericvh@gmail.com> wrote:
> On 2/13/07, Andrew Morton <akpm@linux-foundation.org> wrote:
> > > On Tue, 13 Feb 2007 17:55:31 -0600 Eric Van Hensbergen <ericvh@gmail.com> wrote:
> > > +int v9fs_prepare_write(struct file *file, struct page *page,
> > > +                    unsigned from, unsigned to)
> > > +{
> > > +     if (!PageUptodate(page)) {
> > > +             if (to - from != PAGE_CACHE_SIZE) {
> > > +                     void *kaddr = kmap_atomic(page, KM_USER0);
> > > +                     memset(kaddr, 0, from);
> > > +                     memset(kaddr + to, 0, PAGE_CACHE_SIZE - to);
> > > +                     flush_dcache_page(page);
> > > +                     kunmap_atomic(kaddr, KM_USER0);
> > > +             }
> > > +             SetPageUptodate(page);
> > > +     }
> >
> > This will mark the page uptodate while the piece between `to' and `from' is
> > uninitialised.  A concurrent pagefault can come in and permit a read of
> > that uninitialised data.  Because filemap_nopage() doesn't lock the page if
> > it is uptodate.
> >
> 
> Okay - I snagged this code from fs/libfs.c (simple_prepare_write) --
> is that code also not correct, or am I just using it in the wrong
> context?
> 

libfs.c is wrong.  Nick has fixes, but they got tangled up in other stuff.

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [RESEND][PATCH] 9p: add write-cache support to loose cache mode
  2007-02-14  2:11     ` Andrew Morton
@ 2007-02-14  3:05       ` Nick Piggin
  2007-02-14  3:10         ` Andrew Morton
  0 siblings, 1 reply; 8+ messages in thread
From: Nick Piggin @ 2007-02-14  3:05 UTC (permalink / raw)
  To: Andrew Morton; +Cc: Eric Van Hensbergen, linux-kernel, v9fs-developer

Andrew Morton wrote:
>>On Tue, 13 Feb 2007 20:07:44 -0600 "Eric Van Hensbergen" <ericvh@gmail.com> wrote:
>>On 2/13/07, Andrew Morton <akpm@linux-foundation.org> wrote:
>>
>>>>On Tue, 13 Feb 2007 17:55:31 -0600 Eric Van Hensbergen <ericvh@gmail.com> wrote:
>>>>+int v9fs_prepare_write(struct file *file, struct page *page,
>>>>+                    unsigned from, unsigned to)
>>>>+{
>>>>+     if (!PageUptodate(page)) {
>>>>+             if (to - from != PAGE_CACHE_SIZE) {
>>>>+                     void *kaddr = kmap_atomic(page, KM_USER0);
>>>>+                     memset(kaddr, 0, from);
>>>>+                     memset(kaddr + to, 0, PAGE_CACHE_SIZE - to);
>>>>+                     flush_dcache_page(page);
>>>>+                     kunmap_atomic(kaddr, KM_USER0);
>>>>+             }
>>>>+             SetPageUptodate(page);
>>>>+     }
>>>
>>>This will mark the page uptodate while the piece between `to' and `from' is
>>>uninitialised.  A concurrent pagefault can come in and permit a read of
>>>that uninitialised data.  Because filemap_nopage() doesn't lock the page if
>>>it is uptodate.
>>>
>>
>>Okay - I snagged this code from fs/libfs.c (simple_prepare_write) --
>>is that code also not correct, or am I just using it in the wrong
>>context?
>>
> 
> 
> libfs.c is wrong.  Nick has fixes, but they got tangled up in other stuff.

Yeah. 1/9 in that series should be applied on its own and sent upstream.

Need me to resend?

-- 
SUSE Labs, Novell Inc.
Send instant messages to your online friends http://au.messenger.yahoo.com 

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [RESEND][PATCH] 9p: add write-cache support to loose cache mode
  2007-02-14  3:05       ` Nick Piggin
@ 2007-02-14  3:10         ` Andrew Morton
  0 siblings, 0 replies; 8+ messages in thread
From: Andrew Morton @ 2007-02-14  3:10 UTC (permalink / raw)
  To: Nick Piggin; +Cc: ericvh, linux-kernel, v9fs-developer

> On Wed, 14 Feb 2007 14:05:44 +1100 Nick Piggin <nickpiggin@yahoo.com.au> wrote:
> > libfs.c is wrong.  Nick has fixes, but they got tangled up in other stuff.
> 
> Yeah. 1/9 in that series should be applied on its own and sent upstream.
> 
> Need me to resend?

Yes please.

^ permalink raw reply	[flat|nested] 8+ messages in thread

end of thread, other threads:[~2007-02-14  3:11 UTC | newest]

Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2007-02-13 23:55 [RESEND][PATCH] 9p: add write-cache support to loose cache mode Eric Van Hensbergen
2007-02-14  0:16 ` Andrew Morton
2007-02-14  2:07   ` Eric Van Hensbergen
2007-02-14  2:11     ` Andrew Morton
2007-02-14  3:05       ` Nick Piggin
2007-02-14  3:10         ` Andrew Morton
2007-02-14  0:26 ` Andrew Morton
2007-02-14  0:27 ` Andrew Morton

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).