Skip to contentSkip to conversation

staging: lustre: check result of register_shrinker

PATCH New Dilger 3 participants 9 messages Dec 3, 2017 – Dec 6, 2017 5 messages not in the archive

lore
Share & tools
lore

A message that is not in the archive (sent off-list or not yet received): [email protected]

Dilger

#1 lore raw

PATCH 1/1 staging: lustre: check result of register_shrinker

Quoted 4 lines · Aliaksei Karaliou

Lustre code lacks checking the result of register_shrinker() in several places. register_shrinker() was tagged __must_check recently so that sparse has started reporting it.

Thank you for your patch. Some comments below.

Quoted 13 lines
Signed-off-by Aliaksei Karaliou <[email protected]>

---

drivers/staging/lustre/lustre/ldlm/ldlm_pool.c     |  7 +++++--
drivers/staging/lustre/lustre/obdclass/lu_object.c |  5 +++--
drivers/staging/lustre/lustre/osc/osc_request.c    |  4 +++-
drivers/staging/lustre/lustre/ptlrpc/sec_bulk.c    | 13 ++++++++-----
4 files changed, 19 insertions(+), 10 deletions(-)
drivers/staging/lustre/lustre/ldlm/ldlm_pool.c +0 -0
index da65d00a7811..7795ececa6d3 100644

drivers/staging/lustre/lustre/ldlm/ldlm_pool.c int ldlm_pools_init(void) · line 1093

Quoted 10 lines
drivers/staging/lustre/lustre/ldlm/ldlm_pool.c +5 -2
@@ -1086,8 +1086,11 @@ int ldlm_pools_init(void)
 	int rc;
 
 	rc = ldlm_pools_thread_start();
-	if (rc == 0)
-		register_shrinker(&ldlm_pools_cli_shrinker);
+	if (rc == 0) {
+		rc = register_shrinker(&ldlm_pools_cli_shrinker);
+		if (rc)
+			ldlm_pools_thread_stop();
+	}

Rather than nesting conditionals like this, kernel style prefers to use "goto label" to do the cleanup in one place at the end of the function. That keeps the indentation level reasonable, and avoids making the error handling increasingly complex if more conditions are added in the future. The preferred way to handle this would be:

	rc = ldlm_pools_thread_start();
	if (rc)
		goto out;

	rc = register_shrinker(&ldlm_pools_cli_shrinker);
	if (rc)
		goto out_pools;

	return 0;

out_pools:

	ldlm_pools_thread_stop();
out:
	goto out;
}

Then, if a new error condition is added, it just means an "out_shrinker:" label is added and calling unregister_shrinker() at that point.

Quoted 15 lines
drivers/staging/lustre/lustre/obdclass/lu_object.c +1 -1
index b938a3f9d50a..9e0256ca2079 100644
@@ -1951,7 +1951,7 @@ int lu_global_init(void)
 	 * inode, one for ea. Unfortunately setting this high value results in
 	 * lu_object/inode cache consuming all the memory.
 	 */
-	register_shrinker(&lu_site_shrinker);
+	result = register_shrinker(&lu_site_shrinker);
 
 	return result;

}

drivers/staging/lustre/lustre/obdclass/lu_object.c int lu_global_init(void) · line 1966

