[PATCH 0/5] orangefs: misc attribute fixes

[PATCH 0/5] orangefs: misc attribute fixes

[Martin Brandenburg]

Mike,

Here's a few fixes for OrangeFS.

Most important is to run getattr for size during a mmap page fault.  

I have also found a bug in d_revalidate.  The test is backwards.
However I haven't found a test that demonstrates that it is wrong, which
makes me doubt that all the code here is completely optimal.

The others are supposed to improve performance but appear to be below
the noise floor.

Martin Brandenburg (5):
  orangefs: open code short single-use functions
  orangefs: implement vm_ops->fault
  orangefs: do not invalidate attributes on inode create
  orangefs: do not invalidate attribute cache on setattr
  orangefs: reverse sense of revalidate is-inode-stale test

 fs/orangefs/dcache.c         |  17 +++---
 fs/orangefs/file.c           | 122 ++++++++++++++++---------------------------
 fs/orangefs/inode.c          |   6 ---
 fs/orangefs/namei.c          |   6 ---
 fs/orangefs/orangefs-utils.c |   4 --
 5 files changed, 51 insertions(+), 104 deletions(-)

-- 
2.15.0

[PATCH 1/5] orangefs: open code short single-use functions

[Martin Brandenburg]

Signed-off-by: Martin Brandenburg <martin@omnibond.com>
---
 fs/orangefs/file.c | 95 +++++++++++-------------------------------------------
 1 file changed, 19 insertions(+), 76 deletions(-)

diff --git a/fs/orangefs/file.c b/fs/orangefs/file.c
index 1668fd645c45..44879b1ff33e 100644
--- a/fs/orangefs/file.c
+++ b/fs/orangefs/file.c
@@ -41,70 +41,6 @@ static int flush_racache(struct inode *inode)
 	return ret;
 }
 
-/*
- * Copy to client-core's address space from the buffers specified
- * by the iovec upto total_size bytes.
- * NOTE: the iovector can either contain addresses which
- *       can futher be kernel-space or user-space addresses.
- *       or it can pointers to struct page's
- */
-static int precopy_buffers(int buffer_index,
-			   struct iov_iter *iter,
-			   size_t total_size)
-{
-	int ret = 0;
-	/*
-	 * copy data from application/kernel by pulling it out
-	 * of the iovec.
-	 */
-
-
-	if (total_size) {
-		ret = orangefs_bufmap_copy_from_iovec(iter,
-						      buffer_index,
-						      total_size);
-		if (ret < 0)
-		gossip_err("%s: Failed to copy-in buffers. Please make sure that the pvfs2-client is running. %ld\n",
-			   __func__,
-			   (long)ret);
-	}
-
-	if (ret < 0)
-		gossip_err("%s: Failed to copy-in buffers. Please make sure that the pvfs2-client is running. %ld\n",
-			__func__,
-			(long)ret);
-	return ret;
-}
-
-/*
- * Copy from client-core's address space to the buffers specified
- * by the iovec upto total_size bytes.
- * NOTE: the iovector can either contain addresses which
- *       can futher be kernel-space or user-space addresses.
- *       or it can pointers to struct page's
- */
-static int postcopy_buffers(int buffer_index,
-			    struct iov_iter *iter,
-			    size_t total_size)
-{
-	int ret = 0;
-	/*
-	 * copy data to application/kernel by pushing it out to
-	 * the iovec. NOTE; target buffers can be addresses or
-	 * struct page pointers.
-	 */
-	if (total_size) {
-		ret = orangefs_bufmap_copy_to_iovec(iter,
-						    buffer_index,
-						    total_size);
-		if (ret < 0)
-			gossip_err("%s: Failed to copy-out buffers. Please make sure that the pvfs2-client is running (%ld)\n",
-				__func__,
-				(long)ret);
-	}
-	return ret;
-}
-
 /*
  * Post and wait for the I/O upcall to finish
  */
