9p: Convert to new fscache API

PATCH In review David Howells 2 participants 8 messages Nov 18, 2020

Subsystems (4)

VFS 9P file system Afs filesystem Filesystems netfs library

lore

Share & tools

lore

David Howells

author maintainer

2020-11-18 11:02 UTC #0 lore raw

PATCH 1/1 9p: Convert to new fscache API

Hi Dominique,

Here's a rough draft of a patch to convert 9P to use the rewritten fscache API. It compiles, but I've no way to test it. This is built on top of my fscache-iter branch:

https://git.kernel.org/pub/scm/linux/kernel/git/dhowells/linux-fs.git/log/?h=fscache-iter

Notes:

(*) I've switched to use ITER_XATTR rather than ITER_BVEC in some places.

(*) I've added a pair of helper functions to get the cookie:

	v9fs_inode_cookie()
	v9fs_session_cache()

     These return NULL if CONFIG_9P_FSCACHE=n.

(*) I've moved some of the fscache accesses inline. Using the above helper
functions, it all compiles away due to NULL pointer checks in the header file if fscache is disabled.

(*) 9P's readpage and readpages now just jump into the netfs helpers, as does
write_begin. v9fs_req_issue_op() initiates the I/O on behalf of the helpers.

(*) v9fs_write_begin() now returns the subpage and v9fs_write_end() goes back
out to the head page. thp_size() is also used. This should mean you handle transparent huge pages (THPs) and can turn that on.

(*) I have made an assumption that 9p_client_read() and write can handle I/Os
larger than a page. If this is not the case, v9fs_req_ops will need clamp_length() implementing.

(*) The expand_readahead() and clamp_length() ops should perhaps be
implemented to align and trim with respect to maximum I/O size.

(*) iget and evict acquire and relinquish a cookie.

(*) open and release use and unuse that cookie.

(*) writepage writes the dirty data to the cache.

(*) setattr resizes the cache if necessary.

(*) The cache needs to be invalidated if a 3rd-party change happens, but I
haven't done that.

(*) With these changes, 9p should cache local changes too, not just data
read.

(*) If 9p supports DIO writes, it should invalidate a cache object with
FSCACHE_INVAL_DIO_WRITE when one happens - thereby stopping caching for that file until all file handles on it are closed.

David
---

fs/9p/Kconfig +1 -1

index 66ca72422961..a9407693ef2d 100644
@@ -13,7 +13,7 @@ config 9P_FS
 if 9P_FS
 config 9P_FSCACHE
 	bool "Enable 9P client caching support"
-	depends on 9P_FS=m && FSCACHE_OLD || 9P_FS=y && FSCACHE_OLD=y
+	depends on 9P_FS=m || 9P_FS=y
 	help
 	  Choose Y here to enable persistent, read-only local
 	  caching support for 9p clients using FS-Cache

fs/9p/cache.c +10 ~~-257~~

299 lines not on this page · raw message

fs/9p/cache.h +0 ~~-107~~

126 lines not on this page · raw message

fs/9p/v9fs.c +3 -3

index 39def020a074..f7f9f2f055ec 100644
@@ -467,7 +467,8 @@ struct p9_fid *v9fs_session_init(struct v9fs_session_info *v9ses,
 
 #ifdef CONFIG_9P_FSCACHE
 	/* register the session for caching */
-	v9fs_cache_session_get_cookie(v9ses);
+	if (v9ses->cache == CACHE_LOOSE || v9ses->cache == CACHE_FSCACHE)
+		v9fs_cache_session_get_cookie(v9ses);
 #endif
 	spin_lock(&v9fs_sessionlist_lock);
 	list_add(&v9ses->slist, &v9fs_sessionlist);

@@ -500,8 +501,7 @@ void v9fs_session_close(struct v9fs_session_info *v9ses)
 	}
 
 #ifdef CONFIG_9P_FSCACHE
-	if (v9ses->fscache)
-		v9fs_cache_session_put_cookie(v9ses);
+	fscache_relinquish_cookie(v9fs_session_cache(v9ses), false);
 	kfree(v9ses->cachetag);
 #endif
 	kfree(v9ses->uname);

fs/9p/v9fs.h +19 -1

index 7b763776306e..b248586d7a2c 100644
@@ -109,7 +109,6 @@ struct v9fs_session_info {
 
 struct v9fs_inode {
 #ifdef CONFIG_9P_FSCACHE
-	struct mutex fscache_lock;
 	struct fscache_cookie *fscache;
 #endif
 	struct p9_qid qid;

@@ -124,6 +123,25 @@ static inline struct v9fs_inode *V9FS_I(const struct inode *inode)
 	return container_of(inode, struct v9fs_inode, vfs_inode);
 }
 
+static inline struct fscache_cookie *v9fs_inode_cookie(struct v9fs_inode *v9inode)
+{
+#ifdef CONFIG_9P_FSCACHE
+	return v9inode->fscache;
+#else
+	return NULL;
+#endif	
+}
+
+static inline struct fscache_cookie *v9fs_session_cache(struct v9fs_session_info *v9ses)
+{
+#ifdef CONFIG_9P_FSCACHE
+	return v9ses->fscache;
+#else
+	return NULL;
+#endif	
+}
+
+
 extern int v9fs_show_options(struct seq_file *m, struct dentry *root);
 
 struct p9_fid *v9fs_session_init(struct v9fs_session_info *, const char *,

fs/9p/vfs_addr.c +104 ~~-97~~

314 lines not on this page · raw message

fs/9p/vfs_dir.c +12 -1

index 674d22bf4f6f..eade5091654a 100644
@@ -19,6 +19,7 @@
 #include <linux/idr.h>
 #include <linux/slab.h>
 #include <linux/uio.h>
+#include <linux/fscache.h>
 #include <net/9p/9p.h>
 #include <net/9p/client.h>

@@ -205,13 +206,23 @@ static int v9fs_dir_readdir_dotl(struct file *file, struct dir_context *ctx)
 
 int v9fs_dir_release(struct inode *inode, struct file *filp)
 {
+	struct v9fs_inode *v9inode = V9FS_I(inode);
 	struct p9_fid *fid;
-
+	loff_t i_size;
+	
 	fid = filp->private_data;
 	p9_debug(P9_DEBUG_VFS, "inode: %p filp: %p fid: %d\n",
 		 inode, filp, fid ? fid->fid : -1);
 	if (fid)
 		p9_client_clunk(fid);
+
+	if ((filp->f_mode & FMODE_WRITE)) {
+		i_size = i_size_read(inode);
+		fscache_unuse_cookie(v9fs_inode_cookie(v9inode),
+				     &v9inode->qid.version, &i_size);
+	} else {
+		fscache_unuse_cookie(v9fs_inode_cookie(v9inode), NULL, NULL);
+	}
 	return 0;
 }

fs/9p/vfs_file.c +18 -6

index b177fd3b1eb3..d6b393c223b6 100644
@@ -94,8 +94,7 @@ int v9fs_file_open(struct inode *inode, struct file *file)
 		v9inode->writeback_fid = (void *) fid;
 	}
 	mutex_unlock(&v9inode->v_mutex);
-	if (v9ses->cache == CACHE_LOOSE || v9ses->cache == CACHE_FSCACHE)
-		v9fs_cache_inode_set_cookie(inode, file);
+	fscache_use_cookie(v9fs_inode_cookie(v9inode), file->f_mode & FMODE_WRITE);
 	return 0;
 out_error:
 	p9_client_clunk(file->private_data);

@@ -554,14 +553,27 @@ v9fs_vm_page_mkwrite(struct vm_fault *vmf)
 	p9_debug(P9_DEBUG_VFS, "page %p fid %lx\n",
 		 page, (unsigned long)filp->private_data);
 
+	v9inode = V9FS_I(inode);
+
+	/* Wait for the page to be written to the cache before we allow it to
+	 * be modified.  We then assume the entire page will need writing back.
+	 */
+#ifdef CONFIG_V9FS_FSCACHE
+	if (PageFsCache(page) &&
+	    wait_on_page_bit_killable(page, PG_fscache) < 0)
+		return VM_FAULT_RETRY;
+#endif
+
+	if (PageWriteback(page) &&
+	    wait_on_page_bit_killable(page, PG_writeback) < 0)
+		return VM_FAULT_RETRY;
+
 	/* Update file times before taking page lock */
 	file_update_time(filp);
 
-	v9inode = V9FS_I(inode);
-	/* make sure the cache has finished storing the page */
-	v9fs_fscache_wait_on_page_write(inode, page);
 	BUG_ON(!v9inode->writeback_fid);
-	lock_page(page);
+	if (lock_page_killable(page) < 0)
+		return VM_FAULT_RETRY;
 	if (page->mapping != inode->i_mapping)
 		goto out_unlock;
 	wait_for_stable_page(page);

fs/9p/vfs_inode.c +14 ~~-10~~

index ae0c38ad1fcb..4cdf4660d7c2 100644
@@ -228,7 +228,6 @@ struct inode *v9fs_alloc_inode(struct super_block *sb)
 		return NULL;
 #ifdef CONFIG_9P_FSCACHE
 	v9inode->fscache = NULL;
-	mutex_init(&v9inode->fscache_lock);
 #endif
 	v9inode->writeback_fid = NULL;
 	v9inode->cache_validity = 0;

@@ -377,10 +376,12 @@ void v9fs_evict_inode(struct inode *inode)
 	struct v9fs_inode *v9inode = V9FS_I(inode);
 
 	truncate_inode_pages_final(&inode->i_data);
+	fscache_clear_inode_writeback(v9fs_inode_cookie(v9inode), inode,
+				      &v9inode->qid.version);
 	clear_inode(inode);
 	filemap_fdatawrite(&inode->i_data);
 
-	v9fs_cache_inode_put_cookie(inode);
+	fscache_relinquish_cookie(v9fs_inode_cookie(v9inode), false);
 	/* clunk the fid stashed in writeback_fid */
 	if (v9inode->writeback_fid) {
 		p9_client_clunk(v9inode->writeback_fid);

@@ -846,8 +847,7 @@ v9fs_vfs_atomic_open(struct inode *dir, struct dentry *dentry,
 		goto error;
 
 	file->private_data = fid;
-	if (v9ses->cache == CACHE_LOOSE || v9ses->cache == CACHE_FSCACHE)
-		v9fs_cache_inode_set_cookie(d_inode(dentry), file);
+	fscache_use_cookie(v9fs_inode_cookie(v9inode), file->f_mode & FMODE_WRITE);
 
 	file->f_mode |= FMODE_CREATED;
 out:

@@ -1035,6 +1035,8 @@ v9fs_vfs_getattr(const struct path *path, struct kstat *stat,
 static int v9fs_vfs_setattr(struct dentry *dentry, struct iattr *iattr)
 {
 	int retval;
+	struct inode *inode = d_inode(dentry);
+	struct v9fs_inode *v9inode = V9FS_I(inode);
 	struct v9fs_session_info *v9ses;
 	struct p9_fid *fid = NULL;
 	struct p9_wstat wstat;

@@ -1078,20 +1080,22 @@ static int v9fs_vfs_setattr(struct dentry *dentry, struct iattr *iattr)
 
 	/* Write all dirty data */
 	if (d_is_reg(dentry))
-		filemap_write_and_wait(d_inode(dentry)->i_mapping);
+		filemap_write_and_wait(inode->i_mapping);
 
 	retval = p9_client_wstat(fid, &wstat);
 	if (retval < 0)
 		return retval;
 
 	if ((iattr->ia_valid & ATTR_SIZE) &&
-	    iattr->ia_size != i_size_read(d_inode(dentry)))
-		truncate_setsize(d_inode(dentry), iattr->ia_size);
+	    iattr->ia_size != i_size_read(inode)) {
+		truncate_setsize(inode, iattr->ia_size);
+		fscache_resize_cookie(v9fs_inode_cookie(v9inode), iattr->ia_size);
+	}
 
-	v9fs_invalidate_inode_attr(d_inode(dentry));
+	v9fs_invalidate_inode_attr(inode);
 
-	setattr_copy(d_inode(dentry), iattr);
-	mark_inode_dirty(d_inode(dentry));
+	setattr_copy(inode, iattr);
+	mark_inode_dirty(inode);
 	return 0;
 }

fs/9p/vfs_inode_dotl.c +1 -2

index 0028eccb665a..773ec75eefbe 100644
@@ -340,8 +340,7 @@ v9fs_vfs_atomic_open_dotl(struct inode *dir, struct dentry *dentry,
 	if (err)
 		goto err_clunk_old_fid;
 	file->private_data = ofid;
-	if (v9ses->cache == CACHE_LOOSE || v9ses->cache == CACHE_FSCACHE)
-		v9fs_cache_inode_set_cookie(inode, file);
+	fscache_use_cookie(v9fs_inode_cookie(v9inode), file->f_mode & FMODE_WRITE);
 	file->f_mode |= FMODE_CREATED;
 out:
 	v9fs_put_acl(dacl, pacl);

fs/afs/inode.c +3 -6

index 10ef9c0f32ac..068e5c8e7586 100644
@@ -786,6 +786,7 @@ int afs_drop_inode(struct inode *inode)
  */
 void afs_evict_inode(struct inode *inode)
 {
+	struct afs_vnode_cache_aux aux;
 	struct afs_vnode *vnode = AFS_FS_I(inode);
 
 	_enter("{%llx:%llu.%d}",

@@ -799,12 +800,8 @@ void afs_evict_inode(struct inode *inode)
 
 	truncate_inode_pages_final(&inode->i_data);
 
-	if (inode->i_state & I_PINNING_FSCACHE_WB) {
-		struct afs_vnode_cache_aux aux;
-		loff_t i_size = i_size_read(&vnode->vfs_inode);
-		aux.data_version = vnode->status.data_version;
-		fscache_unuse_cookie(afs_vnode_cache(vnode), &aux, &i_size);
-	}
+	aux.data_version = vnode->status.data_version;
+	fscache_clear_inode_writeback(afs_vnode_cache(vnode), inode, &aux);
 	clear_inode(inode);
 
 	while (!list_empty(&vnode->wb_keys)) {

include/linux/fscache.h +20 -0

index 0613ccea88c1..f129c159e1c1 100644
@@ -682,4 +682,24 @@ static inline void fscache_unpin_writeback(struct writeback_control *wbc,
 		fscache_unuse_cookie(cookie, NULL, NULL);
 }
 
+/**
+ * fscache_clear_inode_writeback - Clear writeback resources pinned by an inode
+ * @cookie: The cookie referring to the cache object
+ * @inode: The inode to clean up
+ * @aux: Auxiliary data to apply to the inode
+ *
+ * Clear any writeback resources held by an inode when the inode is evicted.
+ * This must be called before clear_inode() is called.
+ */
+static inline void fscache_clear_inode_writeback(struct fscache_cookie *cookie,
+						 struct inode *inode,
+						 const void *aux)
+{
+	if (inode->i_state & I_PINNING_FSCACHE_WB) {
+		loff_t i_size = i_size_read(inode);
+		fscache_unuse_cookie(cookie, aux, &i_size);
+	}
+
+}
+
 #endif /* _LINUX_FSCACHE_H */

David Howells

author maintainer

2020-11-18 11:44 UTC #1 lore raw

Apologies, I forgot to commit all the changes to my patch before posting it. There are a couple of additional changes to v9fs_write_end.

David
---

fs/9p/Kconfig +1 -1

index 66ca72422961..a9407693ef2d 100644
@@ -13,7 +13,7 @@ config 9P_FS
 if 9P_FS
 config 9P_FSCACHE
 	bool "Enable 9P client caching support"
-	depends on 9P_FS=m && FSCACHE_OLD || 9P_FS=y && FSCACHE_OLD=y
+	depends on 9P_FS=m || 9P_FS=y
 	help
 	  Choose Y here to enable persistent, read-only local
 	  caching support for 9p clients using FS-Cache

fs/9p/cache.c +10 ~~-257~~

299 lines not on this page · raw message

fs/9p/cache.h +0 ~~-107~~

126 lines not on this page · raw message

fs/9p/v9fs.c +3 -3

index 39def020a074..f7f9f2f055ec 100644
@@ -467,7 +467,8 @@ struct p9_fid *v9fs_session_init(struct v9fs_session_info *v9ses,
 
 #ifdef CONFIG_9P_FSCACHE
 	/* register the session for caching */
-	v9fs_cache_session_get_cookie(v9ses);
+	if (v9ses->cache == CACHE_LOOSE || v9ses->cache == CACHE_FSCACHE)
+		v9fs_cache_session_get_cookie(v9ses);
 #endif
 	spin_lock(&v9fs_sessionlist_lock);
 	list_add(&v9ses->slist, &v9fs_sessionlist);

@@ -500,8 +501,7 @@ void v9fs_session_close(struct v9fs_session_info *v9ses)
 	}
 
 #ifdef CONFIG_9P_FSCACHE
-	if (v9ses->fscache)
-		v9fs_cache_session_put_cookie(v9ses);
+	fscache_relinquish_cookie(v9fs_session_cache(v9ses), false);
 	kfree(v9ses->cachetag);
 #endif
 	kfree(v9ses->uname);

fs/9p/v9fs.h +19 -1

index 7b763776306e..b248586d7a2c 100644
@@ -109,7 +109,6 @@ struct v9fs_session_info {
 
 struct v9fs_inode {
 #ifdef CONFIG_9P_FSCACHE
-	struct mutex fscache_lock;
 	struct fscache_cookie *fscache;
 #endif
 	struct p9_qid qid;

@@ -124,6 +123,25 @@ static inline struct v9fs_inode *V9FS_I(const struct inode *inode)
 	return container_of(inode, struct v9fs_inode, vfs_inode);
 }
 
+static inline struct fscache_cookie *v9fs_inode_cookie(struct v9fs_inode *v9inode)
+{
+#ifdef CONFIG_9P_FSCACHE
+	return v9inode->fscache;
+#else
+	return NULL;
+#endif	
+}
+
+static inline struct fscache_cookie *v9fs_session_cache(struct v9fs_session_info *v9ses)
+{
+#ifdef CONFIG_9P_FSCACHE
+	return v9ses->fscache;
+#else
+	return NULL;
+#endif	
+}
+
+
 extern int v9fs_show_options(struct seq_file *m, struct dentry *root);
 
 struct p9_fid *v9fs_session_init(struct v9fs_session_info *, const char *,

fs/9p/vfs_addr.c +107 ~~-99~~

326 lines not on this page · raw message

fs/9p/vfs_dir.c +12 -1

index 674d22bf4f6f..eade5091654a 100644
@@ -19,6 +19,7 @@
 #include <linux/idr.h>
 #include <linux/slab.h>
 #include <linux/uio.h>
+#include <linux/fscache.h>
 #include <net/9p/9p.h>
 #include <net/9p/client.h>

@@ -205,13 +206,23 @@ static int v9fs_dir_readdir_dotl(struct file *file, struct dir_context *ctx)
 
 int v9fs_dir_release(struct inode *inode, struct file *filp)
 {
+	struct v9fs_inode *v9inode = V9FS_I(inode);
 	struct p9_fid *fid;
-
+	loff_t i_size;
+	
 	fid = filp->private_data;
 	p9_debug(P9_DEBUG_VFS, "inode: %p filp: %p fid: %d\n",
 		 inode, filp, fid ? fid->fid : -1);
 	if (fid)
 		p9_client_clunk(fid);
+
+	if ((filp->f_mode & FMODE_WRITE)) {
+		i_size = i_size_read(inode);
+		fscache_unuse_cookie(v9fs_inode_cookie(v9inode),
+				     &v9inode->qid.version, &i_size);
+	} else {
+		fscache_unuse_cookie(v9fs_inode_cookie(v9inode), NULL, NULL);
+	}
 	return 0;
 }

fs/9p/vfs_file.c +18 -6

index b177fd3b1eb3..d6b393c223b6 100644
@@ -94,8 +94,7 @@ int v9fs_file_open(struct inode *inode, struct file *file)
 		v9inode->writeback_fid = (void *) fid;
 	}
 	mutex_unlock(&v9inode->v_mutex);
-	if (v9ses->cache == CACHE_LOOSE || v9ses->cache == CACHE_FSCACHE)
-		v9fs_cache_inode_set_cookie(inode, file);
+	fscache_use_cookie(v9fs_inode_cookie(v9inode), file->f_mode & FMODE_WRITE);
 	return 0;
 out_error:
 	p9_client_clunk(file->private_data);

@@ -554,14 +553,27 @@ v9fs_vm_page_mkwrite(struct vm_fault *vmf)
 	p9_debug(P9_DEBUG_VFS, "page %p fid %lx\n",
 		 page, (unsigned long)filp->private_data);
 
+	v9inode = V9FS_I(inode);
+
+	/* Wait for the page to be written to the cache before we allow it to
+	 * be modified.  We then assume the entire page will need writing back.
+	 */
+#ifdef CONFIG_V9FS_FSCACHE
+	if (PageFsCache(page) &&
+	    wait_on_page_bit_killable(page, PG_fscache) < 0)
+		return VM_FAULT_RETRY;
+#endif
+
+	if (PageWriteback(page) &&
+	    wait_on_page_bit_killable(page, PG_writeback) < 0)
+		return VM_FAULT_RETRY;
+
 	/* Update file times before taking page lock */
 	file_update_time(filp);
 
-	v9inode = V9FS_I(inode);
-	/* make sure the cache has finished storing the page */
-	v9fs_fscache_wait_on_page_write(inode, page);
 	BUG_ON(!v9inode->writeback_fid);
-	lock_page(page);
+	if (lock_page_killable(page) < 0)
+		return VM_FAULT_RETRY;
 	if (page->mapping != inode->i_mapping)
 		goto out_unlock;
 	wait_for_stable_page(page);

fs/9p/vfs_inode.c +14 ~~-10~~

index ae0c38ad1fcb..4cdf4660d7c2 100644
@@ -228,7 +228,6 @@ struct inode *v9fs_alloc_inode(struct super_block *sb)
 		return NULL;
 #ifdef CONFIG_9P_FSCACHE
 	v9inode->fscache = NULL;
-	mutex_init(&v9inode->fscache_lock);
 #endif
 	v9inode->writeback_fid = NULL;
 	v9inode->cache_validity = 0;

@@ -377,10 +376,12 @@ void v9fs_evict_inode(struct inode *inode)
 	struct v9fs_inode *v9inode = V9FS_I(inode);
 
 	truncate_inode_pages_final(&inode->i_data);
+	fscache_clear_inode_writeback(v9fs_inode_cookie(v9inode), inode,
+				      &v9inode->qid.version);
 	clear_inode(inode);
 	filemap_fdatawrite(&inode->i_data);
 
-	v9fs_cache_inode_put_cookie(inode);
+	fscache_relinquish_cookie(v9fs_inode_cookie(v9inode), false);
 	/* clunk the fid stashed in writeback_fid */
 	if (v9inode->writeback_fid) {
 		p9_client_clunk(v9inode->writeback_fid);

@@ -846,8 +847,7 @@ v9fs_vfs_atomic_open(struct inode *dir, struct dentry *dentry,
 		goto error;
 
 	file->private_data = fid;
-	if (v9ses->cache == CACHE_LOOSE || v9ses->cache == CACHE_FSCACHE)
-		v9fs_cache_inode_set_cookie(d_inode(dentry), file);
+	fscache_use_cookie(v9fs_inode_cookie(v9inode), file->f_mode & FMODE_WRITE);
 
 	file->f_mode |= FMODE_CREATED;
 out:

@@ -1035,6 +1035,8 @@ v9fs_vfs_getattr(const struct path *path, struct kstat *stat,
 static int v9fs_vfs_setattr(struct dentry *dentry, struct iattr *iattr)
 {
 	int retval;
+	struct inode *inode = d_inode(dentry);
+	struct v9fs_inode *v9inode = V9FS_I(inode);
 	struct v9fs_session_info *v9ses;
 	struct p9_fid *fid = NULL;
 	struct p9_wstat wstat;

@@ -1078,20 +1080,22 @@ static int v9fs_vfs_setattr(struct dentry *dentry, struct iattr *iattr)
 
 	/* Write all dirty data */
 	if (d_is_reg(dentry))
-		filemap_write_and_wait(d_inode(dentry)->i_mapping);
+		filemap_write_and_wait(inode->i_mapping);
 
 	retval = p9_client_wstat(fid, &wstat);
 	if (retval < 0)
 		return retval;
 
 	if ((iattr->ia_valid & ATTR_SIZE) &&
-	    iattr->ia_size != i_size_read(d_inode(dentry)))
-		truncate_setsize(d_inode(dentry), iattr->ia_size);
+	    iattr->ia_size != i_size_read(inode)) {
+		truncate_setsize(inode, iattr->ia_size);
+		fscache_resize_cookie(v9fs_inode_cookie(v9inode), iattr->ia_size);
+	}
 
-	v9fs_invalidate_inode_attr(d_inode(dentry));
+	v9fs_invalidate_inode_attr(inode);
 
-	setattr_copy(d_inode(dentry), iattr);
-	mark_inode_dirty(d_inode(dentry));
+	setattr_copy(inode, iattr);
+	mark_inode_dirty(inode);
 	return 0;
 }

fs/9p/vfs_inode_dotl.c +1 -2

index 0028eccb665a..773ec75eefbe 100644
@@ -340,8 +340,7 @@ v9fs_vfs_atomic_open_dotl(struct inode *dir, struct dentry *dentry,
 	if (err)
 		goto err_clunk_old_fid;
 	file->private_data = ofid;
-	if (v9ses->cache == CACHE_LOOSE || v9ses->cache == CACHE_FSCACHE)
-		v9fs_cache_inode_set_cookie(inode, file);
+	fscache_use_cookie(v9fs_inode_cookie(v9inode), file->f_mode & FMODE_WRITE);
 	file->f_mode |= FMODE_CREATED;
 out:
 	v9fs_put_acl(dacl, pacl);

fs/afs/inode.c +3 -6

index 10ef9c0f32ac..068e5c8e7586 100644
@@ -786,6 +786,7 @@ int afs_drop_inode(struct inode *inode)
  */
 void afs_evict_inode(struct inode *inode)
 {
+	struct afs_vnode_cache_aux aux;
 	struct afs_vnode *vnode = AFS_FS_I(inode);
 
 	_enter("{%llx:%llu.%d}",

@@ -799,12 +800,8 @@ void afs_evict_inode(struct inode *inode)
 
 	truncate_inode_pages_final(&inode->i_data);
 
-	if (inode->i_state & I_PINNING_FSCACHE_WB) {
-		struct afs_vnode_cache_aux aux;
-		loff_t i_size = i_size_read(&vnode->vfs_inode);
-		aux.data_version = vnode->status.data_version;
-		fscache_unuse_cookie(afs_vnode_cache(vnode), &aux, &i_size);
-	}
+	aux.data_version = vnode->status.data_version;
+	fscache_clear_inode_writeback(afs_vnode_cache(vnode), inode, &aux);
 	clear_inode(inode);
 
 	while (!list_empty(&vnode->wb_keys)) {

include/linux/fscache.h +20 -0

index 0613ccea88c1..f129c159e1c1 100644
@@ -682,4 +682,24 @@ static inline void fscache_unpin_writeback(struct writeback_control *wbc,
 		fscache_unuse_cookie(cookie, NULL, NULL);
 }
 
+/**
+ * fscache_clear_inode_writeback - Clear writeback resources pinned by an inode
+ * @cookie: The cookie referring to the cache object
+ * @inode: The inode to clean up
+ * @aux: Auxiliary data to apply to the inode
+ *
+ * Clear any writeback resources held by an inode when the inode is evicted.
+ * This must be called before clear_inode() is called.
+ */
+static inline void fscache_clear_inode_writeback(struct fscache_cookie *cookie,
+						 struct inode *inode,
+						 const void *aux)
+{
+	if (inode->i_state & I_PINNING_FSCACHE_WB) {
+		loff_t i_size = i_size_read(inode);
+		fscache_unuse_cookie(cookie, aux, &i_size);
+	}
+
+}
+
 #endif /* _LINUX_FSCACHE_H */

David Howells

author maintainer

2020-11-18 12:01 UTC #2 lore raw

Quoted 4 lines · David Howells

fs/9p/vfs_addr.c +3 -0

+static void v9fs_req_issue_op(struct netfs_read_subrequest *subreq)
 {
+	struct netfs_read_request *rreq = subreq->rreq;
+	struct p9_fid *fid = rreq->netfs_priv;

fs/9p/vfs_addr.c line 48

Quoted 10 lines

fs/9p/vfs_addr.c +6 -0

 	struct iov_iter to;
+	loff_t pos = subreq->start + subreq->transferred;
+	size_t len = subreq->len   - subreq->transferred;
 	int retval, err;
 
+	iov_iter_xarray(&to, READ, &rreq->mapping->i_pages, pos, len);
 
+	retval = p9_client_read(fid, pos, &to, &err);
+	if (retval)
+		subreq->error = retval;

I forgot something: netfs_subreq_terminated() needs to be called when the read is complete. If p9_client_read() is synchronous, then that would be here, probably something like:

	retval = p9_client_read(fid, pos, &to, &err);
	netfs_subreq_terminated(subreq, retval);

is what's required. Manually setting subreq->error can then be removed.

David

Dominique Martinet

maintainer

2020-11-18 12:49 UTC #3 lore raw

In reply to David Howells

David Howells wrote on Wed, Nov 18, 2020:

Quoted 5 lines

Here's a rough draft of a patch to convert 9P to use the rewritten fscache API. It compiles, but I've no way to test it. This is built on top of my fscache-iter branch:
https://git.kernel.org/pub/scm/linux/kernel/git/dhowells/linux-fs.git/log/?h=fscache-iter

Thanks, I'm ashamed I hadn't found time to work on this it's a great help.
I can get some test running with this.

What's the current schedule/plan for the fscache branch merging? Will you be trying this merge window next month?

Couple more questions below

Quoted 19 lines

Notes:
(*) I've switched to use ITER_XATTR rather than ITER_BVEC in some places.
(*) I've added a pair of helper functions to get the cookie:
	v9fs_inode_cookie()
	v9fs_session_cache()

     These return NULL if CONFIG_9P_FSCACHE=n.
(*) I've moved some of the fscache accesses inline. Using the above helper
functions, it all compiles away due to NULL pointer checks in the header file if fscache is disabled.
(*) 9P's readpage and readpages now just jump into the netfs helpers, as does
write_begin. v9fs_req_issue_op() initiates the I/O on behalf of the helpers.

Quoted 7 lines

(*) v9fs_write_begin() now returns the subpage and v9fs_write_end() goes back
out to the head page. thp_size() is also used. This should mean you handle transparent huge pages (THPs) and can turn that on.
(*) I have made an assumption that 9p_client_read() and write can handle I/Os
larger than a page. If this is not the case, v9fs_req_ops will need clamp_length() implementing.

There's a max driven by the client's msize (client->msize - P9_IOHDRSZ ; unfortunately msize is just guaranted to be >= 4k so that means the actual IO size would be smaller in that case even if that's not intended to be common)

Quoted 5 lines

(*) The expand_readahead() and clamp_length() ops should perhaps be
implemented to align and trim with respect to maximum I/O size.
(*) iget and evict acquire and relinquish a cookie.

Quoted 8 lines

(*) open and release use and unuse that cookie.
(*) writepage writes the dirty data to the cache.
(*) setattr resizes the cache if necessary.
(*) The cache needs to be invalidated if a 3rd-party change happens, but I
haven't done that.

There's no concurrent access logic in 9p as far as I'm aware (like NFS does if the mtime changes for example), so I assume we can keep ignoring this.

Quoted 6 lines

(*) With these changes, 9p should cache local changes too, not just data
read.
(*) If 9p supports DIO writes, it should invalidate a cache object with
FSCACHE_INVAL_DIO_WRITE when one happens - thereby stopping caching for that file until all file handles on it are closed.

Not 100% sure actually there is some code about it but comment says it's disabled when cache is active; I'll check just found another problem with some queued patch that need fixing first...

Quoted 3 lines

I forgot something: netfs_subreq_terminated() needs to be called when the read is complete. If p9_client_read() is synchronous, then that would be here,

(it is synchronous; I'll add that suggestion)

Signature · 1 line

Dominique

David Howells

author maintainer

2020-11-18 13:39 UTC #4 lore raw

Quoted 2 lines · Dominique Martinet

What's the current schedule/plan for the fscache branch merging? Will you be trying this merge window next month?

That's the aim. We have afs, ceph and nfs are about ready; I've had a go at doing the 9p conversion, which I'll have to leave to you now, I think, and am having a poke at cifs.

Quoted 5 lines

Quoted 3 lines
(*) I have made an assumption that 9p_client_read() and write can handle I/Os
larger than a page. If this is not the case, v9fs_req_ops will need clamp_length() implementing.
There's a max driven by the client's msize

The netfs read helpers provide you with a couple of options here:

(1) You can use ->clamp_length() to do multiple slices of at least 1 byte each. The assumption being that these represent parallel operations. A new subreq will be generated for each slice.

(2) You can go with large slices that are larger than msize, and just read part of it with each read. After reading, the netfs helper will keep calling you again to read some more of it, provided you didn't return an error and you at least read something.

Quoted 3 lines

(client->msize - P9_IOHDRSZ ; unfortunately msize is just guaranted to be >= 4k so that means the actual IO size would be smaller in that case even if that's not intended to be common)

Does that mean you might be limited to reads of less than PAGE_SIZE on some systems (ppc64 for example)? This isn't a problem for the read helper, but might be an issue for writing from THPs.

Quoted 6 lines

Quoted 2 lines
(*) The cache needs to be invalidated if a 3rd-party change happens, but I
haven't done that.
There's no concurrent access logic in 9p as far as I'm aware (like NFS does if the mtime changes for example), so I assume we can keep ignoring this.

By that, I presume you mean concurrent accesses are just not permitted?

Quoted 7 lines

Quoted 3 lines
(*) If 9p supports DIO writes, it should invalidate a cache object with
FSCACHE_INVAL_DIO_WRITE when one happens - thereby stopping caching for that file until all file handles on it are closed.
Not 100% sure actually there is some code about it but comment says it's disabled when cache is active; I'll check just found another problem with some queued patch that need fixing first...

Ok.

Quoted 5 lines

Quoted 3 lines
I forgot something: netfs_subreq_terminated() needs to be called when the read is complete. If p9_client_read() is synchronous, then that would be here,
(it is synchronous; I'll add that suggestion)

Thanks.

David

Dominique Martinet

maintainer

2020-11-18 14:17 UTC #5 lore raw

David Howells wrote on Wed, Nov 18, 2020:

Quoted 6 lines

Quoted 2 lines
What's the current schedule/plan for the fscache branch merging? Will you be trying this merge window next month?
That's the aim. We have afs, ceph and nfs are about ready; I've had a go at doing the 9p conversion, which I'll have to leave to you now, I think, and am having a poke at cifs.

Ok, will try to polish it up by then.
Worst case as discussed is to have fscache be an alias for cache=loose until it's ready but with the first version you gave me it hopefully won't be needed.

Quoted 8 lines

Quoted 5 lines
Quoted 3 lines
(*) I have made an assumption that 9p_client_read() and write can handle I/Os
larger than a page. If this is not the case, v9fs_req_ops will need clamp_length() implementing.
There's a max driven by the client's msize
The netfs read helpers provide you with a couple of options here:

Quoted 8 lines

(1) You can use ->clamp_length() to do multiple slices of at least 1 byte each. The assumption being that these represent parallel operations. A new subreq will be generated for each slice.
(2) You can go with large slices that are larger than msize, and just read part of it with each read. After reading, the netfs helper will keep calling you again to read some more of it, provided you didn't return an error and you at least read something.

clamp_length looks good for that, if we can get parallel requests out it'll all come back faster.

Quoted 7 lines

Quoted 3 lines
(client->msize - P9_IOHDRSZ ; unfortunately msize is just guaranted to be >= 4k so that means the actual IO size would be smaller in that case even if that's not intended to be common)
Does that mean you might be limited to reads of less than PAGE_SIZE on some systems (ppc64 for example)? This isn't a problem for the read helper, but might be an issue for writing from THPs.

Quite likely, the actual used size varies depending on the backend (64k for tcp, 500k for virtio) but can definietly be less than PAGE_SIZE.

I take it the read helper would just iterate as long as there's data still required to read, writing from THPs wouldn't do that?

Quoted 8 lines

Quoted 6 lines
Quoted 2 lines
(*) The cache needs to be invalidated if a 3rd-party change happens, but I
haven't done that.
There's no concurrent access logic in 9p as far as I'm aware (like NFS does if the mtime changes for example), so I assume we can keep ignoring this.
By that, I presume you mean concurrent accesses are just not permitted?

Sorry - I meant coherency if files are modified on multiple clients isn't guaranted - there are voluntary locks but that's about it, nothing will detect e.g. remote file size modifications.
Concurrency on a given client works fine and should be used if possible.

Signature · 1 line

Dominique

David Howells

author maintainer

2020-11-18 15:03 UTC #6 lore raw

Quoted 2 lines · Dominique Martinet

I take it the read helper would just iterate as long as there's data still required to read, writing from THPs wouldn't do that?

Yep. As long as you read some data, the helper will call you again if you didn't read everything. subreq->transferred keeps track of what has been read so far. You can also tell the helper just to clear the rest by setting NETFS_SREQ_CLEAR_TAIL.

The helper tries to hide the pages from you as far as possible. Using ITER_XARRAY hides that even more.

David

David Howells

author maintainer

2020-11-18 14:59 UTC #7 lore raw

In reply to David Howells

Another bit I forgot: you need something like:

int afs_set_page_dirty(struct page *page)

{
	return fscache_set_page_dirty(page, afs_vnode_cache(AFS_FS_I(page->mapping->host)));
}

and point the set_page_dirty address space op at it.

David