Quoted 7 lines
drivers/staging/lustre/lustre/obdclass/lu_object.c +2 -1
@@ -1961,7 +1961,8 @@ int lu_global_init(void)
 */
 void lu_global_fini(void)
 {
-	unregister_shrinker(&lu_site_shrinker);
+	if (lu_site_shrinker.nr_deferred)
+		unregister_shrinker(&lu_site_shrinker);
 	lu_context_key_degister(&lu_global_key);

Initially, I didn't think this was needed, but it seems that unregister_shrinker() is not safe to be called on a shrinker that was not initialized properly.

While the above check makes this callsite safe, and I'm OK with landing this part of the patch, it might be safer for the kernel as a whole if unregister_shrinker() was made safe against this internally? Something like:

void unregister_shrinker(struct shrinker *shrinker) {
+ if (!shrinker->nr_deferred)
+ return;
+

        down_write(&shrinker_rwsem);
        list_del(&shrinker->list);
        up_write(&shrinker_rwsem);
        kfree(shrinker->nr_deferred);

+ shrinker->nr_deferred = NULL;
}

That avoids the need for the caller to "know" about nr_deferred.

Quoted 33 lines
drivers/staging/lustre/lustre/obdclass/lu_object.c +0 -0
 	/*
drivers/staging/lustre/lustre/osc/osc_request.c +3 -1
index 53eda4c99142..45b1ebf33363 100644
@@ -2844,7 +2844,9 @@ static int __init osc_init(void)
 	if (rc)
 		goto out_kmem;
 
-	register_shrinker(&osc_cache_shrinker);
+	rc = register_shrinker(&osc_cache_shrinker);
+	if (rc)
+		goto out_type;
 
 	/* This is obviously too much memory, only prevent overflow here */
 	if (osc_reqpool_mem_max >= 1 << 12 || osc_reqpool_mem_max == 0) {
drivers/staging/lustre/lustre/ptlrpc/sec_bulk.c +2 -0
index 77a3721beaee..4eeff832fd4a 100644
@@ -396,6 +396,8 @@ static struct shrinker pools_shrinker = {
 
 int sptlrpc_enc_pool_init(void)
 {
+	int rc = -ENOMEM;
+
 	/*
 	 * maximum capacity is 1/8 of total physical memory.
 	 * is the 1/8 a good number?

@@ -429,12 +431,13 @@ int sptlrpc_enc_pool_init(void)
 	page_pools.epp_st_outofmem = 0;

drivers/staging/lustre/lustre/ptlrpc/sec_bulk.c int sptlrpc_enc_pool_init(void) · line 438

Quoted 10 lines
drivers/staging/lustre/lustre/ptlrpc/sec_bulk.c +5 -4
 	enc_pools_alloc();
-	if (!page_pools.epp_pools)
-		return -ENOMEM;
-
-	register_shrinker(&pools_shrinker);
+	if (page_pools.epp_pools) {
+		rc = register_shrinker(&pools_shrinker);
+		if (rc)
+			enc_pools_free();
+	}

Same comment here as above - please use labels to handle error cleanup.

Quoted 8 lines
drivers/staging/lustre/lustre/ptlrpc/sec_bulk.c +1 -1
-	return 0;
+	return rc;
 }
 
 void sptlrpc_enc_pool_fini(void)
Signature · 1 line
2.11.0

Cheers, Andreas
--
Andreas Dilger
Lustre Principal Architect
Intel Corporation

#2 lore raw

PATCH 1/1 staging: lustre: check result of register_shrinker

Quoted 12 lines · Andreas

The preferred way to handle this would be:

	rc = ldlm_pools_thread_start();
	if (rc)
		goto out;

	rc = register_shrinker(&ldlm_pools_cli_shrinker);
	if (rc)
		goto out_pools;

	return 0;
Quoted 5 lines

out_pools:

	ldlm_pools_thread_stop();
out:
	goto out;
}

I so badly hate "out" as a label name... But yours made me laugh.

regards,
dan carpenter

A message that is not in the archive (sent off-list or not yet received): [email protected]

#4 lore raw

PATCH 1/1 staging: lustre: check result of register_shrinker

Quoted 12 lines · ak

Thank you for your extensive comments.

I've also thought about adding more protection into unregister_shrinker(), but not sure how to properly organize the patch set, because there will be three patches:
* change in mm/vmscan that adds protection and sanitizer.
* fixed change for Lustre driver
* there also two explicit usages of shrinker->nr_deferred in drivers - good idea to fix too.
All patches have different lists of maintainers, and second and third depend on first one. And I don't like to send them separately.
So, I'm going to at least prepend this patch with mm/vmscan one.

Fix the style for the Lustre patch and resend. Then patch unregister_shrinker(). Then remove the checks.

The unregister_shrinker() changes seem like a good idea, but I haven't really looked at it. It might be more involved than it seems.

regards,
dan carpenter

A message that is not in the archive (sent off-list or not yet received): [email protected]

#6 lore raw

PATCH 1/1 staging: lustre: check result of register_shrinker

Quoted 36 lines · Aliaksei Karaliou
Quoted 13 lines · Dan Carpenter
Quoted 12 lines · ak

Thank you for your extensive comments.

I've also thought about adding more protection into unregister_shrinker(), but not sure how to properly organize the patch set, because there will be three patches:
* change in mm/vmscan that adds protection and sanitizer.
* fixed change for Lustre driver
* there also two explicit usages of shrinker->nr_deferred in drivers - good idea to fix too.
All patches have different lists of maintainers, and second and third depend on first one. And I don't like to send them separately.
So, I'm going to at least prepend this patch with mm/vmscan one.

Quoted 8 lines

Fix the style for the Lustre patch and resend. Then patch unregister_shrinker(). Then remove the checks.

The unregister_shrinker() changes seem like a good idea, but I haven't really looked at it. It might be more involved than it seems.

regards,
dan carpenter

Thanks for the comments too.
I'll send patch with accumulated fixes.

Quoted 4 lines
Quoted 4 lines
  }
drivers/staging/lustre/lustre/obdclass/lu_object.c +0 -1
index b938a3f9d50a..9e0256ca2079 100644
--- a/drivers/staging/lustre/lustre/obdclass/lu_object.c
Quoted 8 lines

drivers/staging/lustre/lustre/obdclass/lu_object.c int lu_global_init(void) · line 1954

Quoted 7 lines

+++ b/drivers/staging/lustre/lustre/obdclass/lu_object.c

drivers/staging/lustre/lustre/obdclass/lu_object.c +1 -1
@@ -1951,7 +1951,7 @@ int lu_global_init(void)
  	 * inode, one for ea. Unfortunately setting this high value results in
  	 * lu_object/inode cache consuming all the memory.
  	 */
-	register_shrinker(&lu_site_shrinker);
+	result = register_shrinker(&lu_site_shrinker);

There should be some error handling if the register fails.

Quoted 2 lines

Yeah, I think so, but it seems that it is out of scope of this patch. The whole negative branch in the mainline kernel looks broken (IMHO).

No, it's not. If you introduce new error paths, you're expected to make them clean up instead of leaking resources.

regards,
dan carpenter

A message that is not in the archive (sent off-list or not yet received): [email protected]

#8 lore raw

PATCH v2 1/2 staging: lustre: check result of register_shrinker

Quoted 44 lines · Aliaksei Karaliou

Lustre code lacks checking the result of register_shrinker() in several places. register_shrinker() was tagged __must_check recently so that sparse has started reporting it.

Signed-off-by Aliaksei Karaliou <[email protected]>
---
 drivers/staging/lustre/lustre/ldlm/ldlm_pool.c     | 12 +++++++++---
 drivers/staging/lustre/lustre/obdclass/lu_object.c |  5 +++--
 drivers/staging/lustre/lustre/osc/osc_request.c    |  4 +++-
 drivers/staging/lustre/lustre/ptlrpc/sec_bulk.c    |  8 +++++++-
 4 files changed, 22 insertions(+), 7 deletions(-)

v2: Style fixes, as suggested by Cheers, Andreas and Dan Carpenter.
Added one more patch to address resource cleanup, suggested by Dan Carpenter.

drivers/staging/lustre/lustre/ldlm/ldlm_pool.c +9 -3
index da65d00a7811..9fef2d52d6c2 100644
@@ -1086,10 +1086,16 @@ int ldlm_pools_init(void)
 	int rc;
 
 	rc = ldlm_pools_thread_start();
-	if (rc == 0)
-		register_shrinker(&ldlm_pools_cli_shrinker);
+	if (rc)
+		return rc;
 
-	return rc;
+	rc = register_shrinker(&ldlm_pools_cli_shrinker);
+	if (rc) {
+		ldlm_pools_thread_stop();
+		return rc;
+	}
+
+	return 0;
 }
 
 void ldlm_pools_fini(void)
drivers/staging/lustre/lustre/obdclass/lu_object.c +0 -0
index b938a3f9d50a..9e0256ca2079 100644

drivers/staging/lustre/lustre/obdclass/lu_object.c int lu_global_init(void) · line 1956

Quoted 7 lines
drivers/staging/lustre/lustre/obdclass/lu_object.c +1 -1
@@ -1951,7 +1951,7 @@ int lu_global_init(void)
 	 * inode, one for ea. Unfortunately setting this high value results in
 	 * lu_object/inode cache consuming all the memory.
 	 */
-	register_shrinker(&lu_site_shrinker);
+	result = register_shrinker(&lu_site_shrinker);
 
 	return result;

return register_shrinker()?

drivers/staging/lustre/lustre/obdclass/lu_object.c int lu_global_init(void)

Quoted 5 lines
drivers/staging/lustre/lustre/obdclass/lu_object.c +4 -1
-	register_shrinker(&pools_shrinker);
+	rc = register_shrinker(&pools_shrinker);
+	if (rc) {
+		enc_pools_free();
+		return rc;

Drop the return rc, and then just do:

drivers/staging/lustre/lustre/obdclass/lu_object.c

Quoted 3 lines
drivers/staging/lustre/lustre/obdclass/lu_object.c +1 -0
+	}
 
 	return 0;

	return rc;

there. Makes the patch smaller.

thanks,

greg k-h

A message that is not in the archive (sent off-list or not yet received): [email protected]

#10 lore raw

PATCH v2 1/2 staging: lustre: check result of register_shrinker

Quoted 51 lines · Aliaksei Karaliou
Quoted 41 lines · Greg KH
Quoted 40 lines · Aliaksei Karaliou

Lustre code lacks checking the result of register_shrinker() in several places. register_shrinker() was tagged __must_check recently so that sparse has started reporting it.

Signed-off-by Aliaksei Karaliou <[email protected]>
---
  drivers/staging/lustre/lustre/ldlm/ldlm_pool.c     | 12 +++++++++---
  drivers/staging/lustre/lustre/obdclass/lu_object.c |  5 +++--
  drivers/staging/lustre/lustre/osc/osc_request.c    |  4 +++-
  drivers/staging/lustre/lustre/ptlrpc/sec_bulk.c    |  8 +++++++-
  4 files changed, 22 insertions(+), 7 deletions(-)

v2: Style fixes, as suggested by Cheers, Andreas and Dan Carpenter.
Added one more patch to address resource cleanup, suggested by Dan Carpenter.

drivers/staging/lustre/lustre/ldlm/ldlm_pool.c +9 -3
index da65d00a7811..9fef2d52d6c2 100644
@@ -1086,10 +1086,16 @@ int ldlm_pools_init(void)
  	int rc;
  	rc = ldlm_pools_thread_start();
-	if (rc == 0)
-		register_shrinker(&ldlm_pools_cli_shrinker);
+	if (rc)
+		return rc;
-	return rc;
+	rc = register_shrinker(&ldlm_pools_cli_shrinker);
+	if (rc) {
+		ldlm_pools_thread_stop();
+		return rc;
+	}
+
+	return 0;
  }
  void ldlm_pools_fini(void)
drivers/staging/lustre/lustre/obdclass/lu_object.c +0 -0
index b938a3f9d50a..9e0256ca2079 100644
Quoted 8 lines

drivers/staging/lustre/lustre/obdclass/lu_object.c int lu_global_init(void) · line 1955

Quoted 7 lines
drivers/staging/lustre/lustre/obdclass/lu_object.c +1 -1
@@ -1951,7 +1951,7 @@ int lu_global_init(void)
  	 * inode, one for ea. Unfortunately setting this high value results in
  	 * lu_object/inode cache consuming all the memory.
  	 */
-	register_shrinker(&lu_site_shrinker);
+	result = register_shrinker(&lu_site_shrinker);
  	return result;

return register_shrinker()?

Quoted 4 lines

Yes, It looks easier, thank you for your comments.
But still what to do with the fact that this changed place actually leads to resources being not freed (what I actually fix in second patch) ? Should I just merge both commits together ?

Don't write a patch that you know is broken, and then fix it up in a follow-on patch. That's not good at all.

Just fix one call-site at a time, that should be easier, right?

greg k-h

#11 lore raw

PATCH 1/1 staging: lustre: check result of register_shrinker

In reply to a message not in the archive
Quoted 37 lines · Aliaksei Karaliou
Quoted 13 lines · Dan Carpenter
Quoted 12 lines · ak

Thank you for your extensive comments.

I've also thought about adding more protection into unregister_shrinker(), but not sure how to properly organize the patch set, because there will be three patches:
* change in mm/vmscan that adds protection and sanitizer.
* fixed change for Lustre driver
* there also two explicit usages of shrinker->nr_deferred in drivers - good idea to fix too.
All patches have different lists of maintainers, and second and third depend on first one. And I don't like to send them separately.
So, I'm going to at least prepend this patch with mm/vmscan one.

Quoted 8 lines

Fix the style for the Lustre patch and resend. Then patch unregister_shrinker(). Then remove the checks.

The unregister_shrinker() changes seem like a good idea, but I haven't really looked at it. It might be more involved than it seems.

regards,
dan carpenter

Thanks for the comments too.
I'll send patch with accumulated fixes.

Quoted 4 lines
Quoted 4 lines

}

drivers/staging/lustre/lustre/obdclass/lu_object.c +0 -1
index b938a3f9d50a..9e0256ca2079 100644
--- a/drivers/staging/lustre/lustre/obdclass/lu_object.c
Quoted 8 lines

drivers/staging/lustre/lustre/obdclass/lu_object.c int lu_global_init(void) · line 1954

Quoted 7 lines

+++ b/drivers/staging/lustre/lustre/obdclass/lu_object.c

drivers/staging/lustre/lustre/obdclass/lu_object.c +1 -1
@@ -1951,7 +1951,7 @@ int lu_global_init(void)
 	 * inode, one for ea. Unfortunately setting this high value results in
 	 * lu_object/inode cache consuming all the memory.
 	 */
-	register_shrinker(&lu_site_shrinker);
+	result = register_shrinker(&lu_site_shrinker);

There should be some error handling if the register fails.

Quoted 6 lines

Yeah, I think so, but it seems that it is out of scope of this patch.
The whole negative branch in the mainline kernel looks broken (IMHO).
In mainline Lustre's git there is a reworked version of upper function obdclass_init(), which at least calls lu_global_fini() before exiting module_init on further failure, but yeah, still lacks proper cleanup inside lu_global_initcall().
I'll add one more patch in a patch-set so that maintainers may decide what to do with that.

I was looking at the lack of error handling as well, but then I wondered if the module_init() call returns an error, is module_exit() still called, or does the module not load at all in the error case?

Cheers, Andreas
--
Andreas Dilger
Lustre Principal Architect
Intel Corporation

#12 lore raw

PATCH 1/1 staging: lustre: check result of register_shrinker

Quoted 38 lines · Andreas
Quoted 37 lines · Aliaksei Karaliou
Quoted 13 lines · Dan Carpenter
Quoted 12 lines · ak

Thank you for your extensive comments.

I've also thought about adding more protection into unregister_shrinker(), but not sure how to properly organize the patch set, because there will be three patches:
* change in mm/vmscan that adds protection and sanitizer.
* fixed change for Lustre driver
* there also two explicit usages of shrinker->nr_deferred in drivers - good idea to fix too.
All patches have different lists of maintainers, and second and third depend on first one. And I don't like to send them separately.
So, I'm going to at least prepend this patch with mm/vmscan one.

Quoted 8 lines

Fix the style for the Lustre patch and resend. Then patch unregister_shrinker(). Then remove the checks.

The unregister_shrinker() changes seem like a good idea, but I haven't really looked at it. It might be more involved than it seems.

regards,
dan carpenter

Thanks for the comments too.
I'll send patch with accumulated fixes.

Quoted 12 lines

drivers/staging/lustre/lustre/obdclass/lu_object.c int lu_global_init(void) · line 1954

Quoted 11 lines

}

