[PATCH 0/3] staging: lustre: llite: fixups for ll_migrate()

[PATCH 0/3] staging: lustre: llite: fixups for ll_migrate()

[James Simmons]

This patch series covers all the current fixes for the function
ll_migrate(). Originally only the first patch was submitted but
it was pointed out other flaws existed in the code. Two more
patches that cover those flaws are included in this patch set.
These patches are order dependent.

Niu Yawei (1):
  staging: lustre: llite: check reply status in ll_migrate()

wang di (2):
  staging: lustre: llite: check request != NULL in ll_migrate
  staging: lustre: llite: root inode checking for migration

 drivers/staging/lustre/lustre/llite/file.c | 63 ++++++++++++++++--------------
 1 file changed, 34 insertions(+), 29 deletions(-)

--
1.8.3.1

[PATCH 1/3] staging: lustre: llite: check request != NULL in ll_migrate

[James Simmons]

From: wang di <di.wang@intel.com>

Check if the request is NULL, before retrieve reply body
from the request.

Signed-off-by: wang di <di.wang@intel.com>
Intel-bug-id: https://jira.hpdd.intel.com/browse/LU-7396
Reviewed-on: http://review.whamcloud.com/17079
Reviewed-by: John L. Hammond <john.hammond@intel.com>
Reviewed-by: Andreas Dilger <andreas.dilger@intel.com>
Signed-off-by: James Simmons <jsimmons@infradead.org>
---
 drivers/staging/lustre/lustre/llite/file.c | 41 +++++++++++++++++-------------
 1 file changed, 23 insertions(+), 18 deletions(-)

