staging: lustre: grab the cld->cld_lock mutex unconditionally

PATCH New Cihangir Akturk 3 participants 7 messages Aug 1, 2017 – Aug 14, 2017 Staging

Share & tools

Cihangir Akturk

author

2017-08-01 23:12 UTC #0 lore raw

PATCH 1/1 staging: lustre: grab the cld->cld_lock mutex unconditionally

Instead of using the locked variable as a helper to determine the state of the mutex cld->cld_lock, expand the scope of the recover_cld variable and assign to the cld->cld_recover variable depending on whether the value of the recover_cld variable is valid or not.

As a bonus, code size is slightly reduced.

before:

text    data     bss     dec     hex filename
26188    2256    4208   32652    7f8c drivers/staging/lustre/lustre/mgc/mgc_request.o

after:

text    data     bss     dec     hex filename
26140    2256    4208   32604    7f5c drivers/staging/lustre/lustre/mgc/mgc_request.o

Additionally silences the following warning reported by coccinelle:

drivers/staging/lustre/lustre/mgc/mgc_request.c:359:2-12: second lock on line 365

Signed-off-by Cihangir Akturk <[email protected]>

---
 drivers/staging/lustre/lustre/mgc/mgc_request.c | 13 ++++---------
 1 file changed, 4 insertions(+), 9 deletions(-)

drivers/staging/lustre/lustre/mgc/mgc_request.c +4 -9

