Skip to contentSkip to conversation

zram: permit sleeping while in pool zs_malloc()

PATCH In review Sergey Senozhatsky 2 participants 9 messages Oct 30, 2012 Staging

lore
Share & tools
lore
#0 lore raw

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

    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 <[email protected]>

---

 drivers/staging/zram/zram_drv.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
drivers/staging/zram/zram_drv.c +1 -1
index d2e0a85..47f2e3a 100644
@@ -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;
#1 lore raw
Quoted 1 line · Sergey Senozhatsky

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

#2 lore raw
Quoted 12 lines · Greg Kroah-Hartman
Quoted 1 line · Sergey Senozhatsky

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
Quoted 4 lines

thanks,

greg k-h

#3 lore raw
Quoted 25 lines · Sergey Senozhatsky
Quoted 12 lines · Greg Kroah-Hartman
Quoted 1 line · Sergey Senozhatsky

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.

Quoted 7 lines

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

#4 lore raw
Quoted 13 lines · Greg Kroah-Hartman
Quoted 13 lines

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.

Quoted 9 lines
Quoted 1 line

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.

Quoted 4 lines

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
Quoted 4 lines

thanks,

greg k-h

#5 lore raw

cover zram decompress_page and zram_sysfs patch seriess

In reply to Greg Kroah-Hartman

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
#6 lore raw

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

In reply to Greg Kroah-Hartman

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().

---

 drivers/staging/zram/zram_drv.c | 113 +++++++++++++++++-----------------------
 1 file changed, 48 insertions(+), 65 deletions(-)
drivers/staging/zram/zram_drv.c +48 -65
index 6edefde..fb4a7c9 100644
@@ -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;
#7 lore raw

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

Quoted 1 line · Sergey Senozhatsky

zram: factor-out zram_decompress_page() function

What's with the indentation?

And including the Subject: again here?

Quoted 6 lines

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().

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

#8 lore raw

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

In reply to Greg Kroah-Hartman

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

---

 drivers/staging/zram/zram_sysfs.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)
drivers/staging/zram/zram_sysfs.c +4 -4
index edb0ed4..de1eacf 100644
@@ -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) {