diff --git a/drivers/staging/lustre/lustre/llite/file.c b/drivers/staging/lustre/lustre/llite/file.c
index 9870901..0c83bd7 100644
--- a/drivers/staging/lustre/lustre/llite/file.c
+++ b/drivers/staging/lustre/lustre/llite/file.c
@@ -2667,28 +2667,33 @@ int ll_migrate(struct inode *parent, struct file *file, int mdtidx,
 	if (!rc)
 		ll_update_times(request, parent);
 
-	body = req_capsule_server_get(&request->rq_pill, &RMF_MDT_BODY);
-	if (!body) {
-		rc = -EPROTO;
-		goto out_free;
-	}
+	if (request) {
+		body = req_capsule_server_get(&request->rq_pill, &RMF_MDT_BODY);
+		if (!body) {
+			rc = -EPROTO;
+			goto out_free;
+		}
 
-	/*
-	 * If the server does release layout lock, then we cleanup
-	 * the client och here, otherwise release it in out_free:
-	 */
-	if (och && body->mbo_valid & OBD_MD_CLOSE_INTENT_EXECED) {
-		obd_mod_put(och->och_mod);
-		md_clear_open_replay_data(ll_i2sbi(parent)->ll_md_exp, och);
-		och->och_fh.cookie = DEAD_HANDLE_MAGIC;
-		kfree(och);
-		och = NULL;
-	}
+		/*
+		 * If the server does release layout lock, then we cleanup
+		 * the client och here, otherwise release it in out_free:
+		 */
+		if (och && body->mbo_valid & OBD_MD_CLOSE_INTENT_EXECED) {
+			obd_mod_put(och->och_mod);
+			md_clear_open_replay_data(ll_i2sbi(parent)->ll_md_exp,
+						  och);
+			och->och_fh.cookie = DEAD_HANDLE_MAGIC;
+			kfree(och);
+			och = NULL;
+		}
 
-	ptlrpc_req_finished(request);
+		ptlrpc_req_finished(request);
+	}
 	/* Try again if the file layout has changed. */
-	if (rc == -EAGAIN && S_ISREG(child_inode->i_mode))
+	if (rc == -EAGAIN && S_ISREG(child_inode->i_mode)) {
+		request = NULL;
 		goto again;
+	}
 out_free:
 	if (child_inode) {
 		if (och) /* close the file */
-- 
1.8.3.1

[PATCH 2/3] staging: lustre: llite: root inode checking for migration

[James Simmons]

From: wang di <di.wang@intel.com>

Do not migrate root inode.

Signed-off-by: wang di <di.wang@intel.com>
Intel-bug-id: https://jira.hpdd.intel.com/browse/LU-7577
Reviewed-on: http://review.whamcloud.com/17669
Reviewed-by: Lai Siyao <lai.siyao@intel.com>
Reviewed-by: John L. Hammond <john.hammond@intel.com>
Reviewed-by: Oleg Drokin <oleg.drokin@intel.com>
Signed-off-by: James Simmons <jsimmons@infradead.org>
---
 drivers/staging/lustre/lustre/llite/file.c | 30 +++++++++++++++---------------
 1 file changed, 15 insertions(+), 15 deletions(-)

diff --git a/drivers/staging/lustre/lustre/llite/file.c b/drivers/staging/lustre/lustre/llite/file.c
index 0c83bd7..271608d 100644
--- a/drivers/staging/lustre/lustre/llite/file.c
+++ b/drivers/staging/lustre/lustre/llite/file.c
@@ -2626,18 +2626,18 @@ int ll_migrate(struct inode *parent, struct file *file, int mdtidx,
 		       ll_get_fsname(parent->i_sb, NULL, 0), name,
 		       PFID(&op_data->op_fid3));
 		rc = -EINVAL;
-		goto out_free;
+		goto out_unlock;
 	}
 
 	rc = ll_get_mdt_idx_by_fid(ll_i2sbi(parent), &op_data->op_fid3);
 	if (rc < 0)
-		goto out_free;
+		goto out_unlock;
 
 	if (rc == mdtidx) {
 		CDEBUG(D_INFO, "%s:"DFID" is already on MDT%d.\n", name,
 		       PFID(&op_data->op_fid3), mdtidx);
 		rc = 0;
-		goto out_free;
+		goto out_unlock;
 	}
 again:
 	if (S_ISREG(child_inode->i_mode)) {
@@ -2645,13 +2645,13 @@ int ll_migrate(struct inode *parent, struct file *file, int mdtidx,
 		if (IS_ERR(och)) {
 			rc = PTR_ERR(och);
 			och = NULL;
-			goto out_free;
+			goto out_unlock;
 		}
 
 		rc = ll_data_version(child_inode, &data_version,
 				     LL_DV_WR_FLUSH);
 		if (rc)
-			goto out_free;
+			goto out_close;
 
 		op_data->op_handle = och->och_fh;
 		op_data->op_data = och->och_mod;
@@ -2671,12 +2671,12 @@ int ll_migrate(struct inode *parent, struct file *file, int mdtidx,
 		body = req_capsule_server_get(&request->rq_pill, &RMF_MDT_BODY);
 		if (!body) {
 			rc = -EPROTO;
-			goto out_free;
+			goto out_close;
 		}
 
 		/*
 		 * If the server does release layout lock, then we cleanup
-		 * the client och here, otherwise release it in out_free:
+		 * the client och here, otherwise release it in out_close:
 		 */
 		if (och && body->mbo_valid & OBD_MD_CLOSE_INTENT_EXECED) {
 			obd_mod_put(och->och_mod);
@@ -2694,15 +2694,15 @@ int ll_migrate(struct inode *parent, struct file *file, int mdtidx,
 		request = NULL;
 		goto again;
 	}
-out_free:
-	if (child_inode) {
-		if (och) /* close the file */
-			ll_lease_close(och, child_inode, NULL);
+out_close:
+	if (och) /* close the file */
+		ll_lease_close(och, child_inode, NULL);
+	if (!rc)
 		clear_nlink(child_inode);
-		inode_unlock(child_inode);
-		iput(child_inode);
-	}
-
+out_unlock:
+	inode_unlock(child_inode);
+	iput(child_inode);
+out_free:
 	ll_finish_md_op_data(op_data);
 	return rc;
 }
-- 
1.8.3.1

Re: [PATCH 2/3] staging: lustre: llite: root inode checking for migration

[Greg Kroah-Hartman]

On Sat, Feb 11, 2017 at 12:12:38PM -0500, James Simmons wrote:
> From: wang di <di.wang@intel.com>
> 
> Do not migrate root inode.

That says _what_ happens here, but you aren't giving any hint as to
_why_ this is needed.

I'll take this, but be more careful about your changelog comments in the
future...

thanks,

greg k-h

[PATCH 3/3] staging: lustre: llite: check reply status in ll_migrate()

[James Simmons]

From: Niu Yawei <yawei.niu@intel.com>

ll_migrate() should check reply status before trying to read
reply buffer, checking if request is NULL doesn't make sense.

Signed-off-by: Niu Yawei <yawei.niu@intel.com>
Intel-bug-id: https://jira.hpdd.intel.com/browse/LU-8807
Reviewed-on: https://review.whamcloud.com/23666
Reviewed-by: Andreas Dilger <andreas.dilger@intel.com>
Reviewed-by: Lai Siyao <lai.siyao@intel.com>
Reviewed-by: John L. Hammond <john.hammond@intel.com>
Reviewed-by: Oleg Drokin <oleg.drokin@intel.com>
Signed-off-by: James Simmons <jsimmons@infradead.org>
---
 drivers/staging/lustre/lustre/llite/file.c | 18 +++++++++---------
 1 file changed, 9 insertions(+), 9 deletions(-)

diff --git a/drivers/staging/lustre/lustre/llite/file.c b/drivers/staging/lustre/lustre/llite/file.c
index 271608d..10adfcd 100644
--- a/drivers/staging/lustre/lustre/llite/file.c
+++ b/drivers/staging/lustre/lustre/llite/file.c
@@ -2664,15 +2664,12 @@ int ll_migrate(struct inode *parent, struct file *file, int mdtidx,
 	op_data->op_cli_flags = CLI_MIGRATE;
 	rc = md_rename(ll_i2sbi(parent)->ll_md_exp, op_data, name,
 		       namelen, name, namelen, &request);
-	if (!rc)
+	if (!rc) {
+		LASSERT(request);
 		ll_update_times(request, parent);
 
-	if (request) {
 		body = req_capsule_server_get(&request->rq_pill, &RMF_MDT_BODY);
-		if (!body) {
-			rc = -EPROTO;
-			goto out_close;
-		}
+		LASSERT(body);
 
 		/*
 		 * If the server does release layout lock, then we cleanup
@@ -2686,14 +2683,17 @@ int ll_migrate(struct inode *parent, struct file *file, int mdtidx,
 			kfree(och);
 			och = NULL;
 		}
+	}
 
+	if (request) {
 		ptlrpc_req_finished(request);
+		request = NULL;
 	}
+
 	/* Try again if the file layout has changed. */
-	if (rc == -EAGAIN && S_ISREG(child_inode->i_mode)) {
-		request = NULL;
+	if (rc == -EAGAIN && S_ISREG(child_inode->i_mode))
 		goto again;
-	}
+
 out_close:
 	if (och) /* close the file */
 		ll_lease_close(och, child_inode, NULL);
-- 
1.8.3.1