[Ocfs2-devel] [PATCH 3/6] Shift allocation ocfs2_live_connection to user_connect()

[Ocfs2-devel] [PATCH 3/6] Shift allocation ocfs2_live_connection to user_connect()

[Goldwyn Rodrigues]

We perform this because the DLM recovery callbacks will require
the ocfs2_live_connection structure to record the node information
when dlm_new_lockspace() is updated.

[AKPM] rc initialization is not required because it assigned in case of
errors. It will be cleared by compiler anyways.

Signed-off-by: Goldwyn Rodrigues <rgoldwyn@suse.com>
---
 fs/ocfs2/stack_user.c | 35 +++++++++++++++++------------------
 1 file changed, 17 insertions(+), 18 deletions(-)

diff --git a/fs/ocfs2/stack_user.c b/fs/ocfs2/stack_user.c
index 4111855..88259fb 100644
--- a/fs/ocfs2/stack_user.c
+++ b/fs/ocfs2/stack_user.c
@@ -201,14 +201,9 @@ static struct ocfs2_live_connection *ocfs2_connection_find(const char *name)
  * fill_super(), we can't get dupes here.
  */
 static int ocfs2_live_connection_new(struct ocfs2_cluster_connection *conn,
-				     struct ocfs2_live_connection **c_ret)
+				     struct ocfs2_live_connection *c)
 {
 	int rc = 0;
-	struct ocfs2_live_connection *c;
-
-	c = kzalloc(sizeof(struct ocfs2_live_connection), GFP_KERNEL);
-	if (!c)
-		return -ENOMEM;
 
 	mutex_lock(&ocfs2_control_lock);
 	c->oc_conn = conn;
@@ -222,12 +217,6 @@ static int ocfs2_live_connection_new(struct ocfs2_cluster_connection *conn,
 	}
 
 	mutex_unlock(&ocfs2_control_lock);
-
-	if (!rc)
-		*c_ret = c;
-	else
-		kfree(c);
-
 	return rc;
 }
 
@@ -840,12 +829,18 @@ const struct dlm_lockspace_ops ocfs2_ls_ops = {
 static int user_cluster_connect(struct ocfs2_cluster_connection *conn)
 {
 	dlm_lockspace_t *fsdlm;
-	struct ocfs2_live_connection *uninitialized_var(control);
-	int rc = 0;
+	struct ocfs2_live_connection *lc;
+	int rc;
 
 	BUG_ON(conn == NULL);
 
-	rc = ocfs2_live_connection_new(conn, &control);
+	lc = kzalloc(sizeof(struct ocfs2_live_connection), GFP_KERNEL);
+	if (!lc) {
+		rc = -ENOMEM;
+		goto out;
+	}
+
+	rc = ocfs2_live_connection_new(conn, lc);
 	if (rc)
 		goto out;
 
@@ -861,20 +856,24 @@ static int user_cluster_connect(struct ocfs2_cluster_connection *conn)
 		       conn->cc_version.pv_major, conn->cc_version.pv_minor,
 		       running_proto.pv_major, running_proto.pv_minor);
 		rc = -EPROTO;
-		ocfs2_live_connection_drop(control);
+		ocfs2_live_connection_drop(lc);
+		lc = NULL;
 		goto out;
 	}
 
 	rc = dlm_new_lockspace(conn->cc_name, NULL, DLM_LSFL_FS, DLM_LVB_LEN,
 			       NULL, NULL, NULL, &fsdlm);
 	if (rc) {
-		ocfs2_live_connection_drop(control);
+		ocfs2_live_connection_drop(lc);
+		lc = NULL;
 		goto out;
 	}
 
-	conn->cc_private = control;
+	conn->cc_private = lc;
 	conn->cc_lockspace = fsdlm;
 out:
+	if (rc && lc)
+		kfree(lc);
 	return rc;
 }
 
-- 
1.8.1.4


-- 
Goldwyn

[Ocfs2-devel] [PATCH 3/6] Shift allocation ocfs2_live_connection to user_connect()

[Mark Fasheh]

On Fri, Oct 18, 2013 at 09:45:39AM -0500, Goldwyn Rodrigues wrote:
> We perform this because the DLM recovery callbacks will require
> the ocfs2_live_connection structure to record the node information
> when dlm_new_lockspace() is updated.

