[PATCH 2/2] zram: permit sleeping while in pool zs_malloc()

[PATCH 2/2] zram: permit sleeping while in pool zs_malloc()

[Sergey Senozhatsky]

    zram: permit sleeping while in pool zs_malloc()
    
    zram pool is created with GFP_NOIO flag, which may trigger errors because nested allocation
    are able to sleep. set __GFP_WAIT pool flag in zram_init_device() to allow sleeping.
    
     BUG: sleeping function called from invalid context at mm/page_alloc.c:2603
     in_atomic(): 1, irqs_disabled(): 0, pid: 2555, name: mkfs.reiserfs
     2 locks held by mkfs.reiserfs/2555:
      #0:  (&zram->init_lock){+++++.}, at: [<ffffffffa0127d18>] zram_make_request+0x48/0x270 [zram]
      #1:  (&zram->lock){++++..}, at: [<ffffffffa012742b>] zram_bvec_rw+0x3b/0x510 [zram]
     Pid: 2555, comm: mkfs.reiserfs Tainted: G         C   3.7.0-rc3-dbg-01664-gf2d9543-dirty #1401
     Call Trace:
      [<ffffffff8107984a>] __might_sleep+0x15a/0x250
      [<ffffffff8111df9b>] __alloc_pages_nodemask+0x1bb/0x920
      [<ffffffffa00f0b93>] ? zs_malloc+0x63/0x480 [zsmalloc]
      [<ffffffff81320e2d>] ? do_raw_spin_unlock+0x5d/0xb0
      [<ffffffffa00f0cf5>] zs_malloc+0x1c5/0x480 [zsmalloc]
      [<ffffffffa0127574>] zram_bvec_rw+0x184/0x510 [zram]
      [<ffffffffa0127e85>] zram_make_request+0x1b5/0x270 [zram]
      [<ffffffff812ec0c2>] generic_make_request+0xc2/0x110
      [<ffffffff812ec17a>] submit_bio+0x6a/0x140
      [<ffffffff8119f27b>] submit_bh+0xfb/0x130
      [<ffffffff811a2710>] __block_write_full_page+0x220/0x3d0
      [<ffffffff810a7784>] ? __lock_is_held+0x54/0x80
      [<ffffffff8119ffb0>] ? end_buffer_async_read+0x210/0x210
      [<ffffffff811a7aa0>] ? blkdev_get_blocks+0xd0/0xd0
      [<ffffffff811a7aa0>] ? blkdev_get_blocks+0xd0/0xd0
      [<ffffffff8119ffb0>] ? end_buffer_async_read+0x210/0x210
      [<ffffffff811a298f>] block_write_full_page_endio+0xcf/0x100
      [<ffffffff8111f555>] ? clear_page_dirty_for_io+0x105/0x130
      [<ffffffff811a29d5>] block_write_full_page+0x15/0x20
      [<ffffffff811a7038>] blkdev_writepage+0x18/0x20
      [<ffffffff8111f3aa>] __writepage+0x1a/0x50
      [<ffffffff8111f8b0>] write_cache_pages+0x200/0x630
      [<ffffffff8111e883>] ? free_hot_cold_page+0x113/0x1a0
      [<ffffffff8111f390>] ? global_dirtyable_memory+0x40/0x40
      [<ffffffff8111fd2d>] generic_writepages+0x4d/0x70
      [<ffffffff81121071>] do_writepages+0x21/0x50
      [<ffffffff81116939>] __filemap_fdatawrite_range+0x59/0x60
      [<ffffffff81116a40>] filemap_write_and_wait_range+0x50/0x70
      [<ffffffff811a73a4>] blkdev_fsync+0x24/0x50
      [<ffffffff8119d5bd>] do_fsync+0x5d/0x90
      [<ffffffff8119d990>] sys_fsync+0x10/0x20
      [<ffffffff815dce06>] tracesys+0xd4/0xd9
    
    Signed-off-by: Sergey Senozhatsky <sergey.senozhatsky@gmail.com>

