| Message ID | 20131018144534.GA4583@shrek.lan (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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
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.
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
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; }
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(-)