index eee0b66..6718474 100644
@@ -288,7 +288,7 @@ config_log_add(struct obd_device *obd, char *logname,
 	struct config_llog_data *cld;
 	struct config_llog_data *sptlrpc_cld;
 	struct config_llog_data *params_cld;
-	bool locked = false;
+	struct config_llog_data *recover_cld = ERR_PTR(-EINVAL);
 	char			seclogname[32];
 	char			*ptr;
 	int			rc;

@@ -338,8 +338,6 @@ config_log_add(struct obd_device *obd, char *logname,
 
 	LASSERT(lsi->lsi_lmd);
 	if (!(lsi->lsi_lmd->lmd_flags & LMD_FLG_NOIR)) {
-		struct config_llog_data *recover_cld;
-
 		ptr = strrchr(seclogname, '-');
 		if (ptr) {
 			*ptr = 0;

@@ -355,14 +353,11 @@ config_log_add(struct obd_device *obd, char *logname,
 			rc = PTR_ERR(recover_cld);
 			goto out_cld;
 		}
-
-		mutex_lock(&cld->cld_lock);
-		locked = true;
-		cld->cld_recover = recover_cld;
 	}
 
-	if (!locked)
-		mutex_lock(&cld->cld_lock);
+	mutex_lock(&cld->cld_lock);
+	if (!IS_ERR(recover_cld))
+		cld->cld_recover = recover_cld;
 	cld->cld_params = params_cld;
 	cld->cld_sptlrpc = sptlrpc_cld;
 	mutex_unlock(&cld->cld_lock);

Signature · 1 line

2.7.4

Dan Carpenter

2017-08-02 08:52 UTC #1 lore raw

PATCH 1/1 staging: lustre: grab the cld->cld_lock mutex unconditionally

I guess that does look a bit simpler.

Reviewed-by Dan Carpenter <[email protected]>

regards,
dan carpenter

James Simmons

2017-08-03 16:52 UTC #2 lore raw

PATCH 1/1 staging: lustre: grab the cld->cld_lock mutex unconditionally

In reply to Cihangir Akturk

Quoted 31 lines

Instead of using the locked variable as a helper to determine the state of the mutex cld->cld_lock, expand the scope of the recover_cld variable and assign to the cld->cld_recover variable depending on whether the value of the recover_cld variable is valid or not.
As a bonus, code size is slightly reduced.
before:
text    data     bss     dec     hex filename
26188    2256    4208   32652    7f8c drivers/staging/lustre/lustre/mgc/mgc_request.o
after:
text    data     bss     dec     hex filename
26140    2256    4208   32604    7f5c drivers/staging/lustre/lustre/mgc/mgc_request.o
Additionally silences the following warning reported by coccinelle:
drivers/staging/lustre/lustre/mgc/mgc_request.c:359:2-12: second lock on line 365
Signed-off-by Cihangir Akturk <[email protected]>
---
 drivers/staging/lustre/lustre/mgc/mgc_request.c | 13 ++++---------
 1 file changed, 4 insertions(+), 9 deletions(-)
drivers/staging/lustre/lustre/mgc/mgc_request.c +0 -0
index eee0b66..6718474 100644

drivers/staging/lustre/lustre/mgc/mgc_request.c config_log_add(struct obd_device *obd, char *logname, · line 294

Quoted 8 lines

drivers/staging/lustre/lustre/mgc/mgc_request.c +1 -1

@@ -288,7 +288,7 @@ config_log_add(struct obd_device *obd, char *logname,
 	struct config_llog_data *cld;
 	struct config_llog_data *sptlrpc_cld;
 	struct config_llog_data *params_cld;
-	bool locked = false;
+	struct config_llog_data *recover_cld = ERR_PTR(-EINVAL);
 	char			seclogname[32];
 	char			*ptr;
 	int			rc;

Why not just set it to NULL?

Quoted 13 lines

drivers/staging/lustre/lustre/mgc/mgc_request.c +0 -2

@@ -338,8 +338,6 @@ config_log_add(struct obd_device *obd, char *logname,
 
 	LASSERT(lsi->lsi_lmd);
 	if (!(lsi->lsi_lmd->lmd_flags & LMD_FLG_NOIR)) {
-		struct config_llog_data *recover_cld;
-
 		ptr = strrchr(seclogname, '-');
 		if (ptr) {
 			*ptr = 0;

@@ -355,14 +353,11 @@ config_log_add(struct obd_device *obd, char *logname,
 			rc = PTR_ERR(recover_cld);
 			goto out_cld;
 		}

drivers/staging/lustre/lustre/mgc/mgc_request.c config_log_add(struct obd_device *obd, char *logname, · line 359

Quoted 10 lines

drivers/staging/lustre/lustre/mgc/mgc_request.c +2 -6

-
-		mutex_lock(&cld->cld_lock);
-		locked = true;
-		cld->cld_recover = recover_cld;
 	}
 
-	if (!locked)
-		mutex_lock(&cld->cld_lock);
+	mutex_lock(&cld->cld_lock);
+	if (!IS_ERR(recover_cld))

Don't need this test if by default recover_cld = NULL.

Quoted 12 lines

drivers/staging/lustre/lustre/mgc/mgc_request.c +1 -0

+		cld->cld_recover = recover_cld;
 	cld->cld_params = params_cld;
 	cld->cld_sptlrpc = sptlrpc_cld;
 	mutex_unlock(&cld->cld_lock);

Signature · 6 lines

2.7.4

_______________________________________________
devel mailing list
devel at linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

James Simmons

2017-08-03 17:06 UTC #3 lore raw

PATCH 1/1 staging: lustre: grab the cld->cld_lock mutex unconditionally

Quoted 68 lines

Quoted 31 lines
Instead of using the locked variable as a helper to determine the state of the mutex cld->cld_lock, expand the scope of the recover_cld variable and assign to the cld->cld_recover variable depending on whether the value of the recover_cld variable is valid or not.
As a bonus, code size is slightly reduced.
before:
text    data     bss     dec     hex filename
26188    2256    4208   32652    7f8c drivers/staging/lustre/lustre/mgc/mgc_request.o
after:
text    data     bss     dec     hex filename
26140    2256    4208   32604    7f5c drivers/staging/lustre/lustre/mgc/mgc_request.o
Additionally silences the following warning reported by coccinelle:
drivers/staging/lustre/lustre/mgc/mgc_request.c:359:2-12: second lock on line 365
Signed-off-by Cihangir Akturk <[email protected]>
---
 drivers/staging/lustre/lustre/mgc/mgc_request.c | 13 ++++---------
 1 file changed, 4 insertions(+), 9 deletions(-)
drivers/staging/lustre/lustre/mgc/mgc_request.c +0 -0
index eee0b66..6718474 100644
drivers/staging/lustre/lustre/mgc/mgc_request.c config_log_add(struct obd_device *obd, char *logname, · line 294
Quoted 8 lines
drivers/staging/lustre/lustre/mgc/mgc_request.c +1 -1
@@ -288,7 +288,7 @@ config_log_add(struct obd_device *obd, char *logname,
 	struct config_llog_data *cld;
 	struct config_llog_data *sptlrpc_cld;
 	struct config_llog_data *params_cld;
-	bool locked = false;
+	struct config_llog_data *recover_cld = ERR_PTR(-EINVAL);
 	char			seclogname[32];
 	char			*ptr;
 	int			rc;
Why not just set it to NULL?
Quoted 13 lines
drivers/staging/lustre/lustre/mgc/mgc_request.c +0 -2
@@ -338,8 +338,6 @@ config_log_add(struct obd_device *obd, char *logname,
 
 	LASSERT(lsi->lsi_lmd);
 	if (!(lsi->lsi_lmd->lmd_flags & LMD_FLG_NOIR)) {
-		struct config_llog_data *recover_cld;
-
 		ptr = strrchr(seclogname, '-');
 		if (ptr) {
 			*ptr = 0;
@@ -355,14 +353,11 @@ config_log_add(struct obd_device *obd, char *logname,
 			rc = PTR_ERR(recover_cld);
 			goto out_cld;
 		}
drivers/staging/lustre/lustre/mgc/mgc_request.c config_log_add(struct obd_device *obd, char *logname, · line 359
Quoted 10 lines
drivers/staging/lustre/lustre/mgc/mgc_request.c +2 -6
-
-		mutex_lock(&cld->cld_lock);
-		locked = true;
-		cld->cld_recover = recover_cld;
 	}
 
-	if (!locked)
-		mutex_lock(&cld->cld_lock);
+	mutex_lock(&cld->cld_lock);
+	if (!IS_ERR(recover_cld))
Don't need this test if by default recover_cld = NULL.

Quoted 4 lines

drivers/staging/lustre/lustre/mgc/mgc_request.c config_log_add(struct obd_device *obd, char *logname, · line 363
Quoted 4 lines
drivers/staging/lustre/lustre/mgc/mgc_request.c +1 -0
+		cld->cld_recover = recover_cld;
 	cld->cld_params = params_cld;
 	cld->cld_sptlrpc = sptlrpc_cld;
 	mutex_unlock(&cld->cld_lock);

I forgot to mention another reason not to merge this patch. It could lead to future breakage by making people think it is okay to set the cld_XXXX items to ERR_PTR at the start of this function. In the cld_recover case it is last item process but if it wasn't then config_log_put(), which is called in the error path, does not test if it is a err ptr and would blow up. I have a patch that fixes this which I haven't pushed yet. That fix test if cld_XXX is NULL and returns right away.

Cihangir Akturk

author

2017-08-03 17:31 UTC #4 lore raw

PATCH 1/1 staging: lustre: grab the cld->cld_lock mutex unconditionally

In reply to James Simmons

Quoted 26 lines · James Simmons

Quoted 26 lines
Instead of using the locked variable as a helper to determine the state of the mutex cld->cld_lock, expand the scope of the recover_cld variable and assign to the cld->cld_recover variable depending on whether the value of the recover_cld variable is valid or not.
As a bonus, code size is slightly reduced.
before:
text    data     bss     dec     hex filename
26188    2256    4208   32652    7f8c drivers/staging/lustre/lustre/mgc/mgc_request.o
after:
text    data     bss     dec     hex filename
26140    2256    4208   32604    7f5c drivers/staging/lustre/lustre/mgc/mgc_request.o
Additionally silences the following warning reported by coccinelle:
drivers/staging/lustre/lustre/mgc/mgc_request.c:359:2-12: second lock on line 365
Signed-off-by Cihangir Akturk <[email protected]>
---
 drivers/staging/lustre/lustre/mgc/mgc_request.c | 13 ++++---------
 1 file changed, 4 insertions(+), 9 deletions(-)

Quoted 15 lines

drivers/staging/lustre/lustre/mgc/mgc_request.c config_log_add(struct obd_device *obd, char *logname, · line 294
Quoted 13 lines
drivers/staging/lustre/lustre/mgc/mgc_request.c +1 -1
index eee0b66..6718474 100644
@@ -288,7 +288,7 @@ config_log_add(struct obd_device *obd, char *logname,
 	struct config_llog_data *cld;
 	struct config_llog_data *sptlrpc_cld;
 	struct config_llog_data *params_cld;
-	bool locked = false;
+	struct config_llog_data *recover_cld = ERR_PTR(-EINVAL);
 	char			seclogname[32];
 	char			*ptr;
 	int			rc;
Why not just set it to NULL?

Hi,

Because config_recover_log_add() function does not return NULL on error. And I don't think IS_ERR on NULL pointer returns true.

Thanks.

Quoted 38 lines

Quoted 13 lines

drivers/staging/lustre/lustre/mgc/mgc_request.c +0 -2

@@ -338,8 +338,6 @@ config_log_add(struct obd_device *obd, char *logname,
 
 	LASSERT(lsi->lsi_lmd);
 	if (!(lsi->lsi_lmd->lmd_flags & LMD_FLG_NOIR)) {
-		struct config_llog_data *recover_cld;
-
 		ptr = strrchr(seclogname, '-');
 		if (ptr) {
 			*ptr = 0;

@@ -355,14 +353,11 @@ config_log_add(struct obd_device *obd, char *logname,
 			rc = PTR_ERR(recover_cld);
 			goto out_cld;
 		}

drivers/staging/lustre/lustre/mgc/mgc_request.c config_log_add(struct obd_device *obd, char *logname, · line 359

Quoted 10 lines

drivers/staging/lustre/lustre/mgc/mgc_request.c +2 -6

-
-		mutex_lock(&cld->cld_lock);
-		locked = true;
-		cld->cld_recover = recover_cld;
 	}
 
-	if (!locked)
-		mutex_lock(&cld->cld_lock);
+	mutex_lock(&cld->cld_lock);
+	if (!IS_ERR(recover_cld))

Don't need this test if by default recover_cld = NULL.

Quoted 12 lines

drivers/staging/lustre/lustre/mgc/mgc_request.c +1 -0

+		cld->cld_recover = recover_cld;
 	cld->cld_params = params_cld;
 	cld->cld_sptlrpc = sptlrpc_cld;
 	mutex_unlock(&cld->cld_lock);

Signature · 6 lines

2.7.4

_______________________________________________
devel mailing list
devel at linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Cihangir Akturk

author

2017-08-04 07:11 UTC #5 lore raw

PATCH 1/1 staging: lustre: grab the cld->cld_lock mutex unconditionally

Quoted 43 lines · Cihangir Akturk

Quoted 26 lines · James Simmons
Quoted 26 lines
Instead of using the locked variable as a helper to determine the state of the mutex cld->cld_lock, expand the scope of the recover_cld variable and assign to the cld->cld_recover variable depending on whether the value of the recover_cld variable is valid or not.
As a bonus, code size is slightly reduced.
before:
text    data     bss     dec     hex filename
26188    2256    4208   32652    7f8c drivers/staging/lustre/lustre/mgc/mgc_request.o
after:
text    data     bss     dec     hex filename
26140    2256    4208   32604    7f5c drivers/staging/lustre/lustre/mgc/mgc_request.o
Additionally silences the following warning reported by coccinelle:
drivers/staging/lustre/lustre/mgc/mgc_request.c:359:2-12: second lock on line 365
Signed-off-by Cihangir Akturk <[email protected]>
---
 drivers/staging/lustre/lustre/mgc/mgc_request.c | 13 ++++---------
 1 file changed, 4 insertions(+), 9 deletions(-)
Quoted 15 lines
drivers/staging/lustre/lustre/mgc/mgc_request.c config_log_add(struct obd_device *obd, char *logname, · line 294
Quoted 13 lines
drivers/staging/lustre/lustre/mgc/mgc_request.c +1 -1
index eee0b66..6718474 100644
@@ -288,7 +288,7 @@ config_log_add(struct obd_device *obd, char *logname,
 	struct config_llog_data *cld;
 	struct config_llog_data *sptlrpc_cld;
 	struct config_llog_data *params_cld;
-	bool locked = false;
+	struct config_llog_data *recover_cld = ERR_PTR(-EINVAL);
 	char			seclogname[32];
 	char			*ptr;
 	int			rc;
Why not just set it to NULL?

Quoted 4 lines

Hi,
Because config_recover_log_add() function does not return NULL on error. And I don't think IS_ERR on NULL pointer returns true.

Sorry, I've just realized I was wrong. Setting it to null makes perfect sense.

Thanks.

Quoted 41 lines

Thanks.

Quoted 38 lines

Quoted 13 lines

drivers/staging/lustre/lustre/mgc/mgc_request.c +0 -2

@@ -338,8 +338,6 @@ config_log_add(struct obd_device *obd, char *logname,
 
 	LASSERT(lsi->lsi_lmd);
 	if (!(lsi->lsi_lmd->lmd_flags & LMD_FLG_NOIR)) {
-		struct config_llog_data *recover_cld;
-
 		ptr = strrchr(seclogname, '-');
 		if (ptr) {
 			*ptr = 0;

@@ -355,14 +353,11 @@ config_log_add(struct obd_device *obd, char *logname,
 			rc = PTR_ERR(recover_cld);
 			goto out_cld;
 		}

drivers/staging/lustre/lustre/mgc/mgc_request.c config_log_add(struct obd_device *obd, char *logname, · line 359

Quoted 10 lines

drivers/staging/lustre/lustre/mgc/mgc_request.c +2 -6

-
-		mutex_lock(&cld->cld_lock);
-		locked = true;
-		cld->cld_recover = recover_cld;
 	}
 
-	if (!locked)
-		mutex_lock(&cld->cld_lock);
+	mutex_lock(&cld->cld_lock);
+	if (!IS_ERR(recover_cld))

Don't need this test if by default recover_cld = NULL.

Quoted 12 lines

drivers/staging/lustre/lustre/mgc/mgc_request.c +1 -0

+		cld->cld_recover = recover_cld;
 	cld->cld_params = params_cld;
 	cld->cld_sptlrpc = sptlrpc_cld;
 	mutex_unlock(&cld->cld_lock);

Signature · 6 lines

2.7.4

_______________________________________________
devel mailing list
devel at linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

James Simmons

2017-08-14 15:22 UTC #6 lore raw

PATCH 1/1 staging: lustre: grab the cld->cld_lock mutex unconditionally

Quoted 45 lines · Cihangir Akturk

Quoted 43 lines · James Simmons
Quoted 41 lines
Quoted 31 lines
Instead of using the locked variable as a helper to determine the state of the mutex cld->cld_lock, expand the scope of the recover_cld variable and assign to the cld->cld_recover variable depending on whether the value of the recover_cld variable is valid or not.
As a bonus, code size is slightly reduced.
before:
text    data     bss     dec     hex filename
26188    2256    4208   32652    7f8c drivers/staging/lustre/lustre/mgc/mgc_request.o
after:
text    data     bss     dec     hex filename
26140    2256    4208   32604    7f5c drivers/staging/lustre/lustre/mgc/mgc_request.o
Additionally silences the following warning reported by coccinelle:
drivers/staging/lustre/lustre/mgc/mgc_request.c:359:2-12: second lock on line 365
Signed-off-by Cihangir Akturk <[email protected]>
---
 drivers/staging/lustre/lustre/mgc/mgc_request.c | 13 ++++---------
 1 file changed, 4 insertions(+), 9 deletions(-)
drivers/staging/lustre/lustre/mgc/mgc_request.c +0 -0
index eee0b66..6718474 100644
drivers/staging/lustre/lustre/mgc/mgc_request.c config_log_add(struct obd_device *obd, char *logname, · line 294
Quoted 8 lines
drivers/staging/lustre/lustre/mgc/mgc_request.c +1 -1
@@ -288,7 +288,7 @@ config_log_add(struct obd_device *obd, char *logname,
 	struct config_llog_data *cld;
 	struct config_llog_data *sptlrpc_cld;
 	struct config_llog_data *params_cld;
-	bool locked = false;
+	struct config_llog_data *recover_cld = ERR_PTR(-EINVAL);
 	char			seclogname[32];
 	char			*ptr;
 	int			rc;
Why not just set it to NULL?

Quoted 7 lines

Quoted 4 lines
Hi,
Because config_recover_log_add() function does not return NULL on error. And I don't think IS_ERR on NULL pointer returns true.
Sorry, I've just realized I was wrong. Setting it to null makes perfect sense.

Setting it to NULL in this case is correct but you did point out a bug that exist in our newer code. Currently I have a patch that fixes config_log_put() so it return if its NULL but is doesn't check if its a ERR_PTR. I updated a patch I have with this fix so thank you for pointing it out.

Quoted 42 lines

Quoted 41 lines

Thanks.

Quoted 38 lines

Quoted 13 lines

drivers/staging/lustre/lustre/mgc/mgc_request.c +0 -2

@@ -338,8 +338,6 @@ config_log_add(struct obd_device *obd, char *logname,
 
 	LASSERT(lsi->lsi_lmd);
 	if (!(lsi->lsi_lmd->lmd_flags & LMD_FLG_NOIR)) {
-		struct config_llog_data *recover_cld;
-
 		ptr = strrchr(seclogname, '-');
 		if (ptr) {
 			*ptr = 0;

@@ -355,14 +353,11 @@ config_log_add(struct obd_device *obd, char *logname,
 			rc = PTR_ERR(recover_cld);
 			goto out_cld;
 		}

drivers/staging/lustre/lustre/mgc/mgc_request.c config_log_add(struct obd_device *obd, char *logname, · line 359

Quoted 10 lines

drivers/staging/lustre/lustre/mgc/mgc_request.c +2 -6

-
-		mutex_lock(&cld->cld_lock);
-		locked = true;
-		cld->cld_recover = recover_cld;
 	}
 
-	if (!locked)
-		mutex_lock(&cld->cld_lock);
+	mutex_lock(&cld->cld_lock);
+	if (!IS_ERR(recover_cld))

Don't need this test if by default recover_cld = NULL.

Quoted 12 lines

drivers/staging/lustre/lustre/mgc/mgc_request.c +1 -0

+		cld->cld_recover = recover_cld;
 	cld->cld_params = params_cld;
 	cld->cld_sptlrpc = sptlrpc_cld;
 	mutex_unlock(&cld->cld_lock);

Signature · 6 lines

2.7.4

_______________________________________________
devel mailing list
devel at linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel