afr/notify: Fix notify argument for upcall

[rafikc30]

upcall notification expects the upcall data struct gf_upcall_cache_invalidation as the main argument. While AFR requires another argument to identify the child xlator who send the upcall. As of now, afr_notify was using the upcall structure as the xlator variable and was calculating the child xlator index value to a wrong value.

This patch fixes the child index calculation, as well as tries to have a uniform way to calculate it. This patch has also moved the handling of notification for GF_EVENT_TRANSLATOR_OP is the first one to process since this event doesn't have any meaningful child_xlator computation.

Change-Id: I7bf03aea53fdedec298869a82efd9b2c6b4e867c fixes: https://github.com/gluster/glusterfs/issues/3253 Signed-off-by: Mohammed Rafi KC [email protected]

[rafikc30]

/run regression

[gluster-ant]

CLANG-FORMAT FAILURE: Before merging the patch, this diff needs to be considered for passing clang-format

index 88a39cde5..f8197a917 100644
--- a/libglusterfs/src/defaults-tmpl.c
+++ b/libglusterfs/src/defaults-tmpl.c
@@ -183,7 +183,8 @@ default_notify(xlator_t *this, int32_t event, void *data, ...)
             xlator_list_t *parent = this->parents;
 
             if (!parent && this->ctx && this->ctx->root)
-                XLATOR_NOTIFY(ret, ((xlator_t*)(this->ctx->root)), event, data, this);
+                XLATOR_NOTIFY(ret, ((xlator_t *)(this->ctx->root)), event, data,
+                              this);
 
             while (parent) {
                 if (parent->xlator->init_succeeded)
diff --git a/xlators/cluster/afr/src/afr-common.c b/xlators/cluster/afr/src/afr-common.c
index 92e775e20..cd20e57ed 100644
--- a/xlators/cluster/afr/src/afr-common.c
+++ b/xlators/cluster/afr/src/afr-common.c
@@ -6228,22 +6228,22 @@ afr_handle_upcall_event(xlator_t *this, struct gf_upcall *upcall)
     }
 }
 
-xlator_t*
+xlator_t *
 afr_get_child_xlator_for_event(int32_t event, void *data, void *data2)
 {
-        xlator_t *child_xlator = NULL;
-        switch (event) {
-                case GF_EVENT_UPCALL:
-                case GF_EVENT_CHILD_PING:
-                        child_xlator = (xlator_t*)data2;
-                        break;
-                case GF_EVENT_TRANSLATOR_OP:
-                        child_xlator = NULL;
-                        break;
-                default:
-                        child_xlator = (xlator_t*)data;
-        }
-        return child_xlator;
+    xlator_t *child_xlator = NULL;
+    switch (event) {
+        case GF_EVENT_UPCALL:
+        case GF_EVENT_CHILD_PING:
+            child_xlator = (xlator_t *)data2;
+            break;
+        case GF_EVENT_TRANSLATOR_OP:
+            child_xlator = NULL;
+            break;
+        default:
+            child_xlator = (xlator_t *)data;
+    }
+    return child_xlator;
 }
 
 int32_t
@@ -6266,7 +6266,6 @@ afr_notify(xlator_t *this, int32_t event, void *data, void *data2)
     int64_t halo_max_latency_msec = 0;
     int64_t child_latency_msec = -1;
 
-
     priv = this->private;
 
     if (!priv)

[itisravi]

. While AFR requires another argument to identify the child xlator who send the upcall.

@rafikc30 Is this really the case? It looks like when the event is GF_EVENT_UPCALL, afr_notify(xlator_t *this, int32_t event, void *data, void *data2) calls afr_handle_upcall_event(this, data);without needing any info about the child xlator which sent the upcall, and data as correctly interpreted ad struct gf_upcall.

As of now, afr_notify was using the upcall structure as the xlator variable and was calculating the child xlator index value to a wrong value.

True, but this seems like a harmless no-op as the index is not used like I mentioned above for upcall event. But yes, it is good to skip the calculation.

[itisravi]