---

 drivers/staging/zram/zram_drv.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/staging/zram/zram_drv.c b/drivers/staging/zram/zram_drv.c
index d2e0a85..47f2e3a 100644
--- a/drivers/staging/zram/zram_drv.c
+++ b/drivers/staging/zram/zram_drv.c
@@ -576,7 +576,7 @@ int zram_init_device(struct zram *zram)
 	/* zram devices sort of resembles non-rotational disks */
 	queue_flag_set_unlocked(QUEUE_FLAG_NONROT, zram->disk->queue);
 
-	zram->mem_pool = zs_create_pool("zram", GFP_NOIO | __GFP_HIGHMEM);
+	zram->mem_pool = zs_create_pool("zram", GFP_NOIO | __GFP_WAIT | __GFP_HIGHMEM);
 	if (!zram->mem_pool) {
 		pr_err("Error creating memory pool\n");
 		ret = -ENOMEM;

Re: [PATCH 2/2] zram: permit sleeping while in pool zs_malloc()

[Greg Kroah-Hartman]

On Tue, Oct 30, 2012 at 12:03:19PM +0300, Sergey Senozhatsky wrote:
>     zram: permit sleeping while in pool zs_malloc()

2/2?  Huh?  Where is 1/2?

I have a raft of patches from you, all out of order, and full of
responses from Nitin, and so, I really have no idea what should and
should not be applied here.

So, I'm dropping them all.  Please work with Nitin to get a series of
patches that are acceptable and resend them with the proper numbering.

thanks,

greg k-h

Re: [PATCH 2/2] zram: permit sleeping while in pool zs_malloc()

[Sergey Senozhatsky]

On (10/30/12 11:04), Greg Kroah-Hartman wrote:
> On Tue, Oct 30, 2012 at 12:03:19PM +0300, Sergey Senozhatsky wrote:
> >     zram: permit sleeping while in pool zs_malloc()
> 
> 2/2?  Huh?  Where is 1/2?
> 
> I have a raft of patches from you, all out of order, and full of
> responses from Nitin, and so, I really have no idea what should and
> should not be applied here.
> 
> So, I'm dropping them all.  Please work with Nitin to get a series of
> patches that are acceptable and resend them with the proper numbering.
> 

those were two separate series.

the first one:

[PATCH 1/2] zram: factor-out zram_decompress_page() function
[PATCH 2/2] zram: handle mem suffixes in disk size zram_sysfs parameter

ACKed by Nitin.



the second one

[PATCH 2/2] zram: permit sleeping while in pool zs_malloc()
[PATCH 1/2] zram: forbid IO operations from within zram_init_device()  -- droppped by me.


I'll resend first two and next time will make sure not to spam your box. sorry.


	-ss

> thanks,
> 
> greg k-h
> 

Re: [PATCH 2/2] zram: permit sleeping while in pool zs_malloc()

[Greg Kroah-Hartman]

On Tue, Oct 30, 2012 at 09:49:11PM +0300, Sergey Senozhatsky wrote:
> On (10/30/12 11:04), Greg Kroah-Hartman wrote:
> > On Tue, Oct 30, 2012 at 12:03:19PM +0300, Sergey Senozhatsky wrote:
> > >     zram: permit sleeping while in pool zs_malloc()
> > 
> > 2/2?  Huh?  Where is 1/2?
> > 
> > I have a raft of patches from you, all out of order, and full of
> > responses from Nitin, and so, I really have no idea what should and
> > should not be applied here.
> > 
> > So, I'm dropping them all.  Please work with Nitin to get a series of
> > patches that are acceptable and resend them with the proper numbering.
> > 
> 
> those were two separate series.
> 
> the first one:
> 
> [PATCH 1/2] zram: factor-out zram_decompress_page() function
> [PATCH 2/2] zram: handle mem suffixes in disk size zram_sysfs parameter
> 
> ACKed by Nitin.
> 
> 
> 
> the second one
> 
> [PATCH 2/2] zram: permit sleeping while in pool zs_malloc()
> [PATCH 1/2] zram: forbid IO operations from within zram_init_device()  -- droppped by me.
> 
> 
> I'll resend first two and next time will make sure not to spam your box. sorry.

Sending me lots of patches is fine, I don't mind that at all.  It's what
I'm here for it seems :)

But when there is responses, and follow-up patches, and second versions
and lots of other stuff, it gets hard to follow, so I ask for them all
to be resent, like this.

But, what happened with your second series?  As you didn't resend them,
I'll just ignore them for now, please resend if you ever get them worked
out.

thanks,

greg k-h

Re: [PATCH 2/2] zram: permit sleeping while in pool zs_malloc()

[Sergey Senozhatsky]

On (10/30/12 12:18), Greg Kroah-Hartman wrote:
> > the first one:
> > 
> > [PATCH 1/2] zram: factor-out zram_decompress_page() function
> > [PATCH 2/2] zram: handle mem suffixes in disk size zram_sysfs parameter
> > 
> > ACKed by Nitin.
> >  
> > the second one
> > 
> > [PATCH 2/2] zram: permit sleeping while in pool zs_malloc()
> > [PATCH 1/2] zram: forbid IO operations from within zram_init_device()  -- droppped by me.
> > 
> > 
> > I'll resend first two and next time will make sure not to spam your box. sorry.
> 
> Sending me lots of patches is fine, I don't mind that at all.  It's what
> I'm here for it seems :)
> 
> But when there is responses, and follow-up patches, and second versions
> and lots of other stuff, it gets hard to follow, so I ask for them all
> to be resent, like this.
> 

I'll Cc you next time only when we land the final versions.


> But, what happened with your second series?  As you didn't resend them,
> I'll just ignore them for now, please resend if you ever get them worked
> out.
> 

please ignore as I didn't have any response from Nitin yet on them. 

	-ss

> thanks,
> 
> greg k-h
> 

[PATCH 0/2] zram decompress_page and zram_sysfs patch seriess

[Sergey Senozhatsky]

Hello Greg,
I'm gonna send you 2 zram patches shortly, reviewed by Nitin.

	[PATCH 1/2] zram: factor-out zram_decompress_page() function
	[PATCH 2/2] zram: handle mem suffixes in disk size zram_sysfs parameter


Thanks,
	-ss

[PATCH 1/2] zram: factor-out zram_decompress_page() function

[Sergey Senozhatsky]

  zram: factor-out zram_decompress_page() function

  zram_bvec_read() shared decompress functionality with zram_read_before_write() function.
  Factor-out and make commonly used zram_decompress_page() function, which also simplified
  error handling in zram_bvec_read().

  Signed-off-by: Sergey Senozhatsky <sergey.senozhatsky@gmail.com>

---

 drivers/staging/zram/zram_drv.c | 113 +++++++++++++++++-----------------------
 1 file changed, 48 insertions(+), 65 deletions(-)

diff --git a/drivers/staging/zram/zram_drv.c b/drivers/staging/zram/zram_drv.c
index 6edefde..fb4a7c9 100644
--- a/drivers/staging/zram/zram_drv.c
+++ b/drivers/staging/zram/zram_drv.c
@@ -183,62 +183,25 @@ static inline int is_partial_io(struct bio_vec *bvec)
 	return bvec->bv_len != PAGE_SIZE;
 }
 
-static int zram_bvec_read(struct zram *zram, struct bio_vec *bvec,
-			  u32 index, int offset, struct bio *bio)
+static int zram_decompress_page(struct zram *zram, char *mem, u32 index)
 {
-	int ret;
-	size_t clen;
-	struct page *page;
-	unsigned char *user_mem, *cmem, *uncmem = NULL;
-
-	page = bvec->bv_page;
-
-	if (zram_test_flag(zram, index, ZRAM_ZERO)) {
-		handle_zero_page(bvec);
-		return 0;
-	}
+	int ret = LZO_E_OK;
+	size_t clen = PAGE_SIZE;
+	unsigned char *cmem;
+	unsigned long handle = zram->table[index].handle;
 
-	/* Requested page is not present in compressed area */
-	if (unlikely(!zram->table[index].handle)) {
-		pr_debug("Read before write: sector=%lu, size=%u",
-			 (ulong)(bio->bi_sector), bio->bi_size);
-		handle_zero_page(bvec);
+	if (!handle || zram_test_flag(zram, index, ZRAM_ZERO)) {
+		memset(mem, 0, PAGE_SIZE);
 		return 0;
 	}
 
-	if (is_partial_io(bvec)) {
-		/* Use  a temporary buffer to decompress the page */
-		uncmem = kmalloc(PAGE_SIZE, GFP_KERNEL);
-		if (!uncmem) {
-			pr_info("Error allocating temp memory!\n");
-			return -ENOMEM;
-		}
-	}
-
-	user_mem = kmap_atomic(page);
-	if (!is_partial_io(bvec))
-		uncmem = user_mem;
-	clen = PAGE_SIZE;
-
-	cmem = zs_map_object(zram->mem_pool, zram->table[index].handle,
-				ZS_MM_RO);
-
-	if (zram->table[index].size == PAGE_SIZE) {
-		memcpy(uncmem, cmem, PAGE_SIZE);
-		ret = LZO_E_OK;
-	} else {
+	cmem = zs_map_object(zram->mem_pool, handle, ZS_MM_RO);
+	if (zram->table[index].size == PAGE_SIZE)
+		memcpy(mem, cmem, PAGE_SIZE);
+	else
 		ret = lzo1x_decompress_safe(cmem, zram->table[index].size,
-				    uncmem, &clen);
-	}
-
-	if (is_partial_io(bvec)) {
-		memcpy(user_mem + bvec->bv_offset, uncmem + offset,
-		       bvec->bv_len);
-		kfree(uncmem);
-	}
-
-	zs_unmap_object(zram->mem_pool, zram->table[index].handle);
-	kunmap_atomic(user_mem);
+						mem, &clen);
+	zs_unmap_object(zram->mem_pool, handle);
 
 	/* Should NEVER happen. Return bio error if it does. */
 	if (unlikely(ret != LZO_E_OK)) {
@@ -247,36 +210,56 @@ static int zram_bvec_read(struct zram *zram, struct bio_vec *bvec,
 		return ret;
 	}
 
-	flush_dcache_page(page);
-
 	return 0;
 }
 
-static int zram_read_before_write(struct zram *zram, char *mem, u32 index)
+static int zram_bvec_read(struct zram *zram, struct bio_vec *bvec,
+			  u32 index, int offset, struct bio *bio)
 {
 	int ret;
-	size_t clen = PAGE_SIZE;
-	unsigned char *cmem;
-	unsigned long handle = zram->table[index].handle;
+	struct page *page;
+	unsigned char *user_mem, *uncmem = NULL;
 
-	if (zram_test_flag(zram, index, ZRAM_ZERO) || !handle) {
-		memset(mem, 0, PAGE_SIZE);
+	page = bvec->bv_page;
+
+	if (unlikely(!zram->table[index].handle) ||
+			zram_test_flag(zram, index, ZRAM_ZERO)) {
+		handle_zero_page(bvec);
 		return 0;
 	}
 
-	cmem = zs_map_object(zram->mem_pool, handle, ZS_MM_RO);
-	ret = lzo1x_decompress_safe(cmem, zram->table[index].size,
-				    mem, &clen);
-	zs_unmap_object(zram->mem_pool, handle);
+	user_mem = kmap_atomic(page);
+	if (is_partial_io(bvec))
+		/* Use  a temporary buffer to decompress the page */
+		uncmem = kmalloc(PAGE_SIZE, GFP_KERNEL);
+	else
+		uncmem = user_mem;
 
+	if (!uncmem) {
+		pr_info("Unable to allocate temp memory\n");
+		ret = -ENOMEM;
+		goto out_cleanup;
+	}
+
+	ret = zram_decompress_page(zram, uncmem, index);
 	/* Should NEVER happen. Return bio error if it does. */
 	if (unlikely(ret != LZO_E_OK)) {
 		pr_err("Decompression failed! err=%d, page=%u\n", ret, index);
 		zram_stat64_inc(zram, &zram->stats.failed_reads);
-		return ret;
+		goto out_cleanup;
 	}
 
-	return 0;
+	if (is_partial_io(bvec))
+		memcpy(user_mem + bvec->bv_offset, uncmem + offset,
+				bvec->bv_len);
+
+	flush_dcache_page(page);
+	ret = 0;
+out_cleanup:
+	kunmap_atomic(user_mem);
+	if (is_partial_io(bvec))
+		kfree(uncmem);
+	return ret;
 }
 
 static int zram_bvec_write(struct zram *zram, struct bio_vec *bvec, u32 index,
@@ -302,7 +285,7 @@ static int zram_bvec_write(struct zram *zram, struct bio_vec *bvec, u32 index,
 			ret = -ENOMEM;
 			goto out;
 		}
-		ret = zram_read_before_write(zram, uncmem, index);
+		ret = zram_decompress_page(zram, uncmem, index);
 		if (ret) {
 			kfree(uncmem);
 			goto out;

Re: [PATCH 1/2] zram: factor-out zram_decompress_page() function

[Greg Kroah-Hartman]

On Tue, Oct 30, 2012 at 09:58:42PM +0300, Sergey Senozhatsky wrote:
>   zram: factor-out zram_decompress_page() function

What's with the indentation?

And including the Subject: again here?

> 
>   zram_bvec_read() shared decompress functionality with zram_read_before_write() function.
>   Factor-out and make commonly used zram_decompress_page() function, which also simplified
>   error handling in zram_bvec_read().
> 
>   Signed-off-by: Sergey Senozhatsky <sergey.senozhatsky@gmail.com>

Where did Nitin's ack go?

I would have to edit these by hand to apply them, which isn't ok.
Please fix this up, add Nitin's acks, and resend properly.

thanks,

greg k-h

[PATCH 2/2] zram: handle mem suffixes in disk size zram_sysfs parameter

[Sergey Senozhatsky]

  zram: handle mem suffixes in disk size zram_sysfs parameter

  Use memparse() to allow mem suffixes in disksize sysfs number.
  Examples:
       echo 256K > /sys/block/zram0/disksize
       echo 512M > /sys/block/zram0/disksize
       echo 1G > /sys/block/zram0/disksize

  Signed-off-by: Sergey Senozhatsky <sergey.senozhatsky@gmail.com>

---

 drivers/staging/zram/zram_sysfs.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/staging/zram/zram_sysfs.c b/drivers/staging/zram/zram_sysfs.c
index edb0ed4..de1eacf 100644
--- a/drivers/staging/zram/zram_sysfs.c
+++ b/drivers/staging/zram/zram_sysfs.c
@@ -15,6 +15,7 @@
 #include <linux/device.h>
 #include <linux/genhd.h>
 #include <linux/mm.h>
+#include <linux/kernel.h>
 
 #include "zram_drv.h"
 
@@ -54,13 +55,12 @@ static ssize_t disksize_show(struct device *dev,
 static ssize_t disksize_store(struct device *dev,
 		struct device_attribute *attr, const char *buf, size_t len)
 {
-	int ret;
 	u64 disksize;
 	struct zram *zram = dev_to_zram(dev);
 
-	ret = kstrtoull(buf, 10, &disksize);
-	if (ret)
-		return ret;
+	disksize = memparse(buf, NULL);
+	if (!disksize)
+		return -EINVAL;
 
 	down_write(&zram->init_lock);
 	if (zram->init_done) {

[PATCH 2/2] zram: handle mem suffixes in disk size zram_sysfs parameter

[Sergey Senozhatsky]

  zram: handle mem suffixes in disk size zram_sysfs parameter

  Use memparse() to allow mem suffixes in disksize sysfs number.
  Examples:
       echo 256K > /sys/block/zram0/disksize
       echo 512M > /sys/block/zram0/disksize
       echo 1G > /sys/block/zram0/disksize
    
  Signed-off-by: Sergey Senozhatsky <sergey.senozhatsky@gmail.com>

---

 drivers/staging/zram/zram_sysfs.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/staging/zram/zram_sysfs.c b/drivers/staging/zram/zram_sysfs.c
index edb0ed4..6be318e 100644
--- a/drivers/staging/zram/zram_sysfs.c
+++ b/drivers/staging/zram/zram_sysfs.c
@@ -15,6 +15,7 @@
 #include <linux/device.h>
 #include <linux/genhd.h>
 #include <linux/mm.h>
+#include <linux/kernel.h>
 
 #include "zram_drv.h"
 
@@ -54,13 +55,12 @@ static ssize_t disksize_show(struct device *dev,
 static ssize_t disksize_store(struct device *dev,
 		struct device_attribute *attr, const char *buf, size_t len)
 {
-	int ret;
 	u64 disksize;
 	struct zram *zram = dev_to_zram(dev);
 
-	ret = kstrtoull(buf, 10, &disksize);
-	if (ret)
-		return ret;
+	disksize = memparse(buf, NULL);
+	if (disksize < 1)
+		return -EINVAL;
 
 	down_write(&zram->init_lock);
 	if (zram->init_done) {

Re: [PATCH 2/2] zram: handle mem suffixes in disk size zram_sysfs parameter

[Nitin Gupta]

On 10/27/2012 09:03 AM, Sergey Senozhatsky wrote:
>    zram: handle mem suffixes in disk size zram_sysfs parameter
>
>    Use memparse() to allow mem suffixes in disksize sysfs number.
>    Examples:
>         echo 256K > /sys/block/zram0/disksize
>         echo 512M > /sys/block/zram0/disksize
>         echo 1G > /sys/block/zram0/disksize
>
>    Signed-off-by: Sergey Senozhatsky <sergey.senozhatsky@gmail.com>
>
> ---
>
>   drivers/staging/zram/zram_sysfs.c | 8 ++++----
>   1 file changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/staging/zram/zram_sysfs.c b/drivers/staging/zram/zram_sysfs.c
> index edb0ed4..6be318e 100644
> --- a/drivers/staging/zram/zram_sysfs.c
> +++ b/drivers/staging/zram/zram_sysfs.c
> @@ -15,6 +15,7 @@
>   #include <linux/device.h>
>   #include <linux/genhd.h>
>   #include <linux/mm.h>
> +#include <linux/kernel.h>
>
>   #include "zram_drv.h"
>
> @@ -54,13 +55,12 @@ static ssize_t disksize_show(struct device *dev,
>   static ssize_t disksize_store(struct device *dev,
>   		struct device_attribute *attr, const char *buf, size_t len)
>   {
> -	int ret;
>   	u64 disksize;
>   	struct zram *zram = dev_to_zram(dev);
>
> -	ret = kstrtoull(buf, 10, &disksize);
> -	if (ret)
> -		return ret;
> +	disksize = memparse(buf, NULL);
> +	if (disksize < 1)
> +		return -EINVAL;
>

or, just: if (!disksize) ...

Thanks,
Nitin

Re: [PATCH 2/2] zram: handle mem suffixes in disk size zram_sysfs parameter

[Sergey Senozhatsky]

On (10/29/12 10:21), Nitin Gupta wrote:
> >+#include <linux/kernel.h>
> >
> >  #include "zram_drv.h"
> >
> >@@ -54,13 +55,12 @@ static ssize_t disksize_show(struct device *dev,
> >  static ssize_t disksize_store(struct device *dev,
> >  		struct device_attribute *attr, const char *buf, size_t len)
> >  {
> >-	int ret;
> >  	u64 disksize;
> >  	struct zram *zram = dev_to_zram(dev);
> >
> >-	ret = kstrtoull(buf, 10, &disksize);
> >-	if (ret)
> >-		return ret;
> >+	disksize = memparse(buf, NULL);
> >+	if (disksize < 1)
> >+		return -EINVAL;
> >
> 
> or, just: if (!disksize) ...
> 

will resend shortly.

	-ss