drivers/staging/lustre/lustre/obdclass/lu_object.c +1 -1
index b938a3f9d50a..9e0256ca2079 100644
@@ -1951,7 +1951,7 @@ int lu_global_init(void)
 	 * inode, one for ea. Unfortunately setting this high value results in
 	 * lu_object/inode cache consuming all the memory.
 	 */
-	register_shrinker(&lu_site_shrinker);
+	result = register_shrinker(&lu_site_shrinker);

There should be some error handling if the register fails.

Quoted 9 lines
Quoted 6 lines

Yeah, I think so, but it seems that it is out of scope of this patch.
The whole negative branch in the mainline kernel looks broken (IMHO).
In mainline Lustre's git there is a reworked version of upper function obdclass_init(), which at least calls lu_global_fini() before exiting module_init on further failure, but yeah, still lacks proper cleanup inside lu_global_initcall().
I'll add one more patch in a patch-set so that maintainers may decide what to do with that.

I was looking at the lack of error handling as well, but then I wondered if the module_init() call returns an error, is module_exit() still called, or does the module not load at all in the error case?

module_init() is supposed to clean up after itself. module_exit() is only called if init succeeds.

regards,
dan carpenter

#13 lore raw

PATCH 1/1 staging: lustre: check result of register_shrinker

In reply to a message not in the archive
Quoted 19 lines · Aliaksei Karaliou

