[PATCH 1/2] Staging: zram: Turn lockdep off during zram_init_device()

[PATCH 1/2] Staging: zram: Turn lockdep off during zram_init_device()

[Jerome Marchand]

zram->init_lock can be hold over an allocation that may try to reclaim
memory in zram_init_device(). The same lock can later be taken from a
reclaim context in zram_make_request(), thus triggering a lockdep
warning. However, memory can not be reclaimed to an uninitialized zram
device. Therefore, this warning is a false positive.
To prevent the warning to occur, we turn lockdep off during while the
device is initialized.

Signed-off-by: Jerome Marchand <jmarchan@redhat.com>
Reported-by: Witold Baryluk <baryluk@smp.if.uj.edu.pl>
---
 zram_drv.c |   14 ++++++++++++++
 1 file changed, 14 insertions(+)

diff --git a/drivers/staging/zram/zram_drv.c b/drivers/staging/zram/zram_drv.c
index 09de99f..34f4b96 100644
--- a/drivers/staging/zram/zram_drv.c
+++ b/drivers/staging/zram/zram_drv.c
@@ -637,10 +637,22 @@ int zram_init_device(struct zram *zram)
 	int ret;
 	size_t num_pages;
 
+	/*
+	 * Here we hold init_lock over allocations that may try to reclaim
+	 * memory. The same lock is also taken from a reclaim context in
+	 * zram_make_request(). That end up in a lockdep warning.
+	 * Moreover the table can be quite big an it wouldn't be reasonable to
+	 * use kmalloc(GFP_NOFS,...) to allocate it.
+	 * Fortunately, memory can not be reclaimed to an uninitialized zram
+	 * device, so this warning is a false positive. Therefore, to prevent
+	 * the warning, we turn lockdep off here.
+	 */
+	lockdep_off();
 	down_write(&zram->init_lock);
 
 	if (zram->init_done) {
 		up_write(&zram->init_lock);
+		lockdep_on();
 		return 0;
 	}
 
@@ -682,6 +694,7 @@ int zram_init_device(struct zram *zram)
 
 	zram->init_done = 1;
 	up_write(&zram->init_lock);
+	lockdep_on();
 
 	pr_debug("Initialization done!\n");
 	return 0;
@@ -692,6 +705,7 @@ fail_no_table:
 fail:
 	__zram_reset_device(zram);
 	up_write(&zram->init_lock);
+	lockdep_on();
 	pr_err("Initialization failed: err=%d\n", ret);
 	return ret;
 }

[PATCH 2/2] Staging: zram: Add a missing GFP_KERNEL specifier in zram_init_device()

[Jerome Marchand]

The allocation of zram->compress_buffer is misssing a GFP_* specifier.
This is equivalent to GFP_NOWAIT but it is more likely a omission.
Since the allocation just above it uses GFP_KERNEL, there is no reason
to use GFP_NOWAIT here. Therefore, add GFP_KERNEL.

Signed-off-by: Jerome Marchand <jmarchan@redhat.com>
---
 zram_drv.c |    3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/staging/zram/zram_drv.c b/drivers/staging/zram/zram_drv.c
index 34f4b96..a263a1b 100644
--- a/drivers/staging/zram/zram_drv.c
+++ b/drivers/staging/zram/zram_drv.c
@@ -665,7 +665,8 @@ int zram_init_device(struct zram *zram)
 		goto fail_no_table;
 	}
 
-	zram->compress_buffer = (void *)__get_free_pages(__GFP_ZERO, 1);
+	zram->compress_buffer =
+		(void *)__get_free_pages(GFP_KERNEL | __GFP_ZERO, 1);
 	if (!zram->compress_buffer) {
 		pr_err("Error allocating compressor buffer space\n");
 		ret = -ENOMEM;

Re: [PATCH 1/2] Staging: zram: Turn lockdep off during zram_init_device()

[Peter Zijlstra]

On Wed, 2011-11-30 at 14:14 +0100, Jerome Marchand wrote:
> zram->init_lock can be hold over an allocation that may try to reclaim
> memory in zram_init_device(). The same lock can later be taken from a
> reclaim context in zram_make_request(), thus triggering a lockdep
> warning. However, memory can not be reclaimed to an uninitialized zram
> device. Therefore, this warning is a false positive.
> To prevent the warning to occur, we turn lockdep off during while the
> device is initialized.

fuck no! why do you even remotely consider this a sane thing to do?

There's tons of lockdep annotations, try one of those. If you have, your
changelog utterly fails to explain why non of those are suitable and how
your problem is special.

Re: [PATCH 1/2] Staging: zram: Turn lockdep off during zram_init_device()

[Witold Baryluk]

On 11-30 14:59, Peter Zijlstra wrote:
> On Wed, 2011-11-30 at 14:14 +0100, Jerome Marchand wrote:
> > zram->init_lock can be hold over an allocation that may try to reclaim
> > memory in zram_init_device(). The same lock can later be taken from a
> > reclaim context in zram_make_request(), thus triggering a lockdep
> > warning. However, memory can not be reclaimed to an uninitialized zram
> > device. Therefore, this warning is a false positive.
> > To prevent the warning to occur, we turn lockdep off during while the
> > device is initialized.
> 
> fuck no! why do you even remotely consider this a sane thing to do?
> 
> There's tons of lockdep annotations, try one of those. If you have, your
> changelog utterly fails to explain why non of those are suitable and how
> your problem is special.
> 

I must agree. Propsed patch is a hack, and actually can make more trouble
to other subsystems. It is better to live with warning than perform
temporary lockdep off/on.

-- 
Witold Baryluk