Ok but what I see below is that you took the alloc out of
ocfs2_live_connecion_new() and just do it above that call in
user_cluster_connect(), then call ocfs2_live_connection_new().

Aside from that though this doesn't seem to add new functionality, which is
fine if the later patches use what you've done here but I don't where this
change has made any impact on patches 4-6. So my guess is that there's one
of two things going on here:

1) This change was just left in by accident in which case you can just
remove this patch from your series.

2) I made an error reading the later patches and this _is_ used :)
If that's the case do me a favor and point out what I missed :)
Also, if this needs to stay I would rename ocfs2_live_connecion_new() since
_new() usually makes me think "allocates" and we're not doing that any more.
Maybe something like ocfs2_live_connection_attach()?
	--Mark

--
Mark Fasheh

[Ocfs2-devel] [PATCH 3/6] Shift allocation ocfs2_live_connection to user_connect()

[Goldwyn Rodrigues]

On 11/03/2013 05:14 PM, Mark Fasheh wrote:
> On Fri, Oct 18, 2013 at 09:45:39AM -0500, Goldwyn Rodrigues wrote:
>> We perform this because the DLM recovery callbacks will require
>> the ocfs2_live_connection structure to record the node information
>> when dlm_new_lockspace() is updated.
>
> Ok but what I see below is that you took the alloc out of
> ocfs2_live_connecion_new() and just do it above that call in
> user_cluster_connect(), then call ocfs2_live_connection_new().
>
> Aside from that though this doesn't seem to add new functionality, which is
> fine if the later patches use what you've done here but I don't where this
> change has made any impact on patches 4-6. So my guess is that there's one
> of two things going on here:
>
> 1) This change was just left in by accident in which case you can just
> remove this patch from your series.
>
> 2) I made an error reading the later patches and this _is_ used :)
> If that's the case do me a favor and point out what I missed :)

When dlm_new_lockspace is called, it calls recover_done() to inform our 
node number. This is recorded in the live_connection data structure and 
  needs ocfs2_live_connection to be available before the call to 
dlm_lockspce_new(). I will put this information in the next series.

> Also, if this needs to stay I would rename ocfs2_live_connecion_new() since
> _new() usually makes me think "allocates" and we're not doing that any more.
> Maybe something like ocfs2_live_connection_attach()?

Yes, we should change the name of the function.


-- 
Goldwyn

[Ocfs2-devel] [PATCH 3/6] Shift allocation ocfs2_live_connection to user_connect()

[Mark Fasheh]

On Sun, Nov 03, 2013 at 09:46:01PM -0600, Goldwyn Rodrigues wrote:
> On 11/03/2013 05:14 PM, Mark Fasheh wrote:
>> On Fri, Oct 18, 2013 at 09:45:39AM -0500, Goldwyn Rodrigues wrote:
>>> We perform this because the DLM recovery callbacks will require
>>> the ocfs2_live_connection structure to record the node information
>>> when dlm_new_lockspace() is updated.
>>
>> Ok but what I see below is that you took the alloc out of
>> ocfs2_live_connecion_new() and just do it above that call in
>> user_cluster_connect(), then call ocfs2_live_connection_new().
>>
>> Aside from that though this doesn't seem to add new functionality, which is
>> fine if the later patches use what you've done here but I don't where this
>> change has made any impact on patches 4-6. So my guess is that there's one
>> of two things going on here:
>>
>> 1) This change was just left in by accident in which case you can just
>> remove this patch from your series.
>>
>> 2) I made an error reading the later patches and this _is_ used :)
>> If that's the case do me a favor and point out what I missed :)
>
> When dlm_new_lockspace is called, it calls recover_done() to inform our 
> node number. This is recorded in the live_connection data structure and  
> needs ocfs2_live_connection to be available before the call to 
> dlm_lockspce_new(). I will put this information in the next series.

Ok great thanks for clearing that up. Putting that in the description
wouldn't hurt the next review - good call.
	--Mark

--
Mark Fasheh

[Ocfs2-devel] [PATCH 3/6] Shift allocation ocfs2_live_connection to user_connect()

[Goldwyn Rodrigues]

We perform this because the DLM recovery callbacks will require
the ocfs2_live_connection structure to record the node information
when dlm_new_lockspace() is updated (in the last patch of the series).