Lustre code lacks checking the result of register_shrinker() in several places. register_shrinker() was tagged __must_check recently so that sparse has started reporting it.

Signed-off-by Aliaksei Karaliou <[email protected]>
---
 drivers/staging/lustre/lustre/ldlm/ldlm_pool.c     |  7 +++++--
 drivers/staging/lustre/lustre/obdclass/lu_object.c |  5 +++--
 drivers/staging/lustre/lustre/osc/osc_request.c    |  4 +++-
 drivers/staging/lustre/lustre/ptlrpc/sec_bulk.c    | 13 ++++++++-----
 4 files changed, 19 insertions(+), 10 deletions(-)
drivers/staging/lustre/lustre/ldlm/ldlm_pool.c +0 -0
index da65d00a7811..7795ececa6d3 100644
@@ -1086,8 +1086,11 @@ int ldlm_pools_init(void)
 	int rc;

drivers/staging/lustre/lustre/ldlm/ldlm_pool.c int ldlm_pools_init(void) · line 1095

Quoted 10 lines
drivers/staging/lustre/lustre/ldlm/ldlm_pool.c +5 -2
 	rc = ldlm_pools_thread_start();
-	if (rc == 0)
-		register_shrinker(&ldlm_pools_cli_shrinker);
+	if (rc == 0) {
+		rc = register_shrinker(&ldlm_pools_cli_shrinker);
+		if (rc)
+			ldlm_pools_thread_stop();
+	}
 
 	return rc;