@@ -157,14 +93,15 @@ static ssize_t wait_for_direct_io(enum ORANGEFS_io_type type, struct inode *inod
 		     total_size);
 	/*
 	 * Stage 1: copy the buffers into client-core's address space
-	 * precopy_buffers only pertains to writes.
 	 */
-	if (type == ORANGEFS_IO_WRITE) {
-		ret = precopy_buffers(buffer_index,
-				      iter,
-				      total_size);
-		if (ret < 0)
+	if (type == ORANGEFS_IO_WRITE && total_size) {
+		ret = orangefs_bufmap_copy_from_iovec(iter, buffer_index,
+		    total_size);
+		if (ret < 0) {
+			gossip_err("%s: Failed to copy-in buffers. Please make sure that the pvfs2-client is running. %ld\n",
+			    __func__, (long)ret);
 			goto out;
+		}
 	}
 
 	gossip_debug(GOSSIP_FILE_DEBUG,
@@ -260,14 +197,20 @@ static ssize_t wait_for_direct_io(enum ORANGEFS_io_type type, struct inode *inod
 
 	/*
 	 * Stage 3: Post copy buffers from client-core's address space
-	 * postcopy_buffers only pertains to reads.
 	 */
-	if (type == ORANGEFS_IO_READ) {
-		ret = postcopy_buffers(buffer_index,
-				       iter,
-				       new_op->downcall.resp.io.amt_complete);
-		if (ret < 0)
+	if (type == ORANGEFS_IO_READ && new_op->downcall.resp.io.amt_complete) {
+		/*
+		 * NOTE: the iovector can either contain addresses which
+		 *       can futher be kernel-space or user-space addresses.
+		 *       or it can pointers to struct page's
+		 */
+		ret = orangefs_bufmap_copy_to_iovec(iter, buffer_index,
+		    new_op->downcall.resp.io.amt_complete);
+		if (ret < 0) {
+			gossip_err("%s: Failed to copy-out buffers. Please make sure that the pvfs2-client is running (%ld)\n",
+			    __func__, (long)ret);
 			goto out;
+		}
 	}
 	gossip_debug(GOSSIP_FILE_DEBUG,
 	    "%s(%pU): Amount %s, returned by the sys-io call:%d\n",
-- 
2.15.0

[PATCH 2/5] orangefs: implement vm_ops->fault

[Martin Brandenburg]

Must retrieve size before running filemap_fault so the kernel has
an up-to-date size.

This should have been caught by xfstests generic/246, but it was masked
by orangefs_new_inode, which set i_size to PAGE_SIZE.  When nothing
caused a getattr prior to a pagefault, i_size was still PAGE_SIZE.
Since xfstests only read 10 bytes, it did not catch this bug.

When orangefs_new_inode was modified to perform a getattr instead,
i_size was set to zero, as it was a newly created file.  Then
orangefs_file_write_iter did NOT set i_size, instead prefering to
invalidate the attribute cache and letting the next caller retrieve
i_size.  But the fault handler did not know it was supposed to retrieve
i_size.  So during xfstests, i_size was still zero, and filemap_fault
returned VM_FAULT_SIGBUS.

Fixes xfstests generic/080, generic/141, generic/215, generic/247,
generic/248, generic/437, and generic/452.

Signed-off-by: Martin Brandenburg <martin@omnibond.com>
---
 fs/orangefs/file.c | 27 +++++++++++++++++++++++++--
 1 file changed, 25 insertions(+), 2 deletions(-)

diff --git a/fs/orangefs/file.c b/fs/orangefs/file.c
index 44879b1ff33e..c88846dc432a 100644
--- a/fs/orangefs/file.c
+++ b/fs/orangefs/file.c
@@ -531,6 +531,28 @@ static long orangefs_ioctl(struct file *file, unsigned int cmd, unsigned long ar
 	return ret;
 }
 
+static int orangefs_fault(struct vm_fault *vmf)
+{
+	struct file *file = vmf->vma->vm_file;
+	int rc;
+	rc = orangefs_inode_getattr(file->f_mapping->host, 0, 1,
+	    STATX_SIZE);
+	if (rc == -ESTALE)
+		rc = -EIO;
+	if (rc) {
+		gossip_err("%s: orangefs_inode_getattr failed, "
+		    "rc:%d:.\n", __func__, rc);
+		return rc;
+	}
+	return filemap_fault(vmf);
+}
+
+const struct vm_operations_struct orangefs_file_vm_ops = {
+	.fault = orangefs_fault,
+	.map_pages = filemap_map_pages,
+	.page_mkwrite = filemap_page_mkwrite,
+};
+
 /*
  * Memory map a region of a file.
  */
@@ -546,8 +568,9 @@ static int orangefs_file_mmap(struct file *file, struct vm_area_struct *vma)
 	vma->vm_flags |= VM_SEQ_READ;
 	vma->vm_flags &= ~VM_RAND_READ;
 
-	/* Use readonly mmap since we cannot support writable maps. */
-	return generic_file_readonly_mmap(file, vma);
+	file_accessed(file);
+	vma->vm_ops = &orangefs_file_vm_ops;
+	return 0;
 }
 
 #define mapping_nrpages(idata) ((idata)->nrpages)
-- 
2.15.0

[PATCH 3/5] orangefs: do not invalidate attributes on inode create

[Martin Brandenburg]

When an inode is created, we fetch attributes from the server.  There is
no need to turn around and invalidate them.

No need to initialize attributes after the getattr either.  Either we'll
do nothing or do something wrong.

Signed-off-by: Martin Brandenburg <martin@omnibond.com>
---
 fs/orangefs/inode.c | 6 ------
 fs/orangefs/namei.c | 6 ------
 2 files changed, 12 deletions(-)

diff --git a/fs/orangefs/inode.c b/fs/orangefs/inode.c
index fe1d705ad91f..ff0c799f09e2 100644
--- a/fs/orangefs/inode.c
+++ b/fs/orangefs/inode.c
@@ -448,12 +448,6 @@ struct inode *orangefs_new_inode(struct super_block *sb, struct inode *dir,
 		goto out_iput;
 
 	orangefs_init_iops(inode);
-
-	inode->i_mode = mode;
-	inode->i_uid = current_fsuid();
-	inode->i_gid = current_fsgid();
-	inode->i_atime = inode->i_mtime = inode->i_ctime = current_time(inode);
-	inode->i_size = PAGE_SIZE;
 	inode->i_rdev = dev;
 
 	error = insert_inode_locked4(inode, hash, orangefs_test_inode, ref);
diff --git a/fs/orangefs/namei.c b/fs/orangefs/namei.c
index c98bba2dbc94..f79401b2486a 100644
--- a/fs/orangefs/namei.c
+++ b/fs/orangefs/namei.c
@@ -78,8 +78,6 @@ static int orangefs_create(struct inode *dir,
 	d_instantiate(dentry, inode);
 	unlock_new_inode(inode);
 	orangefs_set_timeout(dentry);
-	ORANGEFS_I(inode)->getattr_time = jiffies - 1;
-	ORANGEFS_I(inode)->getattr_mask = STATX_BASIC_STATS;
 
 	gossip_debug(GOSSIP_NAME_DEBUG,
 		     "%s: dentry instantiated for %pd\n",
@@ -335,8 +333,6 @@ static int orangefs_symlink(struct inode *dir,
 	d_instantiate(dentry, inode);
 	unlock_new_inode(inode);
 	orangefs_set_timeout(dentry);
-	ORANGEFS_I(inode)->getattr_time = jiffies - 1;
-	ORANGEFS_I(inode)->getattr_mask = STATX_BASIC_STATS;
 
 	gossip_debug(GOSSIP_NAME_DEBUG,
 		     "Inode (Symlink) %pU -> %pd\n",
@@ -405,8 +401,6 @@ static int orangefs_mkdir(struct inode *dir, struct dentry *dentry, umode_t mode
 	d_instantiate(dentry, inode);
 	unlock_new_inode(inode);
 	orangefs_set_timeout(dentry);
-	ORANGEFS_I(inode)->getattr_time = jiffies - 1;
-	ORANGEFS_I(inode)->getattr_mask = STATX_BASIC_STATS;
 
 	gossip_debug(GOSSIP_NAME_DEBUG,
 		     "Inode (Directory) %pU -> %pd\n",
-- 
2.15.0

[PATCH 4/5] orangefs: do not invalidate attribute cache on setattr

[Martin Brandenburg]

Signed-off-by: Martin Brandenburg <martin@omnibond.com>
---
 fs/orangefs/orangefs-utils.c | 4 ----
 1 file changed, 4 deletions(-)

diff --git a/fs/orangefs/orangefs-utils.c b/fs/orangefs/orangefs-utils.c
index 97fe93129f38..553b3ded51cd 100644
--- a/fs/orangefs/orangefs-utils.c
+++ b/fs/orangefs/orangefs-utils.c
@@ -437,10 +437,6 @@ int orangefs_inode_setattr(struct inode *inode, struct iattr *iattr)
 	}
 
 	op_release(new_op);
-
-	if (ret == 0)
-		orangefs_inode->getattr_time = jiffies - 1;
-
 	return ret;
 }
 
-- 
2.15.0

[PATCH 5/5] orangefs: reverse sense of revalidate is-inode-stale test

[Martin Brandenburg]

Signed-off-by: Martin Brandenburg <martin@omnibond.com>
---
 fs/orangefs/dcache.c | 17 +++++++----------
 1 file changed, 7 insertions(+), 10 deletions(-)

diff --git a/fs/orangefs/dcache.c b/fs/orangefs/dcache.c
index ae782df5c063..c7aa5c923477 100644
--- a/fs/orangefs/dcache.c
+++ b/fs/orangefs/dcache.c
@@ -118,8 +118,12 @@ static int orangefs_d_revalidate(struct dentry *dentry, unsigned int flags)
 		return 0;
 
 	/* We do not need to continue with negative dentries. */
-	if (!dentry->d_inode)
-		goto out;
+	if (!dentry->d_inode) {
+		gossip_debug(GOSSIP_DCACHE_DEBUG,
+		    "%s: negative dentry or positive dentry and inode valid.\n",
+		    __func__);
+		return 1;
+	}
 
 	/* Now we must perform a getattr to validate the inode contents. */
 
@@ -129,14 +133,7 @@ static int orangefs_d_revalidate(struct dentry *dentry, unsigned int flags)
 		    __FILE__, __func__, __LINE__);
 		return 0;
 	}
-	if (ret == 0)
-		return 0;
-
-out:
-	gossip_debug(GOSSIP_DCACHE_DEBUG,
-	    "%s: negative dentry or positive dentry and inode valid.\n",
-	    __func__);
-	return 1;
+	return !ret;
 }
 
 const struct dentry_operations orangefs_dentry_operations = {
-- 
2.15.0

Re: [PATCH 0/5] orangefs: misc attribute fixes

[martin]

On Tue, Nov 28, 2017 at 03:21:56PM -0500, Martin Brandenburg wrote:
> Mike,
> 
> Here's a few fixes for OrangeFS.
> 
> Most important is to run getattr for size during a mmap page fault.  
> 
> I have also found a bug in d_revalidate.  The test is backwards.
> However I haven't found a test that demonstrates that it is wrong, which
> makes me doubt that all the code here is completely optimal.

The reason I can't find a test is because this code only gets run
if the inode being tested has the same handle as the inode in the
dcache.  Of course this happens all the time (it wasn't changed).  We
would return that it was not valid even though it was, which didn't
affect correctness but caused OrangeFS to perform extra round-trips with
the server.

We would return that it was valid even though it wasn't when an inode
is deleted and re-created on the server as a different type or different
symlink target AND has the same handle as the one in the cache.  This
practically never happens.

Martin