@rafikc30 Is this really the case? It looks like when the event is GF_EVENT_UPCALL, afr_notify(xlator_t *this, int32_t event, void *data, void *data2) calls afr_handle_upcall_event(this, data);without needing any info about the child xlator which sent the upcall, and data as correctly interpreted ad struct gf_upcall.

Ah, you are adding the need for the child xlator in https://github.com/gluster/glusterfs/pull/3255

[rafikc30]

@rafikc30 Is this really the case? It looks like when the event is GF_EVENT_UPCALL, afr_notify(xlator_t *this, int32_t event, void *data, void *data2) calls afr_handle_upcall_event(this, data);without needing any info about the child xlator which sent the upcall, and data as correctly interpreted ad struct gf_upcall.

Ah, you are adding the need for the child xlator in #3255

@itisravi Yes. I wanted to use the index to identify the Arbiter node.

[pranithk]

Code looks good to me. Is there a test that can identify the issue you found? If yes could you add it as part of one of the patches?

[gluster-ant]

CLANG-FORMAT FAILURE: Before merging the patch, this diff needs to be considered for passing clang-format

index 88a39cde5..f8197a917 100644
--- a/libglusterfs/src/defaults-tmpl.c
+++ b/libglusterfs/src/defaults-tmpl.c
@@ -183,7 +183,8 @@ default_notify(xlator_t *this, int32_t event, void *data, ...)
             xlator_list_t *parent = this->parents;
 
             if (!parent && this->ctx && this->ctx->root)
-                XLATOR_NOTIFY(ret, ((xlator_t*)(this->ctx->root)), event, data, this);
+                XLATOR_NOTIFY(ret, ((xlator_t *)(this->ctx->root)), event, data,
+                              this);
 
             while (parent) {
                 if (parent->xlator->init_succeeded)
diff --git a/xlators/cluster/afr/src/afr-common.c b/xlators/cluster/afr/src/afr-common.c
index cd6712e19..c24f85fca 100644
--- a/xlators/cluster/afr/src/afr-common.c
+++ b/xlators/cluster/afr/src/afr-common.c
@@ -6251,22 +6251,22 @@ afr_handle_upcall_event(xlator_t *this, struct gf_upcall *upcall, const int idx)
     }
 }
 
-static xlator_t*
+static xlator_t *
 afr_get_child_xlator_for_event(int32_t event, void *data, void *data2)
 {
-        xlator_t *child_xlator = NULL;
-        switch (event) {
-                case GF_EVENT_UPCALL:
-                case GF_EVENT_CHILD_PING:
-                        child_xlator = (xlator_t*)data2;
-                        break;
-                case GF_EVENT_TRANSLATOR_OP:
-                        child_xlator = NULL;
-                        break;
-                default:
-                        child_xlator = (xlator_t*)data;
-        }
-        return child_xlator;
+    xlator_t *child_xlator = NULL;
+    switch (event) {
+        case GF_EVENT_UPCALL:
+        case GF_EVENT_CHILD_PING:
+            child_xlator = (xlator_t *)data2;
+            break;
+        case GF_EVENT_TRANSLATOR_OP:
+            child_xlator = NULL;
+            break;
+        default:
+            child_xlator = (xlator_t *)data;
+    }
+    return child_xlator;
 }
 
 int32_t
@@ -6289,7 +6289,6 @@ afr_notify(xlator_t *this, int32_t event, void *data, void *data2)
     int64_t halo_max_latency_msec = 0;
     int64_t child_latency_msec = -1;
 
-
     priv = this->private;
 
     if (!priv)

[rafikc30]

@pranithk I have added a test case for https://github.com/gluster/glusterfs/issues/3252. I have used delay-gen to create a simple delay in processing an upcall from the arbiter. To support delay-gen for arbiter xlator, I have added a commit.

[rafikc30]

/run regression

[amarts]

looks important. Can this be rebased?

[rafikc30]

/run regression

[stale[bot]]

Thank you for your contributions. Noticed that this issue is not having any activity in last ~6 months! We are marking this issue as stale because it has not had recent activity. It will be closed in 2 weeks if no one responds with a comment here.

[stale[bot]]

Closing this issue as there was no update since my last update on issue. If this is an issue which is still valid, feel free to open it.