Originally, the code had two anti-patterns 1) Making the last check in the function different from the others, 2) Success handling instead of failure handling. Success handling leads to spaghetti code. Write it like this instead.

	rc = ldlm_pools_thread_start();
	if (rc)
		return rc;

	rc = register_shrinker(&ldlm_pools_cli_shrinker);
	if (rc) {
		ldlm_pools_thread_stop();
		return rc;
	}

	return 0;

drivers/staging/lustre/lustre/obdclass/lu_object.c int lu_global_init(void) · line 1954

Quoted 11 lines
drivers/staging/lustre/lustre/ldlm/ldlm_pool.c +0 -0
 }
drivers/staging/lustre/lustre/obdclass/lu_object.c +1 -1
index b938a3f9d50a..9e0256ca2079 100644
@@ -1951,7 +1951,7 @@ int lu_global_init(void)
 	 * inode, one for ea. Unfortunately setting this high value results in
 	 * lu_object/inode cache consuming all the memory.
 	 */
-	register_shrinker(&lu_site_shrinker);
+	result = register_shrinker(&lu_site_shrinker);

There should be some error handling if the register fails.

Quoted 47 lines
drivers/staging/lustre/lustre/obdclass/lu_object.c +2 -1
 	return result;
 }

@@ -1961,7 +1961,8 @@ int lu_global_init(void)
  */
 void lu_global_fini(void)
 {
-	unregister_shrinker(&lu_site_shrinker);
+	if (lu_site_shrinker.nr_deferred)
+		unregister_shrinker(&lu_site_shrinker);
 	lu_context_key_degister(&lu_global_key);
 
 	/*
drivers/staging/lustre/lustre/osc/osc_request.c +3 -1
index 53eda4c99142..45b1ebf33363 100644
@@ -2844,7 +2844,9 @@ static int __init osc_init(void)
 	if (rc)
 		goto out_kmem;
 
-	register_shrinker(&osc_cache_shrinker);
+	rc = register_shrinker(&osc_cache_shrinker);
+	if (rc)
+		goto out_type;
 
 	/* This is obviously too much memory, only prevent overflow here */
 	if (osc_reqpool_mem_max >= 1 << 12 || osc_reqpool_mem_max == 0) {
drivers/staging/lustre/lustre/ptlrpc/sec_bulk.c +2 -2
index 77a3721beaee..4eeff832fd4a 100644
@@ -396,6 +396,8 @@ static struct shrinker pools_shrinker = {
 
 int sptlrpc_enc_pool_init(void)
 {
+	int rc = -ENOMEM;
+
 	/*
 	 * maximum capacity is 1/8 of total physical memory.
 	 * is the 1/8 a good number?

@@ -429,12 +431,13 @@ int sptlrpc_enc_pool_init(void)
 	page_pools.epp_st_outofmem = 0;
 
 	enc_pools_alloc();
-	if (!page_pools.epp_pools)
-		return -ENOMEM;

drivers/staging/lustre/lustre/ptlrpc/sec_bulk.c int sptlrpc_enc_pool_init(void) · line 440

Quoted 10 lines
drivers/staging/lustre/lustre/ptlrpc/sec_bulk.c +6 -3
-
-	register_shrinker(&pools_shrinker);
+	if (page_pools.epp_pools) {
+		rc = register_shrinker(&pools_shrinker);
+		if (rc)
+			enc_pools_free();
+	}
 
-	return 0;
+	return rc;

This new code is *really* hard to read. The original was more readable. Do it like this:

	if (!page_pools.epp_pools)
		return -ENOMEM;

	rc = register_shrinker(&pools_shrinker);
	if (rc) {
		enc_pools_free();
		return rc;
	}

	return 0;

regards,
dan carpenter