Before calling dlm_new_lockspace(), we need the structure ready for
the .recover_done() callback, which would set oc_this_node. This is
the reason we allocate ocfs2_live_connection beforehand in user_connect().

[AKPM] rc initialization is not required because it assigned in case of
errors. It will be cleared by compiler anyways.

Signed-off-by: Goldwyn Rodrigues <rgoldwyn@suse.com>
---
 fs/ocfs2/stack_user.c | 37 ++++++++++++++++++-------------------
 1 file changed, 18 insertions(+), 19 deletions(-)

diff --git a/fs/ocfs2/stack_user.c b/fs/ocfs2/stack_user.c
index 4111855..0afb86d 100644
--- a/fs/ocfs2/stack_user.c
+++ b/fs/ocfs2/stack_user.c
@@ -200,15 +200,10 @@ static struct ocfs2_live_connection *ocfs2_connection_find(const char *name)
  * mount path.  Since the VFS prevents multiple calls to
  * fill_super(), we can't get dupes here.
  */
-static int ocfs2_live_connection_new(struct ocfs2_cluster_connection *conn,
-				     struct ocfs2_live_connection **c_ret)
+static int ocfs2_live_connection_attach(struct ocfs2_cluster_connection *conn,
+				     struct ocfs2_live_connection *c)
 {
 	int rc = 0;
-	struct ocfs2_live_connection *c;
-
-	c = kzalloc(sizeof(struct ocfs2_live_connection), GFP_KERNEL);
-	if (!c)
-		return -ENOMEM;
 
 	mutex_lock(&ocfs2_control_lock);
 	c->oc_conn = conn;
@@ -222,12 +217,6 @@ static int ocfs2_live_connection_new(struct ocfs2_cluster_connection *conn,
 	}
 
 	mutex_unlock(&ocfs2_control_lock);
-
-	if (!rc)
-		*c_ret = c;
-	else
-		kfree(c);
-
 	return rc;
 }
 
@@ -840,12 +829,18 @@ const struct dlm_lockspace_ops ocfs2_ls_ops = {
 static int user_cluster_connect(struct ocfs2_cluster_connection *conn)
 {
 	dlm_lockspace_t *fsdlm;
-	struct ocfs2_live_connection *uninitialized_var(control);
-	int rc = 0;
+	struct ocfs2_live_connection *lc;
+	int rc;
 
 	BUG_ON(conn == NULL);
 
-	rc = ocfs2_live_connection_new(conn, &control);
+	lc = kzalloc(sizeof(struct ocfs2_live_connection), GFP_KERNEL);
+	if (!lc) {
+		rc = -ENOMEM;
+		goto out;
+	}
+
+	rc = ocfs2_live_connection_attach(conn, lc);
 	if (rc)
 		goto out;
 
@@ -861,20 +856,24 @@ static int user_cluster_connect(struct ocfs2_cluster_connection *conn)
 		       conn->cc_version.pv_major, conn->cc_version.pv_minor,
 		       running_proto.pv_major, running_proto.pv_minor);
 		rc = -EPROTO;
-		ocfs2_live_connection_drop(control);
+		ocfs2_live_connection_drop(lc);
+		lc = NULL;
 		goto out;
 	}
 
 	rc = dlm_new_lockspace(conn->cc_name, NULL, DLM_LSFL_FS, DLM_LVB_LEN,
 			       NULL, NULL, NULL, &fsdlm);
 	if (rc) {
-		ocfs2_live_connection_drop(control);
+		ocfs2_live_connection_drop(lc);
+		lc = NULL;
 		goto out;
 	}
 
-	conn->cc_private = control;
+	conn->cc_private = lc;
 	conn->cc_lockspace = fsdlm;
 out:
+	if (rc && lc)
+		kfree(lc);
 	return rc;
 }
 
-- 
1.8.1.4


-- 
Goldwyn

[Ocfs2-devel] [PATCH 3/6] Shift allocation ocfs2_live_connection to user_connect()

[Mark Fasheh]

Ok, we changed ocfs2_live_connection_new() to ocfs2_live_connection_attach()
per my previous request.

Reveiwed-by: Mark Fasheh <mfasheh@suse.de>

On Tue, Nov 12, 2013 at 08:07:14AM -0600, Goldwyn Rodrigues wrote:
> We perform this because the DLM recovery callbacks will require
> the ocfs2_live_connection structure to record the node information
> when dlm_new_lockspace() is updated (in the last patch of the series).
> 
> Before calling dlm_new_lockspace(), we need the structure ready for
> the .recover_done() callback, which would set oc_this_node. This is
> the reason we allocate ocfs2_live_connection beforehand in user_connect().
> 
> [AKPM] rc initialization is not required because it assigned in case of
> errors. It will be cleared by compiler anyways.
> 
> Signed-off-by: Goldwyn Rodrigues <rgoldwyn@suse.com>
> ---
>  fs/ocfs2/stack_user.c | 37 ++++++++++++++++++-------------------
>  1 file changed, 18 insertions(+), 19 deletions(-)
> 
> diff --git a/fs/ocfs2/stack_user.c b/fs/ocfs2/stack_user.c
> index 4111855..0afb86d 100644
> --- a/fs/ocfs2/stack_user.c
> +++ b/fs/ocfs2/stack_user.c
> @@ -200,15 +200,10 @@ static struct ocfs2_live_connection *ocfs2_connection_find(const char *name)
>   * mount path.  Since the VFS prevents multiple calls to
>   * fill_super(), we can't get dupes here.
>   */
> -static int ocfs2_live_connection_new(struct ocfs2_cluster_connection *conn,
> -				     struct ocfs2_live_connection **c_ret)
> +static int ocfs2_live_connection_attach(struct ocfs2_cluster_connection *conn,
> +				     struct ocfs2_live_connection *c)
>  {
>  	int rc = 0;
> -	struct ocfs2_live_connection *c;
> -
> -	c = kzalloc(sizeof(struct ocfs2_live_connection), GFP_KERNEL);
> -	if (!c)
> -		return -ENOMEM;
>  
>  	mutex_lock(&ocfs2_control_lock);
>  	c->oc_conn = conn;
> @@ -222,12 +217,6 @@ static int ocfs2_live_connection_new(struct ocfs2_cluster_connection *conn,
>  	}
>  
>  	mutex_unlock(&ocfs2_control_lock);
> -
> -	if (!rc)
> -		*c_ret = c;
> -	else
> -		kfree(c);
> -
>  	return rc;
>  }
>  
> @@ -840,12 +829,18 @@ const struct dlm_lockspace_ops ocfs2_ls_ops = {
>  static int user_cluster_connect(struct ocfs2_cluster_connection *conn)
>  {
>  	dlm_lockspace_t *fsdlm;
> -	struct ocfs2_live_connection *uninitialized_var(control);
> -	int rc = 0;
> +	struct ocfs2_live_connection *lc;
> +	int rc;
>  
>  	BUG_ON(conn == NULL);
>  
> -	rc = ocfs2_live_connection_new(conn, &control);
> +	lc = kzalloc(sizeof(struct ocfs2_live_connection), GFP_KERNEL);
> +	if (!lc) {
> +		rc = -ENOMEM;
> +		goto out;
> +	}
> +
> +	rc = ocfs2_live_connection_attach(conn, lc);
>  	if (rc)
>  		goto out;
>  
> @@ -861,20 +856,24 @@ static int user_cluster_connect(struct ocfs2_cluster_connection *conn)
>  		       conn->cc_version.pv_major, conn->cc_version.pv_minor,
>  		       running_proto.pv_major, running_proto.pv_minor);
>  		rc = -EPROTO;
> -		ocfs2_live_connection_drop(control);
> +		ocfs2_live_connection_drop(lc);
> +		lc = NULL;
>  		goto out;
>  	}
>  
>  	rc = dlm_new_lockspace(conn->cc_name, NULL, DLM_LSFL_FS, DLM_LVB_LEN,
>  			       NULL, NULL, NULL, &fsdlm);
>  	if (rc) {
> -		ocfs2_live_connection_drop(control);
> +		ocfs2_live_connection_drop(lc);
> +		lc = NULL;
>  		goto out;
>  	}
>  
> -	conn->cc_private = control;
> +	conn->cc_private = lc;
>  	conn->cc_lockspace = fsdlm;
>  out:
> +	if (rc && lc)
> +		kfree(lc);
>  	return rc;
>  }
>  
> -- 
> 1.8.1.4
> 
> 
> -- 
> Goldwyn
--
Mark Fasheh

[Ocfs2-devel] [PATCH 3/6] Shift allocation ocfs2_live_connection to user_connect()

[Goldwyn Rodrigues]

We perform this because the DLM recovery callbacks will require
the ocfs2_live_connection structure to record the node information
when dlm_new_lockspace() is updated (in the last patch of the series).

Before calling dlm_new_lockspace(), we need the structure ready for
the .recover_done() callback, which would set oc_this_node. This is
the reason we allocate ocfs2_live_connection beforehand in user_connect().

[AKPM] rc initialization is not required because it assigned in case of
errors. It will be cleared by compiler anyways.

Reveiwed-by: Mark Fasheh <mfasheh@suse.de>
Signed-off-by: Goldwyn Rodrigues <rgoldwyn@suse.com>
---
 fs/ocfs2/stack_user.c | 37 ++++++++++++++++++-------------------
 1 file changed, 18 insertions(+), 19 deletions(-)

diff --git a/fs/ocfs2/stack_user.c b/fs/ocfs2/stack_user.c
index 4111855..0afb86d 100644
--- a/fs/ocfs2/stack_user.c
+++ b/fs/ocfs2/stack_user.c
@@ -200,15 +200,10 @@ static struct ocfs2_live_connection *ocfs2_connection_find(const char *name)
  * mount path.  Since the VFS prevents multiple calls to
  * fill_super(), we can't get dupes here.
  */
-static int ocfs2_live_connection_new(struct ocfs2_cluster_connection *conn,
-				     struct ocfs2_live_connection **c_ret)
+static int ocfs2_live_connection_attach(struct ocfs2_cluster_connection *conn,
+				     struct ocfs2_live_connection *c)
 {
 	int rc = 0;
-	struct ocfs2_live_connection *c;
-
-	c = kzalloc(sizeof(struct ocfs2_live_connection), GFP_KERNEL);
-	if (!c)
-		return -ENOMEM;
 
 	mutex_lock(&ocfs2_control_lock);
 	c->oc_conn = conn;
@@ -222,12 +217,6 @@ static int ocfs2_live_connection_new(struct ocfs2_cluster_connection *conn,
 	}
 
 	mutex_unlock(&ocfs2_control_lock);
-
-	if (!rc)
-		*c_ret = c;
-	else
-		kfree(c);
-
 	return rc;
 }
 
@@ -840,12 +829,18 @@ const struct dlm_lockspace_ops ocfs2_ls_ops = {
 static int user_cluster_connect(struct ocfs2_cluster_connection *conn)
 {
 	dlm_lockspace_t *fsdlm;
-	struct ocfs2_live_connection *uninitialized_var(control);
-	int rc = 0;
+	struct ocfs2_live_connection *lc;
+	int rc;
 
 	BUG_ON(conn == NULL);
 
-	rc = ocfs2_live_connection_new(conn, &control);
+	lc = kzalloc(sizeof(struct ocfs2_live_connection), GFP_KERNEL);
+	if (!lc) {
+		rc = -ENOMEM;
+		goto out;
+	}
+
+	rc = ocfs2_live_connection_attach(conn, lc);
 	if (rc)
 		goto out;
 
@@ -861,20 +856,24 @@ static int user_cluster_connect(struct ocfs2_cluster_connection *conn)
 		       conn->cc_version.pv_major, conn->cc_version.pv_minor,
 		       running_proto.pv_major, running_proto.pv_minor);
 		rc = -EPROTO;
-		ocfs2_live_connection_drop(control);
+		ocfs2_live_connection_drop(lc);
+		lc = NULL;
 		goto out;
 	}
 
 	rc = dlm_new_lockspace(conn->cc_name, NULL, DLM_LSFL_FS, DLM_LVB_LEN,
 			       NULL, NULL, NULL, &fsdlm);
 	if (rc) {
-		ocfs2_live_connection_drop(control);
+		ocfs2_live_connection_drop(lc);
+		lc = NULL;
 		goto out;
 	}
 
-	conn->cc_private = control;
+	conn->cc_private = lc;
 	conn->cc_lockspace = fsdlm;
 out:
+	if (rc && lc)
+		kfree(lc);
 	return rc;
 }
 
-- 
1.8.4


-- 
Goldwyn