[RFC 01/15] FS: libfs, implement simple_write_to_buffer

[RFC 01/15] FS: libfs, implement simple_write_to_buffer

[Jiri Slaby]

It will be used in suspend code and serves as an easy wrap around
copy_from_user. Similar to simple_read_from_buffer, it takes care
of transfers with proper lengths depending on available and count
parameters and advances ppos appropriately.

Signed-off-by: Jiri Slaby <jslaby@suse.cz>
Cc: Nigel Cunningham <ncunningham@crca.org.au>
Cc: "Rafael J. Wysocki" <rjw@sisk.pl>
Cc: Alexander Viro <viro@zeniv.linux.org.uk>
Cc: linux-fsdevel@vger.kernel.org
---
 fs/libfs.c         |   35 +++++++++++++++++++++++++++++++++++
 include/linux/fs.h |    2 ++
 2 files changed, 37 insertions(+), 0 deletions(-)

diff --git a/fs/libfs.c b/fs/libfs.c
index 9e50bcf..fda73b3 100644
--- a/fs/libfs.c
+++ b/fs/libfs.c
@@ -546,6 +546,40 @@ ssize_t simple_read_from_buffer(void __user *to, size_t count, loff_t *ppos,
 }
 
 /**
+ * simple_write_to_buffer - copy data from user space to the buffer
+ * @to: the buffer to write to
+ * @available: the size of the buffer
+ * @ppos: the current position in the buffer
+ * @from: the user space buffer to read from
+ * @count: the maximum number of bytes to read
+ *
+ * The simple_write_to_buffer() function reads up to @count bytes from the user
+ * space address starting at @from into the buffer @to at offset @ppos.
+ *
+ * On success, the number of bytes written is returned and the offset @ppos is
+ * advanced by this number, or negative value is returned on error.
+ **/
+ssize_t simple_write_to_buffer(void *to, size_t available, loff_t *ppos,
+		const void __user *from, size_t count)
+{
+	loff_t pos = *ppos;
+	size_t ret;
+
+	if (pos < 0)
+		return -EINVAL;
+	if (pos >= available || !count)
+		return 0;
+	if (count > available - pos)
+		count = available - pos;
+	ret = copy_from_user(to + pos, from, count);
+	if (ret == count)
+		return -EFAULT;
+	count -= ret;
+	*ppos = pos + count;
+	return count;
+}
+
+/**
  * memory_read_from_buffer - copy data from the buffer
  * @to: the kernel space buffer to read to
  * @count: the maximum number of bytes to read
@@ -863,6 +897,7 @@ EXPORT_SYMBOL(simple_statfs);
 EXPORT_SYMBOL(simple_sync_file);
 EXPORT_SYMBOL(simple_unlink);
 EXPORT_SYMBOL(simple_read_from_buffer);
+EXPORT_SYMBOL(simple_write_to_buffer);
 EXPORT_SYMBOL(memory_read_from_buffer);
 EXPORT_SYMBOL(simple_transaction_set);
 EXPORT_SYMBOL(simple_transaction_get);
diff --git a/include/linux/fs.h b/include/linux/fs.h
index a4636b6..0f751b6 100644
--- a/include/linux/fs.h
+++ b/include/linux/fs.h
@@ -2370,6 +2370,8 @@ extern void simple_release_fs(struct vfsmount **mount, int *count);
 
 extern ssize_t simple_read_from_buffer(void __user *to, size_t count,
 			loff_t *ppos, const void *from, size_t available);
+extern ssize_t simple_write_to_buffer(void *to, size_t available, loff_t *ppos,
+		const void __user *from, size_t count);
 
 extern int simple_fsync(struct file *, struct dentry *, int);
 
-- 
1.7.0.2

[RFC 02/15] PM / Hibernate: snapshot cleanup

[Jiri Slaby]

From: Jiri Slaby <jirislaby@gmail.com>

Remove support of reads with offset. This means snapshot_read/write_next
now does not accept count parameter.

/dev/snapshot handler is converted to simple_read_from_buffer/simple_write_to_buffer.

Signed-off-by: Jiri Slaby <jslaby@suse.cz>
Cc: Nigel Cunningham <ncunningham@crca.org.au>
Cc: "Rafael J. Wysocki" <rjw@sisk.pl>
---
 kernel/power/power.h    |   18 +-----
 kernel/power/snapshot.c |  145 ++++++++++++++++++-----------------------------
 kernel/power/swap.c     |    8 +-
 kernel/power/user.c     |   39 ++++++++-----
 4 files changed, 88 insertions(+), 122 deletions(-)

diff --git a/kernel/power/power.h b/kernel/power/power.h
index 46c5a26..b1e207d 100644
--- a/kernel/power/power.h
+++ b/kernel/power/power.h
@@ -97,24 +97,12 @@ extern int hibernate_preallocate_memory(void);
  */
 
 struct snapshot_handle {
-	loff_t		offset;	/* number of the last byte ready for reading
-				 * or writing in the sequence
-				 */
 	unsigned int	cur;	/* number of the block of PAGE_SIZE bytes the
 				 * next operation will refer to (ie. current)
 				 */
-	unsigned int	cur_offset;	/* offset with respect to the current
-					 * block (for the next operation)
-					 */
-	unsigned int	prev;	/* number of the block of PAGE_SIZE bytes that
-				 * was the current one previously
-				 */
 	void		*buffer;	/* address of the block to read from
 					 * or write to
 					 */
-	unsigned int	buf_offset;	/* location to read from or write to,
-					 * given as a displacement from 'buffer'
-					 */
 	int		sync_read;	/* Set to one to notify the caller of
 					 * snapshot_write_next() that it may
 					 * need to call wait_on_bio_chain()
@@ -125,12 +113,12 @@ struct snapshot_handle {
  * snapshot_read_next()/snapshot_write_next() is allowed to
  * read/write data after the function returns
  */
-#define data_of(handle)	((handle).buffer + (handle).buf_offset)
+#define data_of(handle)	((handle).buffer)
 
 extern unsigned int snapshot_additional_pages(struct zone *zone);
 extern unsigned long snapshot_get_image_size(void);
-extern int snapshot_read_next(struct snapshot_handle *handle, size_t count);
-extern int snapshot_write_next(struct snapshot_handle *handle, size_t count);
+extern int snapshot_read_next(struct snapshot_handle *handle);
+extern int snapshot_write_next(struct snapshot_handle *handle);
 extern void snapshot_write_finalize(struct snapshot_handle *handle);
 extern int snapshot_image_loaded(struct snapshot_handle *handle);
 
diff --git a/kernel/power/snapshot.c b/kernel/power/snapshot.c
index 830cade..7918351 100644
--- a/kernel/power/snapshot.c
+++ b/kernel/power/snapshot.c
@@ -1603,14 +1603,9 @@ pack_pfns(unsigned long *buf, struct memory_bitmap *bm)
  *	snapshot_handle structure.  The structure gets updated and a pointer
  *	to it should be passed to this function every next time.
  *
- *	The @count parameter should contain the number of bytes the caller
- *	wants to read from the snapshot.  It must not be zero.
- *
  *	On success the function returns a positive number.  Then, the caller
  *	is allowed to read up to the returned number of bytes from the memory
- *	location computed by the data_of() macro.  The number returned
- *	may be smaller than @count, but this only happens if the read would
- *	cross a page boundary otherwise.
+ *	location computed by the data_of() macro.
  *
  *	The function returns 0 to indicate the end of data stream condition,
  *	and a negative number is returned on error.  In such cases the
@@ -1618,7 +1613,7 @@ pack_pfns(unsigned long *buf, struct memory_bitmap *bm)
  *	any more.
  */
 
-int snapshot_read_next(struct snapshot_handle *handle, size_t count)
+int snapshot_read_next(struct snapshot_handle *handle)
 {
 	if (handle->cur > nr_meta_pages + nr_copy_pages)
 		return 0;
@@ -1629,7 +1624,7 @@ int snapshot_read_next(struct snapshot_handle *handle, size_t count)
 		if (!buffer)
 			return -ENOMEM;
 	}
-	if (!handle->offset) {
+	if (!handle->cur) {
 		int error;
 
 		error = init_header((struct swsusp_info *)buffer);
@@ -1638,42 +1633,30 @@ int snapshot_read_next(struct snapshot_handle *handle, size_t count)
 		handle->buffer = buffer;
 		memory_bm_position_reset(&orig_bm);
 		memory_bm_position_reset(&copy_bm);
-	}
-	if (handle->prev < handle->cur) {
-		if (handle->cur <= nr_meta_pages) {
-			memset(buffer, 0, PAGE_SIZE);
-			pack_pfns(buffer, &orig_bm);
-		} else {
-			struct page *page;
+	} else if (handle->cur <= nr_meta_pages) {
+		memset(buffer, 0, PAGE_SIZE);
+		pack_pfns(buffer, &orig_bm);
+	} else {
+		struct page *page;
 
-			page = pfn_to_page(memory_bm_next_pfn(&copy_bm));
-			if (PageHighMem(page)) {
-				/* Highmem pages are copied to the buffer,
-				 * because we can't return with a kmapped
-				 * highmem page (we may not be called again).
-				 */
-				void *kaddr;
+		page = pfn_to_page(memory_bm_next_pfn(&copy_bm));
+		if (PageHighMem(page)) {
+			/* Highmem pages are copied to the buffer,
+			 * because we can't return with a kmapped
+			 * highmem page (we may not be called again).
+			 */
+			void *kaddr;
 
-				kaddr = kmap_atomic(page, KM_USER0);
-				memcpy(buffer, kaddr, PAGE_SIZE);
-				kunmap_atomic(kaddr, KM_USER0);
-				handle->buffer = buffer;
-			} else {
-				handle->buffer = page_address(page);
-			}
+			kaddr = kmap_atomic(page, KM_USER0);
+			memcpy(buffer, kaddr, PAGE_SIZE);
+			kunmap_atomic(kaddr, KM_USER0);
+			handle->buffer = buffer;
+		} else {
+			handle->buffer = page_address(page);
 		}
-		handle->prev = handle->cur;
-	}
-	handle->buf_offset = handle->cur_offset;
-	if (handle->cur_offset + count >= PAGE_SIZE) {
-		count = PAGE_SIZE - handle->cur_offset;
-		handle->cur_offset = 0;
-		handle->cur++;
-	} else {
-		handle->cur_offset += count;
 	}
-	handle->offset += count;
-	return count;
+	handle->cur++;
+	return PAGE_SIZE;
 }
 
 /**
@@ -2132,14 +2115,9 @@ static void *get_buffer(struct memory_bitmap *bm, struct chain_allocator *ca)
  *	snapshot_handle structure.  The structure gets updated and a pointer
  *	to it should be passed to this function every next time.
  *
- *	The @count parameter should contain the number of bytes the caller
- *	wants to write to the image.  It must not be zero.
- *
  *	On success the function returns a positive number.  Then, the caller
  *	is allowed to write up to the returned number of bytes to the memory
- *	location computed by the data_of() macro.  The number returned
- *	may be smaller than @count, but this only happens if the write would
- *	cross a page boundary otherwise.
+ *	location computed by the data_of() macro.
  *
  *	The function returns 0 to indicate the "end of file" condition,
  *	and a negative number is returned on error.  In such cases the
@@ -2147,16 +2125,18 @@ static void *get_buffer(struct memory_bitmap *bm, struct chain_allocator *ca)
  *	any more.
  */
 
-int snapshot_write_next(struct snapshot_handle *handle, size_t count)
+int snapshot_write_next(struct snapshot_handle *handle)
 {
 	static struct chain_allocator ca;
 	int error = 0;
 
 	/* Check if we have already loaded the entire image */
-	if (handle->prev && handle->cur > nr_meta_pages + nr_copy_pages)
+	if (handle->cur > 1 && handle->cur > nr_meta_pages + nr_copy_pages)
 		return 0;
 
-	if (handle->offset == 0) {
+	handle->sync_read = 1;
+
+	if (!handle->cur) {
 		if (!buffer)
 			/* This makes the buffer be freed by swsusp_free() */
 			buffer = get_image_page(GFP_ATOMIC, PG_ANY);
@@ -2165,56 +2145,43 @@ int snapshot_write_next(struct snapshot_handle *handle, size_t count)
 			return -ENOMEM;
 
 		handle->buffer = buffer;
-	}
-	handle->sync_read = 1;
-	if (handle->prev < handle->cur) {
-		if (handle->prev == 0) {
-			error = load_header(buffer);
-			if (error)
-				return error;
+	} else if (handle->cur == 1) {
+		error = load_header(buffer);
+		if (error)
+			return error;
 
-			error = memory_bm_create(&copy_bm, GFP_ATOMIC, PG_ANY);
-			if (error)
-				return error;
+		error = memory_bm_create(&copy_bm, GFP_ATOMIC, PG_ANY);
+		if (error)
+			return error;
+
+	} else if (handle->cur <= nr_meta_pages + 1) {
+		error = unpack_orig_pfns(buffer, &copy_bm);
+		if (error)
+			return error;
 
-		} else if (handle->prev <= nr_meta_pages) {
-			error = unpack_orig_pfns(buffer, &copy_bm);
+		if (handle->cur == nr_meta_pages + 1) {
+			error = prepare_image(&orig_bm, &copy_bm);
 			if (error)
 				return error;
 
-			if (handle->prev == nr_meta_pages) {
-				error = prepare_image(&orig_bm, &copy_bm);
-				if (error)
-					return error;
-
-				chain_init(&ca, GFP_ATOMIC, PG_SAFE);
-				memory_bm_position_reset(&orig_bm);
-				restore_pblist = NULL;
-				handle->buffer = get_buffer(&orig_bm, &ca);
-				handle->sync_read = 0;
-				if (IS_ERR(handle->buffer))
-					return PTR_ERR(handle->buffer);
-			}
-		} else {
-			copy_last_highmem_page();
+			chain_init(&ca, GFP_ATOMIC, PG_SAFE);
+			memory_bm_position_reset(&orig_bm);
+			restore_pblist = NULL;
 			handle->buffer = get_buffer(&orig_bm, &ca);
+			handle->sync_read = 0;
 			if (IS_ERR(handle->buffer))
 				return PTR_ERR(handle->buffer);
-			if (handle->buffer != buffer)
-				handle->sync_read = 0;
 		}
-		handle->prev = handle->cur;
-	}
-	handle->buf_offset = handle->cur_offset;
-	if (handle->cur_offset + count >= PAGE_SIZE) {
-		count = PAGE_SIZE - handle->cur_offset;
-		handle->cur_offset = 0;
-		handle->cur++;
 	} else {
-		handle->cur_offset += count;
+		copy_last_highmem_page();
+		handle->buffer = get_buffer(&orig_bm, &ca);
+		if (IS_ERR(handle->buffer))
+			return PTR_ERR(handle->buffer);
+		if (handle->buffer != buffer)
+			handle->sync_read = 0;
 	}
-	handle->offset += count;
-	return count;
+	handle->cur++;
+	return PAGE_SIZE;
 }
 
 /**
@@ -2229,7 +2196,7 @@ void snapshot_write_finalize(struct snapshot_handle *handle)
 {
 	copy_last_highmem_page();
 	/* Free only if we have loaded the image entirely */
-	if (handle->prev && handle->cur > nr_meta_pages + nr_copy_pages) {
+	if (handle->cur > 1 && handle->cur > nr_meta_pages + nr_copy_pages) {
 		memory_bm_free(&orig_bm, PG_UNSAFE_CLEAR);
 		free_highmem_data();
 	}
diff --git a/kernel/power/swap.c b/kernel/power/swap.c
index 1d57573..4a22f0b 100644
--- a/kernel/power/swap.c
+++ b/kernel/power/swap.c
@@ -430,7 +430,7 @@ static int save_image(struct swap_map_handle *handle,
 	bio = NULL;
 	do_gettimeofday(&start);
 	while (1) {
-		ret = snapshot_read_next(snapshot, PAGE_SIZE);
+		ret = snapshot_read_next(snapshot);
 		if (ret <= 0)
 			break;
 		ret = swap_write_page(handle, data_of(*snapshot), &bio);
@@ -491,7 +491,7 @@ int swsusp_write(unsigned int flags)
 		return error;
 	}
 	memset(&snapshot, 0, sizeof(struct snapshot_handle));
-	error = snapshot_read_next(&snapshot, PAGE_SIZE);
+	error = snapshot_read_next(&snapshot);
 	if (error < PAGE_SIZE) {
 		if (error >= 0)
 			error = -EFAULT;
@@ -614,7 +614,7 @@ static int load_image(struct swap_map_handle *handle,
 	bio = NULL;
 	do_gettimeofday(&start);
 	for ( ; ; ) {
-		error = snapshot_write_next(snapshot, PAGE_SIZE);
+		error = snapshot_write_next(snapshot);
 		if (error <= 0)
 			break;
 		error = swap_read_page(handle, data_of(*snapshot), &bio);
@@ -659,7 +659,7 @@ int swsusp_read(unsigned int *flags_p)
 	*flags_p = swsusp_header->flags;
 
 	memset(&snapshot, 0, sizeof(struct snapshot_handle));
-	error = snapshot_write_next(&snapshot, PAGE_SIZE);
+	error = snapshot_write_next(&snapshot);
 	if (error < PAGE_SIZE)
 		return error < 0 ? error : -EFAULT;
 	header = (struct swsusp_info *)data_of(snapshot);
diff --git a/kernel/power/user.c b/kernel/power/user.c
index 4d22896..68a99c1 100644
--- a/kernel/power/user.c
+++ b/kernel/power/user.c
@@ -151,6 +151,7 @@ static ssize_t snapshot_read(struct file *filp, char __user *buf,
 {
 	struct snapshot_data *data;
 	ssize_t res;
+	loff_t pg_offp = *offp & ~PAGE_MASK;
 
 	mutex_lock(&pm_mutex);
 
@@ -159,13 +160,17 @@ static ssize_t snapshot_read(struct file *filp, char __user *buf,
 		res = -ENODATA;
 		goto Unlock;
 	}
-	res = snapshot_read_next(&data->handle, count);
-	if (res > 0) {
-		if (copy_to_user(buf, data_of(data->handle), res))
-			res = -EFAULT;
-		else
-			*offp = data->handle.offset;
-	}
+	if (!pg_offp) { /* on page boundary? */
+		res = snapshot_read_next(&data->handle);
+		if (res <= 0)
+			goto Unlock;
+	} else
+		res = PAGE_SIZE - pg_offp;
+
+	res = simple_read_from_buffer(buf, count, &pg_offp,
+			data_of(data->handle), res);
+	if (res > 0)
+		*offp += res;
 
  Unlock:
 	mutex_unlock(&pm_mutex);
@@ -178,18 +183,24 @@ static ssize_t snapshot_write(struct file *filp, const char __user *buf,
 {
 	struct snapshot_data *data;
 	ssize_t res;
+	loff_t pg_offp = *offp & ~PAGE_MASK;
 
 	mutex_lock(&pm_mutex);
 
 	data = filp->private_data;
-	res = snapshot_write_next(&data->handle, count);
-	if (res > 0) {
-		if (copy_from_user(data_of(data->handle), buf, res))
-			res = -EFAULT;
-		else
-			*offp = data->handle.offset;
-	}
 
+	if (!pg_offp) {
+		res = snapshot_write_next(&data->handle);
+		if (res <= 0)
+			goto unlock;
+	} else
+		res = PAGE_SIZE - pg_offp;
+
+	res = simple_write_to_buffer(data_of(data->handle), res, &pg_offp,
+			buf, count);
+	if (res > 0)
+		*offp += res;
+unlock:
 	mutex_unlock(&pm_mutex);
 
 	return res;
-- 
1.7.0.2

[RFC 03/15] PM / Hibernate: separate block_io

[Jiri Slaby]

Move block I/O operations to a separate file. It is because it will
be used later not only by the swap writer.

Signed-off-by: Jiri Slaby <jslaby@suse.cz>
Cc: Nigel Cunningham <ncunningham@crca.org.au>
Cc: "Rafael J. Wysocki" <rjw@sisk.pl>
---
 kernel/power/Makefile   |    3 +-
 kernel/power/block_io.c |  103 +++++++++++++++++++++++++++++++++++
 kernel/power/power.h    |    9 +++
 kernel/power/swap.c     |  136 +++++++++--------------------------------------
 4 files changed, 139 insertions(+), 112 deletions(-)
 create mode 100644 kernel/power/block_io.c

diff --git a/kernel/power/Makefile b/kernel/power/Makefile
index 4319181..524e058 100644
--- a/kernel/power/Makefile
+++ b/kernel/power/Makefile
@@ -8,7 +8,8 @@ obj-$(CONFIG_PM_SLEEP)		+= console.o
 obj-$(CONFIG_FREEZER)		+= process.o
 obj-$(CONFIG_SUSPEND)		+= suspend.o
 obj-$(CONFIG_PM_TEST_SUSPEND)	+= suspend_test.o
-obj-$(CONFIG_HIBERNATION)	+= hibernate.o snapshot.o swap.o user.o
+obj-$(CONFIG_HIBERNATION)	+= hibernate.o snapshot.o swap.o user.o \
+				   block_io.o
 obj-$(CONFIG_HIBERNATION_NVS)	+= hibernate_nvs.o
 
 obj-$(CONFIG_MAGIC_SYSRQ)	+= poweroff.o
diff --git a/kernel/power/block_io.c b/kernel/power/block_io.c
new file mode 100644
index 0000000..2b7898a
--- /dev/null
+++ b/kernel/power/block_io.c
@@ -0,0 +1,103 @@
+/*
+ * This file provides functions for block I/O operations on swap/file.
+ *
+ * Copyright (C) 1998,2001-2005 Pavel Machek <pavel@suse.cz>
+ * Copyright (C) 2006 Rafael J. Wysocki <rjw@sisk.pl>
+ *
+ * This file is released under the GPLv2.
+ */
+
+#include <linux/bio.h>
+#include <linux/kernel.h>
+#include <linux/pagemap.h>
+#include <linux/swap.h>
+
+#include "power.h"
+
+/**
+ *	submit - submit BIO request.
+ *	@rw:	READ or WRITE.
+ *	@off	physical offset of page.
+ *	@page:	page we're reading or writing.
+ *	@bio_chain: list of pending biod (for async reading)
+ *
+ *	Straight from the textbook - allocate and initialize the bio.
+ *	If we're reading, make sure the page is marked as dirty.
+ *	Then submit it and, if @bio_chain == NULL, wait.
+ */
+static int submit(int rw, struct block_device *bdev, sector_t sector,
+		struct page *page, struct bio **bio_chain)
+{
+	const int bio_rw = rw | (1 << BIO_RW_SYNCIO) | (1 << BIO_RW_UNPLUG);
+	struct bio *bio;
+
+	bio = bio_alloc(__GFP_WAIT | __GFP_HIGH, 1);
+	bio->bi_sector = sector;
+	bio->bi_bdev = bdev;
+	bio->bi_end_io = end_swap_bio_read;
+
+	if (bio_add_page(bio, page, PAGE_SIZE, 0) < PAGE_SIZE) {
+		printk(KERN_ERR "PM: Adding page to bio failed at %ld\n",
+			sector);
+		bio_put(bio);
+		return -EFAULT;
+	}
+
+	lock_page(page);
+	bio_get(bio);
+
+	if (bio_chain == NULL) {
+		submit_bio(bio_rw, bio);
+		wait_on_page_locked(page);
+		if (rw == READ)
+			bio_set_pages_dirty(bio);
+		bio_put(bio);
+	} else {
+		if (rw == READ)
+			get_page(page);	/* These pages are freed later */
+		bio->bi_private = *bio_chain;
+		*bio_chain = bio;
+		submit_bio(bio_rw, bio);
+	}
+	return 0;
+}
+
+int sws_bio_read_page(pgoff_t page_off, void *addr, struct bio **bio_chain)
+{
+	return submit(READ, sws_resume_bdev, page_off * (PAGE_SIZE >> 9),
+			virt_to_page(addr), bio_chain);
+}
+
+int sws_bio_write_page(pgoff_t page_off, void *addr, struct bio **bio_chain)
+{
+	return submit(WRITE, sws_resume_bdev, page_off * (PAGE_SIZE >> 9),
+			virt_to_page(addr), bio_chain);
+}
+
+int sws_wait_on_bio_chain(struct bio **bio_chain)
+{
+	struct bio *bio;
+	struct bio *next_bio;
+	int ret = 0;
+
+	if (bio_chain == NULL)
+		return 0;
+
+	bio = *bio_chain;
+	if (bio == NULL)
+		return 0;
+	while (bio) {
+		struct page *page;
+
+		next_bio = bio->bi_private;
+		page = bio->bi_io_vec[0].bv_page;
+		wait_on_page_locked(page);
+		if (!PageUptodate(page) || PageError(page))
+			ret = -EIO;
+		put_page(page);
+		bio_put(bio);
+		bio = next_bio;
+	}
+	*bio_chain = NULL;
+	return ret;
+}
diff --git a/kernel/power/power.h b/kernel/power/power.h
index b1e207d..6c4b4fa 100644
--- a/kernel/power/power.h
+++ b/kernel/power/power.h
@@ -142,6 +142,15 @@ extern int swsusp_read(unsigned int *flags_p);
 extern int swsusp_write(unsigned int flags);
 extern void swsusp_close(fmode_t);
 
+/* kernel/power/block_io.c */
+extern struct block_device *sws_resume_bdev;
+
+extern int sws_bio_read_page(pgoff_t page_off, void *addr,
+		struct bio **bio_chain);
+extern int sws_bio_write_page(pgoff_t page_off, void *addr,
+		struct bio **bio_chain);
+extern int sws_wait_on_bio_chain(struct bio **bio_chain);
+
 struct timeval;
 /* kernel/power/swsusp.c */
 extern void swsusp_show_speed(struct timeval *, struct timeval *,
diff --git a/kernel/power/swap.c b/kernel/power/swap.c
index 4a22f0b..d4ff0d1 100644
--- a/kernel/power/swap.c
+++ b/kernel/power/swap.c
@@ -144,93 +144,7 @@ int swsusp_swap_in_use(void)
  */
 
 static unsigned short root_swap = 0xffff;
-static struct block_device *resume_bdev;
-
-/**
- *	submit - submit BIO request.
- *	@rw:	READ or WRITE.
- *	@off	physical offset of page.
- *	@page:	page we're reading or writing.
- *	@bio_chain: list of pending biod (for async reading)
- *
- *	Straight from the textbook - allocate and initialize the bio.
- *	If we're reading, make sure the page is marked as dirty.
- *	Then submit it and, if @bio_chain == NULL, wait.
- */
-static int submit(int rw, pgoff_t page_off, struct page *page,
-			struct bio **bio_chain)
-{
-	const int bio_rw = rw | (1 << BIO_RW_SYNCIO) | (1 << BIO_RW_UNPLUG);
-	struct bio *bio;
-
-	bio = bio_alloc(__GFP_WAIT | __GFP_HIGH, 1);
-	bio->bi_sector = page_off * (PAGE_SIZE >> 9);
-	bio->bi_bdev = resume_bdev;
-	bio->bi_end_io = end_swap_bio_read;
-
-	if (bio_add_page(bio, page, PAGE_SIZE, 0) < PAGE_SIZE) {
-		printk(KERN_ERR "PM: Adding page to bio failed at %ld\n",
-			page_off);
-		bio_put(bio);
-		return -EFAULT;
-	}
-
-	lock_page(page);
-	bio_get(bio);
-
-	if (bio_chain == NULL) {
-		submit_bio(bio_rw, bio);
-		wait_on_page_locked(page);
-		if (rw == READ)
-			bio_set_pages_dirty(bio);
-		bio_put(bio);
-	} else {
-		if (rw == READ)
-			get_page(page);	/* These pages are freed later */
-		bio->bi_private = *bio_chain;
-		*bio_chain = bio;
-		submit_bio(bio_rw, bio);
-	}
-	return 0;
-}
-
-static int bio_read_page(pgoff_t page_off, void *addr, struct bio **bio_chain)
-{
-	return submit(READ, page_off, virt_to_page(addr), bio_chain);
-}
-
-static int bio_write_page(pgoff_t page_off, void *addr, struct bio **bio_chain)
-{
-	return submit(WRITE, page_off, virt_to_page(addr), bio_chain);
-}
-
-static int wait_on_bio_chain(struct bio **bio_chain)
-{
-	struct bio *bio;
-	struct bio *next_bio;
-	int ret = 0;
-
-	if (bio_chain == NULL)
-		return 0;
-
-	bio = *bio_chain;
-	if (bio == NULL)
-		return 0;
-	while (bio) {
-		struct page *page;
-
-		next_bio = bio->bi_private;
-		page = bio->bi_io_vec[0].bv_page;
-		wait_on_page_locked(page);
-		if (!PageUptodate(page) || PageError(page))
-			ret = -EIO;
-		put_page(page);
-		bio_put(bio);
-		bio = next_bio;
-	}
-	*bio_chain = NULL;
-	return ret;
-}
+struct block_device *sws_resume_bdev;
 
 /*
  * Saving part
@@ -240,14 +154,14 @@ static int mark_swapfiles(sector_t start, unsigned int flags)
 {
 	int error;
 
-	bio_read_page(swsusp_resume_block, swsusp_header, NULL);
+	sws_bio_read_page(swsusp_resume_block, swsusp_header, NULL);
 	if (!memcmp("SWAP-SPACE",swsusp_header->sig, 10) ||
 	    !memcmp("SWAPSPACE2",swsusp_header->sig, 10)) {
 		memcpy(swsusp_header->orig_sig,swsusp_header->sig, 10);
 		memcpy(swsusp_header->sig,SWSUSP_SIG, 10);
 		swsusp_header->image = start;
 		swsusp_header->flags = flags;
-		error = bio_write_page(swsusp_resume_block,
+		error = sws_bio_write_page(swsusp_resume_block,
 					swsusp_header, NULL);
 	} else {
 		printk(KERN_ERR "PM: Swap header not found!\n");
@@ -266,18 +180,18 @@ static int swsusp_swap_check(void) /* This is called before saving image */
 	int res;
 
 	res = swap_type_of(swsusp_resume_device, swsusp_resume_block,
-			&resume_bdev);
+			&sws_resume_bdev);
 	if (res < 0)
 		return res;
 
 	root_swap = res;
-	res = blkdev_get(resume_bdev, FMODE_WRITE);
+	res = blkdev_get(sws_resume_bdev, FMODE_WRITE);
 	if (res)
 		return res;
 
-	res = set_blocksize(resume_bdev, PAGE_SIZE);
+	res = set_blocksize(sws_resume_bdev, PAGE_SIZE);
 	if (res < 0)
-		blkdev_put(resume_bdev, FMODE_WRITE);
+		blkdev_put(sws_resume_bdev, FMODE_WRITE);
 
 	return res;
 }
@@ -308,7 +222,7 @@ static int write_page(void *buf, sector_t offset, struct bio **bio_chain)
 	} else {
 		src = buf;
 	}
-	return bio_write_page(offset, src, bio_chain);
+	return sws_bio_write_page(offset, src, bio_chain);
 }
 
 /*
@@ -379,7 +293,7 @@ static int swap_write_page(struct swap_map_handle *handle, void *buf,
 		return error;
 	handle->cur->entries[handle->k++] = offset;
 	if (handle->k >= MAP_PAGE_ENTRIES) {
-		error = wait_on_bio_chain(bio_chain);
+		error = sws_wait_on_bio_chain(bio_chain);
 		if (error)
 			goto out;
 		offset = alloc_swapdev_block(root_swap);
@@ -440,7 +354,7 @@ static int save_image(struct swap_map_handle *handle,
 			printk(KERN_CONT "\b\b\b\b%3d%%", nr_pages / m);
 		nr_pages++;
 	}
-	err2 = wait_on_bio_chain(&bio);
+	err2 = sws_wait_on_bio_chain(&bio);
 	do_gettimeofday(&stop);
 	if (!ret)
 		ret = err2;
@@ -552,7 +466,7 @@ static int get_swap_reader(struct swap_map_handle *handle, sector_t start)
 	if (!handle->cur)
 		return -ENOMEM;
 
-	error = bio_read_page(start, handle->cur, NULL);
+	error = sws_bio_read_page(start, handle->cur, NULL);
 	if (error) {
 		release_swap_reader(handle);
 		return error;
@@ -572,17 +486,17 @@ static int swap_read_page(struct swap_map_handle *handle, void *buf,
 	offset = handle->cur->entries[handle->k];
 	if (!offset)
 		return -EFAULT;
-	error = bio_read_page(offset, buf, bio_chain);
+	error = sws_bio_read_page(offset, buf, bio_chain);
 	if (error)
 		return error;
 	if (++handle->k >= MAP_PAGE_ENTRIES) {
-		error = wait_on_bio_chain(bio_chain);
+		error = sws_wait_on_bio_chain(bio_chain);
 		handle->k = 0;
 		offset = handle->cur->next_swap;
 		if (!offset)
 			release_swap_reader(handle);
 		else if (!error)
-			error = bio_read_page(offset, handle->cur, NULL);
+			error = sws_bio_read_page(offset, handle->cur, NULL);
 	}
 	return error;
 }
@@ -621,14 +535,14 @@ static int load_image(struct swap_map_handle *handle,
 		if (error)
 			break;
 		if (snapshot->sync_read)
-			error = wait_on_bio_chain(&bio);
+			error = sws_wait_on_bio_chain(&bio);
 		if (error)
 			break;
 		if (!(nr_pages % m))
 			printk("\b\b\b\b%3d%%", nr_pages / m);
 		nr_pages++;
 	}
-	err2 = wait_on_bio_chain(&bio);
+	err2 = sws_wait_on_bio_chain(&bio);
 	do_gettimeofday(&stop);
 	if (!error)
 		error = err2;
@@ -685,11 +599,11 @@ int swsusp_check(void)
 {
 	int error;
 
-	resume_bdev = open_by_devnum(swsusp_resume_device, FMODE_READ);
-	if (!IS_ERR(resume_bdev)) {
-		set_blocksize(resume_bdev, PAGE_SIZE);
+	sws_resume_bdev = open_by_devnum(swsusp_resume_device, FMODE_READ);
+	if (!IS_ERR(sws_resume_bdev)) {
+		set_blocksize(sws_resume_bdev, PAGE_SIZE);
 		memset(swsusp_header, 0, PAGE_SIZE);
-		error = bio_read_page(swsusp_resume_block,
+		error = sws_bio_read_page(swsusp_resume_block,
 					swsusp_header, NULL);
 		if (error)
 			goto put;
@@ -697,7 +611,7 @@ int swsusp_check(void)
 		if (!memcmp(SWSUSP_SIG, swsusp_header->sig, 10)) {
 			memcpy(swsusp_header->sig, swsusp_header->orig_sig, 10);
 			/* Reset swap signature now */
-			error = bio_write_page(swsusp_resume_block,
+			error = sws_bio_write_page(swsusp_resume_block,
 						swsusp_header, NULL);
 		} else {
 			error = -EINVAL;
@@ -705,11 +619,11 @@ int swsusp_check(void)
 
 put:
 		if (error)
-			blkdev_put(resume_bdev, FMODE_READ);
+			blkdev_put(sws_resume_bdev, FMODE_READ);
 		else
 			pr_debug("PM: Signature found, resuming\n");
 	} else {
-		error = PTR_ERR(resume_bdev);
+		error = PTR_ERR(sws_resume_bdev);
 	}
 
 	if (error)
@@ -724,12 +638,12 @@ put:
 
 void swsusp_close(fmode_t mode)
 {
-	if (IS_ERR(resume_bdev)) {
+	if (IS_ERR(sws_resume_bdev)) {
 		pr_debug("PM: Image device not initialised\n");
 		return;
 	}
 
-	blkdev_put(resume_bdev, mode);
+	blkdev_put(sws_resume_bdev, mode);
 }
 
 static int swsusp_header_init(void)
-- 
1.7.0.2

[RFC 04/15] PM / Hibernate: move the first_sector out of swsusp_write

[Jiri Slaby]

From: Jiri Slaby <jirislaby@gmail.com>

The first sector knowledge is swap only specific. Move it into the
handle. This will be needed for later non-swap specific code moving
inside snapshot.c.

Signed-off-by: Jiri Slaby <jslaby@suse.cz>
Cc: Nigel Cunningham <ncunningham@crca.org.au>
Cc: "Rafael J. Wysocki" <rjw@sisk.pl>
---
 kernel/power/swap.c |   76 +++++++++++++++++++++++++-------------------------
 1 files changed, 38 insertions(+), 38 deletions(-)

diff --git a/kernel/power/swap.c b/kernel/power/swap.c
index d4ff0d1..a1cff28 100644
--- a/kernel/power/swap.c
+++ b/kernel/power/swap.c
@@ -28,6 +28,40 @@
 
 #define SWSUSP_SIG	"S1SUSPEND"
 
+/*
+ *	The swap map is a data structure used for keeping track of each page
+ *	written to a swap partition.  It consists of many swap_map_page
+ *	structures that contain each an array of MAP_PAGE_SIZE swap entries.
+ *	These structures are stored on the swap and linked together with the
+ *	help of the .next_swap member.
+ *
+ *	The swap map is created during suspend.  The swap map pages are
+ *	allocated and populated one at a time, so we only need one memory
+ *	page to set up the entire structure.
+ *
+ *	During resume we also only need to use one swap_map_page structure
+ *	at a time.
+ */
+
+#define MAP_PAGE_ENTRIES	(PAGE_SIZE / sizeof(sector_t) - 1)
+
+struct swap_map_page {
+	sector_t entries[MAP_PAGE_ENTRIES];
+	sector_t next_swap;
+};
+
+/**
+ *	The swap_map_handle structure is used for handling swap in
+ *	a file-alike way
+ */
+
+struct swap_map_handle {
+	struct swap_map_page *cur;
+	sector_t cur_swap;
+	sector_t first_sector;
+	unsigned int k;
+};
+
 struct swsusp_header {
 	char reserved[PAGE_SIZE - 20 - sizeof(sector_t) - sizeof(int)];
 	sector_t image;
@@ -150,7 +184,7 @@ struct block_device *sws_resume_bdev;
  * Saving part
  */
 
-static int mark_swapfiles(sector_t start, unsigned int flags)
+static int mark_swapfiles(struct swap_map_handle *handle, unsigned int flags)
 {
 	int error;
 
@@ -159,7 +193,7 @@ static int mark_swapfiles(sector_t start, unsigned int flags)
 	    !memcmp("SWAPSPACE2",swsusp_header->sig, 10)) {
 		memcpy(swsusp_header->orig_sig,swsusp_header->sig, 10);
 		memcpy(swsusp_header->sig,SWSUSP_SIG, 10);
-		swsusp_header->image = start;
+		swsusp_header->image = handle->first_sector;
 		swsusp_header->flags = flags;
 		error = sws_bio_write_page(swsusp_resume_block,
 					swsusp_header, NULL);
@@ -225,39 +259,6 @@ static int write_page(void *buf, sector_t offset, struct bio **bio_chain)
 	return sws_bio_write_page(offset, src, bio_chain);
 }
 
-/*
- *	The swap map is a data structure used for keeping track of each page
- *	written to a swap partition.  It consists of many swap_map_page
- *	structures that contain each an array of MAP_PAGE_SIZE swap entries.
- *	These structures are stored on the swap and linked together with the
- *	help of the .next_swap member.
- *
- *	The swap map is created during suspend.  The swap map pages are
- *	allocated and populated one at a time, so we only need one memory
- *	page to set up the entire structure.
- *
- *	During resume we also only need to use one swap_map_page structure
- *	at a time.
- */
-
-#define MAP_PAGE_ENTRIES	(PAGE_SIZE / sizeof(sector_t) - 1)
-
-struct swap_map_page {
-	sector_t entries[MAP_PAGE_ENTRIES];
-	sector_t next_swap;
-};
-
-/**
- *	The swap_map_handle structure is used for handling swap in
- *	a file-alike way
- */
-
-struct swap_map_handle {
-	struct swap_map_page *cur;
-	sector_t cur_swap;
-	unsigned int k;
-};
-
 static void release_swap_writer(struct swap_map_handle *handle)
 {
 	if (handle->cur)
@@ -276,6 +277,7 @@ static int get_swap_writer(struct swap_map_handle *handle)
 		return -ENOSPC;
 	}
 	handle->k = 0;
+	handle->first_sector = handle->cur_swap;
 	return 0;
 }
 
@@ -420,8 +422,6 @@ int swsusp_write(unsigned int flags)
 	}
 	error = get_swap_writer(&handle);
 	if (!error) {
-		sector_t start = handle.cur_swap;
-
 		error = swap_write_page(&handle, header, NULL);
 		if (!error)
 			error = save_image(&handle, &snapshot,
@@ -430,7 +430,7 @@ int swsusp_write(unsigned int flags)
 		if (!error) {
 			flush_swap_writer(&handle);
 			printk(KERN_INFO "PM: S");
-			error = mark_swapfiles(start, flags);
+			error = mark_swapfiles(&handle, flags);
 			printk("|\n");
 		}
 	}
-- 
1.7.0.2

[RFC 05/15] PM / Hibernate: group swap ops

[Jiri Slaby]

Move all the swap processing into one function. It will make swap
calls from a non-swap code easier.

Signed-off-by: Jiri Slaby <jslaby@suse.cz>
Cc: Nigel Cunningham <ncunningham@crca.org.au>
Cc: "Rafael J. Wysocki" <rjw@sisk.pl>
---
 kernel/power/swap.c |  117 ++++++++++++++++++++++++++++++++-------------------
 1 files changed, 74 insertions(+), 43 deletions(-)

diff --git a/kernel/power/swap.c b/kernel/power/swap.c
index a1cff28..2edf742 100644
--- a/kernel/power/swap.c
+++ b/kernel/power/swap.c
@@ -207,9 +207,10 @@ static int mark_swapfiles(struct swap_map_handle *handle, unsigned int flags)
 /**
  *	swsusp_swap_check - check if the resume device is a swap device
  *	and get its index (if so)
+ *
+ *	This is called before saving image
  */
-
-static int swsusp_swap_check(void) /* This is called before saving image */
+static int swsusp_swap_check(void)
 {
 	int res;
 
@@ -268,17 +269,33 @@ static void release_swap_writer(struct swap_map_handle *handle)
 
 static int get_swap_writer(struct swap_map_handle *handle)
 {
+	int ret;
+
+	ret = swsusp_swap_check();
+	if (ret) {
+		if (ret != -ENOSPC)
+			printk(KERN_ERR "PM: Cannot find swap device, try "
+					"swapon -a.\n");
+		return ret;
+	}
 	handle->cur = (struct swap_map_page *)get_zeroed_page(GFP_KERNEL);
-	if (!handle->cur)
-		return -ENOMEM;
+	if (!handle->cur) {
+		ret = -ENOMEM;
+		goto err_close;
+	}
 	handle->cur_swap = alloc_swapdev_block(root_swap);
 	if (!handle->cur_swap) {
-		release_swap_writer(handle);
-		return -ENOSPC;
+		ret = -ENOSPC;
+		goto err_rel;
 	}
 	handle->k = 0;
 	handle->first_sector = handle->cur_swap;
 	return 0;
+err_rel:
+	release_swap_writer(handle);
+err_close:
+	swsusp_close(FMODE_WRITE);
+	return ret;
 }
 
 static int swap_write_page(struct swap_map_handle *handle, void *buf,
@@ -321,6 +338,24 @@ static int flush_swap_writer(struct swap_map_handle *handle)
 		return -EINVAL;
 }
 
+static int put_swap_writer(struct swap_map_handle *handle,
+		unsigned int flags, int error)
+{
+	if (!error) {
+		flush_swap_writer(handle);
+		printk(KERN_INFO "PM: S");
+		error = mark_swapfiles(handle, flags);
+		printk("|\n");
+	}
+
+	if (error)
+		free_all_swap_pages(root_swap);
+	release_swap_writer(handle);
+	swsusp_close(FMODE_WRITE);
+
+	return error;
+}
+
 /**
  *	save_image - save the suspend image data
  */
@@ -398,48 +433,34 @@ int swsusp_write(unsigned int flags)
 	struct swap_map_handle handle;
 	struct snapshot_handle snapshot;
 	struct swsusp_info *header;
+	unsigned long pages;
 	int error;
 
-	error = swsusp_swap_check();
+	pages = snapshot_get_image_size();
+	error = get_swap_writer(&handle);
 	if (error) {
-		printk(KERN_ERR "PM: Cannot find swap device, try "
-				"swapon -a.\n");
+		printk(KERN_ERR "PM: Cannot get swap writer\n");
 		return error;
 	}
+	if (!enough_swap(pages)) {
+		printk(KERN_ERR "PM: Not enough free swap\n");
+		error = -ENOSPC;
+		goto out_finish;
+	}
 	memset(&snapshot, 0, sizeof(struct snapshot_handle));
 	error = snapshot_read_next(&snapshot);
 	if (error < PAGE_SIZE) {
 		if (error >= 0)
 			error = -EFAULT;
 
-		goto out;
+		goto out_finish;
 	}
 	header = (struct swsusp_info *)data_of(snapshot);
-	if (!enough_swap(header->pages)) {
-		printk(KERN_ERR "PM: Not enough free swap\n");
-		error = -ENOSPC;
-		goto out;
-	}
-	error = get_swap_writer(&handle);
-	if (!error) {
-		error = swap_write_page(&handle, header, NULL);
-		if (!error)
-			error = save_image(&handle, &snapshot,
-					header->pages - 1);
-
-		if (!error) {
-			flush_swap_writer(&handle);
-			printk(KERN_INFO "PM: S");
-			error = mark_swapfiles(&handle, flags);
-			printk("|\n");
-		}
-	}
-	if (error)
-		free_all_swap_pages(root_swap);
-
-	release_swap_writer(&handle);
- out:
-	swsusp_close(FMODE_WRITE);
+	error = swap_write_page(&handle, header, NULL);
+	if (!error)
+		error = save_image(&handle, &snapshot, pages - 1);
+out_finish:
+	error = put_swap_writer(&handle, flags, error);
 	return error;
 }
 
@@ -455,18 +476,21 @@ static void release_swap_reader(struct swap_map_handle *handle)
 	handle->cur = NULL;
 }
 
-static int get_swap_reader(struct swap_map_handle *handle, sector_t start)
+static int get_swap_reader(struct swap_map_handle *handle,
+		unsigned int *flags_p)
 {
 	int error;
 
-	if (!start)
+	*flags_p = swsusp_header->flags;
+
+	if (!swsusp_header->image) /* how can this happen? */
 		return -EINVAL;
 
 	handle->cur = (struct swap_map_page *)get_zeroed_page(__GFP_WAIT | __GFP_HIGH);
 	if (!handle->cur)
 		return -ENOMEM;
 
-	error = sws_bio_read_page(start, handle->cur, NULL);
+	error = sws_bio_read_page(swsusp_header->image, handle->cur, NULL);
 	if (error) {
 		release_swap_reader(handle);
 		return error;
@@ -501,6 +525,13 @@ static int swap_read_page(struct swap_map_handle *handle, void *buf,
 	return error;
 }
 
+static int put_swap_reader(struct swap_map_handle *handle)
+{
+	release_swap_reader(handle);
+
+	return 0;
+}
+
 /**
  *	load_image - load the image using the swap map handle
  *	@handle and the snapshot handle @snapshot
@@ -570,20 +601,20 @@ int swsusp_read(unsigned int *flags_p)
 	struct snapshot_handle snapshot;
 	struct swsusp_info *header;
 
-	*flags_p = swsusp_header->flags;
-
 	memset(&snapshot, 0, sizeof(struct snapshot_handle));
 	error = snapshot_write_next(&snapshot);
 	if (error < PAGE_SIZE)
 		return error < 0 ? error : -EFAULT;
 	header = (struct swsusp_info *)data_of(snapshot);
-	error = get_swap_reader(&handle, swsusp_header->image);
+	error = get_swap_reader(&handle, flags_p);
+	if (error)
+		goto end;
 	if (!error)
 		error = swap_read_page(&handle, header, NULL);
 	if (!error)
 		error = load_image(&handle, &snapshot, header->pages - 1);
-	release_swap_reader(&handle);
-
+	put_swap_reader(&handle);
+end:
 	if (!error)
 		pr_debug("PM: Image successfully loaded\n");
 	else
-- 
1.7.0.2

[RFC 06/15] PM / Hibernate: swap, remove swap_map_handle usages

[Jiri Slaby]

Some code, which will be moved out of swap.c, needs know nothing about
swap. There will be also other than swap writers later, so that it
won't make sense at all.

Make it a global static in swap.c as a singleton.

Signed-off-by: Jiri Slaby <jslaby@suse.cz>
Cc: Nigel Cunningham <ncunningham@crca.org.au>
Cc: "Rafael J. Wysocki" <rjw@sisk.pl>
---
 kernel/power/swap.c |   58 +++++++++++++++++++++++++-------------------------
 1 files changed, 29 insertions(+), 29 deletions(-)

diff --git a/kernel/power/swap.c b/kernel/power/swap.c
index 2edf742..ac1a351 100644
--- a/kernel/power/swap.c
+++ b/kernel/power/swap.c
@@ -70,6 +70,7 @@ struct swsusp_header {
 	char	sig[10];
 } __attribute__((packed));
 
+static struct swap_map_handle swap_map_handle;
 static struct swsusp_header *swsusp_header;
 
 /**
@@ -267,8 +268,9 @@ static void release_swap_writer(struct swap_map_handle *handle)
 	handle->cur = NULL;
 }
 
-static int get_swap_writer(struct swap_map_handle *handle)
+static int get_swap_writer(void)
 {
+	struct swap_map_handle *handle = &swap_map_handle;
 	int ret;
 
 	ret = swsusp_swap_check();
@@ -278,6 +280,7 @@ static int get_swap_writer(struct swap_map_handle *handle)
 					"swapon -a.\n");
 		return ret;
 	}
+	memset(handle, 0, sizeof(*handle));
 	handle->cur = (struct swap_map_page *)get_zeroed_page(GFP_KERNEL);
 	if (!handle->cur) {
 		ret = -ENOMEM;
@@ -298,9 +301,9 @@ err_close:
 	return ret;
 }
 
-static int swap_write_page(struct swap_map_handle *handle, void *buf,
-				struct bio **bio_chain)
+static int swap_write_page(void *buf, struct bio **bio_chain)
 {
+	struct swap_map_handle *handle = &swap_map_handle;
 	int error = 0;
 	sector_t offset;
 
@@ -338,9 +341,10 @@ static int flush_swap_writer(struct swap_map_handle *handle)
 		return -EINVAL;
 }
 
-static int put_swap_writer(struct swap_map_handle *handle,
-		unsigned int flags, int error)
+static int put_swap_writer(unsigned int flags, int error)
 {
+	struct swap_map_handle *handle = &swap_map_handle;
+
 	if (!error) {
 		flush_swap_writer(handle);
 		printk(KERN_INFO "PM: S");
@@ -360,8 +364,7 @@ static int put_swap_writer(struct swap_map_handle *handle,
  *	save_image - save the suspend image data
  */
 
-static int save_image(struct swap_map_handle *handle,
-                      struct snapshot_handle *snapshot,
+static int save_image(struct snapshot_handle *snapshot,
                       unsigned int nr_to_write)
 {
 	unsigned int m;
@@ -384,7 +387,7 @@ static int save_image(struct swap_map_handle *handle,
 		ret = snapshot_read_next(snapshot);
 		if (ret <= 0)
 			break;
-		ret = swap_write_page(handle, data_of(*snapshot), &bio);
+		ret = swap_write_page(data_of(*snapshot), &bio);
 		if (ret)
 			break;
 		if (!(nr_pages % m))
@@ -430,14 +433,13 @@ static int enough_swap(unsigned int nr_pages)
 
 int swsusp_write(unsigned int flags)
 {
-	struct swap_map_handle handle;
 	struct snapshot_handle snapshot;
 	struct swsusp_info *header;
 	unsigned long pages;
 	int error;
 
 	pages = snapshot_get_image_size();
-	error = get_swap_writer(&handle);
+	error = get_swap_writer();
 	if (error) {
 		printk(KERN_ERR "PM: Cannot get swap writer\n");
 		return error;
@@ -456,11 +458,11 @@ int swsusp_write(unsigned int flags)
 		goto out_finish;
 	}
 	header = (struct swsusp_info *)data_of(snapshot);
-	error = swap_write_page(&handle, header, NULL);
+	error = swap_write_page(header, NULL);
 	if (!error)
-		error = save_image(&handle, &snapshot, pages - 1);
+		error = save_image(&snapshot, pages - 1);
 out_finish:
-	error = put_swap_writer(&handle, flags, error);
+	error = put_swap_writer(flags, error);
 	return error;
 }
 
@@ -476,9 +478,9 @@ static void release_swap_reader(struct swap_map_handle *handle)
 	handle->cur = NULL;
 }
 
-static int get_swap_reader(struct swap_map_handle *handle,
-		unsigned int *flags_p)
+static int get_swap_reader(unsigned int *flags_p)
 {
+	struct swap_map_handle *handle = &swap_map_handle;
 	int error;
 
 	*flags_p = swsusp_header->flags;
@@ -486,6 +488,7 @@ static int get_swap_reader(struct swap_map_handle *handle,
 	if (!swsusp_header->image) /* how can this happen? */
 		return -EINVAL;
 
+	memset(handle, 0, sizeof(*handle));
 	handle->cur = (struct swap_map_page *)get_zeroed_page(__GFP_WAIT | __GFP_HIGH);
 	if (!handle->cur)
 		return -ENOMEM;
@@ -499,9 +502,9 @@ static int get_swap_reader(struct swap_map_handle *handle,
 	return 0;
 }
 
-static int swap_read_page(struct swap_map_handle *handle, void *buf,
-				struct bio **bio_chain)
+static int swap_read_page(void *buf, struct bio **bio_chain)
 {
+	struct swap_map_handle *handle = &swap_map_handle;
 	sector_t offset;
 	int error;
 
@@ -525,22 +528,20 @@ static int swap_read_page(struct swap_map_handle *handle, void *buf,
 	return error;
 }
 
-static int put_swap_reader(struct swap_map_handle *handle)
+static int put_swap_reader(void)
 {
-	release_swap_reader(handle);
+	release_swap_reader(&swap_map_handle);
 
 	return 0;
 }
 
 /**
- *	load_image - load the image using the swap map handle
+ *	load_image - load the image
  *	@handle and the snapshot handle @snapshot
  *	(assume there are @nr_pages pages to load)
  */
 
-static int load_image(struct swap_map_handle *handle,
-                      struct snapshot_handle *snapshot,
-                      unsigned int nr_to_read)
+static int load_image(struct snapshot_handle *snapshot, unsigned int nr_to_read)
 {
 	unsigned int m;
 	int error = 0;
@@ -562,7 +563,7 @@ static int load_image(struct swap_map_handle *handle,
 		error = snapshot_write_next(snapshot);
 		if (error <= 0)
 			break;
-		error = swap_read_page(handle, data_of(*snapshot), &bio);
+		error = swap_read_page(data_of(*snapshot), &bio);
 		if (error)
 			break;
 		if (snapshot->sync_read)
@@ -597,7 +598,6 @@ static int load_image(struct swap_map_handle *handle,
 int swsusp_read(unsigned int *flags_p)
 {
 	int error;
-	struct swap_map_handle handle;
 	struct snapshot_handle snapshot;
 	struct swsusp_info *header;
 
@@ -606,14 +606,14 @@ int swsusp_read(unsigned int *flags_p)
 	if (error < PAGE_SIZE)
 		return error < 0 ? error : -EFAULT;
 	header = (struct swsusp_info *)data_of(snapshot);
-	error = get_swap_reader(&handle, flags_p);
+	error = get_swap_reader(flags_p);
 	if (error)
 		goto end;
 	if (!error)
-		error = swap_read_page(&handle, header, NULL);
+		error = swap_read_page(header, NULL);
 	if (!error)
-		error = load_image(&handle, &snapshot, header->pages - 1);
-	put_swap_reader(&handle);
+		error = load_image(&snapshot, header->pages - 1);
+	put_swap_reader();
 end:
 	if (!error)
 		pr_debug("PM: Image successfully loaded\n");
-- 
1.7.0.2

[RFC 07/15] PM / Hibernate: add sws_modules_ops

[Jiri Slaby]

For now they will only hold swap operations. In next patches, user
support will be converted to ops as well to have a single layer and
can push pages instead of pulling them.

Signed-off-by: Jiri Slaby <jslaby@suse.cz>
Cc: Nigel Cunningham <ncunningham@crca.org.au>
Cc: "Rafael J. Wysocki" <rjw@sisk.pl>
---
 kernel/power/hibernate.c |    2 +
 kernel/power/power.h     |   13 +++++++++++
 kernel/power/swap.c      |   51 ++++++++++++++++++++++++++++++---------------
 3 files changed, 49 insertions(+), 17 deletions(-)

diff --git a/kernel/power/hibernate.c b/kernel/power/hibernate.c
index da5288e..762431e 100644
--- a/kernel/power/hibernate.c
+++ b/kernel/power/hibernate.c
@@ -34,6 +34,8 @@ dev_t swsusp_resume_device;
 sector_t swsusp_resume_block;
 int in_suspend __nosavedata = 0;
 
+struct sws_module_ops *sws_io_ops;
+
 enum {
 	HIBERNATION_INVALID,
 	HIBERNATION_PLATFORM,
diff --git a/kernel/power/power.h b/kernel/power/power.h
index 6c4b4fa..0f08de4 100644
--- a/kernel/power/power.h
+++ b/kernel/power/power.h
@@ -115,6 +115,17 @@ struct snapshot_handle {
  */
 #define data_of(handle)	((handle).buffer)
 
+struct sws_module_ops {
+	unsigned long (*storage_available)(void);
+
+	int (*get_reader)(unsigned int *flags_p);
+	int (*put_reader)(void);
+	int (*get_writer)(void);
+	int (*put_writer)(unsigned int flags, int error);
+	int (*read_page)(void *addr, struct bio **bio_chain);
+	int (*write_page)(void *addr, struct bio **bio_chain);
+};
+
 extern unsigned int snapshot_additional_pages(struct zone *zone);
 extern unsigned long snapshot_get_image_size(void);
 extern int snapshot_read_next(struct snapshot_handle *handle);
@@ -213,6 +224,8 @@ enum {
 
 extern int pm_test_level;
 
+extern struct sws_module_ops *sws_io_ops;
+
 #ifdef CONFIG_SUSPEND_FREEZER
 static inline int suspend_freeze_processes(void)
 {
diff --git a/kernel/power/swap.c b/kernel/power/swap.c
index ac1a351..cc79ed1 100644
--- a/kernel/power/swap.c
+++ b/kernel/power/swap.c
@@ -387,7 +387,7 @@ static int save_image(struct snapshot_handle *snapshot,
 		ret = snapshot_read_next(snapshot);
 		if (ret <= 0)
 			break;
-		ret = swap_write_page(data_of(*snapshot), &bio);
+		ret = sws_io_ops->write_page(data_of(*snapshot), &bio);
 		if (ret)
 			break;
 		if (!(nr_pages % m))
@@ -407,18 +407,18 @@ static int save_image(struct snapshot_handle *snapshot,
 }
 
 /**
- *	enough_swap - Make sure we have enough swap to save the image.
+ *	enough_space - Make sure we have enough space to save the image.
  *
- *	Returns TRUE or FALSE after checking the total amount of swap
- *	space avaiable from the resume partition.
+ *	Returns TRUE or FALSE after checking the total amount of
+ *	space avaiable from the resume block.
  */
 
-static int enough_swap(unsigned int nr_pages)
+static int enough_space(unsigned int nr_pages)
 {
-	unsigned int free_swap = count_swap_pages(root_swap, 1);
+	unsigned int free_pages = sws_io_ops->storage_available();
 
-	pr_debug("PM: Free swap pages: %u\n", free_swap);
-	return free_swap > nr_pages + PAGES_FOR_IO;
+	pr_debug("PM: Free storage pages: %u\n", free_pages);
+	return free_pages > nr_pages + PAGES_FOR_IO;
 }
 
 /**
@@ -439,13 +439,13 @@ int swsusp_write(unsigned int flags)
 	int error;
 
 	pages = snapshot_get_image_size();
-	error = get_swap_writer();
+	error = sws_io_ops->get_writer();
 	if (error) {
 		printk(KERN_ERR "PM: Cannot get swap writer\n");
 		return error;
 	}
-	if (!enough_swap(pages)) {
-		printk(KERN_ERR "PM: Not enough free swap\n");
+	if (!enough_space(pages)) {
+		printk(KERN_ERR "PM: Not enough free space for image\n");
 		error = -ENOSPC;
 		goto out_finish;
 	}
@@ -458,14 +458,19 @@ int swsusp_write(unsigned int flags)
 		goto out_finish;
 	}
 	header = (struct swsusp_info *)data_of(snapshot);
-	error = swap_write_page(header, NULL);
+	error = sws_io_ops->write_page(header, NULL);
 	if (!error)
 		error = save_image(&snapshot, pages - 1);
 out_finish:
-	error = put_swap_writer(flags, error);
+	error = sws_io_ops->put_writer(flags, error);
 	return error;
 }
 
+static unsigned long swap_storage_available(void)
+{
+	return count_swap_pages(root_swap, 1);
+}
+
 /**
  *	The following functions allow us to read data using a swap map
  *	in a file-alike way
@@ -535,6 +540,17 @@ static int put_swap_reader(void)
 	return 0;
 }
 
+struct sws_module_ops swap_ops = {
+	.storage_available = swap_storage_available,
+
+	.get_reader = get_swap_reader,
+	.put_reader = put_swap_reader,
+	.get_writer = get_swap_writer,
+	.put_writer = put_swap_writer,
+	.read_page = swap_read_page,
+	.write_page = swap_write_page,
+};
+
 /**
  *	load_image - load the image
  *	@handle and the snapshot handle @snapshot
@@ -563,7 +579,7 @@ static int load_image(struct snapshot_handle *snapshot, unsigned int nr_to_read)
 		error = snapshot_write_next(snapshot);
 		if (error <= 0)
 			break;
-		error = swap_read_page(data_of(*snapshot), &bio);
+		error = sws_io_ops->read_page(data_of(*snapshot), &bio);
 		if (error)
 			break;
 		if (snapshot->sync_read)
@@ -606,14 +622,14 @@ int swsusp_read(unsigned int *flags_p)
 	if (error < PAGE_SIZE)
 		return error < 0 ? error : -EFAULT;
 	header = (struct swsusp_info *)data_of(snapshot);
-	error = get_swap_reader(flags_p);
+	error = sws_io_ops->get_reader(flags_p);
 	if (error)
 		goto end;
 	if (!error)
-		error = swap_read_page(header, NULL);
+		error = sws_io_ops->read_page(header, NULL);
 	if (!error)
 		error = load_image(&snapshot, header->pages - 1);
-	put_swap_reader();
+	sws_io_ops->put_reader();
 end:
 	if (!error)
 		pr_debug("PM: Image successfully loaded\n");
@@ -682,6 +698,7 @@ static int swsusp_header_init(void)
 	swsusp_header = (struct swsusp_header*) __get_free_page(GFP_KERNEL);
 	if (!swsusp_header)
 		panic("Could not allocate memory for swsusp_header\n");
+	sws_io_ops = &swap_ops;
 	return 0;
 }
 
-- 
1.7.0.2

[RFC 08/15] PM / Hibernate: add user module_ops

[Jiri Slaby]

Add sws_module_ops into /dev/snapshot user interface, so far only with
.storage_available. The structure will be shortly filled while converting
it to ops.

Also, when using this interface, switch to the ops on open/release.

Signed-off-by: Jiri Slaby <jslaby@suse.cz>
Cc: Nigel Cunningham <ncunningham@crca.org.au>
Cc: "Rafael J. Wysocki" <rjw@sisk.pl>
---
 kernel/power/user.c |   15 +++++++++++++++
 1 files changed, 15 insertions(+), 0 deletions(-)

diff --git a/kernel/power/user.c b/kernel/power/user.c
index 68a99c1..20bf34c 100644
--- a/kernel/power/user.c
+++ b/kernel/power/user.c
@@ -61,10 +61,20 @@ static struct snapshot_data {
 	char frozen;
 	char ready;
 	char platform_support;
+	struct sws_module_ops *prev_ops;
 } snapshot_state;
 
 atomic_t snapshot_device_available = ATOMIC_INIT(1);
 
+static unsigned long user_storage_available(void)
+{
+	return ~0UL; /* we have no idea, maybe we will fail later */
+}
+
+struct sws_module_ops user_ops = {
+	.storage_available = user_storage_available,
+};
+
 static int snapshot_open(struct inode *inode, struct file *filp)
 {
 	struct snapshot_data *data;
@@ -115,6 +125,10 @@ static int snapshot_open(struct inode *inode, struct file *filp)
 	}
 	if (error)
 		atomic_inc(&snapshot_device_available);
+	else {
+		data->prev_ops = sws_io_ops;
+		sws_io_ops = &user_ops;
+	}
 	data->frozen = 0;
 	data->ready = 0;
 	data->platform_support = 0;
@@ -139,6 +153,7 @@ static int snapshot_release(struct inode *inode, struct file *filp)
 		thaw_processes();
 	pm_notifier_call_chain(data->mode == O_WRONLY ?
 			PM_POST_HIBERNATION : PM_POST_RESTORE);
+	sws_io_ops = data->prev_ops;
 	atomic_inc(&snapshot_device_available);
 
 	mutex_unlock(&pm_mutex);
-- 
1.7.0.2

Re: [RFC 08/15] PM / Hibernate: add user module_ops

[Rafael J. Wysocki]

On Tuesday 23 March 2010, Jiri Slaby wrote:
> Add sws_module_ops into /dev/snapshot user interface, so far only with
> .storage_available. The structure will be shortly filled while converting
> it to ops.
> 
> Also, when using this interface, switch to the ops on open/release.

Quite frankly, I don't really understand the purpose of this change in this
particular form.  It looks like it may be folded into one of the subsequent
changes just fine.

Rafael


> Signed-off-by: Jiri Slaby <jslaby@suse.cz>
> Cc: Nigel Cunningham <ncunningham@crca.org.au>
> Cc: "Rafael J. Wysocki" <rjw@sisk.pl>
> ---
>  kernel/power/user.c |   15 +++++++++++++++
>  1 files changed, 15 insertions(+), 0 deletions(-)
> 
> diff --git a/kernel/power/user.c b/kernel/power/user.c
> index 68a99c1..20bf34c 100644
> --- a/kernel/power/user.c
> +++ b/kernel/power/user.c
> @@ -61,10 +61,20 @@ static struct snapshot_data {
>  	char frozen;
>  	char ready;
>  	char platform_support;
> +	struct sws_module_ops *prev_ops;
>  } snapshot_state;
>  
>  atomic_t snapshot_device_available = ATOMIC_INIT(1);
>  
> +static unsigned long user_storage_available(void)
> +{
> +	return ~0UL; /* we have no idea, maybe we will fail later */
> +}
> +
> +struct sws_module_ops user_ops = {
> +	.storage_available = user_storage_available,
> +};
> +
>  static int snapshot_open(struct inode *inode, struct file *filp)
>  {
>  	struct snapshot_data *data;
> @@ -115,6 +125,10 @@ static int snapshot_open(struct inode *inode, struct file *filp)
>  	}
>  	if (error)
>  		atomic_inc(&snapshot_device_available);
> +	else {
> +		data->prev_ops = sws_io_ops;
> +		sws_io_ops = &user_ops;
> +	}
>  	data->frozen = 0;
>  	data->ready = 0;
>  	data->platform_support = 0;
> @@ -139,6 +153,7 @@ static int snapshot_release(struct inode *inode, struct file *filp)
>  		thaw_processes();
>  	pm_notifier_call_chain(data->mode == O_WRONLY ?
>  			PM_POST_HIBERNATION : PM_POST_RESTORE);
> +	sws_io_ops = data->prev_ops;
>  	atomic_inc(&snapshot_device_available);
>  
>  	mutex_unlock(&pm_mutex);
> 

Re: [RFC 08/15] PM / Hibernate: add user module_ops

[Jiri Slaby]

On 03/25/2010 11:07 PM, Rafael J. Wysocki wrote:
> On Tuesday 23 March 2010, Jiri Slaby wrote:
>> Add sws_module_ops into /dev/snapshot user interface, so far only with
>> .storage_available. The structure will be shortly filled while converting
>> it to ops.
>>
>> Also, when using this interface, switch to the ops on open/release.
> 
> Quite frankly, I don't really understand the purpose of this change in this
> particular form.  It looks like it may be folded into one of the subsequent
> changes just fine.

Yup, maybe the single-change-per-patch approach was overzealous here.
Squashed to 09/15.

thanks,
-- 
js

[RFC 09/15] PM / Hibernate: user, implement user_ops writer

[Jiri Slaby]

Switch /dev/snapshot writer to sws_module_ops approach so that we
can transparently rewrite the rest of the snapshot from pages pulling
to their pushing through layers.

Signed-off-by: Jiri Slaby <jslaby@suse.cz>
Cc: Nigel Cunningham <ncunningham@crca.org.au>
Cc: "Rafael J. Wysocki" <rjw@sisk.pl>
---
 kernel/power/user.c |  113 +++++++++++++++++++++++++++++++++++++++++++++++----
 1 files changed, 105 insertions(+), 8 deletions(-)

diff --git a/kernel/power/user.c b/kernel/power/user.c
index 20bf34c..748567d 100644
--- a/kernel/power/user.c
+++ b/kernel/power/user.c
@@ -66,15 +66,82 @@ static struct snapshot_data {
 
 atomic_t snapshot_device_available = ATOMIC_INIT(1);
 
+static void *to_do_buf;
+static struct workqueue_struct *suspend_worker;
+static DECLARE_WAIT_QUEUE_HEAD(to_do_wait);
+static DECLARE_WAIT_QUEUE_HEAD(to_do_done);
+static DECLARE_BITMAP(to_do_flags, 10);
+
+#define TODO_WORK	1
+#define TODO_FINISH	2
+#define TODO_CLOSED	3
+#define TODO_ERROR	4
+
 static unsigned long user_storage_available(void)
 {
 	return ~0UL; /* we have no idea, maybe we will fail later */
 }
 
+static int get_user_writer(void)
+{
+	return 0;
+}
+
+static int user_write_page(void *buf, struct bio **bio_chain)
+{
+	int err = 0;
+
+	if (test_bit(TODO_CLOSED, to_do_flags))
+		return -EIO;
+
+	to_do_buf = buf;
+	wmb();
+	set_bit(TODO_WORK, to_do_flags);
+	wake_up_interruptible(&to_do_wait);
+
+	wait_event(to_do_done, !test_bit(TODO_WORK, to_do_flags) ||
+			(err = test_bit(TODO_CLOSED, to_do_flags)));
+
+	return err ? -EIO : 0;
+}
+
+static int put_user_writer(unsigned int flags, int error)
+{
+	int err = 0;
+
+	if (error)
+		set_bit(TODO_ERROR, to_do_flags);
+	set_bit(TODO_FINISH, to_do_flags);
+	wake_up_interruptible(&to_do_wait);
+
+	wait_event(to_do_done, !test_bit(TODO_FINISH, to_do_flags) ||
+			(err = test_bit(TODO_CLOSED, to_do_flags)));
+
+	if (!error && err)
+		error = -EIO;
+
+	return error;
+}
+
 struct sws_module_ops user_ops = {
 	.storage_available = user_storage_available,
+
+	.get_writer = get_user_writer,
+	.put_writer = put_user_writer,
+	.write_page = user_write_page,
 };
 
+static void snapshot_writer(struct work_struct *work)
+{
+	int ret;
+
+	ret = swsusp_write(0);
+	if (ret)
+		printk(KERN_ERR "PM: write failed with %d\n", ret);
+}
+
+static DECLARE_WORK(snapshot_writer_w, snapshot_writer);
+
 static int snapshot_open(struct inode *inode, struct file *filp)
 {
 	struct snapshot_data *data;
@@ -132,6 +199,7 @@ static int snapshot_open(struct inode *inode, struct file *filp)
 	data->frozen = 0;
 	data->ready = 0;
 	data->platform_support = 0;
+	memset(to_do_flags, 0, sizeof(*to_do_flags));
 
  Unlock:
 	mutex_unlock(&pm_mutex);
@@ -145,6 +213,10 @@ static int snapshot_release(struct inode *inode, struct file *filp)
 
 	mutex_lock(&pm_mutex);
 
+	set_bit(TODO_CLOSED, to_do_flags);
+	wake_up(&to_do_done);
+	flush_workqueue(suspend_worker);
+
 	swsusp_free();
 	free_basic_memory_bitmaps();
 	data = filp->private_data;
@@ -167,6 +239,7 @@ static ssize_t snapshot_read(struct file *filp, char __user *buf,
 	struct snapshot_data *data;
 	ssize_t res;
 	loff_t pg_offp = *offp & ~PAGE_MASK;
+	int fin = 0;
 
 	mutex_lock(&pm_mutex);
 
@@ -176,17 +249,29 @@ static ssize_t snapshot_read(struct file *filp, char __user *buf,
 		goto Unlock;
 	}
 	if (!pg_offp) { /* on page boundary? */
-		res = snapshot_read_next(&data->handle);
-		if (res <= 0)
+		res = wait_event_interruptible(to_do_wait,
+			test_bit(TODO_WORK, to_do_flags) ||
+			(fin = test_and_clear_bit(TODO_FINISH, to_do_flags)));
+		if (res)
 			goto Unlock;
-	} else
-		res = PAGE_SIZE - pg_offp;
+		if (test_bit(TODO_ERROR, to_do_flags)) {
+			res = -EIO;
+			goto Unlock;
+		}
+		if (fin)
+			goto wake;
+	}
+	res = PAGE_SIZE - pg_offp;
 
-	res = simple_read_from_buffer(buf, count, &pg_offp,
-			data_of(data->handle), res);
+	res = simple_read_from_buffer(buf, count, &pg_offp, to_do_buf, res);
 	if (res > 0)
 		*offp += res;
 
+	if (!(pg_offp & ~PAGE_MASK)) {
+		clear_bit(TODO_WORK, to_do_flags);
+wake:
+		wake_up(&to_do_done);
+	}
  Unlock:
 	mutex_unlock(&pm_mutex);
 
@@ -291,8 +376,11 @@ static long snapshot_ioctl(struct file *filp, unsigned int cmd,
 		error = hibernation_snapshot(data->platform_support);
 		if (!error)
 			error = put_user(in_suspend, (int __user *)arg);
-		if (!error)
+		if (!error) {
+			if (in_suspend)
+				queue_work(suspend_worker, &snapshot_writer_w);
 			data->ready = 1;
+		}
 		break;
 
 	case SNAPSHOT_ATOMIC_RESTORE:
@@ -486,7 +574,16 @@ static struct miscdevice snapshot_device = {
 
 static int __init snapshot_device_init(void)
 {
-	return misc_register(&snapshot_device);
+	int ret;
+
+	suspend_worker = create_singlethread_workqueue("suspend_worker");
+	if (!suspend_worker)
+		return -ENOMEM;
+
+	ret = misc_register(&snapshot_device);
+	if (ret)
+		destroy_workqueue(suspend_worker);
+	return ret;
 };
 
 device_initcall(snapshot_device_init);
-- 
1.7.0.2

[RFC 10/15] PM / Hibernate: user, implement user_ops reader

[Jiri Slaby]

Switch /dev/snapshot reader to sws_module_ops approach so that we
can transparently rewrite the rest of the snapshot from pages pulling
to their pushing through layers.

Signed-off-by: Jiri Slaby <jslaby@suse.cz>
Cc: Nigel Cunningham <ncunningham@crca.org.au>
Cc: "Rafael J. Wysocki" <rjw@sisk.pl>
---
 kernel/power/user.c |   73 +++++++++++++++++++++++++++++++++++++++++++++-----
 1 files changed, 65 insertions(+), 8 deletions(-)

diff --git a/kernel/power/user.c b/kernel/power/user.c
index 748567d..1b5d2e1 100644
--- a/kernel/power/user.c
+++ b/kernel/power/user.c
@@ -76,6 +76,7 @@ static DECLARE_BITMAP(to_do_flags, 10);
 #define TODO_FINISH	2
 #define TODO_CLOSED	3
 #define TODO_ERROR	4
+#define TODO_RD_RUNNING	5
 
 static unsigned long user_storage_available(void)
 {
@@ -123,12 +124,41 @@ static int put_user_writer(unsigned int flags, int error)
 	return error;
 }
 
+static int get_user_reader(unsigned int *flags_p)
+{
+	return 0;
+}
+
+static int user_read_page(void *addr, struct bio **bio_chain)
+{
+	int err = 0;
+
+	to_do_buf = addr;
+	wmb();
+	set_bit(TODO_WORK, to_do_flags);
+	wake_up_interruptible(&to_do_wait);
+
+	wait_event(to_do_done, !test_bit(TODO_WORK, to_do_flags) ||
+			(err = test_bit(TODO_CLOSED, to_do_flags)));
+
+	return err ? -EIO : 0;
+}
+
+static int put_user_reader(void)
+{
+	return 0;
+}
+
 struct sws_module_ops user_ops = {
 	.storage_available = user_storage_available,
 
 	.get_writer = get_user_writer,
 	.put_writer = put_user_writer,
 	.write_page = user_write_page,
+
+	.get_reader = get_user_reader,
+	.put_reader = put_user_reader,
+	.read_page = user_read_page,
 };
 
 static void snapshot_writer(struct work_struct *work)
@@ -142,6 +172,22 @@ static void snapshot_writer(struct work_struct *work)
 
 static DECLARE_WORK(snapshot_writer_w, snapshot_writer);
 
+static void snapshot_reader(struct work_struct *work)
+{
+	int ret;
+
+	set_bit(TODO_RD_RUNNING, to_do_flags);
+	ret = swsusp_read(NULL);
+	if (ret) {
+		printk(KERN_ERR "PM: read failed with %d\n", ret);
+		set_bit(TODO_ERROR, to_do_flags);
+	}
+	clear_bit(TODO_RD_RUNNING, to_do_flags);
+	wake_up_interruptible(&to_do_wait);
+}
+
+static DECLARE_WORK(snapshot_reader_w, snapshot_reader);
+
 static int snapshot_open(struct inode *inode, struct file *filp)
 {
 	struct snapshot_data *data;
@@ -287,19 +333,27 @@ static ssize_t snapshot_write(struct file *filp, const char __user *buf,
 
 	mutex_lock(&pm_mutex);
 
+	if (!test_bit(TODO_RD_RUNNING, to_do_flags))
+		queue_work(suspend_worker, &snapshot_reader_w);
+
 	data = filp->private_data;
 
 	if (!pg_offp) {
-		res = snapshot_write_next(&data->handle);
-		if (res <= 0)
+		res = wait_event_interruptible(to_do_wait,
+				test_bit(TODO_WORK, to_do_flags));
+		if (res)
 			goto unlock;
-	} else
-		res = PAGE_SIZE - pg_offp;
+	}
+	res = PAGE_SIZE - pg_offp;
 
-	res = simple_write_to_buffer(data_of(data->handle), res, &pg_offp,
-			buf, count);
+	res = simple_write_to_buffer(to_do_buf, res, &pg_offp, buf, count);
 	if (res > 0)
 		*offp += res;
+
+	if (!(pg_offp & ~PAGE_MASK)) {
+		clear_bit(TODO_WORK, to_do_flags);
+		wake_up(&to_do_done);
+	}
 unlock:
 	mutex_unlock(&pm_mutex);
 
@@ -384,9 +438,12 @@ static long snapshot_ioctl(struct file *filp, unsigned int cmd,
 		break;
 
 	case SNAPSHOT_ATOMIC_RESTORE:
-		snapshot_write_finalize(&data->handle);
+		error = wait_event_interruptible(to_do_wait,
+				!test_bit(TODO_RD_RUNNING, to_do_flags));
+		if (error)
+			break;
 		if (data->mode != O_WRONLY || !data->frozen ||
-		    !snapshot_image_loaded(&data->handle)) {
+				test_bit(TODO_ERROR, to_do_flags)) {
 			error = -EPERM;
 			break;
 		}
-- 
1.7.0.2

[RFC 11/15] PM / Hibernate: add chunk i/o support

[Jiri Slaby]

Chunk support is useful when not writing whole pages at once. It takes
care of joining the buffers into the pages and writing at once when
needed.

In the end when pages are compressed they use this interface as well
(because they are indeed shorter than PAGE_SIZE).

Signed-off-by: Jiri Slaby <jslaby@suse.cz>
Cc: Nigel Cunningham <ncunningham@crca.org.au>
Cc: "Rafael J. Wysocki" <rjw@sisk.pl>
---
 kernel/power/block_io.c |   81 +++++++++++++++++++++++++++++++++++++++++++++++
 kernel/power/power.h    |    6 +++
 2 files changed, 87 insertions(+), 0 deletions(-)

diff --git a/kernel/power/block_io.c b/kernel/power/block_io.c
index 2b7898a..37412f7 100644
--- a/kernel/power/block_io.c
+++ b/kernel/power/block_io.c
@@ -3,6 +3,7 @@
  *
  * Copyright (C) 1998,2001-2005 Pavel Machek <pavel@suse.cz>
  * Copyright (C) 2006 Rafael J. Wysocki <rjw@sisk.pl>
+ * Copyright (C) 2004-2008 Nigel Cunningham (nigel at tuxonice net)
  *
  * This file is released under the GPLv2.
  */
@@ -14,6 +15,9 @@
 
 #include "power.h"
 
+static char *sws_writer_buffer;
+static unsigned long sws_writer_buffer_pos;
+
 /**
  *	submit - submit BIO request.
  *	@rw:	READ or WRITE.
@@ -74,6 +78,83 @@ int sws_bio_write_page(pgoff_t page_off, void *addr, struct bio **bio_chain)
 			virt_to_page(addr), bio_chain);
 }
 
+int sws_rw_buffer_init(int writing)
+{
+	BUG_ON(sws_writer_buffer || sws_writer_buffer_pos);
+
+	sws_writer_buffer = (void *)get_zeroed_page(__GFP_WAIT | __GFP_HIGH);
+	if (sws_writer_buffer && !writing)
+		sws_writer_buffer_pos = PAGE_SIZE;
+
+	return sws_writer_buffer ? 0 : -ENOMEM;
+}
+
+/**
+ * sws_rw_buffer - combine smaller buffers into PAGE_SIZE I/O
+ * @writing:		Bool - whether writing (or reading).
+ * @buffer:		The start of the buffer to write or fill.
+ * @buffer_size:	The size of the buffer to write or fill.
+ **/
+int sws_rw_buffer(int writing, char *buffer, int buffer_size)
+{
+	int bytes_left = buffer_size, ret;
+
+	while (bytes_left) {
+		char *source_start = buffer + buffer_size - bytes_left;
+		char *dest_start = sws_writer_buffer + sws_writer_buffer_pos;
+		int capacity = PAGE_SIZE - sws_writer_buffer_pos;
+		char *to = writing ? dest_start : source_start;
+		char *from = writing ? source_start : dest_start;
+
+		if (bytes_left <= capacity) {
+			memcpy(to, from, bytes_left);
+			sws_writer_buffer_pos += bytes_left;
+			return 0;
+		}
+
+		/* Complete this page and start a new one */
+		memcpy(to, from, capacity);
+		bytes_left -= capacity;
+
+		if (writing) {
+			ret = sws_io_ops->write_page(sws_writer_buffer, NULL);
+			clear_page(sws_writer_buffer);
+			if (ret)
+				return ret;
+		} else {
+			ret = sws_io_ops->read_page(sws_writer_buffer, NULL);
+			if (ret)
+				return ret;
+		}
+
+		sws_writer_buffer_pos = 0;
+	}
+
+	return 0;
+}
+
+int sws_rw_buffer_flush_page(int writing)
+{
+	int ret = 0;
+	if (writing && sws_writer_buffer_pos)
+		ret = sws_io_ops->write_page(sws_writer_buffer, NULL);
+	sws_writer_buffer_pos = writing ? 0 : PAGE_SIZE;
+	return ret;
+}
+
+int sws_rw_buffer_finish(int writing)
+{
+	int ret = 0;
+
+	ret = sws_rw_buffer_flush_page(writing);
+
+	free_page((unsigned long)sws_writer_buffer);
+	sws_writer_buffer = NULL;
+	sws_writer_buffer_pos = 0;
+
+	return ret;
+}
+
 int sws_wait_on_bio_chain(struct bio **bio_chain)
 {
 	struct bio *bio;
diff --git a/kernel/power/power.h b/kernel/power/power.h
index 0f08de4..50a888a 100644
--- a/kernel/power/power.h
+++ b/kernel/power/power.h
@@ -3,6 +3,8 @@
 #include <linux/utsname.h>
 #include <linux/freezer.h>
 
+struct swap_map_handle;
+
 struct swsusp_info {
 	struct new_utsname	uts;
 	u32			version_code;
@@ -161,6 +163,10 @@ extern int sws_bio_read_page(pgoff_t page_off, void *addr,
 extern int sws_bio_write_page(pgoff_t page_off, void *addr,
 		struct bio **bio_chain);
 extern int sws_wait_on_bio_chain(struct bio **bio_chain);
+extern int sws_rw_buffer_init(int writing);
+extern int sws_rw_buffer_finish(int writing);
+extern int sws_rw_buffer_flush_page(int writing);
+extern int sws_rw_buffer(int writing, char *buffer, int buffer_size);
 
 struct timeval;
 /* kernel/power/swsusp.c */
-- 
1.7.0.2

[RFC 12/15] PM / Hibernate: split snapshot_read_next

[Jiri Slaby]

When writing the snapshot, do the initialization and header write in
a separate function. This makes the code more readable and lowers
complexity of snapshot_read_next.

Signed-off-by: Jiri Slaby <jslaby@suse.cz>
Cc: Nigel Cunningham <ncunningham@crca.org.au>
Cc: "Rafael J. Wysocki" <rjw@sisk.pl>
---
 kernel/power/power.h    |    1 +
 kernel/power/snapshot.c |   63 +++++++++++++++++++++++++++++-----------------
 kernel/power/swap.c     |   14 +++-------
 3 files changed, 45 insertions(+), 33 deletions(-)

diff --git a/kernel/power/power.h b/kernel/power/power.h
index 50a888a..638a97c 100644
--- a/kernel/power/power.h
+++ b/kernel/power/power.h
@@ -130,6 +130,7 @@ struct sws_module_ops {
 
 extern unsigned int snapshot_additional_pages(struct zone *zone);
 extern unsigned long snapshot_get_image_size(void);
+extern int snapshot_write_init(struct snapshot_handle *handle);
 extern int snapshot_read_next(struct snapshot_handle *handle);
 extern int snapshot_write_next(struct snapshot_handle *handle);
 extern void snapshot_write_finalize(struct snapshot_handle *handle);
diff --git a/kernel/power/snapshot.c b/kernel/power/snapshot.c
index 7918351..c8864de 100644
--- a/kernel/power/snapshot.c
+++ b/kernel/power/snapshot.c
@@ -1597,10 +1597,44 @@ pack_pfns(unsigned long *buf, struct memory_bitmap *bm)
 }
 
 /**
+ *	snapshot_write_init - initialization before writing the snapshot to
+ *	a backing storage
+ *
+ *	This function *must* be called before snapshot_read_next to initialize
+ *	@handle and write a header.
+ *
+ *	@handle: snapshot handle to init
+ */
+int snapshot_write_init(struct snapshot_handle *handle)
+{
+	int ret;
+
+	/* This makes the buffer be freed by swsusp_free() */
+	buffer = get_image_page(GFP_ATOMIC, PG_ANY);
+	if (!buffer)
+		return -ENOMEM;
+	init_header(buffer);
+	ret = sws_rw_buffer_init(1);
+	if (ret)
+		return ret;
+	ret = sws_rw_buffer(1, buffer, sizeof(struct swsusp_info));
+	if (ret)
+		goto finish;
+	sws_rw_buffer_finish(1);
+	memory_bm_position_reset(&orig_bm);
+	memory_bm_position_reset(&copy_bm);
+	handle->buffer = buffer;
+	return 0;
+finish:
+	sws_rw_buffer_finish(1);
+	return ret;
+}
+
+/**
  *	snapshot_read_next - used for reading the system memory snapshot.
  *
- *	On the first call to it @handle should point to a zeroed
- *	snapshot_handle structure.  The structure gets updated and a pointer
+ *	Before calling this function, snapshot_write_init has to be called with
+ *	handle passed as @handle here. The structure gets updated and a pointer
  *	to it should be passed to this function every next time.
  *
  *	On success the function returns a positive number.  Then, the caller
@@ -1612,31 +1646,12 @@ pack_pfns(unsigned long *buf, struct memory_bitmap *bm)
  *	structure pointed to by @handle is not updated and should not be used
  *	any more.
  */
-
 int snapshot_read_next(struct snapshot_handle *handle)
 {
-	if (handle->cur > nr_meta_pages + nr_copy_pages)
-		return 0;
-
-	if (!buffer) {
-		/* This makes the buffer be freed by swsusp_free() */
-		buffer = get_image_page(GFP_ATOMIC, PG_ANY);
-		if (!buffer)
-			return -ENOMEM;
-	}
-	if (!handle->cur) {
-		int error;
-
-		error = init_header((struct swsusp_info *)buffer);
-		if (error)
-			return error;
-		handle->buffer = buffer;
-		memory_bm_position_reset(&orig_bm);
-		memory_bm_position_reset(&copy_bm);
-	} else if (handle->cur <= nr_meta_pages) {
+	if (handle->cur < nr_meta_pages) {
 		memset(buffer, 0, PAGE_SIZE);
 		pack_pfns(buffer, &orig_bm);
-	} else {
+	} else if (handle->cur < nr_meta_pages + nr_copy_pages) {
 		struct page *page;
 
 		page = pfn_to_page(memory_bm_next_pfn(&copy_bm));
@@ -1654,6 +1669,8 @@ int snapshot_read_next(struct snapshot_handle *handle)
 		} else {
 			handle->buffer = page_address(page);
 		}
+	} else {
+		return 0;
 	}
 	handle->cur++;
 	return PAGE_SIZE;
diff --git a/kernel/power/swap.c b/kernel/power/swap.c
index cc79ed1..1b0ed28 100644
--- a/kernel/power/swap.c
+++ b/kernel/power/swap.c
@@ -434,7 +434,6 @@ static int enough_space(unsigned int nr_pages)
 int swsusp_write(unsigned int flags)
 {
 	struct snapshot_handle snapshot;
-	struct swsusp_info *header;
 	unsigned long pages;
 	int error;
 
@@ -450,17 +449,12 @@ int swsusp_write(unsigned int flags)
 		goto out_finish;
 	}
 	memset(&snapshot, 0, sizeof(struct snapshot_handle));
-	error = snapshot_read_next(&snapshot);
-	if (error < PAGE_SIZE) {
-		if (error >= 0)
-			error = -EFAULT;
-
+	error = snapshot_write_init(&snapshot);
+	if (error) {
+		printk(KERN_ERR "PM: Cannot init writer\n");
 		goto out_finish;
 	}
-	header = (struct swsusp_info *)data_of(snapshot);
-	error = sws_io_ops->write_page(header, NULL);
-	if (!error)
-		error = save_image(&snapshot, pages - 1);
+	error = save_image(&snapshot, pages - 1);
 out_finish:
 	error = sws_io_ops->put_writer(flags, error);
 	return error;
-- 
1.7.0.2

[RFC 13/15] PM / Hibernate: split snapshot_write_next

[Jiri Slaby]

When reading the snapshot, do the initialization and header read in
a separate function. This makes the code more readable and lowers
complexity of snapshot_write_next.

Signed-off-by: Jiri Slaby <jslaby@suse.cz>
Cc: Nigel Cunningham <ncunningham@crca.org.au>
Cc: "Rafael J. Wysocki" <rjw@sisk.pl>
---
 kernel/power/power.h    |    1 +
 kernel/power/snapshot.c |   98 +++++++++++++++++++++++++++++-----------------
 kernel/power/swap.c     |   19 +++------
 3 files changed, 70 insertions(+), 48 deletions(-)

diff --git a/kernel/power/power.h b/kernel/power/power.h
index 638a97c..842d27b 100644
--- a/kernel/power/power.h
+++ b/kernel/power/power.h
@@ -130,6 +130,7 @@ struct sws_module_ops {
 
 extern unsigned int snapshot_additional_pages(struct zone *zone);
 extern unsigned long snapshot_get_image_size(void);
+extern int snapshot_read_init(struct snapshot_handle *handle);
 extern int snapshot_write_init(struct snapshot_handle *handle);
 extern int snapshot_read_next(struct snapshot_handle *handle);
 extern int snapshot_write_next(struct snapshot_handle *handle);
diff --git a/kernel/power/snapshot.c b/kernel/power/snapshot.c
index c8864de..d432e87 100644
--- a/kernel/power/snapshot.c
+++ b/kernel/power/snapshot.c
@@ -2126,10 +2126,52 @@ static void *get_buffer(struct memory_bitmap *bm, struct chain_allocator *ca)
 }
 
 /**
+ *	snapshot_read_init - initialization before reading the snapshot from
+ *	a backing storage
+ *
+ *	This function *must* be called before snapshot_write_next to initialize
+ *	@handle and read header.
+ *
+ *	@handle: snapshot handle to init
+ */
+int snapshot_read_init(struct snapshot_handle *handle)
+{
+	int ret;
+
+	/* This makes the buffer be freed by swsusp_free() */
+	buffer = get_image_page(GFP_ATOMIC, PG_ANY);
+	if (!buffer)
+		return -ENOMEM;
+
+	ret = sws_rw_buffer_init(0);
+	if (ret)
+		return ret;
+	ret = sws_rw_buffer(0, buffer, sizeof(struct swsusp_info));
+	if (ret)
+		goto finish;
+	sws_rw_buffer_finish(0);
+
+	ret = load_header(buffer);
+	if (ret)
+		return ret;
+
+	ret = memory_bm_create(&copy_bm, GFP_ATOMIC, PG_ANY);
+	if (ret)
+		return ret;
+
+	handle->buffer = buffer;
+	handle->sync_read = 1;
+	return 0;
+finish:
+	sws_rw_buffer_finish(0);
+	return ret;
+}
+
+/**
  *	snapshot_write_next - used for writing the system memory snapshot.
  *
- *	On the first call to it @handle should point to a zeroed
- *	snapshot_handle structure.  The structure gets updated and a pointer
+ *	Before calling this function, snapshot_read_init has to be called with
+ *	handle passed as @handle here. The structure gets updated and a pointer
  *	to it should be passed to this function every next time.
  *
  *	On success the function returns a positive number.  Then, the caller
@@ -2141,42 +2183,20 @@ static void *get_buffer(struct memory_bitmap *bm, struct chain_allocator *ca)
  *	structure pointed to by @handle is not updated and should not be used
  *	any more.
  */
-
 int snapshot_write_next(struct snapshot_handle *handle)
 {
 	static struct chain_allocator ca;
 	int error = 0;
 
-	/* Check if we have already loaded the entire image */
-	if (handle->cur > 1 && handle->cur > nr_meta_pages + nr_copy_pages)
-		return 0;
-
 	handle->sync_read = 1;
-
-	if (!handle->cur) {
-		if (!buffer)
-			/* This makes the buffer be freed by swsusp_free() */
-			buffer = get_image_page(GFP_ATOMIC, PG_ANY);
-
-		if (!buffer)
-			return -ENOMEM;
-
-		handle->buffer = buffer;
-	} else if (handle->cur == 1) {
-		error = load_header(buffer);
-		if (error)
-			return error;
-
-		error = memory_bm_create(&copy_bm, GFP_ATOMIC, PG_ANY);
-		if (error)
-			return error;
-
-	} else if (handle->cur <= nr_meta_pages + 1) {
+	if (handle->cur < nr_meta_pages) {
 		error = unpack_orig_pfns(buffer, &copy_bm);
 		if (error)
 			return error;
 
-		if (handle->cur == nr_meta_pages + 1) {
+		/* well, this was the last meta page
+		   prepare for ordinary pages */
+		if (handle->cur + 1 == nr_meta_pages) {
 			error = prepare_image(&orig_bm, &copy_bm);
 			if (error)
 				return error;
@@ -2189,16 +2209,22 @@ int snapshot_write_next(struct snapshot_handle *handle)
 			if (IS_ERR(handle->buffer))
 				return PTR_ERR(handle->buffer);
 		}
+		error = PAGE_SIZE;
 	} else {
 		copy_last_highmem_page();
-		handle->buffer = get_buffer(&orig_bm, &ca);
-		if (IS_ERR(handle->buffer))
-			return PTR_ERR(handle->buffer);
-		if (handle->buffer != buffer)
-			handle->sync_read = 0;
+		/* prepare next unless this was the last one */
+		if (handle->cur + 1 < nr_meta_pages + nr_copy_pages) {
+			handle->buffer = get_buffer(&orig_bm, &ca);
+			if (IS_ERR(handle->buffer))
+				return PTR_ERR(handle->buffer);
+			if (handle->buffer != buffer)
+				handle->sync_read = 0;
+			error = PAGE_SIZE;
+		}
 	}
+
 	handle->cur++;
-	return PAGE_SIZE;
+	return error;
 }
 
 /**
@@ -2213,7 +2239,7 @@ void snapshot_write_finalize(struct snapshot_handle *handle)
 {
 	copy_last_highmem_page();
 	/* Free only if we have loaded the image entirely */
-	if (handle->cur > 1 && handle->cur > nr_meta_pages + nr_copy_pages) {
+	if (handle->cur >= nr_meta_pages + nr_copy_pages) {
 		memory_bm_free(&orig_bm, PG_UNSAFE_CLEAR);
 		free_highmem_data();
 	}
@@ -2222,7 +2248,7 @@ void snapshot_write_finalize(struct snapshot_handle *handle)
 int snapshot_image_loaded(struct snapshot_handle *handle)
 {
 	return !(!nr_copy_pages || !last_highmem_page_copied() ||
-			handle->cur <= nr_meta_pages + nr_copy_pages);
+			handle->cur < nr_meta_pages + nr_copy_pages);
 }
 
 #ifdef CONFIG_HIGHMEM
diff --git a/kernel/power/swap.c b/kernel/power/swap.c
index 1b0ed28..4472cf3 100644
--- a/kernel/power/swap.c
+++ b/kernel/power/swap.c
@@ -570,9 +570,6 @@ static int load_image(struct snapshot_handle *snapshot, unsigned int nr_to_read)
 	bio = NULL;
 	do_gettimeofday(&start);
 	for ( ; ; ) {
-		error = snapshot_write_next(snapshot);
-		if (error <= 0)
-			break;
 		error = sws_io_ops->read_page(data_of(*snapshot), &bio);
 		if (error)
 			break;
@@ -580,9 +577,13 @@ static int load_image(struct snapshot_handle *snapshot, unsigned int nr_to_read)
 			error = sws_wait_on_bio_chain(&bio);
 		if (error)
 			break;
+		error = snapshot_write_next(snapshot);
+		if (error >= 0)
+			nr_pages++;
+		if (error <= 0)
+			break;
 		if (!(nr_pages % m))
 			printk("\b\b\b\b%3d%%", nr_pages / m);
-		nr_pages++;
 	}
 	err2 = sws_wait_on_bio_chain(&bio);
 	do_gettimeofday(&stop);
@@ -609,20 +610,14 @@ int swsusp_read(unsigned int *flags_p)
 {
 	int error;
 	struct snapshot_handle snapshot;
-	struct swsusp_info *header;
 
 	memset(&snapshot, 0, sizeof(struct snapshot_handle));
-	error = snapshot_write_next(&snapshot);
-	if (error < PAGE_SIZE)
-		return error < 0 ? error : -EFAULT;
-	header = (struct swsusp_info *)data_of(snapshot);
 	error = sws_io_ops->get_reader(flags_p);
 	if (error)
 		goto end;
+	error = snapshot_read_init(&snapshot);
 	if (!error)
-		error = sws_io_ops->read_page(header, NULL);
-	if (!error)
-		error = load_image(&snapshot, header->pages - 1);
+		error = load_image(&snapshot, snapshot_get_image_size() - 1);
 	sws_io_ops->put_reader();
 end:
 	if (!error)
-- 
1.7.0.2

[RFC 14/15] PM / Hibernate: dealign swsusp_info

[Jiri Slaby]

From: Jiri Slaby <jirislaby@gmail.com>

Now there is no need to have swsusp_info page aligned thanks to chunk
i/o support. We may add more info after it on the very same page.
Later...

Signed-off-by: Jiri Slaby <jirislaby@gmail.com>
Cc: Nigel Cunningham <ncunningham@crca.org.au>
Cc: "Rafael J. Wysocki" <rjw@sisk.pl>
---
 kernel/power/power.h |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/kernel/power/power.h b/kernel/power/power.h
index 842d27b..cf1450f 100644
--- a/kernel/power/power.h
+++ b/kernel/power/power.h
@@ -13,7 +13,7 @@ struct swsusp_info {
 	unsigned long		image_pages;
 	unsigned long		pages;
 	unsigned long		size;
-} __attribute__((aligned(PAGE_SIZE)));
+};
 
 #ifdef CONFIG_HIBERNATION
 #ifdef CONFIG_ARCH_HIBERNATION_HEADER
-- 
1.7.0.2

[RFC 15/15] PM / Hibernate: move non-swap code to snapshot.c

[Jiri Slaby]

Now, when all the swap-independent code was separated, it's time to
move it into snapshot.c, because it is snapshot related.

Signed-off-by: Jiri Slaby <jslaby@suse.cz>
Cc: Nigel Cunningham <ncunningham@crca.org.au>
Cc: "Rafael J. Wysocki" <rjw@sisk.pl>
---
 kernel/power/power.h    |    7 --
 kernel/power/snapshot.c |  196 +++++++++++++++++++++++++++++++++++++++++++++--
 kernel/power/swap.c     |  182 -------------------------------------------
 3 files changed, 189 insertions(+), 196 deletions(-)

diff --git a/kernel/power/power.h b/kernel/power/power.h
index cf1450f..29c450a 100644
--- a/kernel/power/power.h
+++ b/kernel/power/power.h
@@ -128,14 +128,7 @@ struct sws_module_ops {
 	int (*write_page)(void *addr, struct bio **bio_chain);
 };
 
-extern unsigned int snapshot_additional_pages(struct zone *zone);
 extern unsigned long snapshot_get_image_size(void);
-extern int snapshot_read_init(struct snapshot_handle *handle);
-extern int snapshot_write_init(struct snapshot_handle *handle);
-extern int snapshot_read_next(struct snapshot_handle *handle);
-extern int snapshot_write_next(struct snapshot_handle *handle);
-extern void snapshot_write_finalize(struct snapshot_handle *handle);
-extern int snapshot_image_loaded(struct snapshot_handle *handle);
 
 /* If unset, the snapshot device cannot be open. */
 extern atomic_t snapshot_device_available;
diff --git a/kernel/power/snapshot.c b/kernel/power/snapshot.c
index d432e87..a8a28da 100644
--- a/kernel/power/snapshot.c
+++ b/kernel/power/snapshot.c
@@ -789,7 +789,7 @@ void free_basic_memory_bitmaps(void)
  *	zone (usually the returned value is greater than the exact number)
  */
 
-unsigned int snapshot_additional_pages(struct zone *zone)
+static unsigned int snapshot_additional_pages(struct zone *zone)
 {
 	unsigned int res;
 
@@ -1605,7 +1605,7 @@ pack_pfns(unsigned long *buf, struct memory_bitmap *bm)
  *
  *	@handle: snapshot handle to init
  */
-int snapshot_write_init(struct snapshot_handle *handle)
+static int snapshot_write_init(struct snapshot_handle *handle)
 {
 	int ret;
 
@@ -1646,7 +1646,7 @@ finish:
  *	structure pointed to by @handle is not updated and should not be used
  *	any more.
  */
-int snapshot_read_next(struct snapshot_handle *handle)
+static int snapshot_read_next(struct snapshot_handle *handle)
 {
 	if (handle->cur < nr_meta_pages) {
 		memset(buffer, 0, PAGE_SIZE);
@@ -2134,7 +2134,7 @@ static void *get_buffer(struct memory_bitmap *bm, struct chain_allocator *ca)
  *
  *	@handle: snapshot handle to init
  */
-int snapshot_read_init(struct snapshot_handle *handle)
+static int snapshot_read_init(struct snapshot_handle *handle)
 {
 	int ret;
 
@@ -2183,7 +2183,7 @@ finish:
  *	structure pointed to by @handle is not updated and should not be used
  *	any more.
  */
-int snapshot_write_next(struct snapshot_handle *handle)
+static int snapshot_write_next(struct snapshot_handle *handle)
 {
 	static struct chain_allocator ca;
 	int error = 0;
@@ -2235,7 +2235,7 @@ int snapshot_write_next(struct snapshot_handle *handle)
  *	used any more.
  */
 
-void snapshot_write_finalize(struct snapshot_handle *handle)
+static void snapshot_write_finalize(struct snapshot_handle *handle)
 {
 	copy_last_highmem_page();
 	/* Free only if we have loaded the image entirely */
@@ -2245,12 +2245,194 @@ void snapshot_write_finalize(struct snapshot_handle *handle)
 	}
 }
 
-int snapshot_image_loaded(struct snapshot_handle *handle)
+static int snapshot_image_loaded(struct snapshot_handle *handle)
 {
 	return !(!nr_copy_pages || !last_highmem_page_copied() ||
 			handle->cur < nr_meta_pages + nr_copy_pages);
 }
 
+/**
+ *	save_image - save the suspend image data
+ */
+
+static int save_image(struct snapshot_handle *snapshot,
+                      unsigned int nr_to_write)
+{
+	unsigned int m;
+	int ret;
+	int nr_pages;
+	int err2;
+	struct bio *bio;
+	struct timeval start;
+	struct timeval stop;
+
+	printk(KERN_INFO "PM: Saving image data pages (%u pages) ...     ",
+		nr_to_write);
+	m = nr_to_write / 100;
+	if (!m)
+		m = 1;
+	nr_pages = 0;
+	bio = NULL;
+	do_gettimeofday(&start);
+	while (1) {
+		ret = snapshot_read_next(snapshot);
+		if (ret <= 0)
+			break;
+		ret = sws_io_ops->write_page(data_of(*snapshot), &bio);
+		if (ret)
+			break;
+		if (!(nr_pages % m))
+			printk(KERN_CONT "\b\b\b\b%3d%%", nr_pages / m);
+		nr_pages++;
+	}
+	err2 = sws_wait_on_bio_chain(&bio);
+	do_gettimeofday(&stop);
+	if (!ret)
+		ret = err2;
+	if (!ret)
+		printk(KERN_CONT "\b\b\b\bdone\n");
+	else
+		printk(KERN_CONT "\n");
+	swsusp_show_speed(&start, &stop, nr_to_write, "Wrote");
+	return ret;
+}
+
+/**
+ *	enough_space - Make sure we have enough space to save the image.
+ *
+ *	Returns TRUE or FALSE after checking the total amount of
+ *	space avaiable from the resume block.
+ */
+
+static int enough_space(unsigned int nr_pages)
+{
+	unsigned int free_pages = sws_io_ops->storage_available();
+
+	pr_debug("PM: Free storage pages: %u\n", free_pages);
+	return free_pages > nr_pages + PAGES_FOR_IO;
+}
+
+/**
+ *	swsusp_write - Write entire image and metadata.
+ *	@flags: flags to pass to the "boot" kernel in the image header
+ *
+ *	It is important _NOT_ to umount filesystems at this point. We want
+ *	them synced (in case something goes wrong) but we DO not want to mark
+ *	filesystem clean: it is not. (And it does not matter, if we resume
+ *	correctly, we'll mark system clean, anyway.)
+ */
+
+int swsusp_write(unsigned int flags)
+{
+	struct snapshot_handle snapshot;
+	unsigned long pages;
+	int error;
+
+	pages = snapshot_get_image_size();
+	error = sws_io_ops->get_writer();
+	if (error) {
+		printk(KERN_ERR "PM: Cannot get swap writer\n");
+		return error;
+	}
+	if (!enough_space(pages)) {
+		printk(KERN_ERR "PM: Not enough free space for image\n");
+		error = -ENOSPC;
+		goto out_finish;
+	}
+	memset(&snapshot, 0, sizeof(struct snapshot_handle));
+	error = snapshot_write_init(&snapshot);
+	if (error) {
+		printk(KERN_ERR "PM: Cannot init writer\n");
+		goto out_finish;
+	}
+	error = save_image(&snapshot, pages - 1);
+out_finish:
+	error = sws_io_ops->put_writer(flags, error);
+	return error;
+}
+
+/**
+ *	load_image - load the image
+ *	@handle and the snapshot handle @snapshot
+ *	(assume there are @nr_pages pages to load)
+ */
+
+static int load_image(struct snapshot_handle *snapshot, unsigned int nr_to_read)
+{
+	unsigned int m;
+	int error = 0;
+	struct timeval start;
+	struct timeval stop;
+	struct bio *bio;
+	int err2;
+	unsigned nr_pages;
+
+	printk(KERN_INFO "PM: Loading image data pages (%u pages) ...     ",
+		nr_to_read);
+	m = nr_to_read / 100;
+	if (!m)
+		m = 1;
+	nr_pages = 0;
+	bio = NULL;
+	do_gettimeofday(&start);
+	for ( ; ; ) {
+		error = sws_io_ops->read_page(data_of(*snapshot), &bio);
+		if (error)
+			break;
+		if (snapshot->sync_read)
+			error = sws_wait_on_bio_chain(&bio);
+		if (error)
+			break;
+		error = snapshot_write_next(snapshot);
+		if (error >= 0)
+			nr_pages++;
+		if (error <= 0)
+			break;
+		if (!(nr_pages % m))
+			printk("\b\b\b\b%3d%%", nr_pages / m);
+	}
+	err2 = sws_wait_on_bio_chain(&bio);
+	do_gettimeofday(&stop);
+	if (!error)
+		error = err2;
+	if (!error) {
+		printk("\b\b\b\bdone\n");
+		snapshot_write_finalize(snapshot);
+		if (!snapshot_image_loaded(snapshot))
+			error = -ENODATA;
+	} else
+		printk("\n");
+	swsusp_show_speed(&start, &stop, nr_to_read, "Read");
+	return error;
+}
+
+/**
+ *	swsusp_read - read the hibernation image.
+ *	@flags_p: flags passed by the "frozen" kernel in the image header should
+ *		  be written into this memeory location
+ */
+
+int swsusp_read(unsigned int *flags_p)
+{
+	int error;
+	struct snapshot_handle snapshot;
+
+	memset(&snapshot, 0, sizeof(struct snapshot_handle));
+	error = sws_io_ops->get_reader(flags_p);
+	if (error)
+		goto end;
+	error = snapshot_read_init(&snapshot);
+	if (!error)
+		error = load_image(&snapshot, snapshot_get_image_size() - 1);
+	sws_io_ops->put_reader();
+end:
+	if (!error)
+		pr_debug("PM: Image successfully loaded\n");
+	else
+		pr_debug("PM: Error %d resuming\n", error);
+	return error;
+}
+
 #ifdef CONFIG_HIGHMEM
 /* Assumes that @buf is ready and points to a "safe" page */
 static inline void
diff --git a/kernel/power/swap.c b/kernel/power/swap.c
index 4472cf3..ddd7238 100644
--- a/kernel/power/swap.c
+++ b/kernel/power/swap.c
@@ -360,106 +360,6 @@ static int put_swap_writer(unsigned int flags, int error)
 	return error;
 }
 
-/**
- *	save_image - save the suspend image data
- */
-
-static int save_image(struct snapshot_handle *snapshot,
-                      unsigned int nr_to_write)
-{
-	unsigned int m;
-	int ret;
-	int nr_pages;
-	int err2;
-	struct bio *bio;
-	struct timeval start;
-	struct timeval stop;
-
-	printk(KERN_INFO "PM: Saving image data pages (%u pages) ...     ",
-		nr_to_write);
-	m = nr_to_write / 100;
-	if (!m)
-		m = 1;
-	nr_pages = 0;
-	bio = NULL;
-	do_gettimeofday(&start);
-	while (1) {
-		ret = snapshot_read_next(snapshot);
-		if (ret <= 0)
-			break;
-		ret = sws_io_ops->write_page(data_of(*snapshot), &bio);
-		if (ret)
-			break;
-		if (!(nr_pages % m))
-			printk(KERN_CONT "\b\b\b\b%3d%%", nr_pages / m);
-		nr_pages++;
-	}
-	err2 = sws_wait_on_bio_chain(&bio);
-	do_gettimeofday(&stop);
-	if (!ret)
-		ret = err2;
-	if (!ret)
-		printk(KERN_CONT "\b\b\b\bdone\n");
-	else
-		printk(KERN_CONT "\n");
-	swsusp_show_speed(&start, &stop, nr_to_write, "Wrote");
-	return ret;
-}
-
-/**
- *	enough_space - Make sure we have enough space to save the image.
- *
- *	Returns TRUE or FALSE after checking the total amount of
- *	space avaiable from the resume block.
- */
-
-static int enough_space(unsigned int nr_pages)
-{
-	unsigned int free_pages = sws_io_ops->storage_available();
-
-	pr_debug("PM: Free storage pages: %u\n", free_pages);
-	return free_pages > nr_pages + PAGES_FOR_IO;
-}
-
-/**
- *	swsusp_write - Write entire image and metadata.
- *	@flags: flags to pass to the "boot" kernel in the image header
- *
- *	It is important _NOT_ to umount filesystems at this point. We want
- *	them synced (in case something goes wrong) but we DO not want to mark
- *	filesystem clean: it is not. (And it does not matter, if we resume
- *	correctly, we'll mark system clean, anyway.)
- */
-
-int swsusp_write(unsigned int flags)
-{
-	struct snapshot_handle snapshot;
-	unsigned long pages;
-	int error;
-
-	pages = snapshot_get_image_size();
-	error = sws_io_ops->get_writer();
-	if (error) {
-		printk(KERN_ERR "PM: Cannot get swap writer\n");
-		return error;
-	}
-	if (!enough_space(pages)) {
-		printk(KERN_ERR "PM: Not enough free space for image\n");
-		error = -ENOSPC;
-		goto out_finish;
-	}
-	memset(&snapshot, 0, sizeof(struct snapshot_handle));
-	error = snapshot_write_init(&snapshot);
-	if (error) {
-		printk(KERN_ERR "PM: Cannot init writer\n");
-		goto out_finish;
-	}
-	error = save_image(&snapshot, pages - 1);
-out_finish:
-	error = sws_io_ops->put_writer(flags, error);
-	return error;
-}
-
 static unsigned long swap_storage_available(void)
 {
 	return count_swap_pages(root_swap, 1);
@@ -546,88 +446,6 @@ struct sws_module_ops swap_ops = {
 };
 
 /**
- *	load_image - load the image
- *	@handle and the snapshot handle @snapshot
- *	(assume there are @nr_pages pages to load)
- */
-
-static int load_image(struct snapshot_handle *snapshot, unsigned int nr_to_read)
-{
-	unsigned int m;
-	int error = 0;
-	struct timeval start;
-	struct timeval stop;
-	struct bio *bio;
-	int err2;
-	unsigned nr_pages;
-
-	printk(KERN_INFO "PM: Loading image data pages (%u pages) ...     ",
-		nr_to_read);
-	m = nr_to_read / 100;
-	if (!m)
-		m = 1;
-	nr_pages = 0;
-	bio = NULL;
-	do_gettimeofday(&start);
-	for ( ; ; ) {
-		error = sws_io_ops->read_page(data_of(*snapshot), &bio);
-		if (error)
-			break;
-		if (snapshot->sync_read)
-			error = sws_wait_on_bio_chain(&bio);
-		if (error)
-			break;
-		error = snapshot_write_next(snapshot);
-		if (error >= 0)
-			nr_pages++;
-		if (error <= 0)
-			break;
-		if (!(nr_pages % m))
-			printk("\b\b\b\b%3d%%", nr_pages / m);
-	}
-	err2 = sws_wait_on_bio_chain(&bio);
-	do_gettimeofday(&stop);
-	if (!error)
-		error = err2;
-	if (!error) {
-		printk("\b\b\b\bdone\n");
-		snapshot_write_finalize(snapshot);
-		if (!snapshot_image_loaded(snapshot))
-			error = -ENODATA;
-	} else
-		printk("\n");
-	swsusp_show_speed(&start, &stop, nr_to_read, "Read");
-	return error;
-}
-
-/**
- *	swsusp_read - read the hibernation image.
- *	@flags_p: flags passed by the "frozen" kernel in the image header should
- *		  be written into this memeory location
- */
-
-int swsusp_read(unsigned int *flags_p)
-{
-	int error;
-	struct snapshot_handle snapshot;
-
-	memset(&snapshot, 0, sizeof(struct snapshot_handle));
-	error = sws_io_ops->get_reader(flags_p);
-	if (error)
-		goto end;
-	error = snapshot_read_init(&snapshot);
-	if (!error)
-		error = load_image(&snapshot, snapshot_get_image_size() - 1);
-	sws_io_ops->put_reader();
-end:
-	if (!error)
-		pr_debug("PM: Image successfully loaded\n");
-	else
-		pr_debug("PM: Error %d resuming\n", error);
-	return error;
-}
-
-/**
  *      swsusp_check - Check for swsusp signature in the resume device
  */
 
-- 
1.7.0.2

Re: [RFC 01/15] FS: libfs, implement simple_write_to_buffer

[Rafael J. Wysocki]

On Tuesday 23 March 2010, Jiri Slaby wrote:
> It will be used in suspend code and serves as an easy wrap around
> copy_from_user. Similar to simple_read_from_buffer, it takes care
> of transfers with proper lengths depending on available and count
> parameters and advances ppos appropriately.
> 
> Signed-off-by: Jiri Slaby <jslaby@suse.cz>
> Cc: Nigel Cunningham <ncunningham@crca.org.au>
> Cc: "Rafael J. Wysocki" <rjw@sisk.pl>
> Cc: Alexander Viro <viro@zeniv.linux.org.uk>
> Cc: linux-fsdevel@vger.kernel.org

Thanks for doing this job, I really appreciate it.

I'll do my best to review the patches in detail later today or tomorrow.

Best,
Rafael


> ---
>  fs/libfs.c         |   35 +++++++++++++++++++++++++++++++++++
>  include/linux/fs.h |    2 ++
>  2 files changed, 37 insertions(+), 0 deletions(-)
> 
> diff --git a/fs/libfs.c b/fs/libfs.c
> index 9e50bcf..fda73b3 100644
> --- a/fs/libfs.c
> +++ b/fs/libfs.c
> @@ -546,6 +546,40 @@ ssize_t simple_read_from_buffer(void __user *to, size_t count, loff_t *ppos,
>  }
>  
>  /**
> + * simple_write_to_buffer - copy data from user space to the buffer
> + * @to: the buffer to write to
> + * @available: the size of the buffer
> + * @ppos: the current position in the buffer
> + * @from: the user space buffer to read from
> + * @count: the maximum number of bytes to read
> + *
> + * The simple_write_to_buffer() function reads up to @count bytes from the user
> + * space address starting at @from into the buffer @to at offset @ppos.
> + *
> + * On success, the number of bytes written is returned and the offset @ppos is
> + * advanced by this number, or negative value is returned on error.
> + **/
> +ssize_t simple_write_to_buffer(void *to, size_t available, loff_t *ppos,
> +		const void __user *from, size_t count)
> +{
> +	loff_t pos = *ppos;
> +	size_t ret;
> +
> +	if (pos < 0)
> +		return -EINVAL;
> +	if (pos >= available || !count)
> +		return 0;
> +	if (count > available - pos)
> +		count = available - pos;
> +	ret = copy_from_user(to + pos, from, count);
> +	if (ret == count)
> +		return -EFAULT;
> +	count -= ret;
> +	*ppos = pos + count;
> +	return count;
> +}
> +
> +/**
>   * memory_read_from_buffer - copy data from the buffer
>   * @to: the kernel space buffer to read to
>   * @count: the maximum number of bytes to read
> @@ -863,6 +897,7 @@ EXPORT_SYMBOL(simple_statfs);
>  EXPORT_SYMBOL(simple_sync_file);
>  EXPORT_SYMBOL(simple_unlink);
>  EXPORT_SYMBOL(simple_read_from_buffer);
> +EXPORT_SYMBOL(simple_write_to_buffer);
>  EXPORT_SYMBOL(memory_read_from_buffer);
>  EXPORT_SYMBOL(simple_transaction_set);
>  EXPORT_SYMBOL(simple_transaction_get);
> diff --git a/include/linux/fs.h b/include/linux/fs.h
> index a4636b6..0f751b6 100644
> --- a/include/linux/fs.h
> +++ b/include/linux/fs.h
> @@ -2370,6 +2370,8 @@ extern void simple_release_fs(struct vfsmount **mount, int *count);
>  
>  extern ssize_t simple_read_from_buffer(void __user *to, size_t count,
>  			loff_t *ppos, const void *from, size_t available);
> +extern ssize_t simple_write_to_buffer(void *to, size_t available, loff_t *ppos,
> +		const void __user *from, size_t count);
>  
>  extern int simple_fsync(struct file *, struct dentry *, int);
>  
> 

Re: [RFC 01/15] FS: libfs, implement simple_write_to_buffer

[Nigel Cunningham]

Hi Jiri and others.

Just a general comment, having quickly read through the patches while 
not fully understanding the uswsusp side of things, they look good to 
me. Like Rafael has just said, I'll seek to take a more thorough look later.

Thanks!

Nigel

Re: [RFC 02/15] PM / Hibernate: snapshot cleanup

[Pavel Machek]

On Tue 2010-03-23 17:17:30, Jiri Slaby wrote:
> From: Jiri Slaby <jirislaby@gmail.com>
> 
> Remove support of reads with offset. This means snapshot_read/write_next
> now does not accept count parameter.
> 
> /dev/snapshot handler is converted to simple_read_from_buffer/simple_write_to_buffer.
> 
> Signed-off-by: Jiri Slaby <jslaby@suse.cz>
> Cc: Nigel Cunningham <ncunningham@crca.org.au>
> Cc: "Rafael J. Wysocki" <rjw@sisk.pl>

Well, if Al likes the vfs changes... why not.

-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

Re: [RFC 02/15] PM / Hibernate: snapshot cleanup

[Rafael J. Wysocki]

On Tuesday 23 March 2010, Jiri Slaby wrote:
> From: Jiri Slaby <jirislaby@gmail.com>
> 
> Remove support of reads with offset. This means snapshot_read/write_next
> now does not accept count parameter.
> 
> /dev/snapshot handler is converted to simple_read_from_buffer/simple_write_to_buffer.

Makes sense.

One coding style comment, though.

> Signed-off-by: Jiri Slaby <jslaby@suse.cz>
> Cc: Nigel Cunningham <ncunningham@crca.org.au>
> Cc: "Rafael J. Wysocki" <rjw@sisk.pl>
> ---
>  kernel/power/power.h    |   18 +-----
>  kernel/power/snapshot.c |  145 ++++++++++++++++++-----------------------------
>  kernel/power/swap.c     |    8 +-
>  kernel/power/user.c     |   39 ++++++++-----
>  4 files changed, 88 insertions(+), 122 deletions(-)
> 
...  
> @@ -159,13 +160,17 @@ static ssize_t snapshot_read(struct file *filp, char __user *buf,
>  		res = -ENODATA;
>  		goto Unlock;
>  	}
> -	res = snapshot_read_next(&data->handle, count);
> -	if (res > 0) {
> -		if (copy_to_user(buf, data_of(data->handle), res))
> -			res = -EFAULT;
> -		else
> -			*offp = data->handle.offset;
> -	}
> +	if (!pg_offp) { /* on page boundary? */
> +		res = snapshot_read_next(&data->handle);
> +		if (res <= 0)
> +			goto Unlock;
> +	} else
> +		res = PAGE_SIZE - pg_offp;

The official kernel coding style is to put single instructions under if/else
like this in braces if the other branch of the if/else is multiline (and
therefore naturally in braces).   So please do:

+	if (!pg_offp) { /* on page boundary? */
+		res = snapshot_read_next(&data->handle);
+		if (res <= 0)
+			goto Unlock;
+	} else {
+		res = PAGE_SIZE - pg_offp;
+	}

and analogously wherever applicable.

Thanks,
Rafael

Re: [RFC 02/15] PM / Hibernate: snapshot cleanup

[Pavel Machek]

On Tue 2010-03-23 17:17:30, Jiri Slaby wrote:
> From: Jiri Slaby <jirislaby@gmail.com>
> 
> Remove support of reads with offset. This means snapshot_read/write_next
> now does not accept count parameter.
> 
> /dev/snapshot handler is converted to simple_read_from_buffer/simple_write_to_buffer.
> 
> Signed-off-by: Jiri Slaby <jslaby@suse.cz>
> Cc: Nigel Cunningham <ncunningham@crca.org.au>
> Cc: "Rafael J. Wysocki" <rjw@sisk.pl>

Well, if Al likes the vfs changes... why not.

-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

Re: [RFC 03/15] PM / Hibernate: separate block_io

[Pavel Machek]

On Tue 2010-03-23 17:17:31, Jiri Slaby wrote:
> Move block I/O operations to a separate file. It is because it will
> be used later not only by the swap writer.

So... what is the plan here?

> +int sws_bio_read_page(pgoff_t page_off, void *addr, struct bio **bio_chain)
> +{
> +	return submit(READ, sws_resume_bdev, page_off * (PAGE_SIZE >> 9),
> +			virt_to_page(addr), bio_chain);
> +}

sws_ is kind of strange prefix. We were trying to get away from
"swsuspend" name for quite some time...
									Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

Re: [RFC 03/15] PM / Hibernate: separate block_io

[Jiri Slaby]

On 03/24/2010 09:30 PM, Pavel Machek wrote:
> On Tue 2010-03-23 17:17:31, Jiri Slaby wrote:
>> +int sws_bio_read_page(pgoff_t page_off, void *addr, struct bio **bio_chain)
>> +{
>> +	return submit(READ, sws_resume_bdev, page_off * (PAGE_SIZE >> 9),
>> +			virt_to_page(addr), bio_chain);
>> +}
> 
> sws_ is kind of strange prefix. We were trying to get away from
> "swsuspend" name for quite some time...

No problem to change the prefix to anything else. Do you (anybody)
suggest anything?

-- 
js

Re: [RFC 03/15] PM / Hibernate: separate block_io

[Nigel Cunningham]

Hi.

On 25/03/10 08:22, Jiri Slaby wrote:
> On 03/24/2010 09:30 PM, Pavel Machek wrote:
>> On Tue 2010-03-23 17:17:31, Jiri Slaby wrote:
>>> +int sws_bio_read_page(pgoff_t page_off, void *addr, struct bio **bio_chain)
>>> +{
>>> +	return submit(READ, sws_resume_bdev, page_off * (PAGE_SIZE>>  9),
>>> +			virt_to_page(addr), bio_chain);
>>> +}
>>
>> sws_ is kind of strange prefix. We were trying to get away from
>> "swsuspend" name for quite some time...
>
> No problem to change the prefix to anything else. Do you (anybody)
> suggest anything?
>

How about some abbreviation of hibernate? "hib"?

Regards,

Nigel

Re: [RFC 03/15] PM / Hibernate: separate block_io

[Nigel Cunningham]

Hi again.

On 25/03/10 09:58, Nigel Cunningham wrote:
> Hi.
>
> On 25/03/10 08:22, Jiri Slaby wrote:
>> On 03/24/2010 09:30 PM, Pavel Machek wrote:
>>> On Tue 2010-03-23 17:17:31, Jiri Slaby wrote:
>>>> +int sws_bio_read_page(pgoff_t page_off, void *addr, struct bio **bio_chain)
>>>> +{
>>>> +	return submit(READ, sws_resume_bdev, page_off * (PAGE_SIZE>>   9),
>>>> +			virt_to_page(addr), bio_chain);
>>>> +}
>>>
>>> sws_ is kind of strange prefix. We were trying to get away from
>>> "swsuspend" name for quite some time...
>>
>> No problem to change the prefix to anything else. Do you (anybody)
>> suggest anything?
>>
>
> How about some abbreviation of hibernate? "hib"?

On further reflection, how about "std" (suspend to disk)? I think that's 
less ugly than the 'hib' suggestion :)

Regards,

Nigel

Re: [RFC 03/15] PM / Hibernate: separate block_io

[Pavel Machek]

On Thu 2010-03-25 09:58:04, Nigel Cunningham wrote:
> Hi.
>
> On 25/03/10 08:22, Jiri Slaby wrote:
>> On 03/24/2010 09:30 PM, Pavel Machek wrote:
>>> On Tue 2010-03-23 17:17:31, Jiri Slaby wrote:
>>>> +int sws_bio_read_page(pgoff_t page_off, void *addr, struct bio **bio_chain)
>>>> +{
>>>> +	return submit(READ, sws_resume_bdev, page_off * (PAGE_SIZE>>  9),
>>>> +			virt_to_page(addr), bio_chain);
>>>> +}
>>>
>>> sws_ is kind of strange prefix. We were trying to get away from
>>> "swsuspend" name for quite some time...
>>
>> No problem to change the prefix to anything else. Do you (anybody)
>> suggest anything?
>>
>
> How about some abbreviation of hibernate? "hib"?

hib_ certainly sounds better.

-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

Re: [RFC 03/15] PM / Hibernate: separate block_io

[Rafael J. Wysocki]

On Thursday 25 March 2010, Nigel Cunningham wrote:
> Hi again.
> 
> On 25/03/10 09:58, Nigel Cunningham wrote:
> > Hi.
> >
> > On 25/03/10 08:22, Jiri Slaby wrote:
> >> On 03/24/2010 09:30 PM, Pavel Machek wrote:
> >>> On Tue 2010-03-23 17:17:31, Jiri Slaby wrote:
> >>>> +int sws_bio_read_page(pgoff_t page_off, void *addr, struct bio **bio_chain)
> >>>> +{
> >>>> +	return submit(READ, sws_resume_bdev, page_off * (PAGE_SIZE>>   9),
> >>>> +			virt_to_page(addr), bio_chain);
> >>>> +}
> >>>
> >>> sws_ is kind of strange prefix. We were trying to get away from
> >>> "swsuspend" name for quite some time...
> >>
> >> No problem to change the prefix to anything else. Do you (anybody)
> >> suggest anything?
> >>
> >
> > How about some abbreviation of hibernate? "hib"?
> 
> On further reflection, how about "std" (suspend to disk)? I think that's 
> less ugly than the 'hib' suggestion :)

But it also decodes as "standard" if someone is not in the right context. :-)

If the "bio" part of the name is not essential (ie. there's no conflicting name
already), we could call it simply hibernate_read_page().

Rafael

Re: [RFC 03/15] PM / Hibernate: separate block_io

[Nigel Cunningham]

Hi.

On 26/03/10 07:12, Rafael J. Wysocki wrote:
> On Thursday 25 March 2010, Nigel Cunningham wrote:
>> Hi again.
>>
>> On 25/03/10 09:58, Nigel Cunningham wrote:
>>> Hi.
>>>
>>> On 25/03/10 08:22, Jiri Slaby wrote:
>>>> On 03/24/2010 09:30 PM, Pavel Machek wrote:
>>>>> On Tue 2010-03-23 17:17:31, Jiri Slaby wrote:
>>>>>> +int sws_bio_read_page(pgoff_t page_off, void *addr, struct bio **bio_chain)
>>>>>> +{
>>>>>> +	return submit(READ, sws_resume_bdev, page_off * (PAGE_SIZE>>    9),
>>>>>> +			virt_to_page(addr), bio_chain);
>>>>>> +}
>>>>>
>>>>> sws_ is kind of strange prefix. We were trying to get away from
>>>>> "swsuspend" name for quite some time...
>>>>
>>>> No problem to change the prefix to anything else. Do you (anybody)
>>>> suggest anything?
>>>>
>>>
>>> How about some abbreviation of hibernate? "hib"?
>>
>> On further reflection, how about "std" (suspend to disk)? I think that's
>> less ugly than the 'hib' suggestion :)
>
> But it also decodes as "standard" if someone is not in the right context. :-)

Ah...

> If the "bio" part of the name is not essential (ie. there's no conflicting name
> already), we could call it simply hibernate_read_page().

Yeah. So we're going with hibernate or hib_ if it needs abbreviating?

Nigel

Re: [RFC 03/15] PM / Hibernate: separate block_io

[Rafael J. Wysocki]

On Thursday 25 March 2010, Nigel Cunningham wrote:
> Hi.
> 
> On 26/03/10 07:12, Rafael J. Wysocki wrote:
> > On Thursday 25 March 2010, Nigel Cunningham wrote:
> >> Hi again.
> >>
> >> On 25/03/10 09:58, Nigel Cunningham wrote:
> >>> Hi.
> >>>
> >>> On 25/03/10 08:22, Jiri Slaby wrote:
> >>>> On 03/24/2010 09:30 PM, Pavel Machek wrote:
> >>>>> On Tue 2010-03-23 17:17:31, Jiri Slaby wrote:
> >>>>>> +int sws_bio_read_page(pgoff_t page_off, void *addr, struct bio **bio_chain)
> >>>>>> +{
> >>>>>> +	return submit(READ, sws_resume_bdev, page_off * (PAGE_SIZE>>    9),
> >>>>>> +			virt_to_page(addr), bio_chain);
> >>>>>> +}
> >>>>>
> >>>>> sws_ is kind of strange prefix. We were trying to get away from
> >>>>> "swsuspend" name for quite some time...
> >>>>
> >>>> No problem to change the prefix to anything else. Do you (anybody)
> >>>> suggest anything?
> >>>>
> >>>
> >>> How about some abbreviation of hibernate? "hib"?
> >>
> >> On further reflection, how about "std" (suspend to disk)? I think that's
> >> less ugly than the 'hib' suggestion :)
> >
> > But it also decodes as "standard" if someone is not in the right context. :-)
> 
> Ah...
> 
> > If the "bio" part of the name is not essential (ie. there's no conflicting name
> > already), we could call it simply hibernate_read_page().
> 
> Yeah. So we're going with hibernate or hib_ if it needs abbreviating?

I'd just use "hibernate" without abbreviating if reasonably possible.
We can also use "image_" in some cases I guess.

Rafael

Re: [RFC 03/15] PM / Hibernate: separate block_io

[Nigel Cunningham]

Hi.

On 26/03/10 07:33, Rafael J. Wysocki wrote:
> On Thursday 25 March 2010, Nigel Cunningham wrote:
>> Hi.
>>
>> On 26/03/10 07:12, Rafael J. Wysocki wrote:
>>> On Thursday 25 March 2010, Nigel Cunningham wrote:
>>>> Hi again.
>>>>
>>>> On 25/03/10 09:58, Nigel Cunningham wrote:
>>>>> Hi.
>>>>>
>>>>> On 25/03/10 08:22, Jiri Slaby wrote:
>>>>>> On 03/24/2010 09:30 PM, Pavel Machek wrote:
>>>>>>> On Tue 2010-03-23 17:17:31, Jiri Slaby wrote:
>>>>>>>> +int sws_bio_read_page(pgoff_t page_off, void *addr, struct bio **bio_chain)
>>>>>>>> +{
>>>>>>>> +	return submit(READ, sws_resume_bdev, page_off * (PAGE_SIZE>>     9),
>>>>>>>> +			virt_to_page(addr), bio_chain);
>>>>>>>> +}
>>>>>>>
>>>>>>> sws_ is kind of strange prefix. We were trying to get away from
>>>>>>> "swsuspend" name for quite some time...
>>>>>>
>>>>>> No problem to change the prefix to anything else. Do you (anybody)
>>>>>> suggest anything?
>>>>>>
>>>>>
>>>>> How about some abbreviation of hibernate? "hib"?
>>>>
>>>> On further reflection, how about "std" (suspend to disk)? I think that's
>>>> less ugly than the 'hib' suggestion :)
>>>
>>> But it also decodes as "standard" if someone is not in the right context. :-)
>>
>> Ah...
>>
>>> If the "bio" part of the name is not essential (ie. there's no conflicting name
>>> already), we could call it simply hibernate_read_page().
>>
>> Yeah. So we're going with hibernate or hib_ if it needs abbreviating?
>
> I'd just use "hibernate" without abbreviating if reasonably possible.
> We can also use "image_" in some cases I guess.

k.

Nigel

Re: [RFC 03/15] PM / Hibernate: separate block_io

[Pavel Machek]

Hi!

> > > How about some abbreviation of hibernate? "hib"?
> > 
> > On further reflection, how about "std" (suspend to disk)? I think that's 
> > less ugly than the 'hib' suggestion :)
> 
> But it also decodes as "standard" if someone is not in the right context. :-)
> 

Hmm, and when you are in really wrong context, you get rather nasty
diseases...

std_prepare() seems like rather bad idea :-).
									Pavel

-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

Re: [RFC 04/15] PM / Hibernate: move the first_sector out of swsusp_write

[Pavel Machek]

On Tue 2010-03-23 17:17:32, Jiri Slaby wrote:
> From: Jiri Slaby <jirislaby@gmail.com>
> 
> The first sector knowledge is swap only specific. Move it into the
> handle. This will be needed for later non-swap specific code moving
> inside snapshot.c.
> 
> Signed-off-by: Jiri Slaby <jslaby@suse.cz>
> Cc: Nigel Cunningham <ncunningham@crca.org.au>
> Cc: "Rafael J. Wysocki" <rjw@sisk.pl>

Seems ok. ACK.

-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

Re: [RFC 04/15] PM / Hibernate: move the first_sector out of swsusp_write

[Rafael J. Wysocki]

On Wednesday 24 March 2010, Pavel Machek wrote:
> On Tue 2010-03-23 17:17:32, Jiri Slaby wrote:
> > From: Jiri Slaby <jirislaby@gmail.com>
> > 
> > The first sector knowledge is swap only specific. Move it into the
> > handle. This will be needed for later non-swap specific code moving
> > inside snapshot.c.
> > 
> > Signed-off-by: Jiri Slaby <jslaby@suse.cz>
> > Cc: Nigel Cunningham <ncunningham@crca.org.au>
> > Cc: "Rafael J. Wysocki" <rjw@sisk.pl>
> 
> Seems ok. ACK.

Yes, it looks good.

Rafael

Re: [RFC 06/15] PM / Hibernate: swap, remove swap_map_handle usages

[Pavel Machek]

On Tue 2010-03-23 17:17:34, Jiri Slaby wrote:
> Some code, which will be moved out of swap.c, needs know nothing about
> swap. There will be also other than swap writers later, so that it
> won't make sense at all.
> 
> Make it a global static in swap.c as a singleton.

I guess I just dislike global static. Logically, methods do operate on
handles, so...

I don't see a point and I do not think the change is an improvement.
									Pavel


> Signed-off-by: Jiri Slaby <jslaby@suse.cz>
> Cc: Nigel Cunningham <ncunningham@crca.org.au>
> Cc: "Rafael J. Wysocki" <rjw@sisk.pl>
> ---
>  kernel/power/swap.c |   58 +++++++++++++++++++++++++-------------------------
>  1 files changed, 29 insertions(+), 29 deletions(-)
> 
> diff --git a/kernel/power/swap.c b/kernel/power/swap.c
> index 2edf742..ac1a351 100644
> --- a/kernel/power/swap.c
> +++ b/kernel/power/swap.c
> @@ -70,6 +70,7 @@ struct swsusp_header {
>  	char	sig[10];
>  } __attribute__((packed));
>  
> +static struct swap_map_handle swap_map_handle;
>  static struct swsusp_header *swsusp_header;
>  
>  /**
> @@ -267,8 +268,9 @@ static void release_swap_writer(struct swap_map_handle *handle)
>  	handle->cur = NULL;
>  }
>  
> -static int get_swap_writer(struct swap_map_handle *handle)
> +static int get_swap_writer(void)
>  {
> +	struct swap_map_handle *handle = &swap_map_handle;
>  	int ret;
>  
>  	ret = swsusp_swap_check();
> @@ -278,6 +280,7 @@ static int get_swap_writer(struct swap_map_handle *handle)
>  					"swapon -a.\n");
>  		return ret;
>  	}
> +	memset(handle, 0, sizeof(*handle));
>  	handle->cur = (struct swap_map_page *)get_zeroed_page(GFP_KERNEL);
>  	if (!handle->cur) {
>  		ret = -ENOMEM;
> @@ -298,9 +301,9 @@ err_close:
>  	return ret;
>  }
>  
> -static int swap_write_page(struct swap_map_handle *handle, void *buf,
> -				struct bio **bio_chain)
> +static int swap_write_page(void *buf, struct bio **bio_chain)
>  {
> +	struct swap_map_handle *handle = &swap_map_handle;
>  	int error = 0;
>  	sector_t offset;
>  
> @@ -338,9 +341,10 @@ static int flush_swap_writer(struct swap_map_handle *handle)
>  		return -EINVAL;
>  }
>  
> -static int put_swap_writer(struct swap_map_handle *handle,
> -		unsigned int flags, int error)
> +static int put_swap_writer(unsigned int flags, int error)
>  {
> +	struct swap_map_handle *handle = &swap_map_handle;
> +
>  	if (!error) {
>  		flush_swap_writer(handle);
>  		printk(KERN_INFO "PM: S");
> @@ -360,8 +364,7 @@ static int put_swap_writer(struct swap_map_handle *handle,
>   *	save_image - save the suspend image data
>   */
>  
> -static int save_image(struct swap_map_handle *handle,
> -                      struct snapshot_handle *snapshot,
> +static int save_image(struct snapshot_handle *snapshot,
>                        unsigned int nr_to_write)
>  {
>  	unsigned int m;
> @@ -384,7 +387,7 @@ static int save_image(struct swap_map_handle *handle,
>  		ret = snapshot_read_next(snapshot);
>  		if (ret <= 0)
>  			break;
> -		ret = swap_write_page(handle, data_of(*snapshot), &bio);
> +		ret = swap_write_page(data_of(*snapshot), &bio);
>  		if (ret)
>  			break;
>  		if (!(nr_pages % m))
> @@ -430,14 +433,13 @@ static int enough_swap(unsigned int nr_pages)
>  
>  int swsusp_write(unsigned int flags)
>  {
> -	struct swap_map_handle handle;
>  	struct snapshot_handle snapshot;
>  	struct swsusp_info *header;
>  	unsigned long pages;
>  	int error;
>  
>  	pages = snapshot_get_image_size();
> -	error = get_swap_writer(&handle);
> +	error = get_swap_writer();
>  	if (error) {
>  		printk(KERN_ERR "PM: Cannot get swap writer\n");
>  		return error;
> @@ -456,11 +458,11 @@ int swsusp_write(unsigned int flags)
>  		goto out_finish;
>  	}
>  	header = (struct swsusp_info *)data_of(snapshot);
> -	error = swap_write_page(&handle, header, NULL);
> +	error = swap_write_page(header, NULL);
>  	if (!error)
> -		error = save_image(&handle, &snapshot, pages - 1);
> +		error = save_image(&snapshot, pages - 1);
>  out_finish:
> -	error = put_swap_writer(&handle, flags, error);
> +	error = put_swap_writer(flags, error);
>  	return error;
>  }
>  
> @@ -476,9 +478,9 @@ static void release_swap_reader(struct swap_map_handle *handle)
>  	handle->cur = NULL;
>  }
>  
> -static int get_swap_reader(struct swap_map_handle *handle,
> -		unsigned int *flags_p)
> +static int get_swap_reader(unsigned int *flags_p)
>  {
> +	struct swap_map_handle *handle = &swap_map_handle;
>  	int error;
>  
>  	*flags_p = swsusp_header->flags;
> @@ -486,6 +488,7 @@ static int get_swap_reader(struct swap_map_handle *handle,
>  	if (!swsusp_header->image) /* how can this happen? */
>  		return -EINVAL;
>  
> +	memset(handle, 0, sizeof(*handle));
>  	handle->cur = (struct swap_map_page *)get_zeroed_page(__GFP_WAIT | __GFP_HIGH);
>  	if (!handle->cur)
>  		return -ENOMEM;
> @@ -499,9 +502,9 @@ static int get_swap_reader(struct swap_map_handle *handle,
>  	return 0;
>  }
>  
> -static int swap_read_page(struct swap_map_handle *handle, void *buf,
> -				struct bio **bio_chain)
> +static int swap_read_page(void *buf, struct bio **bio_chain)
>  {
> +	struct swap_map_handle *handle = &swap_map_handle;
>  	sector_t offset;
>  	int error;
>  
> @@ -525,22 +528,20 @@ static int swap_read_page(struct swap_map_handle *handle, void *buf,
>  	return error;
>  }
>  
> -static int put_swap_reader(struct swap_map_handle *handle)
> +static int put_swap_reader(void)
>  {
> -	release_swap_reader(handle);
> +	release_swap_reader(&swap_map_handle);
>  
>  	return 0;
>  }
>  
>  /**
> - *	load_image - load the image using the swap map handle
> + *	load_image - load the image
>   *	@handle and the snapshot handle @snapshot
>   *	(assume there are @nr_pages pages to load)
>   */
>  
> -static int load_image(struct swap_map_handle *handle,
> -                      struct snapshot_handle *snapshot,
> -                      unsigned int nr_to_read)
> +static int load_image(struct snapshot_handle *snapshot, unsigned int nr_to_read)
>  {
>  	unsigned int m;
>  	int error = 0;
> @@ -562,7 +563,7 @@ static int load_image(struct swap_map_handle *handle,
>  		error = snapshot_write_next(snapshot);
>  		if (error <= 0)
>  			break;
> -		error = swap_read_page(handle, data_of(*snapshot), &bio);
> +		error = swap_read_page(data_of(*snapshot), &bio);
>  		if (error)
>  			break;
>  		if (snapshot->sync_read)
> @@ -597,7 +598,6 @@ static int load_image(struct swap_map_handle *handle,
>  int swsusp_read(unsigned int *flags_p)
>  {
>  	int error;
> -	struct swap_map_handle handle;
>  	struct snapshot_handle snapshot;
>  	struct swsusp_info *header;
>  
> @@ -606,14 +606,14 @@ int swsusp_read(unsigned int *flags_p)
>  	if (error < PAGE_SIZE)
>  		return error < 0 ? error : -EFAULT;
>  	header = (struct swsusp_info *)data_of(snapshot);
> -	error = get_swap_reader(&handle, flags_p);
> +	error = get_swap_reader(flags_p);
>  	if (error)
>  		goto end;
>  	if (!error)
> -		error = swap_read_page(&handle, header, NULL);
> +		error = swap_read_page(header, NULL);
>  	if (!error)
> -		error = load_image(&handle, &snapshot, header->pages - 1);
> -	put_swap_reader(&handle);
> +		error = load_image(&snapshot, header->pages - 1);
> +	put_swap_reader();
>  end:
>  	if (!error)
>  		pr_debug("PM: Image successfully loaded\n");

-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

Re: [RFC 06/15] PM / Hibernate: swap, remove swap_map_handle usages

[Jiri Slaby]

On 03/24/2010 09:33 PM, Pavel Machek wrote:
> On Tue 2010-03-23 17:17:34, Jiri Slaby wrote:
>> Some code, which will be moved out of swap.c, needs know nothing about
>> swap. There will be also other than swap writers later, so that it
>> won't make sense at all.
>>
>> Make it a global static in swap.c as a singleton.
> 
> I guess I just dislike global static. Logically, methods do operate on
> handles, so...

Ok, "upper layers" may get a handle via .get_reader/writer. The downside
is that they would have to get (void *) and pass (void *) down again. I
wanted to avoid that (taking into account that it's a singleton).

> I don't see a point and I do not think the change is an improvement.

The point was to avoid (void *)'s and save users from transferring
pointer as a handle. No matter what, the decision is not up to me,
discussion indeed welcome.

-- 
js

Re: [RFC 06/15] PM / Hibernate: swap, remove swap_map_handle usages

[Rafael J. Wysocki]

On Wednesday 24 March 2010, Jiri Slaby wrote:
> On 03/24/2010 09:33 PM, Pavel Machek wrote:
> > On Tue 2010-03-23 17:17:34, Jiri Slaby wrote:
> >> Some code, which will be moved out of swap.c, needs know nothing about
> >> swap. There will be also other than swap writers later, so that it
> >> won't make sense at all.
> >>
> >> Make it a global static in swap.c as a singleton.
> > 
> > I guess I just dislike global static. Logically, methods do operate on
> > handles, so...
> 
> Ok, "upper layers" may get a handle via .get_reader/writer. The downside
> is that they would have to get (void *) and pass (void *) down again. I
> wanted to avoid that (taking into account that it's a singleton).
> 
> > I don't see a point and I do not think the change is an improvement.
> 
> The point was to avoid (void *)'s and save users from transferring
> pointer as a handle. No matter what, the decision is not up to me,
> discussion indeed welcome.

The whole thing boils down to whether or not there may be more than one
swap map in use at a time.

Perhaps it's better to use a static pointer, though?

And I don't really know at this point how exactly this change is going to make
your life easier down the road.  Care to elaborate?

Rafael

Re: [RFC 07/15] PM / Hibernate: add sws_modules_ops

[Pavel Machek]

On Tue 2010-03-23 17:17:35, Jiri Slaby wrote:
> For now they will only hold swap operations. In next patches, user
> support will be converted to ops as well to have a single layer and
> can push pages instead of pulling them.

> +struct sws_module_ops {
> +	unsigned long (*storage_available)(void);
> +
> +	int (*get_reader)(unsigned int *flags_p);
> +	int (*put_reader)(void);
> +	int (*get_writer)(void);
> +	int (*put_writer)(unsigned int flags, int error);
> +	int (*read_page)(void *addr, struct bio **bio_chain);
> +	int (*write_page)(void *addr, struct bio **bio_chain);
> +};

sws_ prefix is strange.

Plus, could we get some docs what it does? Stuff like "get_writer" is
not entirely self-documenting.

									Pavel

-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

Re: [RFC 07/15] PM / Hibernate: add sws_modules_ops

[Jiri Slaby]

On 03/24/2010 09:36 PM, Pavel Machek wrote:
> On Tue 2010-03-23 17:17:35, Jiri Slaby wrote:
>> +struct sws_module_ops {
...
> sws_ prefix is strange.

Yeah, noted on this in the previous mail.

> Plus, could we get some docs what it does? Stuff like "get_writer" is
> not entirely self-documenting.

Ah, this I wanted to do. Thanks for the reminder.

-- 
js

Re: [RFC 07/15] PM / Hibernate: add sws_modules_ops

[Rafael J. Wysocki]

On Tuesday 23 March 2010, Jiri Slaby wrote:
> For now they will only hold swap operations. In next patches, user
> support will be converted to ops as well to have a single layer and
> can push pages instead of pulling them.
> 
> Signed-off-by: Jiri Slaby <jslaby@suse.cz>
> Cc: Nigel Cunningham <ncunningham@crca.org.au>
> Cc: "Rafael J. Wysocki" <rjw@sisk.pl>
> ---
>  kernel/power/hibernate.c |    2 +
>  kernel/power/power.h     |   13 +++++++++++
>  kernel/power/swap.c      |   51 ++++++++++++++++++++++++++++++---------------
>  3 files changed, 49 insertions(+), 17 deletions(-)
> 
> diff --git a/kernel/power/hibernate.c b/kernel/power/hibernate.c
> index da5288e..762431e 100644
> --- a/kernel/power/hibernate.c
> +++ b/kernel/power/hibernate.c
> @@ -34,6 +34,8 @@ dev_t swsusp_resume_device;
>  sector_t swsusp_resume_block;
>  int in_suspend __nosavedata = 0;
>  
> +struct sws_module_ops *sws_io_ops;
> +
>  enum {
>  	HIBERNATION_INVALID,
>  	HIBERNATION_PLATFORM,
> diff --git a/kernel/power/power.h b/kernel/power/power.h
> index 6c4b4fa..0f08de4 100644
> --- a/kernel/power/power.h
> +++ b/kernel/power/power.h
> @@ -115,6 +115,17 @@ struct snapshot_handle {
>   */
>  #define data_of(handle)	((handle).buffer)
>  
> +struct sws_module_ops {

I'd call that hibernate_io_ops FWIW.

> +	unsigned long (*storage_available)(void);

free_space() ?

> +
> +	int (*get_reader)(unsigned int *flags_p);
> +	int (*put_reader)(void);
> +	int (*get_writer)(void);
> +	int (*put_writer)(unsigned int flags, int error);

I agree with Pavel that these names are not exactly self-documenting.

Would it make sense to use names like reader_start(), reader_finish() and
similarly for "writer"?

> +	int (*read_page)(void *addr, struct bio **bio_chain);
> +	int (*write_page)(void *addr, struct bio **bio_chain);

Also one might think of adding a data pointer to this structure that will
be passed to the callbacks.  This way it would be easier to think of our
storage spaces as objects, each with a set of methods and a pointer to
the data they operate on.

Rafael

Re: [RFC 09/15] PM / Hibernate: user, implement user_ops writer

[Pavel Machek]

Hi!

> Switch /dev/snapshot writer to sws_module_ops approach so that we
> can transparently rewrite the rest of the snapshot from pages pulling
> to their pushing through layers.
> 
> Signed-off-by: Jiri Slaby <jslaby@suse.cz>
> Cc: Nigel Cunningham <ncunningham@crca.org.au>
> Cc: "Rafael J. Wysocki" <rjw@sisk.pl>
> ---
>  kernel/power/user.c |  113 +++++++++++++++++++++++++++++++++++++++++++++++----
>  1 files changed, 105 insertions(+), 8 deletions(-)
>

> +	if (test_bit(TODO_CLOSED, to_do_flags))
> +		return -EIO;
> +
> +	to_do_buf = buf;
> +	wmb();
> +	set_bit(TODO_WORK, to_do_flags);
> +	wake_up_interruptible(&to_do_wait);

Uhuh, open-coded barriers... these need to be commented, and I guess
you just should not play this kind of trickery.

spnlocks? Normal locks?

> +
> +	wait_event(to_do_done, !test_bit(TODO_WORK, to_do_flags) ||
> +			(err = test_bit(TODO_CLOSED, to_do_flags)));
> +
> +	return err ? -EIO : 0;
> +}
> +
> +static int put_user_writer(unsigned int flags, int error)
> +{
> +	int err = 0;
> +
> +	if (error)
> +		set_bit(TODO_ERROR, to_do_flags);
> +	set_bit(TODO_FINISH, to_do_flags);
> +	wake_up_interruptible(&to_do_wait);
> +
> +	wait_event(to_do_done, !test_bit(TODO_FINISH, to_do_flags) ||
> +			(err = test_bit(TODO_CLOSED, to_do_flags)));
> +
> +	if (!error && err)
> +		error = -EIO;
> +
> +	return error;
> +}
> +
>  struct sws_module_ops user_ops = {
									Pavel

-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

Re: [RFC 09/15] PM / Hibernate: user, implement user_ops writer

[Jiri Slaby]

On 03/24/2010 09:42 PM, Pavel Machek wrote:
>> +	if (test_bit(TODO_CLOSED, to_do_flags))
>> +		return -EIO;
>> +
>> +	to_do_buf = buf;
>> +	wmb();
>> +	set_bit(TODO_WORK, to_do_flags);
>> +	wake_up_interruptible(&to_do_wait);
> 
> Uhuh, open-coded barriers... these need to be commented, and I guess
> you just should not play this kind of trickery.

It's just to ensure the to_do_buf store is not reordered with the
set_bit. I wanted to avoid locks as too heavy tools here.

thanks,
-- 
js

Re: [RFC 09/15] PM / Hibernate: user, implement user_ops writer

[Pavel Machek]

On Wed 2010-03-24 22:40:56, Jiri Slaby wrote:
> On 03/24/2010 09:42 PM, Pavel Machek wrote:
> >> +	if (test_bit(TODO_CLOSED, to_do_flags))
> >> +		return -EIO;
> >> +
> >> +	to_do_buf = buf;
> >> +	wmb();
> >> +	set_bit(TODO_WORK, to_do_flags);
> >> +	wake_up_interruptible(&to_do_wait);
> > 
> > Uhuh, open-coded barriers... these need to be commented, and I guess
> > you just should not play this kind of trickery.
> 
> It's just to ensure the to_do_buf store is not reordered with the
> set_bit. I wanted to avoid locks as too heavy tools here.

Locks are the only sane choice here. Open coding them is not an option.
									Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

Re: [RFC 09/15] PM / Hibernate: user, implement user_ops writer

[Rafael J. Wysocki]

On Wednesday 24 March 2010, Jiri Slaby wrote:
> On 03/24/2010 09:42 PM, Pavel Machek wrote:
> >> +	if (test_bit(TODO_CLOSED, to_do_flags))
> >> +		return -EIO;
> >> +
> >> +	to_do_buf = buf;
> >> +	wmb();
> >> +	set_bit(TODO_WORK, to_do_flags);
> >> +	wake_up_interruptible(&to_do_wait);
> > 
> > Uhuh, open-coded barriers... these need to be commented, and I guess
> > you just should not play this kind of trickery.
> 
> It's just to ensure the to_do_buf store is not reordered with the
> set_bit. I wanted to avoid locks as too heavy tools here.

No, please use them, at least in a prototype version.

We can always optimize things out later, but doing optimizations upfront
doesn't really work well from my experience.

So, if you'd use a lock somewhere, please use it, or maybe use a completion if
that fits the design better.  In the majority of cases it's not as heavy wieght
as it seems at first sight.

Rafael

Re: [RFC 09/15] PM / Hibernate: user, implement user_ops writer

[Jiri Slaby]

On 03/25/2010 11:14 PM, Rafael J. Wysocki wrote:
> On Wednesday 24 March 2010, Jiri Slaby wrote:
>> On 03/24/2010 09:42 PM, Pavel Machek wrote:
>>>> +	if (test_bit(TODO_CLOSED, to_do_flags))
>>>> +		return -EIO;
>>>> +
>>>> +	to_do_buf = buf;
>>>> +	wmb();
>>>> +	set_bit(TODO_WORK, to_do_flags);
>>>> +	wake_up_interruptible(&to_do_wait);
>>>
>>> Uhuh, open-coded barriers... these need to be commented, and I guess
>>> you just should not play this kind of trickery.
>>
>> It's just to ensure the to_do_buf store is not reordered with the
>> set_bit. I wanted to avoid locks as too heavy tools here.
> 
> No, please use them, at least in a prototype version.
> 
> We can always optimize things out later, but doing optimizations upfront
> doesn't really work well from my experience.
> 
> So, if you'd use a lock somewhere, please use it, or maybe use a completion if
> that fits the design better.

That's it, I don't think a lock is appropriate here (I didn't even think
of that) -- I don't know what to lock (OK, I see it, but it's not that
clear). There is no potential for race per se, I only need to disable
reordering (which locks do as a side-effect). I need the steps to be
done in the A-B order where there is a barrier appropriate. Here, A is
store to to_do_buf, B is set_bit. It's I set to_do_buf, flag that it may
be used, the consumer will see the flag and use to_do_buf, in this order.

Above that if I introduce locks the wait_event on the other side will
grow into an unreadable mess. I would need to hold a lock when checking
the condition and hold it until I reach to_do_buf use, but also unlock
it on all paths that do not reach that point. Yeah, it's indeed doable,
but I don't think, it will improve things.

I also don't think completion is appropriate here, as I have a condition
to check for and it differs over wake_up sites.

thanks,
-- 
js

Re: [RFC 09/15] PM / Hibernate: user, implement user_ops writer

[Rafael J. Wysocki]

On Friday 26 March 2010, Jiri Slaby wrote:
> On 03/25/2010 11:14 PM, Rafael J. Wysocki wrote:
> > On Wednesday 24 March 2010, Jiri Slaby wrote:
> >> On 03/24/2010 09:42 PM, Pavel Machek wrote:
> >>>> +	if (test_bit(TODO_CLOSED, to_do_flags))
> >>>> +		return -EIO;
> >>>> +
> >>>> +	to_do_buf = buf;
> >>>> +	wmb();
> >>>> +	set_bit(TODO_WORK, to_do_flags);
> >>>> +	wake_up_interruptible(&to_do_wait);
> >>>
> >>> Uhuh, open-coded barriers... these need to be commented, and I guess
> >>> you just should not play this kind of trickery.
> >>
> >> It's just to ensure the to_do_buf store is not reordered with the
> >> set_bit. I wanted to avoid locks as too heavy tools here.
> > 
> > No, please use them, at least in a prototype version.
> > 
> > We can always optimize things out later, but doing optimizations upfront
> > doesn't really work well from my experience.
> > 
> > So, if you'd use a lock somewhere, please use it, or maybe use a completion if
> > that fits the design better.
> 
> That's it, I don't think a lock is appropriate here (I didn't even think
> of that) -- I don't know what to lock (OK, I see it, but it's not that
> clear). There is no potential for race per se, I only need to disable
> reordering (which locks do as a side-effect). I need the steps to be
> done in the A-B order where there is a barrier appropriate. Here, A is
> store to to_do_buf, B is set_bit. It's I set to_do_buf, flag that it may
> be used, the consumer will see the flag and use to_do_buf, in this order.
> 
> Above that if I introduce locks the wait_event on the other side will
> grow into an unreadable mess. I would need to hold a lock when checking
> the condition and hold it until I reach to_do_buf use, but also unlock
> it on all paths that do not reach that point. Yeah, it's indeed doable,
> but I don't think, it will improve things.
> 
> I also don't think completion is appropriate here, as I have a condition
> to check for and it differs over wake_up sites.

OK

I have some other comments to this patch, but I'd like to understand what
really happens here, since the changelog is not too verbose.

Please explain the design here.

Thanks,
Rafael

Re: [RFC 09/15] PM / Hibernate: user, implement user_ops writer

[Pavel Machek]

On Fri 2010-03-26 10:34:59, Jiri Slaby wrote:
> On 03/25/2010 11:14 PM, Rafael J. Wysocki wrote:
> > On Wednesday 24 March 2010, Jiri Slaby wrote:
> >> On 03/24/2010 09:42 PM, Pavel Machek wrote:
> >>>> +	if (test_bit(TODO_CLOSED, to_do_flags))
> >>>> +		return -EIO;
> >>>> +
> >>>> +	to_do_buf = buf;
> >>>> +	wmb();
> >>>> +	set_bit(TODO_WORK, to_do_flags);
> >>>> +	wake_up_interruptible(&to_do_wait);
> >>>
> >>> Uhuh, open-coded barriers... these need to be commented, and I guess
> >>> you just should not play this kind of trickery.
> >>
> >> It's just to ensure the to_do_buf store is not reordered with the
> >> set_bit. I wanted to avoid locks as too heavy tools here.
> > 
> > No, please use them, at least in a prototype version.
> > 
> > We can always optimize things out later, but doing optimizations upfront
> > doesn't really work well from my experience.
> > 
> > So, if you'd use a lock somewhere, please use it, or maybe use a completion if
> > that fits the design better.
> 
> That's it, I don't think a lock is appropriate here (I didn't even think
> of that) -- I don't know what to lock (OK, I see it, but it's not that
> clear). There is no potential for race per se, I only need to disable
> reordering (which locks do as a side-effect). I need the steps to be
> done in the A-B order where there is a barrier appropriate. Here, A is
> store to to_do_buf, B is set_bit. It's I set to_do_buf, flag that it may
> be used, the consumer will see the flag and use to_do_buf, in this
> order.

Could you just use same locking user.c uses? It does not really do any
magic...
									Pavel

-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

Re: [RFC 09/15] PM / Hibernate: user, implement user_ops writer

[Jiri Slaby]

On 03/26/2010 11:04 PM, Rafael J. Wysocki wrote:
> I have some other comments to this patch, but I'd like to understand what
> really happens here, since the changelog is not too verbose.
> 
> Please explain the design here.

Yeah, my bad.

What I want to achieve is to revert the behaviour of current
implementation to the one used in toi. Now, it is that when writing a
page to the swap, the hibernation does snapshot_read_page(page) with
swap_write_page(page) in a loop. This is OK until one needs to do
anything with the page.

When compression or encryption (or both) are needed to be done, this
scenario does not work well because page returned by snapshot_read_page,
after going through the crypto layers, is no longer PAGE_SIZE big.

Probably the easiest solution is to revert it as noted above: a page is
taken from snapshot (with patches I have here the snapshot layer is only
told to "store next page" without returning a page to the caller), fed
through crypto layers as needed and finally given to chunk writer which
assembles PAGE_SIZE blocks from the chunks. Then whole pages of
compressed/encrypted data are given to user or in-kernel block io by
hibernate_io_ops->write_page. In-kernel .write_page simply calls
swap_write_page (which in turn hib_bio_write_page while storing swap
sector entries).

User .write_page, with buf as a parameter, does the following:
* to_do_buf = buf
* set WORK bit
* wake_up .read (below)
* wait_for WORK bit clear

Writer in this case is a user process performing fops->read. .read does
the following:
* wait_for WORK bit
* copy_to_user to_do_buf
* clear WORK bit
* wake_up .write_page


I need the barrier to ensure the "to_do_buf = buf" assignment is not
reordered with the "set WORK bit" in .write_page. Otherwise .read will
see WORK bit set, but have to_do_buf not updated. I certainly can lock
the two operation together, but (a) I see no point in doing so; (b) it
makes the code worse (I need to unlock and relock in wait_event and
unlock on all fail paths).


The very similar happens for fops->write, ergo hibernate_io_ops->read_page.

thanks,
-- 
js

Re: [RFC 09/15] PM / Hibernate: user, implement user_ops writer

[Rafael J. Wysocki]

On Monday 29 March 2010, Jiri Slaby wrote:
> On 03/26/2010 11:04 PM, Rafael J. Wysocki wrote:
> > I have some other comments to this patch, but I'd like to understand what
> > really happens here, since the changelog is not too verbose.
> > 
> > Please explain the design here.
> 
> Yeah, my bad.
> 
> What I want to achieve is to revert the behaviour of current
> implementation to the one used in toi. Now, it is that when writing a
> page to the swap, the hibernation does snapshot_read_page(page) with
> swap_write_page(page) in a loop. This is OK until one needs to do
> anything with the page.
> 
> When compression or encryption (or both) are needed to be done, this
> scenario does not work well because page returned by snapshot_read_page,
> after going through the crypto layers, is no longer PAGE_SIZE big.

Actually, s2disk has the same problem, because it only receives entire pages
from the kernel and it needs to compress/encrypt them before writing to the
storage (which also is written in blocks that are page size-aligned).  Still,
s2disk uses snapshot_[read\write]_next() through the functions in user.c, so
I don't think we need to reorder things in there.

> Probably the easiest solution is to revert it as noted above: a page is
> taken from snapshot (with patches I have here the snapshot layer is only
> told to "store next page" without returning a page to the caller), fed
> through crypto layers as needed and finally given to chunk writer which
> assembles PAGE_SIZE blocks from the chunks. Then whole pages of
> compressed/encrypted data are given to user or in-kernel block io by
> hibernate_io_ops->write_page. In-kernel .write_page simply calls
> swap_write_page (which in turn hib_bio_write_page while storing swap
> sector entries).

That's fine if I understand correctly.

> User .write_page, with buf as a parameter, does the following:
> * to_do_buf = buf
> * set WORK bit
> * wake_up .read (below)
> * wait_for WORK bit clear
> 
> Writer in this case is a user process performing fops->read. .read does
> the following:
> * wait_for WORK bit
> * copy_to_user to_do_buf
> * clear WORK bit
> * wake_up .write_page
> 
> I need the barrier to ensure the "to_do_buf = buf" assignment is not
> reordered with the "set WORK bit" in .write_page. Otherwise .read will
> see WORK bit set, but have to_do_buf not updated. I certainly can lock
> the two operation together, but (a) I see no point in doing so; (b) it
> makes the code worse (I need to unlock and relock in wait_event and
> unlock on all fail paths).
> 
> The very similar happens for fops->write, ergo hibernate_io_ops->read_page.

You can't open /dev/snapshot for reading and writing at the same time, because
that wouldn't make any sense.

/dev/snapshot represents a hibernation image which is presented to user space
as a "file", because the user space is then able to use "regular" read() and
write() on it.  However, the situation during hibernation is much different
than during resume.  Namely, during hibernation the image is there in memory
and it can't be modified.  It can only be read and written to the storage.  So,
at this stage it only makes sense to open /dev/snapshot for reading.

Now, during resume the image is not present in memory at all.  In fact, we
don't even use any contiguous region to store the pages read from the on-disk
image.  Instead, the pages are put directly into their destination page frames,
if they are free.  Otherwise, they are put into free page frames and then
copied back to the target ones right before returning control to the image
kernel.  So during resume there's nothing that could be read from /dev/snapshot
and you can only open it for writing.

The idea is that snapshot_read_next() will only be used during hibernation
for reading all of the image pages as contiguous stream of data (the real image
is not contiguous in general), because that's what the user space needs.  Then,
the observation goes that the swap.c functions can use the same mechanism,
although that is not really necessary in principle.

Analogously, snapshot_write_next() is only useful during resume for writing
all of the image pages as a contiguous stream of data, because that's needed
by the user space and _can_ be used by the in-kernel code as well.

[Note that snapshot_[read|write]_next() belong to the memory management
part of the hibernation subsystem and that's exactly why they are in
snapshot.c.]

Now, compression can happen in two places: while the image is created
or after it has been created (current behavior).  In the latter case, the image
pages need not be compressed in place, they may be compressed after being
returned by snapshot_read_next(), in a temporary buffer (that's now s2disk
does that).  So you can arrange things like this:

create image
repeat:
- snapshot_read_next() -> buffer
- if buffer is full, compress it (possibly encrypt it) and write the result to
  the storage

This way you'd just avoid all of the complications and I fail to see any
drawbacks.

Now, an attractive thing would be to compress data while creating the image
and that may be done in the following way:

have a buffer ready
repeat:
- copy image pages to the buffer (instead of copying them directly into the
  image storage space)
- if buffer is full, compress it and copy the result to the image storage
  space, page by page.

Thanks,
Rafael

Re: [RFC 09/15] PM / Hibernate: user, implement user_ops writer

[Nigel Cunningham]

Hi.

On 30/03/10 09:09, Rafael J. Wysocki wrote:
> On Monday 29 March 2010, Jiri Slaby wrote:
>> On 03/26/2010 11:04 PM, Rafael J. Wysocki wrote:
>>> I have some other comments to this patch, but I'd like to understand what
>>> really happens here, since the changelog is not too verbose.
>>>
>>> Please explain the design here.
>>
>> Yeah, my bad.
>>
>> What I want to achieve is to revert the behaviour of current
>> implementation to the one used in toi. Now, it is that when writing a
>> page to the swap, the hibernation does snapshot_read_page(page) with
>> swap_write_page(page) in a loop. This is OK until one needs to do
>> anything with the page.
>>
>> When compression or encryption (or both) are needed to be done, this
>> scenario does not work well because page returned by snapshot_read_page,
>> after going through the crypto layers, is no longer PAGE_SIZE big.
>
> Actually, s2disk has the same problem, because it only receives entire pages
> from the kernel and it needs to compress/encrypt them before writing to the
> storage (which also is written in blocks that are page size-aligned).  Still,
> s2disk uses snapshot_[read\write]_next() through the functions in user.c, so
> I don't think we need to reorder things in there.
>
>> Probably the easiest solution is to revert it as noted above: a page is
>> taken from snapshot (with patches I have here the snapshot layer is only
>> told to "store next page" without returning a page to the caller), fed
>> through crypto layers as needed and finally given to chunk writer which
>> assembles PAGE_SIZE blocks from the chunks. Then whole pages of
>> compressed/encrypted data are given to user or in-kernel block io by
>> hibernate_io_ops->write_page. In-kernel .write_page simply calls
>> swap_write_page (which in turn hib_bio_write_page while storing swap
>> sector entries).
>
> That's fine if I understand correctly.
>
>> User .write_page, with buf as a parameter, does the following:
>> * to_do_buf = buf
>> * set WORK bit
>> * wake_up .read (below)
>> * wait_for WORK bit clear
>>
>> Writer in this case is a user process performing fops->read. .read does
>> the following:
>> * wait_for WORK bit
>> * copy_to_user to_do_buf
>> * clear WORK bit
>> * wake_up .write_page
>>
>> I need the barrier to ensure the "to_do_buf = buf" assignment is not
>> reordered with the "set WORK bit" in .write_page. Otherwise .read will
>> see WORK bit set, but have to_do_buf not updated. I certainly can lock
>> the two operation together, but (a) I see no point in doing so; (b) it
>> makes the code worse (I need to unlock and relock in wait_event and
>> unlock on all fail paths).
>>
>> The very similar happens for fops->write, ergo hibernate_io_ops->read_page.

I'm still trying to find the time to wrap my head around the uswsusp 
interface and locking bit (Easter is a busy time for me), so I'll not 
say anything here.

> You can't open /dev/snapshot for reading and writing at the same time, because
> that wouldn't make any sense.
>
> /dev/snapshot represents a hibernation image which is presented to user space
> as a "file", because the user space is then able to use "regular" read() and
> write() on it.  However, the situation during hibernation is much different
> than during resume.  Namely, during hibernation the image is there in memory
> and it can't be modified.  It can only be read and written to the storage.  So,
> at this stage it only makes sense to open /dev/snapshot for reading.
>
> Now, during resume the image is not present in memory at all.  In fact, we
> don't even use any contiguous region to store the pages read from the on-disk
> image.  Instead, the pages are put directly into their destination page frames,
> if they are free.  Otherwise, they are put into free page frames and then
> copied back to the target ones right before returning control to the image
> kernel.  So during resume there's nothing that could be read from /dev/snapshot
> and you can only open it for writing.
>
> The idea is that snapshot_read_next() will only be used during hibernation
> for reading all of the image pages as contiguous stream of data (the real image
> is not contiguous in general), because that's what the user space needs.  Then,
> the observation goes that the swap.c functions can use the same mechanism,
> although that is not really necessary in principle.
>
> Analogously, snapshot_write_next() is only useful during resume for writing
> all of the image pages as a contiguous stream of data, because that's needed
> by the user space and _can_ be used by the in-kernel code as well.

Okay. So the read or write terminology is from userspace's perspective, 
right? Perhaps that's part of what I've found confusing about the kernel 
side of things.

> [Note that snapshot_[read|write]_next() belong to the memory management
> part of the hibernation subsystem and that's exactly why they are in
> snapshot.c.]
>
> Now, compression can happen in two places: while the image is created
> or after it has been created (current behavior).  In the latter case, the image
> pages need not be compressed in place, they may be compressed after being
> returned by snapshot_read_next(), in a temporary buffer (that's now s2disk
> does that).  So you can arrange things like this:
>
> create image
> repeat:
> - snapshot_read_next() ->  buffer
> - if buffer is full, compress it (possibly encrypt it) and write the result to
>    the storage
>
> This way you'd just avoid all of the complications and I fail to see any
> drawbacks.
>
> Now, an attractive thing would be to compress data while creating the image
> and that may be done in the following way:
>
> have a buffer ready
> repeat:
> - copy image pages to the buffer (instead of copying them directly into the
>    image storage space)
> - if buffer is full, compress it and copy the result to the image storage
>    space, page by page.

A few points that might be worth considering:

Wouldn't compressing the image while creating it rather than while 
writing increase the overall time taken to hibernate (since the time 
taken can't then be combined with the time for writing the image)?

Wouldn't it also increase the memory requirements? I realise that this 
may not be an issue when we're thinking of freeing memory anyway, but if 
we eventually do the TuxOnIce thing of trying not to free memory and 
writing a two part image...

TuxOnIce has support for multithreaded compression, which is proving 
particularly useful as storage speeds up. At the moment, I have threads 
bound to secondary CPUs doing the compression and the boot cpu's thread 
coalescing their output into pages that are then written to disk 
(reverse at resume time, with readahead to maximise throughput). Doing 
the compression at image creation time would disallow multithreaded 
compression and slow things down.

Hope that helps.

Nigel

Re: [RFC 09/15] PM / Hibernate: user, implement user_ops writer

[Rafael J. Wysocki]

On Tuesday 30 March 2010, Nigel Cunningham wrote:
> Hi.
> 
> On 30/03/10 09:09, Rafael J. Wysocki wrote:
> > On Monday 29 March 2010, Jiri Slaby wrote:
... 
> Okay. So the read or write terminology is from userspace's perspective, 
> right?

Yes.

> Perhaps that's part of what I've found confusing about the kernel side of things.
> 
> > [Note that snapshot_[read|write]_next() belong to the memory management
> > part of the hibernation subsystem and that's exactly why they are in
> > snapshot.c.]
> >
> > Now, compression can happen in two places: while the image is created
> > or after it has been created (current behavior).  In the latter case, the image
> > pages need not be compressed in place, they may be compressed after being
> > returned by snapshot_read_next(), in a temporary buffer (that's now s2disk
> > does that).  So you can arrange things like this:
> >
> > create image
> > repeat:
> > - snapshot_read_next() ->  buffer
> > - if buffer is full, compress it (possibly encrypt it) and write the result to
> >    the storage
> >
> > This way you'd just avoid all of the complications and I fail to see any
> > drawbacks.
> >
> > Now, an attractive thing would be to compress data while creating the image
> > and that may be done in the following way:
> >
> > have a buffer ready
> > repeat:
> > - copy image pages to the buffer (instead of copying them directly into the
> >    image storage space)
> > - if buffer is full, compress it and copy the result to the image storage
> >    space, page by page.
> 
> A few points that might be worth considering:
> 
> Wouldn't compressing the image while creating it rather than while 
> writing increase the overall time taken to hibernate (since the time 
> taken can't then be combined with the time for writing the image)?

It would, but that's attractive anyway, because the image could be larger than
1/2 of memory this way without using the LRU pages as temporary storage
space (which I admit I'm reluctant to do).

> Wouldn't it also increase the memory requirements?

Not really, or just a little bit (the size of the buffer).  I'm talking about
the image that's created atomically after we've frozen devices.

Rafael

Re: [RFC 09/15] PM / Hibernate: user, implement user_ops writer

[Nigel Cunningham]

Hi.

On 31/03/10 08:03, Rafael J. Wysocki wrote:
>>> Now, an attractive thing would be to compress data while creating the image
>>> and that may be done in the following way:
>>>
>>> have a buffer ready
>>> repeat:
>>> - copy image pages to the buffer (instead of copying them directly into the
>>>     image storage space)
>>> - if buffer is full, compress it and copy the result to the image storage
>>>     space, page by page.
>>
>> A few points that might be worth considering:
>>
>> Wouldn't compressing the image while creating it rather than while
>> writing increase the overall time taken to hibernate (since the time
>> taken can't then be combined with the time for writing the image)?
>
> It would, but that's attractive anyway, because the image could be larger than
> 1/2 of memory this way without using the LRU pages as temporary storage
> space (which I admit I'm reluctant to do).
>
>> Wouldn't it also increase the memory requirements?
>
> Not really, or just a little bit (the size of the buffer).  I'm talking about
> the image that's created atomically after we've frozen devices.

The buffer would be the size of the compressed image. Assuming you rely 
on 50+% compression, you'll still need to ensure that at least 1/3 of 
memory is available for the buffer for the compressed data. This would 
give maximum image size of only 1/6th of memory more than without 
compression - not much of a gain.

It's also ugly because if you find that you don't achieve the expected 
compression, you'll need to undo the going atomic, free some more memory 
and try again - or just give up (which I hope you won't consider to be a 
real option).

Regarding using LRU pages as temporary storage, if it wasn't safe and 
reliable, I would have stopped doing it ages ago. The only problem I can 
recall having was when KMS was introduced, and that was solved by adding 
a very small amount of code to let us find those pages and make sure 
they're atomically copied instead. We did have a period where we were 
paranoid about the issue - the support for checksumming LRU pages prior 
to writing the LRU, then checking the checksums prior to the atomic copy 
is still in TuxOnIce and used. Searching through my mail archives, I'm 
unable to find anyone with a debugging message that says something other 
than "0 pages resaved in atomic copy".

Regards,

Nigel

Re: [RFC 09/15] PM / Hibernate: user, implement user_ops writer

[Pavel Machek]

Hi!

> Regarding using LRU pages as temporary storage, if it wasn't safe and 
> reliable, I would have stopped doing it ages ago. The only problem I can 

What kind of argument is that?

And what prevents people from introducing more KMS-like breakage in
future?
								Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

Re: [RFC 09/15] PM / Hibernate: user, implement user_ops writer

[Nigel Cunningham]

Hi.

On 01/04/10 01:47, Pavel Machek wrote:
> Hi!
>
>> Regarding using LRU pages as temporary storage, if it wasn't safe and
>> reliable, I would have stopped doing it ages ago. The only problem I can
>
> What kind of argument is that?
>
> And what prevents people from introducing more KMS-like breakage in
> future?

Nothing. And we catch it when it occurs and resave the pages. It is of 
course also possible for us to take a more complicated approach and save 
just what we know is safe.

Or we could just use kexec...

Nigel

Re: [RFC 09/15] PM / Hibernate: user, implement user_ops writer

[Rafael J. Wysocki]

On Wednesday 31 March 2010, Nigel Cunningham wrote:
> Hi.
> 
> On 31/03/10 08:03, Rafael J. Wysocki wrote:
> >>> Now, an attractive thing would be to compress data while creating the image
> >>> and that may be done in the following way:
> >>>
> >>> have a buffer ready
> >>> repeat:
> >>> - copy image pages to the buffer (instead of copying them directly into the
> >>>     image storage space)
> >>> - if buffer is full, compress it and copy the result to the image storage
> >>>     space, page by page.
> >>
> >> A few points that might be worth considering:
> >>
> >> Wouldn't compressing the image while creating it rather than while
> >> writing increase the overall time taken to hibernate (since the time
> >> taken can't then be combined with the time for writing the image)?
> >
> > It would, but that's attractive anyway, because the image could be larger than
> > 1/2 of memory this way without using the LRU pages as temporary storage
> > space (which I admit I'm reluctant to do).
> >
> >> Wouldn't it also increase the memory requirements?
> >
> > Not really, or just a little bit (the size of the buffer).  I'm talking about
> > the image that's created atomically after we've frozen devices.
> 
> The buffer would be the size of the compressed image.

Not necessarily if the image is compressed in chunks.  According to
measurements I did some time ago, 256 KiB chunks were sufficient.

> Assuming you rely on 50+% compression, you'll still need to ensure that at
> least 1/3 of memory is available for the buffer for the compressed data.

That's correct.

> This would give maximum image size of only 1/6th of memory more than without 
> compression - not much of a gain.

Still, on a 1 GiB machine that's about 170 MiB which is quite some data.

> It's also ugly because if you find that you don't achieve the expected 
> compression, you'll need to undo the going atomic, free some more memory 
> and try again - or just give up (which I hope you won't consider to be a 
> real option).

>From my experience we can safely assume 50% compression in all cases.

> Regarding using LRU pages as temporary storage, if it wasn't safe and 
> reliable, I would have stopped doing it ages ago.

We've been through that already and as you can see I'm still not convinced.
Sorry, but that's how it goes.  The fact that ToI uses this approach without
seeing any major breakage is a good indication that it _may_ be safe in
general, not that it _is_ safe in all cases one can imagine.

Besides, that would be a constraint on the future changes of the mm subsystem
that I'm not sure we should introduce.  At least the mm people would need to
accept that and there's a long way before we're even able to ask them.

Rafael

Re: [RFC 09/15] PM / Hibernate: user, implement user_ops writer

[Nigel Cunningham]

Hi.

On 01/04/10 07:25, Rafael J. Wysocki wrote:
> On Wednesday 31 March 2010, Nigel Cunningham wrote:
>> Hi.
>>
>> On 31/03/10 08:03, Rafael J. Wysocki wrote:
>>>>> Now, an attractive thing would be to compress data while creating the image
>>>>> and that may be done in the following way:
>>>>>
>>>>> have a buffer ready
>>>>> repeat:
>>>>> - copy image pages to the buffer (instead of copying them directly into the
>>>>>      image storage space)
>>>>> - if buffer is full, compress it and copy the result to the image storage
>>>>>      space, page by page.
>>>>
>>>> A few points that might be worth considering:
>>>>
>>>> Wouldn't compressing the image while creating it rather than while
>>>> writing increase the overall time taken to hibernate (since the time
>>>> taken can't then be combined with the time for writing the image)?
>>>
>>> It would, but that's attractive anyway, because the image could be larger than
>>> 1/2 of memory this way without using the LRU pages as temporary storage
>>> space (which I admit I'm reluctant to do).
>>>
>>>> Wouldn't it also increase the memory requirements?
>>>
>>> Not really, or just a little bit (the size of the buffer).  I'm talking about
>>> the image that's created atomically after we've frozen devices.
>>
>> The buffer would be the size of the compressed image.
>
> Not necessarily if the image is compressed in chunks.  According to
> measurements I did some time ago, 256 KiB chunks were sufficient.

I must be missing something. You're talking about doing compression of 
the image during the atomic copy, right? If that's the case, where do 
the 256KiB chunks come in?

>> Assuming you rely on 50+% compression, you'll still need to ensure that at
>> least 1/3 of memory is available for the buffer for the compressed data.
>
> That's correct.
>
>> This would give maximum image size of only 1/6th of memory more than without
>> compression - not much of a gain.
>
> Still, on a 1 GiB machine that's about 170 MiB which is quite some data.
>
>> It's also ugly because if you find that you don't achieve the expected
>> compression, you'll need to undo the going atomic, free some more memory
>> and try again - or just give up (which I hope you won't consider to be a
>> real option).
>
>  From my experience we can safely assume 50% compression in all cases.

I've seen it lower in some cases - lots of video or such like in memory. 
But then perhaps that's just because I'm not discarding anything.

>> Regarding using LRU pages as temporary storage, if it wasn't safe and
>> reliable, I would have stopped doing it ages ago.
>
> We've been through that already and as you can see I'm still not convinced.
> Sorry, but that's how it goes.  The fact that ToI uses this approach without
> seeing any major breakage is a good indication that it _may_ be safe in
> general, not that it _is_ safe in all cases one can imagine.

It's not "any major breakage", but no breakage at all over a course of 
about 6 or 7 years of usage. I agree that it's not mathematical proof, 
but still...

> Besides, that would be a constraint on the future changes of the mm subsystem
> that I'm not sure we should introduce.  At least the mm people would need to
> accept that and there's a long way before we're even able to ask them.

It doesn't need to be that way. As with KMS, a simple way of flagging 
which pages need to be atomically copied is all that's necessary.

Regards,

Nigel

Re: [RFC 09/15] PM / Hibernate: user, implement user_ops writer

[Rafael J. Wysocki]

On Wednesday 31 March 2010, Nigel Cunningham wrote:
> Hi.
> 
> On 01/04/10 07:25, Rafael J. Wysocki wrote:
> > On Wednesday 31 March 2010, Nigel Cunningham wrote:
> >> Hi.
> >>
> >> On 31/03/10 08:03, Rafael J. Wysocki wrote:
...
> > Not necessarily if the image is compressed in chunks.  According to
> > measurements I did some time ago, 256 KiB chunks were sufficient.
> 
> I must be missing something. You're talking about doing compression of 
> the image during the atomic copy, right? If that's the case, where do 
> the 256KiB chunks come in?

I thought about an algorithm like this:

Have a sufficient number of free pages to store the compressed image data.
repeat:
- copy image pages to a buffer until it's full
- compress the buffer (perhaps use another buffer to store the result)
- copy the result to the free pages

and hence the buffer.

> >> Assuming you rely on 50+% compression, you'll still need to ensure that at
> >> least 1/3 of memory is available for the buffer for the compressed data.
> >
> > That's correct.
> >
> >> This would give maximum image size of only 1/6th of memory more than without
> >> compression - not much of a gain.
> >
> > Still, on a 1 GiB machine that's about 170 MiB which is quite some data.
> >
> >> It's also ugly because if you find that you don't achieve the expected
> >> compression, you'll need to undo the going atomic, free some more memory
> >> and try again - or just give up (which I hope you won't consider to be a
> >> real option).
> >
> >  From my experience we can safely assume 50% compression in all cases.
> 
> I've seen it lower in some cases - lots of video or such like in memory. 
> But then perhaps that's just because I'm not discarding anything.
> 
> >> Regarding using LRU pages as temporary storage, if it wasn't safe and
> >> reliable, I would have stopped doing it ages ago.
> >
> > We've been through that already and as you can see I'm still not convinced.
> > Sorry, but that's how it goes.  The fact that ToI uses this approach without
> > seeing any major breakage is a good indication that it _may_ be safe in
> > general, not that it _is_ safe in all cases one can imagine.
> 
> It's not "any major breakage", but no breakage at all over a course of 
> about 6 or 7 years of usage. I agree that it's not mathematical proof, 
> but still...

I'd say without any reported breakage you could blame on the usage of LRU
pages.  But I think even if such things were reported, it wouldn't be really
straightforward to track them down to the LRU, because they wouldn't be
reproducible.

> > Besides, that would be a constraint on the future changes of the mm subsystem
> > that I'm not sure we should introduce.  At least the mm people would need to
> > accept that and there's a long way before we're even able to ask them.
> 
> It doesn't need to be that way. As with KMS, a simple way of flagging 
> which pages need to be atomically copied is all that's necessary.

I'm not sure about that.

Besides, assuming that the LRU pages are really safe, I'd prefer to save them
directly as a part of the image along with the atomic copy instead of using
them as temporary storage.

Thanks,
Rafael

Re: [RFC 09/15] PM / Hibernate: user, implement user_ops writer

[Nigel Cunningham]

Hi again.

On 01/04/10 08:36, Rafael J. Wysocki wrote:
>>>> Regarding using LRU pages as temporary storage, if it wasn't safe and
>>>> reliable, I would have stopped doing it ages ago.
>>>
>>> We've been through that already and as you can see I'm still not convinced.
>>> Sorry, but that's how it goes.  The fact that ToI uses this approach without
>>> seeing any major breakage is a good indication that it _may_ be safe in
>>> general, not that it _is_ safe in all cases one can imagine.
>>
>> It's not "any major breakage", but no breakage at all over a course of
>> about 6 or 7 years of usage. I agree that it's not mathematical proof,
>> but still...
>
> I'd say without any reported breakage you could blame on the usage of LRU
> pages.  But I think even if such things were reported, it wouldn't be really
> straightforward to track them down to the LRU, because they wouldn't be
> reproducible.

That depends on the cause. There are only so many things that can change 
the contents of the LRU. Freezing tasks significantly reduces that 
number, and with memory allocation tracking, it shouldn't be that hard 
to figure out what caused the issue - especially when we find the 
problem while atomic and can therefore dump etc data structures without 
worrying about locking. Ugly, yes. But it would reliably find the cause.

>>> Besides, that would be a constraint on the future changes of the mm subsystem
>>> that I'm not sure we should introduce.  At least the mm people would need to
>>> accept that and there's a long way before we're even able to ask them.
>>
>> It doesn't need to be that way. As with KMS, a simple way of flagging
>> which pages need to be atomically copied is all that's necessary.
>
> I'm not sure about that.
>
> Besides, assuming that the LRU pages are really safe, I'd prefer to save them
> directly as a part of the image along with the atomic copy instead of using
> them as temporary storage.

We're obviously not going to agree on this. Can we nevertheless find a 
way ahead in which we're both happy? Seek to minimise the differences, 
even if some of the TuxOnIce code isn't merged?

Regards,

Nigel

Re: [RFC 09/15] PM / Hibernate: user, implement user_ops writer

[Rafael J. Wysocki]

On Monday 05 April 2010, Nigel Cunningham wrote:
> Hi again.
> 
> On 01/04/10 08:36, Rafael J. Wysocki wrote:
> >>>> Regarding using LRU pages as temporary storage, if it wasn't safe and
> >>>> reliable, I would have stopped doing it ages ago.
> >>>
> >>> We've been through that already and as you can see I'm still not convinced.
> >>> Sorry, but that's how it goes.  The fact that ToI uses this approach without
> >>> seeing any major breakage is a good indication that it _may_ be safe in
> >>> general, not that it _is_ safe in all cases one can imagine.
> >>
> >> It's not "any major breakage", but no breakage at all over a course of
> >> about 6 or 7 years of usage. I agree that it's not mathematical proof,
> >> but still...
> >
> > I'd say without any reported breakage you could blame on the usage of LRU
> > pages.  But I think even if such things were reported, it wouldn't be really
> > straightforward to track them down to the LRU, because they wouldn't be
> > reproducible.
> 
> That depends on the cause. There are only so many things that can change 
> the contents of the LRU. Freezing tasks significantly reduces that 
> number, and with memory allocation tracking, it shouldn't be that hard 
> to figure out what caused the issue - especially when we find the 
> problem while atomic and can therefore dump etc data structures without 
> worrying about locking. Ugly, yes. But it would reliably find the cause.

Possibly.

However, I think we can also do something different.  Namely, add a mechanism
tracking modifications of the LRU pages like copy-on-write.  We can safely
assume that the number of LRU pages modified during hibernation will be very
limited, so it should be possible to keep some spare pages for saving them
if need be and use the following high-level hibernation algorithm:
- freeze tasks
- make room for atomic copying of non-LRU memory
- freeze devices
- create the copy of non-LRU memory, start monitoring LRU
- thaw devices
- save the copy of non-LRU memory and the contents of LRU pages
  to the storage; if an LRU page is modified in the process, copy its contents
  to a spare page (before the modification) and save it later (that can be done
  using pages previously used to store the copy of non-LRU memory that have
  been already saved)

That sounds pretty straightforward to me and doesn't seem to depend on
assumptions that would be difficult to verify.

> >>> Besides, that would be a constraint on the future changes of the mm subsystem
> >>> that I'm not sure we should introduce.  At least the mm people would need to
> >>> accept that and there's a long way before we're even able to ask them.
> >>
> >> It doesn't need to be that way. As with KMS, a simple way of flagging
> >> which pages need to be atomically copied is all that's necessary.
> >
> > I'm not sure about that.
> >
> > Besides, assuming that the LRU pages are really safe, I'd prefer to save them
> > directly as a part of the image along with the atomic copy instead of using
> > them as temporary storage.
> 
> We're obviously not going to agree on this. Can we nevertheless find a 
> way ahead in which we're both happy?

I believe so.

> Seek to minimise the differences, even if some of the TuxOnIce code isn't
> merged?

Sure.

Thanks,
Rafael

Re: [RFC 09/15] PM / Hibernate: user, implement user_ops writer

[Nigel Cunningham]

Hi.

On 01/04/10 08:06, Nigel Cunningham wrote:
>>>>> Wouldn't it also increase the memory requirements?
>>>>
>>>> Not really, or just a little bit (the size of the buffer).  I'm talking about
>>>> the image that's created atomically after we've frozen devices.
>>>
>>> The buffer would be the size of the compressed image.
>>
>> Not necessarily if the image is compressed in chunks.  According to
>> measurements I did some time ago, 256 KiB chunks were sufficient.
>
> I must be missing something. You're talking about doing compression of
> the image during the atomic copy, right? If that's the case, where do
> the 256KiB chunks come in?

Ah, we're talking about different things. I wasn't thinking about the 
size of buffers for compression, but instead where this compressed image 
would be stored (leading into the 1/6th extra below).

Regards,

Nigel

Re: [RFC 09/15] PM / Hibernate: user, implement user_ops writer

[Jiri Slaby]

On 03/30/2010 12:09 AM, Rafael J. Wysocki wrote:
> On Monday 29 March 2010, Jiri Slaby wrote:
>> Probably the easiest solution is to revert it as noted above: a page is
>> taken from snapshot (with patches I have here the snapshot layer is only
>> told to "store next page" without returning a page to the caller), fed
>> through crypto layers as needed and finally given to chunk writer which
>> assembles PAGE_SIZE blocks from the chunks. Then whole pages of
>> compressed/encrypted data are given to user or in-kernel block io by
>> hibernate_io_ops->write_page. In-kernel .write_page simply calls
>> swap_write_page (which in turn hib_bio_write_page while storing swap
>> sector entries).
> 
> That's fine if I understand correctly.

(It is in fact the same what s2disk does, except it works on a single
page and the block io chaining is up to the block layer.)

>> The very similar happens for fops->write, ergo hibernate_io_ops->read_page.
> 
> You can't open /dev/snapshot for reading and writing at the same time, because
> that wouldn't make any sense.

Err, I didn't mean to write that. fops->read and
hibernate_io_ops->write_page are both involved solely in image writing
and are consumer and producer respectively. Vice versa for image reading.

> Now, during resume the image is not present in memory at all.  In fact, we

<snipped> -- we both understand the code the same.

> Now, compression can happen in two places: while the image is created
> or after it has been created (current behavior).  In the latter case, the image
> pages need not be compressed in place, they may be compressed after being
> returned by snapshot_read_next(), in a temporary buffer (that's now s2disk
> does that).  So you can arrange things like this:
> 
> create image
> repeat:
> - snapshot_read_next() -> buffer
> - if buffer is full, compress it (possibly encrypt it) and write the result to
>   the storage
> 
> This way you'd just avoid all of the complications and I fail to see any
> drawbacks.

Yes, this was the intention. Except I wanted snapshot_read_next to be
something like snapshot_write_next_page which would call
hibernate_io_ops->write_page(buf, len) somewhere in the deep.
hibernate_io_ops is an alias for the first module which accepts the page
and feeds it further. E.g. for hibernate_io_ops being compress_ops it
may be a chain like compress_ops->write_page => encrypt_ops->write_page
=> swap_ops->write_page.

But if you want to preserve snapshot_read_next, then it would look like
repeat:
  snapshot_read_next() -> buffer, len = PAGE_SIZE
  compress_ops->write_page(buffer, len) =>
    encrypt_ops->write_page(buffer, len) =>
    swap_ops->write_page(buffer, len)

instead of
repeat:
  snapshot_write_next_page()
In this case its work is to fetch a next page and call appropriate
.write_page.

> Now, an attractive thing would be to compress data while creating the image
> and that may be done in the following way:

I wouldn't go for this. We should balance I/O and CPU and this can be
done only when writing the image, if I can say. OTOH I must admit I have
no numbers.

thanks,
-- 
js

Re: [RFC 09/15] PM / Hibernate: user, implement user_ops writer

[Rafael J. Wysocki]

On Tuesday 30 March 2010, Jiri Slaby wrote:
> On 03/30/2010 12:09 AM, Rafael J. Wysocki wrote:
> > On Monday 29 March 2010, Jiri Slaby wrote:
...
> Err, I didn't mean to write that. fops->read and
> hibernate_io_ops->write_page are both involved solely in image writing
> and are consumer and producer respectively. Vice versa for image reading.

Ah, I misunderstood your description, then.

> > Now, during resume the image is not present in memory at all.  In fact, we
> 
> <snipped> -- we both understand the code the same.
> 
> > Now, compression can happen in two places: while the image is created
> > or after it has been created (current behavior).  In the latter case, the image
> > pages need not be compressed in place, they may be compressed after being
> > returned by snapshot_read_next(), in a temporary buffer (that's now s2disk
> > does that).  So you can arrange things like this:
> > 
> > create image
> > repeat:
> > - snapshot_read_next() -> buffer
> > - if buffer is full, compress it (possibly encrypt it) and write the result to
> >   the storage
> > 
> > This way you'd just avoid all of the complications and I fail to see any
> > drawbacks.
> 
> Yes, this was the intention. Except I wanted snapshot_read_next to be
> something like snapshot_write_next_page which would call
> hibernate_io_ops->write_page(buf, len) somewhere in the deep.
> hibernate_io_ops is an alias for the first module which accepts the page
> and feeds it further. E.g. for hibernate_io_ops being compress_ops it
> may be a chain like compress_ops->write_page => encrypt_ops->write_page
> => swap_ops->write_page.

Well, but I don't think we need the kernel compression to be used by s2disk
(unless it's done while creating the image, but see below).  So we can
assume the s2disk case will always operate full pages.

> But if you want to preserve snapshot_read_next, then it would look like
> repeat:
>   snapshot_read_next() -> buffer, len = PAGE_SIZE
>   compress_ops->write_page(buffer, len) =>
>     encrypt_ops->write_page(buffer, len) =>
>     swap_ops->write_page(buffer, len)
> 
> instead of
> repeat:
>   snapshot_write_next_page()
> In this case its work is to fetch a next page and call appropriate
> .write_page.

Yes, that'd be fine.

> > Now, an attractive thing would be to compress data while creating the image
> > and that may be done in the following way:
> 
> I wouldn't go for this. We should balance I/O and CPU and this can be
> done only when writing the image, if I can say. OTOH I must admit I have
> no numbers.

_But_ that would allow us to break the 1/2 memory barrier without doing arcane
things in the memory management area.

I did the measurements when I added the multithreaded image writing to s2disk
and I must say a substantial gain from using multiple threads was only in the
entryption+compression case.

So, I definitely would like to see numbers.

Rafael

Re: [RFC 09/15] PM / Hibernate: user, implement user_ops writer

[Jiri Slaby]

[-- Attachment #1: Type: text/plain, Size: 900 bytes --]

On 03/30/2010 10:50 PM, Rafael J. Wysocki wrote:
> So, I definitely would like to see numbers.

With the patch attached, I get similar results for
uncompressed image -> user.c -> s2disk (compress+threads)
compressed image -> user.c -> s2disk (none of compress+threads)

BUT note that there are differences in pages _copied_ in the "user.c ->
s2disk" phase. Compression was about 40 %. So in the former case 100000
pages went through and in the latter one only ~ 38000. So presumably
there will be savings if the pages are compressed in the kernel in the
save-image phase instead of in userspace.

I'll test (after I hack that up) also the case of image compression when
saving the image and let you know.

It was tested on a machine with openSUSE factory with init 5 with KDE 4.
The memory consumption was ~ 400M (100000 pages) after boot every time.
Hibernation lasted 12-13s in both cases.

-- 
js

[-- Attachment #2: image_compress.patch --]
[-- Type: text/x-patch, Size: 6111 bytes --]

diff --git a/kernel/power/snapshot.c b/kernel/power/snapshot.c
index 1ceea79..1bb7dc0 100644
--- a/kernel/power/snapshot.c
+++ b/kernel/power/snapshot.c
@@ -26,6 +26,7 @@
 #include <linux/console.h>
 #include <linux/highmem.h>
 #include <linux/list.h>
+#include <linux/lzo.h>
 
 #include <asm/uaccess.h>
 #include <asm/mmu_context.h>
@@ -35,6 +36,10 @@
 
 #include "power.h"
 
+static void *lzo_buf;
+static void *page_buf;
+struct timeval hib_start_time;
+
 struct memory_bitmap;
 
 static int swsusp_page_is_free(struct page *);
@@ -933,15 +938,76 @@ static unsigned int count_data_pages(void)
 	return n;
 }
 
+static void cccopy(struct memory_bitmap *copy_bm, const char *buffer, int buffer_size)
+{
+	static unsigned long dst_page_pos;
+	static char *dst_page;
+	int bytes_left = buffer_size;
+
+	if (!dst_page)
+		dst_page = phys_to_virt(PFN_PHYS(memory_bm_next_pfn(copy_bm)));
+
+	while (bytes_left) {
+		const char *from = buffer + buffer_size - bytes_left;
+		char *to = dst_page + dst_page_pos;
+		int capacity = PAGE_SIZE - dst_page_pos;
+		int n;
+
+//		printk("%s: to=%p from=%p\n", __func__, to, from);
+		if (bytes_left <= capacity) {
+			for (n = 0; n < bytes_left; n++)
+				to[n] = from[n];
+			dst_page_pos += bytes_left;
+			return;
+		}
+
+		/* Complete this page and start a new one */
+		for (n = 0; n < capacity; n++)
+			to[n] = from[n];
+		bytes_left -= capacity;
+
+		dst_page = phys_to_virt(PFN_PHYS(memory_bm_next_pfn(copy_bm)));
+
+		dst_page_pos = 0;
+	}
+}
+
+#define COMPRESS_IMAGE 0
+
 /* This is needed, because copy_page and memcpy are not usable for copying
  * task structs.
  */
-static inline void do_copy_page(long *dst, long *src)
+static unsigned long do_copy_page(struct memory_bitmap *copy_bm,
+		const unsigned char *src)
 {
-	int n;
+//	static int first = true;
+#if COMPRESS_IMAGE
+	size_t dst_len;
+	int ret;
 
-	for (n = PAGE_SIZE / sizeof(long); n; n--)
-		*dst++ = *src++;
+//	printk("%s: %p\n", __func__, page_buf);
+	ret = lzo1x_1_compress(src, PAGE_SIZE, page_buf, &dst_len, lzo_buf);
+	if (ret < 0) {
+		printk(KERN_EMERG "%s: compress failed: ret=%d dst_len=%zu\n",
+				__func__, ret, dst_len);
+		BUG();
+	}
+
+	cccopy(copy_bm, (char *)&dst_len, sizeof(dst_len));
+	cccopy(copy_bm, page_buf, dst_len);
+
+	return sizeof(dst_len) + dst_len;
+#else
+	cccopy(copy_bm, src, PAGE_SIZE);
+	return PAGE_SIZE;
+#endif
+
+/*	if (first) {
+		print_hex_dump_bytes("O:", DUMP_PREFIX_OFFSET, src, PAGE_SIZE);
+		printk(KERN_DEBUG "len=%zu\n", dst_len);
+		print_hex_dump_bytes("N:", DUMP_PREFIX_OFFSET, dst, dst_len);
+		first = false;
+	}*/
 }
 
 
@@ -951,6 +1017,7 @@ static inline void do_copy_page(long *dst, long *src)
  *		CONFIG_DEBUG_PAGEALLOC is not set and in that case
  *		kernel_page_present() always returns 'true').
  */
+#if 0
 static void safe_copy_page(void *dst, struct page *s_page)
 {
 	if (kernel_page_present(s_page)) {
@@ -962,7 +1029,6 @@ static void safe_copy_page(void *dst, struct page *s_page)
 	}
 }
 
-
 #ifdef CONFIG_HIGHMEM
 static inline struct page *
 page_is_saveable(struct zone *zone, unsigned long pfn)
@@ -999,7 +1065,6 @@ static void copy_data_page(unsigned long dst_pfn, unsigned long src_pfn)
 	}
 }
 #else
-#define page_is_saveable(zone, pfn)	saveable_page(zone, pfn)
 
 static inline void copy_data_page(unsigned long dst_pfn, unsigned long src_pfn)
 {
@@ -1007,12 +1072,14 @@ static inline void copy_data_page(unsigned long dst_pfn, unsigned long src_pfn)
 				pfn_to_page(src_pfn));
 }
 #endif /* CONFIG_HIGHMEM */
+#endif
+#define page_is_saveable(zone, pfn)	saveable_page(zone, pfn)
 
-static void
+static unsigned long
 copy_data_pages(struct memory_bitmap *copy_bm, struct memory_bitmap *orig_bm)
 {
 	struct zone *zone;
-	unsigned long pfn;
+	unsigned long pfn, copied;
 
 	for_each_populated_zone(zone) {
 		unsigned long max_zone_pfn;
@@ -1025,12 +1092,15 @@ copy_data_pages(struct memory_bitmap *copy_bm, struct memory_bitmap *orig_bm)
 	}
 	memory_bm_position_reset(orig_bm);
 	memory_bm_position_reset(copy_bm);
+	copied = 0;
 	for(;;) {
 		pfn = memory_bm_next_pfn(orig_bm);
 		if (unlikely(pfn == BM_END_OF_MAP))
 			break;
-		copy_data_page(memory_bm_next_pfn(copy_bm), pfn);
+		copied += do_copy_page(copy_bm, page_address(pfn_to_page(pfn)));
+//		copy_data_page(memory_bm_next_pfn(copy_bm), pfn);
 	}
+	return copied;
 }
 
 /* Total number of image pages */
@@ -1265,8 +1335,19 @@ int hibernate_preallocate_memory(void)
 	struct timeval start, stop;
 	int error;
 
+	if (!lzo_buf)
+		lzo_buf = vmalloc(LZO1X_MEM_COMPRESS);
+
+	if (!page_buf)
+		page_buf = (void *)__get_free_pages(GFP_KERNEL,
+				get_order(lzo1x_worst_compress(PAGE_SIZE)));
+
+	BUG_ON(!lzo_buf);
+	BUG_ON(!page_buf);
+
 	printk(KERN_INFO "PM: Preallocating image memory... ");
 	do_gettimeofday(&start);
+	hib_start_time = start;
 
 	error = memory_bm_create(&orig_bm, GFP_IMAGE, PG_ANY);
 	if (error)
@@ -1525,7 +1606,7 @@ asmlinkage int swsusp_save(void)
 	 * Kill them.
 	 */
 	drain_local_pages(NULL);
-	copy_data_pages(&copy_bm, &orig_bm);
+	nr_copy_pages = DIV_ROUND_UP(copy_data_pages(&copy_bm, &orig_bm), PAGE_SIZE);
 
 	/*
 	 * End of critical section. From now on, we can write to memory,
@@ -1534,11 +1615,11 @@ asmlinkage int swsusp_save(void)
 	 */
 
 	nr_pages += nr_highmem;
-	nr_copy_pages = nr_pages;
+//	nr_copy_pages = nr_pages;
 	nr_meta_pages = DIV_ROUND_UP(nr_pages * sizeof(long), PAGE_SIZE);
 
-	printk(KERN_INFO "PM: Hibernation image created (%d pages copied)\n",
-		nr_pages);
+	printk(KERN_INFO "PM: Hibernation image created (%d of %d pages copied)\n",
+		nr_copy_pages, nr_pages);
 
 	return 0;
 }
diff --git a/kernel/power/user.c b/kernel/power/user.c
index 92688ce..0f2d6c2 100644
--- a/kernel/power/user.c
+++ b/kernel/power/user.c
@@ -541,6 +541,12 @@ static long snapshot_ioctl(struct file *filp, unsigned int cmd,
 		break;
 
 	case SNAPSHOT_POWER_OFF:
+		{
+		extern struct timeval hib_start_time;
+		struct timeval stop;
+		do_gettimeofday(&stop);
+		swsusp_show_speed(&hib_start_time, &stop, 0, "Overall");
+		}
 		if (data->platform_support)
 			error = hibernation_platform_enter();
 		break;

[-- Attachment #3: Type: text/plain, Size: 0 bytes --]

Re: [RFC 09/15] PM / Hibernate: user, implement user_ops writer

[Rafael J. Wysocki]

On Wednesday 31 March 2010, Jiri Slaby wrote:
> On 03/30/2010 10:50 PM, Rafael J. Wysocki wrote:
> > So, I definitely would like to see numbers.
> 
> With the patch attached, I get similar results for
> uncompressed image -> user.c -> s2disk (compress+threads)
> compressed image -> user.c -> s2disk (none of compress+threads)
> 
> BUT note that there are differences in pages _copied_ in the "user.c ->
> s2disk" phase. Compression was about 40 %. So in the former case 100000
> pages went through and in the latter one only ~ 38000. So presumably
> there will be savings if the pages are compressed in the kernel in the
> save-image phase instead of in userspace.
> 
> I'll test (after I hack that up) also the case of image compression when
> saving the image and let you know.
> 
> It was tested on a machine with openSUSE factory with init 5 with KDE 4.
> The memory consumption was ~ 400M (100000 pages) after boot every time.
> Hibernation lasted 12-13s in both cases.

So that's pretty much the same and I don't think feeding compressed images to
s2disk is not worth the effort.  Let's assume s2disk will always receive a raw
image (that will make it backwards compatible too).

Rafael

Re: [RFC 09/15] PM / Hibernate: user, implement user_ops writer

[Jiri Slaby]

[-- Attachment #1: Type: text/plain, Size: 1863 bytes --]

On 03/31/2010 10:29 PM, Rafael J. Wysocki wrote:
> On Wednesday 31 March 2010, Jiri Slaby wrote:
>> On 03/30/2010 10:50 PM, Rafael J. Wysocki wrote:
>>> So, I definitely would like to see numbers.
>>
>> With the patch attached, I get similar results for
>> uncompressed image -> user.c -> s2disk (compress+threads)
>> compressed image -> user.c -> s2disk (none of compress+threads)
>>
>> BUT note that there are differences in pages _copied_ in the "user.c ->
>> s2disk" phase. Compression was about 40 %. So in the former case 100000
>> pages went through and in the latter one only ~ 38000. So presumably
>> there will be savings if the pages are compressed in the kernel in the
>> save-image phase instead of in userspace.
>>
>> I'll test (after I hack that up) also the case of image compression when
>> saving the image and let you know.
>>
>> It was tested on a machine with openSUSE factory with init 5 with KDE 4.
>> The memory consumption was ~ 400M (100000 pages) after boot every time.
>> Hibernation lasted 12-13s in both cases.
> 
> So that's pretty much the same and I don't think feeding compressed images to
> s2disk is not worth the effort.  Let's assume s2disk will always receive a raw
> image (that will make it backwards compatible too).

Hi.

Yes, I did it solely for comparison purposes.

Now, with the patch attached (depending on either COMPRESS_IMAGE or
COMPRESS_IMAGE1 defined) I tried to compare compression while doing
atomic copy with compression done when image storage takes place. The
latter outperformed the former a bit. I did 8 measurements with the
setup same as above. The average for them is 9684.32 and 10855.07 stored
raw (uncompressed) pages per second. That is 4.5M/s more.

There is a sheet with the numbers at:
http://spreadsheets.google.com/ccc?key=0AvVn7xYA1DnodHFFYzg3Y2tpMUl5NFlURXRtR0xQdmc&hl=en

regards,
-- 
js

[-- Attachment #2: compress-image.patch --]
[-- Type: text/x-patch, Size: 3826 bytes --]

---
 kernel/power/snapshot.c |   80 +++++++++++++++++++++++++++++++++++++++++++++--
 1 files changed, 77 insertions(+), 3 deletions(-)

diff --git a/kernel/power/snapshot.c b/kernel/power/snapshot.c
index ea7bd50..d702953 100644
--- a/kernel/power/snapshot.c
+++ b/kernel/power/snapshot.c
@@ -39,6 +39,7 @@
 
 static void *lzo_buf;
 static void *page_buf;
+static void *chunk_buf;
 struct timeval hib_start_time;
 
 static int swsusp_page_is_free(struct page *);
@@ -1339,8 +1340,12 @@ int hibernate_preallocate_memory(void)
 		page_buf = (void *)__get_free_pages(GFP_KERNEL,
 				get_order(lzo1x_worst_compress(PAGE_SIZE)));
 
+	if (!chunk_buf)
+		chunk_buf = (void *)__get_free_page(GFP_KERNEL);
+
 	BUG_ON(!lzo_buf);
 	BUG_ON(!page_buf);
+	BUG_ON(!chunk_buf);
 
 	printk(KERN_INFO "PM: Preallocating image memory... ");
 	do_gettimeofday(&start);
@@ -2333,6 +2338,73 @@ static int snapshot_image_loaded(struct snapshot_handle *handle)
 			handle->cur < nr_meta_pages + nr_copy_pages);
 }
 
+static void wwwrite(struct bio **bio, const char *buffer, int buffer_size)
+{
+	static unsigned long dst_page_pos;
+	int bytes_left = buffer_size;
+
+	while (bytes_left) {
+		const char *from = buffer + buffer_size - bytes_left;
+		char *to = chunk_buf + dst_page_pos;
+		int capacity = PAGE_SIZE - dst_page_pos;
+		int n;
+
+//		printk("%s: to=%p from=%p\n", __func__, to, from);
+		if (bytes_left <= capacity) {
+			for (n = 0; n < bytes_left; n++)
+				to[n] = from[n];
+			dst_page_pos += bytes_left;
+			return;
+		}
+
+		/* Complete this page and start a new one */
+		for (n = 0; n < capacity; n++)
+			to[n] = from[n];
+		bytes_left -= capacity;
+
+		BUG_ON(hibernate_io_ops->write_page(chunk_buf, bio));
+
+		dst_page_pos = 0;
+	}
+}
+
+#define COMPRESS_IMAGE1 1
+
+/* This is needed, because copy_page and memcpy are not usable for copying
+ * task structs.
+ */
+static unsigned long do_write_page(struct bio **bio,
+		const unsigned char *src)
+{
+//	static int first = true;
+#if COMPRESS_IMAGE1
+	size_t dst_len;
+	int ret;
+
+//	printk("%s: %p\n", __func__, page_buf);
+	ret = lzo1x_1_compress(src, PAGE_SIZE, page_buf, &dst_len, lzo_buf);
+	if (ret < 0) {
+		printk(KERN_EMERG "%s: compress failed: ret=%d dst_len=%zu\n",
+				__func__, ret, dst_len);
+		BUG();
+	}
+
+	wwwrite(bio, (char *)&dst_len, sizeof(dst_len));
+	wwwrite(bio, page_buf, dst_len);
+
+	return sizeof(dst_len) + dst_len;
+#else
+	wwwrite(bio, src, PAGE_SIZE);
+	return PAGE_SIZE;
+#endif
+
+/*	if (first) {
+		print_hex_dump_bytes("O:", DUMP_PREFIX_OFFSET, src, PAGE_SIZE);
+		printk(KERN_DEBUG "len=%zu\n", dst_len);
+		print_hex_dump_bytes("N:", DUMP_PREFIX_OFFSET, dst, dst_len);
+		first = false;
+	}*/
+}
 /**
  *	save_image - save the suspend image data
  */
@@ -2340,6 +2412,7 @@ static int snapshot_image_loaded(struct snapshot_handle *handle)
 static int save_image(struct snapshot_handle *snapshot,
                       unsigned int nr_to_write)
 {
+	unsigned long whole = 0;
 	unsigned int m;
 	int ret;
 	int nr_pages;
@@ -2360,9 +2433,10 @@ static int save_image(struct snapshot_handle *snapshot,
 		ret = snapshot_read_next(snapshot);
 		if (ret <= 0)
 			break;
-		ret = hibernate_io_ops->write_page(data_of(*snapshot), &bio);
+		whole += do_write_page(&bio, data_of(*snapshot));
+/*		ret = hibernate_io_ops->write_page(data_of(*snapshot), &bio);
 		if (ret)
-			break;
+			break;*/
 		if (!(nr_pages % m))
 			printk(KERN_CONT "\b\b\b\b%3d%%", nr_pages / m);
 		nr_pages++;
@@ -2372,7 +2446,7 @@ static int save_image(struct snapshot_handle *snapshot,
 	if (!ret)
 		ret = err2;
 	if (!ret)
-		printk(KERN_CONT "\b\b\b\bdone\n");
+		printk(KERN_CONT "\b\b\b\bdone\n%lu of %d\n", DIV_ROUND_UP(whole, PAGE_SIZE), nr_pages);
 	else
 		printk(KERN_CONT "\n");
 	swsusp_show_speed(&start, &stop, nr_to_write, "Wrote");
-- 
1.7.0.3


[-- Attachment #3: Type: text/plain, Size: 0 bytes --]

Re: [RFC 09/15] PM / Hibernate: user, implement user_ops writer

[Rafael J. Wysocki]

On Wednesday 21 April 2010, Jiri Slaby wrote:
> On 03/31/2010 10:29 PM, Rafael J. Wysocki wrote:
> > On Wednesday 31 March 2010, Jiri Slaby wrote:
> >> On 03/30/2010 10:50 PM, Rafael J. Wysocki wrote:
> >>> So, I definitely would like to see numbers.
> >>
> >> With the patch attached, I get similar results for
> >> uncompressed image -> user.c -> s2disk (compress+threads)
> >> compressed image -> user.c -> s2disk (none of compress+threads)
> >>
> >> BUT note that there are differences in pages _copied_ in the "user.c ->
> >> s2disk" phase. Compression was about 40 %. So in the former case 100000
> >> pages went through and in the latter one only ~ 38000. So presumably
> >> there will be savings if the pages are compressed in the kernel in the
> >> save-image phase instead of in userspace.
> >>
> >> I'll test (after I hack that up) also the case of image compression when
> >> saving the image and let you know.
> >>
> >> It was tested on a machine with openSUSE factory with init 5 with KDE 4.
> >> The memory consumption was ~ 400M (100000 pages) after boot every time.
> >> Hibernation lasted 12-13s in both cases.
> > 
> > So that's pretty much the same and I don't think feeding compressed images to
> > s2disk is not worth the effort.  Let's assume s2disk will always receive a raw
> > image (that will make it backwards compatible too).
> 
> Hi.
> 
> Yes, I did it solely for comparison purposes.
> 
> Now, with the patch attached (depending on either COMPRESS_IMAGE or
> COMPRESS_IMAGE1 defined) I tried to compare compression while doing
> atomic copy with compression done when image storage takes place. The
> latter outperformed the former a bit. I did 8 measurements with the
> setup same as above. The average for them is 9684.32 and 10855.07 stored
> raw (uncompressed) pages per second. That is 4.5M/s more.
> 
> There is a sheet with the numbers at:
> http://spreadsheets.google.com/ccc?key=0AvVn7xYA1DnodHFFYzg3Y2tpMUl5NFlURXRtR0xQdmc&hl=en

Thanks for doing that work!

So clearly from performance POV it is better to compress while saving.

Rafael

Re: [RFC 01/15] FS: libfs, implement simple_write_to_buffer

[Rafael J. Wysocki]

On Tuesday 23 March 2010, Jiri Slaby wrote:
> It will be used in suspend code and serves as an easy wrap around
> copy_from_user. Similar to simple_read_from_buffer, it takes care
> of transfers with proper lengths depending on available and count
> parameters and advances ppos appropriately.
> 
> Signed-off-by: Jiri Slaby <jslaby@suse.cz>
> Cc: Nigel Cunningham <ncunningham@crca.org.au>
> Cc: "Rafael J. Wysocki" <rjw@sisk.pl>
> Cc: Alexander Viro <viro@zeniv.linux.org.uk>
> Cc: linux-fsdevel@vger.kernel.org

No objections from me.

Is there any concern from the fs side?

Rafael


> ---
>  fs/libfs.c         |   35 +++++++++++++++++++++++++++++++++++
>  include/linux/fs.h |    2 ++
>  2 files changed, 37 insertions(+), 0 deletions(-)
> 
> diff --git a/fs/libfs.c b/fs/libfs.c
> index 9e50bcf..fda73b3 100644
> --- a/fs/libfs.c
> +++ b/fs/libfs.c
> @@ -546,6 +546,40 @@ ssize_t simple_read_from_buffer(void __user *to, size_t count, loff_t *ppos,
>  }
>  
>  /**
> + * simple_write_to_buffer - copy data from user space to the buffer
> + * @to: the buffer to write to
> + * @available: the size of the buffer
> + * @ppos: the current position in the buffer
> + * @from: the user space buffer to read from
> + * @count: the maximum number of bytes to read
> + *
> + * The simple_write_to_buffer() function reads up to @count bytes from the user
> + * space address starting at @from into the buffer @to at offset @ppos.
> + *
> + * On success, the number of bytes written is returned and the offset @ppos is
> + * advanced by this number, or negative value is returned on error.
> + **/
> +ssize_t simple_write_to_buffer(void *to, size_t available, loff_t *ppos,
> +		const void __user *from, size_t count)
> +{
> +	loff_t pos = *ppos;
> +	size_t ret;
> +
> +	if (pos < 0)
> +		return -EINVAL;
> +	if (pos >= available || !count)
> +		return 0;
> +	if (count > available - pos)
> +		count = available - pos;
> +	ret = copy_from_user(to + pos, from, count);
> +	if (ret == count)
> +		return -EFAULT;
> +	count -= ret;
> +	*ppos = pos + count;
> +	return count;
> +}
> +
> +/**
>   * memory_read_from_buffer - copy data from the buffer
>   * @to: the kernel space buffer to read to
>   * @count: the maximum number of bytes to read
> @@ -863,6 +897,7 @@ EXPORT_SYMBOL(simple_statfs);
>  EXPORT_SYMBOL(simple_sync_file);
>  EXPORT_SYMBOL(simple_unlink);
>  EXPORT_SYMBOL(simple_read_from_buffer);
> +EXPORT_SYMBOL(simple_write_to_buffer);
>  EXPORT_SYMBOL(memory_read_from_buffer);
>  EXPORT_SYMBOL(simple_transaction_set);
>  EXPORT_SYMBOL(simple_transaction_get);
> diff --git a/include/linux/fs.h b/include/linux/fs.h
> index a4636b6..0f751b6 100644
> --- a/include/linux/fs.h
> +++ b/include/linux/fs.h
> @@ -2370,6 +2370,8 @@ extern void simple_release_fs(struct vfsmount **mount, int *count);
>  
>  extern ssize_t simple_read_from_buffer(void __user *to, size_t count,
>  			loff_t *ppos, const void *from, size_t available);
> +extern ssize_t simple_write_to_buffer(void *to, size_t available, loff_t *ppos,
> +		const void __user *from, size_t count);
>  
>  extern int simple_fsync(struct file *, struct dentry *, int);
>  
> 

what the patches do Re: [RFC 10/15] PM / Hibernate: user, implement user_ops reader

[Pavel Machek]

Hi!

> Switch /dev/snapshot reader to sws_module_ops approach so that we
> can transparently rewrite the rest of the snapshot from pages pulling
> to their pushing through layers.

>  struct sws_module_ops user_ops = {
>  	.storage_available = user_storage_available,
>  
>  	.get_writer = get_user_writer,
>  	.put_writer = put_user_writer,
>  	.write_page = user_write_page,
> +
> +	.get_reader = get_user_reader,
> +	.put_reader = put_user_reader,
> +	.read_page = user_read_page,
>  };

Ok, I guess that now I see what you are doing.... adding interface
layer between /dev/snapshot and core hibernation code.

To recap, 2.6.33 hibernation looks like:

			core hibernation
				/\
			       /  \
			swsusp	  /dev/snapshot
			swap            \
			writing      -------- read/write/ioctl interface
                                          \
					 s2disk

and after your patches, we'd get

			core hibernation
				/\
                            ---------- sws_module_ops interface
			       /  \
			swsusp	  /dev/snapshot
			swap            \
			writing      -------- read/write/ioctl interface
                                          \
					 s2disk

(Right? Did I understand the patches correctly?)

I have some problems with sws_module_ops interface (handcoded locking
is too ugly to live), but it is better than I expected. But there may
be better solution available, one that does not need two interfaces to
maintain (we can't really get rid of userland interface). What about
this?



			core hibernation
				 \
			          \
			      	  /dev/snapshot
		                      / \
			           ---------- read/write/ioctl interface
                                    /     \
				swsusp	 s2disk
				swap
				writing

? That way, we have just one interface, and still keep the advantages
of modularity / defined interfaces.

(You could literary call sys_read() from inside the kernel -- after
set_fs() -- but going to that extreme is probably not neccessary. But
having interface very similar to what /dev/snapshot provides -- with
the same locking rules -- should result in better code.)

									Pavel 

-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

Re: what the patches do Re: [RFC 10/15] PM / Hibernate: user, implement user_ops reader

[Nigel Cunningham]

Hi.

On 25/03/10 16:30, Pavel Machek wrote:
[...]

> I have some problems with sws_module_ops interface (handcoded locking
> is too ugly to live), but it is better than I expected. But there may
> be better solution available, one that does not need two interfaces to
> maintain (we can't really get rid of userland interface). What about
> this?

Just picking up on that bracketed part: Can we flag the userland 
interface (and uswsusp) as being planned for eventual removal now... or 
at least agree to work toward that?

I'm asking because if we're going to make a go of getting the in-kernel 
code in much better shape, and we have Rafael, Jiri and I - and you? - 
all pulling in the same direction to improve it, there's going to come a 
point (hopefully not too far away) where uswsusp is just making life too 
difficult, and getting rid of it will be a big help.

Regards,

Nigel

Re: what the patches do Re: [RFC 10/15] PM / Hibernate: user, implement user_ops reader

[Nigel Cunningham]

Hi again.

On 25/03/10 16:42, Nigel Cunningham wrote:
> On 25/03/10 16:30, Pavel Machek wrote:
> [...]
>
>> I have some problems with sws_module_ops interface (handcoded locking
>> is too ugly to live), but it is better than I expected. But there may
>> be better solution available, one that does not need two interfaces to
>> maintain (we can't really get rid of userland interface). What about
>> this?
>
> Just picking up on that bracketed part: Can we flag the userland
> interface (and uswsusp) as being planned for eventual removal now... or
> at least agree to work toward that?
>
> I'm asking because if we're going to make a go of getting the in-kernel
> code in much better shape, and we have Rafael, Jiri and I - and you? -
> all pulling in the same direction to improve it, there's going to come a

I realised after sending this that the " - and you? - " was ambiguous. I 
wasn't meaning to suggest that you might pull in a different direction, 
but rather uncertainty as to whether you might help with the effort - 
it's been a while (AFAIR) since you've done any hibernaton patches.

Humble apologies for the ambiguity!

Nigel

Re: what the patches do Re: [RFC 10/15] PM / Hibernate: user, implement user_ops reader

[Rafael J. Wysocki]

On Thursday 25 March 2010, Nigel Cunningham wrote:
> Hi.
> 
> On 25/03/10 16:30, Pavel Machek wrote:
> [...]
> 
> > I have some problems with sws_module_ops interface (handcoded locking
> > is too ugly to live), but it is better than I expected. But there may
> > be better solution available, one that does not need two interfaces to
> > maintain (we can't really get rid of userland interface). What about
> > this?
> 
> Just picking up on that bracketed part: Can we flag the userland 
> interface (and uswsusp) as being planned for eventual removal now... or 
> at least agree to work toward that?

No, we can't.

> I'm asking because if we're going to make a go of getting the in-kernel 
> code in much better shape, and we have Rafael, Jiri and I - and you? - 
> all pulling in the same direction to improve it, there's going to come a 
> point (hopefully not too far away) where uswsusp is just making life too 
> difficult, and getting rid of it will be a big help.

We're not dropping user space interfaces used by every distro I know of.

Rafael

Re: what the patches do Re: [RFC 10/15] PM / Hibernate: user, implement user_ops reader

[Nigel Cunningham]

Hi.

On 26/03/10 07:14, Rafael J. Wysocki wrote:
> On Thursday 25 March 2010, Nigel Cunningham wrote:
>> Hi.
>>
>> On 25/03/10 16:30, Pavel Machek wrote:
>> [...]
>>
>>> I have some problems with sws_module_ops interface (handcoded locking
>>> is too ugly to live), but it is better than I expected. But there may
>>> be better solution available, one that does not need two interfaces to
>>> maintain (we can't really get rid of userland interface). What about
>>> this?
>>
>> Just picking up on that bracketed part: Can we flag the userland
>> interface (and uswsusp) as being planned for eventual removal now... or
>> at least agree to work toward that?
>
> No, we can't.
>
>> I'm asking because if we're going to make a go of getting the in-kernel
>> code in much better shape, and we have Rafael, Jiri and I - and you? -
>> all pulling in the same direction to improve it, there's going to come a
>> point (hopefully not too far away) where uswsusp is just making life too
>> difficult, and getting rid of it will be a big help.
>
> We're not dropping user space interfaces used by every distro I know of.

So what's your long term plan then?

Regards,

Nigel

Re: what the patches do Re: [RFC 10/15] PM / Hibernate: user, implement user_ops reader

[Rafael J. Wysocki]

On Thursday 25 March 2010, Nigel Cunningham wrote:
> Hi.
> 
> On 26/03/10 07:14, Rafael J. Wysocki wrote:
> > On Thursday 25 March 2010, Nigel Cunningham wrote:
> >> Hi.
> >>
> >> On 25/03/10 16:30, Pavel Machek wrote:
> >> [...]
> >>
> >>> I have some problems with sws_module_ops interface (handcoded locking
> >>> is too ugly to live), but it is better than I expected. But there may
> >>> be better solution available, one that does not need two interfaces to
> >>> maintain (we can't really get rid of userland interface). What about
> >>> this?
> >>
> >> Just picking up on that bracketed part: Can we flag the userland
> >> interface (and uswsusp) as being planned for eventual removal now... or
> >> at least agree to work toward that?
> >
> > No, we can't.
> >
> >> I'm asking because if we're going to make a go of getting the in-kernel
> >> code in much better shape, and we have Rafael, Jiri and I - and you? -
> >> all pulling in the same direction to improve it, there's going to come a
> >> point (hopefully not too far away) where uswsusp is just making life too
> >> difficult, and getting rid of it will be a big help.
> >
> > We're not dropping user space interfaces used by every distro I know of.
> 
> So what's your long term plan then?

First, improve the in-kernel thing, second, switch people to it, _then_ remove
the s2disk interface (after we're reasonably sure it's not used by any major
distro) and _finally_ simplify things after it's been removed.

Does that sound reasonable?

Rafael

Re: what the patches do Re: [RFC 10/15] PM / Hibernate: user, implement user_ops reader

[Nigel Cunningham]

Hi.

On 26/03/10 07:29, Rafael J. Wysocki wrote:
> On Thursday 25 March 2010, Nigel Cunningham wrote:
>> Hi.
>>
>> On 26/03/10 07:14, Rafael J. Wysocki wrote:
>>> On Thursday 25 March 2010, Nigel Cunningham wrote:
>>>> Hi.
>>>>
>>>> On 25/03/10 16:30, Pavel Machek wrote:
>>>> [...]
>>>>
>>>>> I have some problems with sws_module_ops interface (handcoded locking
>>>>> is too ugly to live), but it is better than I expected. But there may
>>>>> be better solution available, one that does not need two interfaces to
>>>>> maintain (we can't really get rid of userland interface). What about
>>>>> this?
>>>>
>>>> Just picking up on that bracketed part: Can we flag the userland
>>>> interface (and uswsusp) as being planned for eventual removal now... or
>>>> at least agree to work toward that?
>>>
>>> No, we can't.
>>>
>>>> I'm asking because if we're going to make a go of getting the in-kernel
>>>> code in much better shape, and we have Rafael, Jiri and I - and you? -
>>>> all pulling in the same direction to improve it, there's going to come a
>>>> point (hopefully not too far away) where uswsusp is just making life too
>>>> difficult, and getting rid of it will be a big help.
>>>
>>> We're not dropping user space interfaces used by every distro I know of.
>>
>> So what's your long term plan then?
>
> First, improve the in-kernel thing, second, switch people to it, _then_ remove
> the s2disk interface (after we're reasonably sure it's not used by any major
> distro) and _finally_ simplify things after it's been removed.
>
> Does that sound reasonable?

Well, that's pretty much what I was thinking too - improve then remove. 
I was just suggesting that we flag now that this is our plan, so it 
doesn't come as a surprise to anyone later and we can proceed more 
quickly than might otherwise be the case. I'm imagining it won't take 
long to get uswsusp features into the kernel code.

Regards,

Nigel

Re: what the patches do Re: [RFC 10/15] PM / Hibernate: user, implement user_ops reader

[Rafael J. Wysocki]

On Thursday 25 March 2010, Nigel Cunningham wrote:
> Hi.
> 
> On 26/03/10 07:29, Rafael J. Wysocki wrote:
> > On Thursday 25 March 2010, Nigel Cunningham wrote:
> >> Hi.
> >>
> >> On 26/03/10 07:14, Rafael J. Wysocki wrote:
> >>> On Thursday 25 March 2010, Nigel Cunningham wrote:
> >>>> Hi.
> >>>>
> >>>> On 25/03/10 16:30, Pavel Machek wrote:
> >>>> [...]
> >>>>
> >>>>> I have some problems with sws_module_ops interface (handcoded locking
> >>>>> is too ugly to live), but it is better than I expected. But there may
> >>>>> be better solution available, one that does not need two interfaces to
> >>>>> maintain (we can't really get rid of userland interface). What about
> >>>>> this?
> >>>>
> >>>> Just picking up on that bracketed part: Can we flag the userland
> >>>> interface (and uswsusp) as being planned for eventual removal now... or
> >>>> at least agree to work toward that?
> >>>
> >>> No, we can't.
> >>>
> >>>> I'm asking because if we're going to make a go of getting the in-kernel
> >>>> code in much better shape, and we have Rafael, Jiri and I - and you? -
> >>>> all pulling in the same direction to improve it, there's going to come a
> >>>> point (hopefully not too far away) where uswsusp is just making life too
> >>>> difficult, and getting rid of it will be a big help.
> >>>
> >>> We're not dropping user space interfaces used by every distro I know of.
> >>
> >> So what's your long term plan then?
> >
> > First, improve the in-kernel thing, second, switch people to it, _then_ remove
> > the s2disk interface (after we're reasonably sure it's not used by any major
> > distro) and _finally_ simplify things after it's been removed.
> >
> > Does that sound reasonable?
> 
> Well, that's pretty much what I was thinking too - improve then remove. 
> I was just suggesting that we flag now that this is our plan, so it 
> doesn't come as a surprise to anyone later and we can proceed more 
> quickly than might otherwise be the case.

I think it's too early for that at this point.

Rafael

Re: what the patches do Re: [RFC 10/15] PM / Hibernate: user, implement user_ops reader

[Pavel Machek]

Hi!

> > >> I'm asking because if we're going to make a go of getting the in-kernel
> > >> code in much better shape, and we have Rafael, Jiri and I - and you? -
> > >> all pulling in the same direction to improve it, there's going to come a
> > >> point (hopefully not too far away) where uswsusp is just making life too
> > >> difficult, and getting rid of it will be a big help.
> > >
> > > We're not dropping user space interfaces used by every distro I know of.

Good.

> > So what's your long term plan then?
> 
> First, improve the in-kernel thing, second, switch people to it, _then_ remove
> the s2disk interface (after we're reasonably sure it's not used by any major
> distro) and _finally_ simplify things after it's been removed.

I'd really prefer to keep s2disk interface. It allows advanced stuff
like internet suspend resume (yep someone is doing it), crazy stuff
like multiple resumes from same image for fast booting (yep, some
embedded people are doing that) and might-be-useful stuff like
s2both...
									Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

Re: what the patches do Re: [RFC 10/15] PM / Hibernate: user, implement user_ops reader

[Nigel Cunningham]

Hi.

On 26/03/10 08:26, Pavel Machek wrote:
> Hi!
>
>>>>> I'm asking because if we're going to make a go of getting the in-kernel
>>>>> code in much better shape, and we have Rafael, Jiri and I - and you? -
>>>>> all pulling in the same direction to improve it, there's going to come a
>>>>> point (hopefully not too far away) where uswsusp is just making life too
>>>>> difficult, and getting rid of it will be a big help.
>>>>
>>>> We're not dropping user space interfaces used by every distro I know of.
>
> Good.
>
>>> So what's your long term plan then?
>>
>> First, improve the in-kernel thing, second, switch people to it, _then_ remove
>> the s2disk interface (after we're reasonably sure it's not used by any major
>> distro) and _finally_ simplify things after it's been removed.
>
> I'd really prefer to keep s2disk interface. It allows advanced stuff
> like internet suspend resume (yep someone is doing it), crazy stuff
> like multiple resumes from same image for fast booting (yep, some
> embedded people are doing that) and might-be-useful stuff like
> s2both...

Neither of those are impossible with in-kernel code, so I'd argue that 
there's no need to keep the s2disk interface long-term. Userspace 
helpers might be necessary for the first one (to manage the network 
interface), but I already have multiple-resumes-from-the-same-image 
support in TuxOnice (and more 'crazy stuff' like support for resuming a 
different image after writing one - that can be used to switch to an 
initramfs containing the binaries needed to power down a UPS).

Regards,

Nigel

Re: what the patches do Re: [RFC 10/15] PM / Hibernate: user, implement user_ops reader

[Pavel Machek]

Hi!

>>>> So what's your long term plan then?
>>>
>>> First, improve the in-kernel thing, second, switch people to it, _then_ remove
>>> the s2disk interface (after we're reasonably sure it's not used by any major
>>> distro) and _finally_ simplify things after it's been removed.
>>
>> I'd really prefer to keep s2disk interface. It allows advanced stuff
>> like internet suspend resume (yep someone is doing it), crazy stuff
>> like multiple resumes from same image for fast booting (yep, some
>> embedded people are doing that) and might-be-useful stuff like
>> s2both...
>
> Neither of those are impossible with in-kernel code, so I'd argue that  
> there's no need to keep the s2disk interface long-term. Userspace  

I disagree.

> helpers might be necessary for the first one (to manage the network  
> interface), but I already have multiple-resumes-from-the-same-image  
> support in TuxOnice (and more 'crazy stuff' like support for resuming a  
> different image after writing one - that can be used to switch to an  
> initramfs containing the binaries needed to power down a UPS).

The fact that it can be done in kernel -- anything can -- but whether
it should. I'd very much like to keep the 'crazy stuff' in userspace.
									Pavel

-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

Re: what the patches do Re: [RFC 10/15] PM / Hibernate: user, implement user_ops reader

[Rafael J. Wysocki]

On Friday 02 April 2010, Pavel Machek wrote:
...
> > Neither of those are impossible with in-kernel code, so I'd argue that  
> > there's no need to keep the s2disk interface long-term. Userspace  
> 
> I disagree.
> 
> > helpers might be necessary for the first one (to manage the network  
> > interface), but I already have multiple-resumes-from-the-same-image  
> > support in TuxOnice (and more 'crazy stuff' like support for resuming a  
> > different image after writing one - that can be used to switch to an  
> > initramfs containing the binaries needed to power down a UPS).
> 
> The fact that it can be done in kernel -- anything can -- but whether
> it should. I'd very much like to keep the 'crazy stuff' in userspace.

IMO the question really boils down to what's the benefit of doing these things
in the kernel vs doing them in the user space.

If doing them in the kernel reduces the overall complexity of design (including
the user space interface involved etc.) I don't really see why not to.

Rafael

Re: what the patches do Re: [RFC 10/15] PM / Hibernate: user, implement user_ops reader

[Rafael J. Wysocki]

On Thursday 25 March 2010, Pavel Machek wrote:
> Hi!
> 
> > Switch /dev/snapshot reader to sws_module_ops approach so that we
> > can transparently rewrite the rest of the snapshot from pages pulling
> > to their pushing through layers.
> 
> >  struct sws_module_ops user_ops = {
> >  	.storage_available = user_storage_available,
> >  
> >  	.get_writer = get_user_writer,
> >  	.put_writer = put_user_writer,
> >  	.write_page = user_write_page,
> > +
> > +	.get_reader = get_user_reader,
> > +	.put_reader = put_user_reader,
> > +	.read_page = user_read_page,
> >  };
> 
> Ok, I guess that now I see what you are doing.... adding interface
> layer between /dev/snapshot and core hibernation code.
> 
> To recap, 2.6.33 hibernation looks like:
> 
> 			core hibernation
> 				/\
> 			       /  \
> 			swsusp	  /dev/snapshot
> 			swap            \
> 			writing      -------- read/write/ioctl interface
>                                           \
> 					 s2disk
> 
> and after your patches, we'd get
> 
> 			core hibernation
> 				/\
>                             ---------- sws_module_ops interface
> 			       /  \
> 			swsusp	  /dev/snapshot
> 			swap            \
> 			writing      -------- read/write/ioctl interface
>                                           \
> 					 s2disk
> 
> (Right? Did I understand the patches correctly?)
> 
> I have some problems with sws_module_ops interface (handcoded locking
> is too ugly to live), but it is better than I expected. But there may
> be better solution available, one that does not need two interfaces to
> maintain (we can't really get rid of userland interface). What about
> this?
> 
> 
> 
> 			core hibernation
> 				 \
> 			          \
> 			      	  /dev/snapshot
> 		                      / \
> 			           ---------- read/write/ioctl interface
>                                     /     \
> 				swsusp	 s2disk
> 				swap
> 				writing
> 
> ? That way, we have just one interface, and still keep the advantages
> of modularity / defined interfaces.
> 
> (You could literary call sys_read() from inside the kernel -- after
> set_fs() -- but going to that extreme is probably not neccessary. But
> having interface very similar to what /dev/snapshot provides -- with
> the same locking rules -- should result in better code.)

The user space interface does things that the in-kernel one doesn't really care
for, so I don't think that would be a good thing to do.  I admit it's a bit
like this right now (snapshot_[read|write]_next() do some things to satisfy
the user space interface's needs), but I don't really think it should go any
further than that.

Moreover, the Jiri's approach allows us to handle other types of storage as
well as swap using uniform interface.

Rafael

Re: [RFC 05/15] PM / Hibernate: group swap ops

[Rafael J. Wysocki]

On Tuesday 23 March 2010, Jiri Slaby wrote:
> Move all the swap processing into one function. It will make swap
> calls from a non-swap code easier.
> 
> Signed-off-by: Jiri Slaby <jslaby@suse.cz>
> Cc: Nigel Cunningham <ncunningham@crca.org.au>
> Cc: "Rafael J. Wysocki" <rjw@sisk.pl>

Looks good.

Rafael


> ---
>  kernel/power/swap.c |  117 ++++++++++++++++++++++++++++++++-------------------
>  1 files changed, 74 insertions(+), 43 deletions(-)
> 
> diff --git a/kernel/power/swap.c b/kernel/power/swap.c
> index a1cff28..2edf742 100644
> --- a/kernel/power/swap.c
> +++ b/kernel/power/swap.c
> @@ -207,9 +207,10 @@ static int mark_swapfiles(struct swap_map_handle *handle, unsigned int flags)
>  /**
>   *	swsusp_swap_check - check if the resume device is a swap device
>   *	and get its index (if so)
> + *
> + *	This is called before saving image
>   */
> -
> -static int swsusp_swap_check(void) /* This is called before saving image */
> +static int swsusp_swap_check(void)
>  {
>  	int res;
>  
> @@ -268,17 +269,33 @@ static void release_swap_writer(struct swap_map_handle *handle)
>  
>  static int get_swap_writer(struct swap_map_handle *handle)
>  {
> +	int ret;
> +
> +	ret = swsusp_swap_check();
> +	if (ret) {
> +		if (ret != -ENOSPC)
> +			printk(KERN_ERR "PM: Cannot find swap device, try "
> +					"swapon -a.\n");
> +		return ret;
> +	}
>  	handle->cur = (struct swap_map_page *)get_zeroed_page(GFP_KERNEL);
> -	if (!handle->cur)
> -		return -ENOMEM;
> +	if (!handle->cur) {
> +		ret = -ENOMEM;
> +		goto err_close;
> +	}
>  	handle->cur_swap = alloc_swapdev_block(root_swap);
>  	if (!handle->cur_swap) {
> -		release_swap_writer(handle);
> -		return -ENOSPC;
> +		ret = -ENOSPC;
> +		goto err_rel;
>  	}
>  	handle->k = 0;
>  	handle->first_sector = handle->cur_swap;
>  	return 0;
> +err_rel:
> +	release_swap_writer(handle);
> +err_close:
> +	swsusp_close(FMODE_WRITE);
> +	return ret;
>  }
>  
>  static int swap_write_page(struct swap_map_handle *handle, void *buf,
> @@ -321,6 +338,24 @@ static int flush_swap_writer(struct swap_map_handle *handle)
>  		return -EINVAL;
>  }
>  
> +static int put_swap_writer(struct swap_map_handle *handle,
> +		unsigned int flags, int error)
> +{
> +	if (!error) {
> +		flush_swap_writer(handle);
> +		printk(KERN_INFO "PM: S");
> +		error = mark_swapfiles(handle, flags);
> +		printk("|\n");
> +	}
> +
> +	if (error)
> +		free_all_swap_pages(root_swap);
> +	release_swap_writer(handle);
> +	swsusp_close(FMODE_WRITE);
> +
> +	return error;
> +}
> +
>  /**
>   *	save_image - save the suspend image data
>   */
> @@ -398,48 +433,34 @@ int swsusp_write(unsigned int flags)
>  	struct swap_map_handle handle;
>  	struct snapshot_handle snapshot;
>  	struct swsusp_info *header;
> +	unsigned long pages;
>  	int error;
>  
> -	error = swsusp_swap_check();
> +	pages = snapshot_get_image_size();
> +	error = get_swap_writer(&handle);
>  	if (error) {
> -		printk(KERN_ERR "PM: Cannot find swap device, try "
> -				"swapon -a.\n");
> +		printk(KERN_ERR "PM: Cannot get swap writer\n");
>  		return error;
>  	}
> +	if (!enough_swap(pages)) {
> +		printk(KERN_ERR "PM: Not enough free swap\n");
> +		error = -ENOSPC;
> +		goto out_finish;
> +	}
>  	memset(&snapshot, 0, sizeof(struct snapshot_handle));
>  	error = snapshot_read_next(&snapshot);
>  	if (error < PAGE_SIZE) {
>  		if (error >= 0)
>  			error = -EFAULT;
>  
> -		goto out;
> +		goto out_finish;
>  	}
>  	header = (struct swsusp_info *)data_of(snapshot);
> -	if (!enough_swap(header->pages)) {
> -		printk(KERN_ERR "PM: Not enough free swap\n");
> -		error = -ENOSPC;
> -		goto out;
> -	}
> -	error = get_swap_writer(&handle);
> -	if (!error) {
> -		error = swap_write_page(&handle, header, NULL);
> -		if (!error)
> -			error = save_image(&handle, &snapshot,
> -					header->pages - 1);
> -
> -		if (!error) {
> -			flush_swap_writer(&handle);
> -			printk(KERN_INFO "PM: S");
> -			error = mark_swapfiles(&handle, flags);
> -			printk("|\n");
> -		}
> -	}
> -	if (error)
> -		free_all_swap_pages(root_swap);
> -
> -	release_swap_writer(&handle);
> - out:
> -	swsusp_close(FMODE_WRITE);
> +	error = swap_write_page(&handle, header, NULL);
> +	if (!error)
> +		error = save_image(&handle, &snapshot, pages - 1);
> +out_finish:
> +	error = put_swap_writer(&handle, flags, error);
>  	return error;
>  }
>  
> @@ -455,18 +476,21 @@ static void release_swap_reader(struct swap_map_handle *handle)
>  	handle->cur = NULL;
>  }
>  
> -static int get_swap_reader(struct swap_map_handle *handle, sector_t start)
> +static int get_swap_reader(struct swap_map_handle *handle,
> +		unsigned int *flags_p)
>  {
>  	int error;
>  
> -	if (!start)
> +	*flags_p = swsusp_header->flags;
> +
> +	if (!swsusp_header->image) /* how can this happen? */
>  		return -EINVAL;
>  
>  	handle->cur = (struct swap_map_page *)get_zeroed_page(__GFP_WAIT | __GFP_HIGH);
>  	if (!handle->cur)
>  		return -ENOMEM;
>  
> -	error = sws_bio_read_page(start, handle->cur, NULL);
> +	error = sws_bio_read_page(swsusp_header->image, handle->cur, NULL);
>  	if (error) {
>  		release_swap_reader(handle);
>  		return error;
> @@ -501,6 +525,13 @@ static int swap_read_page(struct swap_map_handle *handle, void *buf,
>  	return error;
>  }
>  
> +static int put_swap_reader(struct swap_map_handle *handle)
> +{
> +	release_swap_reader(handle);
> +
> +	return 0;
> +}
> +
>  /**
>   *	load_image - load the image using the swap map handle
>   *	@handle and the snapshot handle @snapshot
> @@ -570,20 +601,20 @@ int swsusp_read(unsigned int *flags_p)
>  	struct snapshot_handle snapshot;
>  	struct swsusp_info *header;
>  
> -	*flags_p = swsusp_header->flags;
> -
>  	memset(&snapshot, 0, sizeof(struct snapshot_handle));
>  	error = snapshot_write_next(&snapshot);
>  	if (error < PAGE_SIZE)
>  		return error < 0 ? error : -EFAULT;
>  	header = (struct swsusp_info *)data_of(snapshot);
> -	error = get_swap_reader(&handle, swsusp_header->image);
> +	error = get_swap_reader(&handle, flags_p);
> +	if (error)
> +		goto end;
>  	if (!error)
>  		error = swap_read_page(&handle, header, NULL);
>  	if (!error)
>  		error = load_image(&handle, &snapshot, header->pages - 1);
> -	release_swap_reader(&handle);
> -
> +	put_swap_reader(&handle);
> +end:
>  	if (!error)
>  		pr_debug("PM: Image successfully loaded\n");
>  	else
> 

Re: [RFC 11/15] PM / Hibernate: add chunk i/o support

[Rafael J. Wysocki]

On Tuesday 23 March 2010, Jiri Slaby wrote:
> Chunk support is useful when not writing whole pages at once. It takes
> care of joining the buffers into the pages and writing at once when
> needed.
> 
> In the end when pages are compressed they use this interface as well
> (because they are indeed shorter than PAGE_SIZE).
> 
> Signed-off-by: Jiri Slaby <jslaby@suse.cz>
> Cc: Nigel Cunningham <ncunningham@crca.org.au>
> Cc: "Rafael J. Wysocki" <rjw@sisk.pl>
> ---
>  kernel/power/block_io.c |   81 +++++++++++++++++++++++++++++++++++++++++++++++
>  kernel/power/power.h    |    6 +++
>  2 files changed, 87 insertions(+), 0 deletions(-)
> 
> diff --git a/kernel/power/block_io.c b/kernel/power/block_io.c
> index 2b7898a..37412f7 100644
> --- a/kernel/power/block_io.c
> +++ b/kernel/power/block_io.c
> @@ -3,6 +3,7 @@
>   *
>   * Copyright (C) 1998,2001-2005 Pavel Machek <pavel@suse.cz>
>   * Copyright (C) 2006 Rafael J. Wysocki <rjw@sisk.pl>
> + * Copyright (C) 2004-2008 Nigel Cunningham (nigel at tuxonice net)
>   *
>   * This file is released under the GPLv2.
>   */
> @@ -14,6 +15,9 @@
>  
>  #include "power.h"
>  
> +static char *sws_writer_buffer;
> +static unsigned long sws_writer_buffer_pos;
> +
>  /**
>   *	submit - submit BIO request.
>   *	@rw:	READ or WRITE.
> @@ -74,6 +78,83 @@ int sws_bio_write_page(pgoff_t page_off, void *addr, struct bio **bio_chain)
>  			virt_to_page(addr), bio_chain);
>  }
>  
> +int sws_rw_buffer_init(int writing)
> +{
> +	BUG_ON(sws_writer_buffer || sws_writer_buffer_pos);

Please don't do that.  Fail the operation instead.  You can also use WARN_ON
or WARN if you _really_ want the user to notice the failure.

BUG_ON's like this are annoying like hell for testers who trigger them.

> +	sws_writer_buffer = (void *)get_zeroed_page(__GFP_WAIT | __GFP_HIGH);
> +	if (sws_writer_buffer && !writing)
> +		sws_writer_buffer_pos = PAGE_SIZE;
> +
> +	return sws_writer_buffer ? 0 : -ENOMEM;
> +}
> +
> +/**
> + * sws_rw_buffer - combine smaller buffers into PAGE_SIZE I/O
> + * @writing:		Bool - whether writing (or reading).
> + * @buffer:		The start of the buffer to write or fill.
> + * @buffer_size:	The size of the buffer to write or fill.
> + **/
> +int sws_rw_buffer(int writing, char *buffer, int buffer_size)
> +{
> +	int bytes_left = buffer_size, ret;
> +
> +	while (bytes_left) {
> +		char *source_start = buffer + buffer_size - bytes_left;
> +		char *dest_start = sws_writer_buffer + sws_writer_buffer_pos;
> +		int capacity = PAGE_SIZE - sws_writer_buffer_pos;
> +		char *to = writing ? dest_start : source_start;
> +		char *from = writing ? source_start : dest_start;
> +
> +		if (bytes_left <= capacity) {
> +			memcpy(to, from, bytes_left);
> +			sws_writer_buffer_pos += bytes_left;
> +			return 0;
> +		}
> +
> +		/* Complete this page and start a new one */
> +		memcpy(to, from, capacity);
> +		bytes_left -= capacity;
> +
> +		if (writing) {
> +			ret = sws_io_ops->write_page(sws_writer_buffer, NULL);
> +			clear_page(sws_writer_buffer);

Why do we need that clear_page()?

> +			if (ret)
> +				return ret;

Please move these two lines out of the if()/else.

> +		} else {
> +			ret = sws_io_ops->read_page(sws_writer_buffer, NULL);
> +			if (ret)
> +				return ret;
> +		}
> +
> +		sws_writer_buffer_pos = 0;
> +	}
> +
> +	return 0;
> +}
> +
> +int sws_rw_buffer_flush_page(int writing)
> +{
> +	int ret = 0;
> +	if (writing && sws_writer_buffer_pos)
> +		ret = sws_io_ops->write_page(sws_writer_buffer, NULL);
> +	sws_writer_buffer_pos = writing ? 0 : PAGE_SIZE;
> +	return ret;
> +}

I'd split the above into two functions, one for writing and the other for
reading.

Doing the same with sws_rw_buffer() (under a better name), for the sake of
clarity, also might make some sense, apparently.

> +
> +int sws_rw_buffer_finish(int writing)
> +{
> +	int ret = 0;
> +
> +	ret = sws_rw_buffer_flush_page(writing);
> +
> +	free_page((unsigned long)sws_writer_buffer);
> +	sws_writer_buffer = NULL;
> +	sws_writer_buffer_pos = 0;
> +
> +	return ret;
> +}
> +
>  int sws_wait_on_bio_chain(struct bio **bio_chain)
>  {
>  	struct bio *bio;
> diff --git a/kernel/power/power.h b/kernel/power/power.h
> index 0f08de4..50a888a 100644
> --- a/kernel/power/power.h
> +++ b/kernel/power/power.h
> @@ -3,6 +3,8 @@
>  #include <linux/utsname.h>
>  #include <linux/freezer.h>
>  
> +struct swap_map_handle;
> +
>  struct swsusp_info {
>  	struct new_utsname	uts;
>  	u32			version_code;
> @@ -161,6 +163,10 @@ extern int sws_bio_read_page(pgoff_t page_off, void *addr,
>  extern int sws_bio_write_page(pgoff_t page_off, void *addr,
>  		struct bio **bio_chain);
>  extern int sws_wait_on_bio_chain(struct bio **bio_chain);
> +extern int sws_rw_buffer_init(int writing);
> +extern int sws_rw_buffer_finish(int writing);
> +extern int sws_rw_buffer_flush_page(int writing);
> +extern int sws_rw_buffer(int writing, char *buffer, int buffer_size);
>  
>  struct timeval;
>  /* kernel/power/swsusp.c */
> 

Re: [RFC 11/15] PM / Hibernate: add chunk i/o support

[Jiri Slaby]

On 03/25/2010 11:38 PM, Rafael J. Wysocki wrote:
>> +int sws_rw_buffer_init(int writing)
>> +{
>> +	BUG_ON(sws_writer_buffer || sws_writer_buffer_pos);
> 
> Please don't do that.  Fail the operation instead.  You can also use WARN_ON
> or WARN if you _really_ want the user to notice the failure.

It's not a failure, it's a bug when we leak memory or forgot to
read/write all data.

> BUG_ON's like this are annoying like hell for testers who trigger them.

I think BUG is appropriate here (the system or image is in an
inconsitent state for the latter condition), but if you prefer the
WARN-family here, I can switch it to that.

>> +		if (writing) {
>> +			ret = sws_io_ops->write_page(sws_writer_buffer, NULL);
>> +			clear_page(sws_writer_buffer);
> 
> Why do we need that clear_page()?

Functionally for nothing, it was for my sakeness. Will remove.

>> +int sws_rw_buffer_flush_page(int writing)
>> +{
>> +	int ret = 0;
>> +	if (writing && sws_writer_buffer_pos)
>> +		ret = sws_io_ops->write_page(sws_writer_buffer, NULL);
>> +	sws_writer_buffer_pos = writing ? 0 : PAGE_SIZE;
>> +	return ret;
>> +}
> 
> I'd split the above into two functions, one for writing and the other for
> reading.
> 
> Doing the same with sws_rw_buffer() (under a better name), for the sake of
> clarity, also might make some sense, apparently.

Do you mean adding hib*_buffer_read + hib*_buffer_write which would call
static hib*_rw_buffer? sws_rw_buffer has much common code for R and W,
so I would not make 2 functions from that.

Nigel, you use _rw_ functions in toi, are there any pros opposing to _r_
+ _w_ (apart from exporting twice as symbols)?

thanks,
-- 
js

Re: [RFC 11/15] PM / Hibernate: add chunk i/o support

[Nigel Cunningham]

Hi.

On 26/03/10 20:09, Jiri Slaby wrote:
> On 03/25/2010 11:38 PM, Rafael J. Wysocki wrote:
>>> +int sws_rw_buffer_init(int writing)
>>> +{
>>> +	BUG_ON(sws_writer_buffer || sws_writer_buffer_pos);
>>
>> Please don't do that.  Fail the operation instead.  You can also use WARN_ON
>> or WARN if you _really_ want the user to notice the failure.
>
> It's not a failure, it's a bug when we leak memory or forgot to
> read/write all data.
>
>> BUG_ON's like this are annoying like hell for testers who trigger them.
>
> I think BUG is appropriate here (the system or image is in an
> inconsitent state for the latter condition), but if you prefer the
> WARN-family here, I can switch it to that.
>
>>> +		if (writing) {
>>> +			ret = sws_io_ops->write_page(sws_writer_buffer, NULL);
>>> +			clear_page(sws_writer_buffer);
>>
>> Why do we need that clear_page()?
>
> Functionally for nothing, it was for my sakeness. Will remove.
>
>>> +int sws_rw_buffer_flush_page(int writing)
>>> +{
>>> +	int ret = 0;
>>> +	if (writing&&  sws_writer_buffer_pos)
>>> +		ret = sws_io_ops->write_page(sws_writer_buffer, NULL);
>>> +	sws_writer_buffer_pos = writing ? 0 : PAGE_SIZE;
>>> +	return ret;
>>> +}
>>
>> I'd split the above into two functions, one for writing and the other for
>> reading.
>>
>> Doing the same with sws_rw_buffer() (under a better name), for the sake of
>> clarity, also might make some sense, apparently.
>
> Do you mean adding hib*_buffer_read + hib*_buffer_write which would call
> static hib*_rw_buffer? sws_rw_buffer has much common code for R and W,
> so I would not make 2 functions from that.
>
> Nigel, you use _rw_ functions in toi, are there any pros opposing to _r_
> + _w_ (apart from exporting twice as symbols)?

I forget now why I used rw functions to begin with (it's been a long 
time!). I do know I've never worried about exporting twice as many symbols.

As I look at the code now, I think it makes more sense to split things 
up. This is especially true when I consider that a user with 4 cores has 
driven me to work on scalability, which will only diverge the code paths 
more.

Regards,

Nigel

Re: [RFC 12/15] PM / Hibernate: split snapshot_read_next

[Rafael J. Wysocki]

On Tuesday 23 March 2010, Jiri Slaby wrote:
> When writing the snapshot, do the initialization and header write in
> a separate function. This makes the code more readable and lowers
> complexity of snapshot_read_next.

Good idea overall, but ->

> Signed-off-by: Jiri Slaby <jslaby@suse.cz>
> Cc: Nigel Cunningham <ncunningham@crca.org.au>
> Cc: "Rafael J. Wysocki" <rjw@sisk.pl>
> ---
>  kernel/power/power.h    |    1 +
>  kernel/power/snapshot.c |   63 +++++++++++++++++++++++++++++-----------------
>  kernel/power/swap.c     |   14 +++-------
>  3 files changed, 45 insertions(+), 33 deletions(-)
> 
> diff --git a/kernel/power/power.h b/kernel/power/power.h
> index 50a888a..638a97c 100644
> --- a/kernel/power/power.h
> +++ b/kernel/power/power.h
> @@ -130,6 +130,7 @@ struct sws_module_ops {
>  
>  extern unsigned int snapshot_additional_pages(struct zone *zone);
>  extern unsigned long snapshot_get_image_size(void);
> +extern int snapshot_write_init(struct snapshot_handle *handle);
>  extern int snapshot_read_next(struct snapshot_handle *handle);
>  extern int snapshot_write_next(struct snapshot_handle *handle);
>  extern void snapshot_write_finalize(struct snapshot_handle *handle);
> diff --git a/kernel/power/snapshot.c b/kernel/power/snapshot.c
> index 7918351..c8864de 100644
> --- a/kernel/power/snapshot.c
> +++ b/kernel/power/snapshot.c
> @@ -1597,10 +1597,44 @@ pack_pfns(unsigned long *buf, struct memory_bitmap *bm)
>  }
>  
>  /**
> + *	snapshot_write_init - initialization before writing the snapshot to
> + *	a backing storage
> + *
> + *	This function *must* be called before snapshot_read_next to initialize
> + *	@handle and write a header.
> + *
> + *	@handle: snapshot handle to init
> + */
> +int snapshot_write_init(struct snapshot_handle *handle)
> +{
> +	int ret;
> +
> +	/* This makes the buffer be freed by swsusp_free() */
> +	buffer = get_image_page(GFP_ATOMIC, PG_ANY);
> +	if (!buffer)
> +		return -ENOMEM;
> +	init_header(buffer);
> +	ret = sws_rw_buffer_init(1);

-> That's hardly readable.  You could define something like
HIBERNATE_WRITING and HIBERNATE_READING and pass one of them instead of the '1'.
Or something.

Likewise in a few places below.

[Or even better IMO, define hibernate_write_buffer_init() and
hibernate_read_buffer_init() etc.]

> +	if (ret)
> +		return ret;
> +	ret = sws_rw_buffer(1, buffer, sizeof(struct swsusp_info));
> +	if (ret)
> +		goto finish;
> +	sws_rw_buffer_finish(1);
> +	memory_bm_position_reset(&orig_bm);
> +	memory_bm_position_reset(&copy_bm);
> +	handle->buffer = buffer;
> +	return 0;
> +finish:
> +	sws_rw_buffer_finish(1);
> +	return ret;
> +}
> +
> +/**
>   *	snapshot_read_next - used for reading the system memory snapshot.
>   *
> - *	On the first call to it @handle should point to a zeroed
> - *	snapshot_handle structure.  The structure gets updated and a pointer
> + *	Before calling this function, snapshot_write_init has to be called with
> + *	handle passed as @handle here. The structure gets updated and a pointer
>   *	to it should be passed to this function every next time.
>   *
>   *	On success the function returns a positive number.  Then, the caller

Rafael

Re: [RFC 13/15] PM / Hibernate: split snapshot_write_next

[Rafael J. Wysocki]

On Tuesday 23 March 2010, Jiri Slaby wrote:
> When reading the snapshot, do the initialization and header read in
> a separate function. This makes the code more readable and lowers
> complexity of snapshot_write_next.

Again, good idea, but the same comments as for the previous patch apply here too.

Rafael

Re: [RFC 14/15] PM / Hibernate: dealign swsusp_info

[Rafael J. Wysocki]

On Tuesday 23 March 2010, Jiri Slaby wrote:
> From: Jiri Slaby <jirislaby@gmail.com>
> 
> Now there is no need to have swsusp_info page aligned thanks to chunk
> i/o support. We may add more info after it on the very same page.
> Later...
> 
> Signed-off-by: Jiri Slaby <jirislaby@gmail.com>
> Cc: Nigel Cunningham <ncunningham@crca.org.au>
> Cc: "Rafael J. Wysocki" <rjw@sisk.pl>

Fine by me.

Rafael


> ---
>  kernel/power/power.h |    2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)
> 
> diff --git a/kernel/power/power.h b/kernel/power/power.h
> index 842d27b..cf1450f 100644
> --- a/kernel/power/power.h
> +++ b/kernel/power/power.h
> @@ -13,7 +13,7 @@ struct swsusp_info {
>  	unsigned long		image_pages;
>  	unsigned long		pages;
>  	unsigned long		size;
> -} __attribute__((aligned(PAGE_SIZE)));
> +};
>  
>  #ifdef CONFIG_HIBERNATION
>  #ifdef CONFIG_ARCH_HIBERNATION_HEADER
> 

Re: [RFC 15/15] PM / Hibernate: move non-swap code to snapshot.c

[Rafael J. Wysocki]

On Tuesday 23 March 2010, Jiri Slaby wrote:
> Now, when all the swap-independent code was separated, it's time to
> move it into snapshot.c, because it is snapshot related.
> 
> Signed-off-by: Jiri Slaby <jslaby@suse.cz>
> Cc: Nigel Cunningham <ncunningham@crca.org.au>
> Cc: "Rafael J. Wysocki" <rjw@sisk.pl>

Quite frankly, I'd keep memory management in snapshot.c and move this stuff
to a separate file.

I have a plan to move snapshot.c to mm (perhaps under a different name) in
future, so that the mm people don't overlook it when they redesign things. ;-)

Rafael


> ---
>  kernel/power/power.h    |    7 --
>  kernel/power/snapshot.c |  196 +++++++++++++++++++++++++++++++++++++++++++++--
>  kernel/power/swap.c     |  182 -------------------------------------------
>  3 files changed, 189 insertions(+), 196 deletions(-)
> 
> diff --git a/kernel/power/power.h b/kernel/power/power.h
> index cf1450f..29c450a 100644
> --- a/kernel/power/power.h
> +++ b/kernel/power/power.h
> @@ -128,14 +128,7 @@ struct sws_module_ops {
>  	int (*write_page)(void *addr, struct bio **bio_chain);
>  };
>  
> -extern unsigned int snapshot_additional_pages(struct zone *zone);
>  extern unsigned long snapshot_get_image_size(void);
> -extern int snapshot_read_init(struct snapshot_handle *handle);
> -extern int snapshot_write_init(struct snapshot_handle *handle);
> -extern int snapshot_read_next(struct snapshot_handle *handle);
> -extern int snapshot_write_next(struct snapshot_handle *handle);
> -extern void snapshot_write_finalize(struct snapshot_handle *handle);
> -extern int snapshot_image_loaded(struct snapshot_handle *handle);
>  
>  /* If unset, the snapshot device cannot be open. */
>  extern atomic_t snapshot_device_available;
> diff --git a/kernel/power/snapshot.c b/kernel/power/snapshot.c
> index d432e87..a8a28da 100644
> --- a/kernel/power/snapshot.c
> +++ b/kernel/power/snapshot.c
> @@ -789,7 +789,7 @@ void free_basic_memory_bitmaps(void)
>   *	zone (usually the returned value is greater than the exact number)
>   */
>  
> -unsigned int snapshot_additional_pages(struct zone *zone)
> +static unsigned int snapshot_additional_pages(struct zone *zone)
>  {
>  	unsigned int res;
>  
> @@ -1605,7 +1605,7 @@ pack_pfns(unsigned long *buf, struct memory_bitmap *bm)
>   *
>   *	@handle: snapshot handle to init
>   */
> -int snapshot_write_init(struct snapshot_handle *handle)
> +static int snapshot_write_init(struct snapshot_handle *handle)
>  {
>  	int ret;
>  
> @@ -1646,7 +1646,7 @@ finish:
>   *	structure pointed to by @handle is not updated and should not be used
>   *	any more.
>   */
> -int snapshot_read_next(struct snapshot_handle *handle)
> +static int snapshot_read_next(struct snapshot_handle *handle)
>  {
>  	if (handle->cur < nr_meta_pages) {
>  		memset(buffer, 0, PAGE_SIZE);
> @@ -2134,7 +2134,7 @@ static void *get_buffer(struct memory_bitmap *bm, struct chain_allocator *ca)
>   *
>   *	@handle: snapshot handle to init
>   */
> -int snapshot_read_init(struct snapshot_handle *handle)
> +static int snapshot_read_init(struct snapshot_handle *handle)
>  {
>  	int ret;
>  
> @@ -2183,7 +2183,7 @@ finish:
>   *	structure pointed to by @handle is not updated and should not be used
>   *	any more.
>   */
> -int snapshot_write_next(struct snapshot_handle *handle)
> +static int snapshot_write_next(struct snapshot_handle *handle)
>  {
>  	static struct chain_allocator ca;
>  	int error = 0;
> @@ -2235,7 +2235,7 @@ int snapshot_write_next(struct snapshot_handle *handle)
>   *	used any more.
>   */
>  
> -void snapshot_write_finalize(struct snapshot_handle *handle)
> +static void snapshot_write_finalize(struct snapshot_handle *handle)
>  {
>  	copy_last_highmem_page();
>  	/* Free only if we have loaded the image entirely */
> @@ -2245,12 +2245,194 @@ void snapshot_write_finalize(struct snapshot_handle *handle)
>  	}
>  }
>  
> -int snapshot_image_loaded(struct snapshot_handle *handle)
> +static int snapshot_image_loaded(struct snapshot_handle *handle)
>  {
>  	return !(!nr_copy_pages || !last_highmem_page_copied() ||
>  			handle->cur < nr_meta_pages + nr_copy_pages);
>  }
>  
> +/**
> + *	save_image - save the suspend image data
> + */
> +
> +static int save_image(struct snapshot_handle *snapshot,
> +                      unsigned int nr_to_write)
> +{
> +	unsigned int m;
> +	int ret;
> +	int nr_pages;
> +	int err2;
> +	struct bio *bio;
> +	struct timeval start;
> +	struct timeval stop;
> +
> +	printk(KERN_INFO "PM: Saving image data pages (%u pages) ...     ",
> +		nr_to_write);
> +	m = nr_to_write / 100;
> +	if (!m)
> +		m = 1;
> +	nr_pages = 0;
> +	bio = NULL;
> +	do_gettimeofday(&start);
> +	while (1) {
> +		ret = snapshot_read_next(snapshot);
> +		if (ret <= 0)
> +			break;
> +		ret = sws_io_ops->write_page(data_of(*snapshot), &bio);
> +		if (ret)
> +			break;
> +		if (!(nr_pages % m))
> +			printk(KERN_CONT "\b\b\b\b%3d%%", nr_pages / m);
> +		nr_pages++;
> +	}
> +	err2 = sws_wait_on_bio_chain(&bio);
> +	do_gettimeofday(&stop);
> +	if (!ret)
> +		ret = err2;
> +	if (!ret)
> +		printk(KERN_CONT "\b\b\b\bdone\n");
> +	else
> +		printk(KERN_CONT "\n");
> +	swsusp_show_speed(&start, &stop, nr_to_write, "Wrote");
> +	return ret;
> +}
> +
> +/**
> + *	enough_space - Make sure we have enough space to save the image.
> + *
> + *	Returns TRUE or FALSE after checking the total amount of
> + *	space avaiable from the resume block.
> + */
> +
> +static int enough_space(unsigned int nr_pages)
> +{
> +	unsigned int free_pages = sws_io_ops->storage_available();
> +
> +	pr_debug("PM: Free storage pages: %u\n", free_pages);
> +	return free_pages > nr_pages + PAGES_FOR_IO;
> +}
> +
> +/**
> + *	swsusp_write - Write entire image and metadata.
> + *	@flags: flags to pass to the "boot" kernel in the image header
> + *
> + *	It is important _NOT_ to umount filesystems at this point. We want
> + *	them synced (in case something goes wrong) but we DO not want to mark
> + *	filesystem clean: it is not. (And it does not matter, if we resume
> + *	correctly, we'll mark system clean, anyway.)
> + */
> +
> +int swsusp_write(unsigned int flags)
> +{
> +	struct snapshot_handle snapshot;
> +	unsigned long pages;
> +	int error;
> +
> +	pages = snapshot_get_image_size();
> +	error = sws_io_ops->get_writer();
> +	if (error) {
> +		printk(KERN_ERR "PM: Cannot get swap writer\n");
> +		return error;
> +	}
> +	if (!enough_space(pages)) {
> +		printk(KERN_ERR "PM: Not enough free space for image\n");
> +		error = -ENOSPC;
> +		goto out_finish;
> +	}
> +	memset(&snapshot, 0, sizeof(struct snapshot_handle));
> +	error = snapshot_write_init(&snapshot);
> +	if (error) {
> +		printk(KERN_ERR "PM: Cannot init writer\n");
> +		goto out_finish;
> +	}
> +	error = save_image(&snapshot, pages - 1);
> +out_finish:
> +	error = sws_io_ops->put_writer(flags, error);
> +	return error;
> +}
> +
> +/**
> + *	load_image - load the image
> + *	@handle and the snapshot handle @snapshot
> + *	(assume there are @nr_pages pages to load)
> + */
> +
> +static int load_image(struct snapshot_handle *snapshot, unsigned int nr_to_read)
> +{
> +	unsigned int m;
> +	int error = 0;
> +	struct timeval start;
> +	struct timeval stop;
> +	struct bio *bio;
> +	int err2;
> +	unsigned nr_pages;
> +
> +	printk(KERN_INFO "PM: Loading image data pages (%u pages) ...     ",
> +		nr_to_read);
> +	m = nr_to_read / 100;
> +	if (!m)
> +		m = 1;
> +	nr_pages = 0;
> +	bio = NULL;
> +	do_gettimeofday(&start);
> +	for ( ; ; ) {
> +		error = sws_io_ops->read_page(data_of(*snapshot), &bio);
> +		if (error)
> +			break;
> +		if (snapshot->sync_read)
> +			error = sws_wait_on_bio_chain(&bio);
> +		if (error)
> +			break;
> +		error = snapshot_write_next(snapshot);
> +		if (error >= 0)
> +			nr_pages++;
> +		if (error <= 0)
> +			break;
> +		if (!(nr_pages % m))
> +			printk("\b\b\b\b%3d%%", nr_pages / m);
> +	}
> +	err2 = sws_wait_on_bio_chain(&bio);
> +	do_gettimeofday(&stop);
> +	if (!error)
> +		error = err2;
> +	if (!error) {
> +		printk("\b\b\b\bdone\n");
> +		snapshot_write_finalize(snapshot);
> +		if (!snapshot_image_loaded(snapshot))
> +			error = -ENODATA;
> +	} else
> +		printk("\n");
> +	swsusp_show_speed(&start, &stop, nr_to_read, "Read");
> +	return error;
> +}
> +
> +/**
> + *	swsusp_read - read the hibernation image.
> + *	@flags_p: flags passed by the "frozen" kernel in the image header should
> + *		  be written into this memeory location
> + */
> +
> +int swsusp_read(unsigned int *flags_p)
> +{
> +	int error;
> +	struct snapshot_handle snapshot;
> +
> +	memset(&snapshot, 0, sizeof(struct snapshot_handle));
> +	error = sws_io_ops->get_reader(flags_p);
> +	if (error)
> +		goto end;
> +	error = snapshot_read_init(&snapshot);
> +	if (!error)
> +		error = load_image(&snapshot, snapshot_get_image_size() - 1);
> +	sws_io_ops->put_reader();
> +end:
> +	if (!error)
> +		pr_debug("PM: Image successfully loaded\n");
> +	else
> +		pr_debug("PM: Error %d resuming\n", error);
> +	return error;
> +}
> +
>  #ifdef CONFIG_HIGHMEM
>  /* Assumes that @buf is ready and points to a "safe" page */
>  static inline void
> diff --git a/kernel/power/swap.c b/kernel/power/swap.c
> index 4472cf3..ddd7238 100644
> --- a/kernel/power/swap.c
> +++ b/kernel/power/swap.c
> @@ -360,106 +360,6 @@ static int put_swap_writer(unsigned int flags, int error)
>  	return error;
>  }
>  
> -/**
> - *	save_image - save the suspend image data
> - */
> -
> -static int save_image(struct snapshot_handle *snapshot,
> -                      unsigned int nr_to_write)
> -{
> -	unsigned int m;
> -	int ret;
> -	int nr_pages;
> -	int err2;
> -	struct bio *bio;
> -	struct timeval start;
> -	struct timeval stop;
> -
> -	printk(KERN_INFO "PM: Saving image data pages (%u pages) ...     ",
> -		nr_to_write);
> -	m = nr_to_write / 100;
> -	if (!m)
> -		m = 1;
> -	nr_pages = 0;
> -	bio = NULL;
> -	do_gettimeofday(&start);
> -	while (1) {
> -		ret = snapshot_read_next(snapshot);
> -		if (ret <= 0)
> -			break;
> -		ret = sws_io_ops->write_page(data_of(*snapshot), &bio);
> -		if (ret)
> -			break;
> -		if (!(nr_pages % m))
> -			printk(KERN_CONT "\b\b\b\b%3d%%", nr_pages / m);
> -		nr_pages++;
> -	}
> -	err2 = sws_wait_on_bio_chain(&bio);
> -	do_gettimeofday(&stop);
> -	if (!ret)
> -		ret = err2;
> -	if (!ret)
> -		printk(KERN_CONT "\b\b\b\bdone\n");
> -	else
> -		printk(KERN_CONT "\n");
> -	swsusp_show_speed(&start, &stop, nr_to_write, "Wrote");
> -	return ret;
> -}
> -
> -/**
> - *	enough_space - Make sure we have enough space to save the image.
> - *
> - *	Returns TRUE or FALSE after checking the total amount of
> - *	space avaiable from the resume block.
> - */
> -
> -static int enough_space(unsigned int nr_pages)
> -{
> -	unsigned int free_pages = sws_io_ops->storage_available();
> -
> -	pr_debug("PM: Free storage pages: %u\n", free_pages);
> -	return free_pages > nr_pages + PAGES_FOR_IO;
> -}
> -
> -/**
> - *	swsusp_write - Write entire image and metadata.
> - *	@flags: flags to pass to the "boot" kernel in the image header
> - *
> - *	It is important _NOT_ to umount filesystems at this point. We want
> - *	them synced (in case something goes wrong) but we DO not want to mark
> - *	filesystem clean: it is not. (And it does not matter, if we resume
> - *	correctly, we'll mark system clean, anyway.)
> - */
> -
> -int swsusp_write(unsigned int flags)
> -{
> -	struct snapshot_handle snapshot;
> -	unsigned long pages;
> -	int error;
> -
> -	pages = snapshot_get_image_size();
> -	error = sws_io_ops->get_writer();
> -	if (error) {
> -		printk(KERN_ERR "PM: Cannot get swap writer\n");
> -		return error;
> -	}
> -	if (!enough_space(pages)) {
> -		printk(KERN_ERR "PM: Not enough free space for image\n");
> -		error = -ENOSPC;
> -		goto out_finish;
> -	}
> -	memset(&snapshot, 0, sizeof(struct snapshot_handle));
> -	error = snapshot_write_init(&snapshot);
> -	if (error) {
> -		printk(KERN_ERR "PM: Cannot init writer\n");
> -		goto out_finish;
> -	}
> -	error = save_image(&snapshot, pages - 1);
> -out_finish:
> -	error = sws_io_ops->put_writer(flags, error);
> -	return error;
> -}
> -
>  static unsigned long swap_storage_available(void)
>  {
>  	return count_swap_pages(root_swap, 1);
> @@ -546,88 +446,6 @@ struct sws_module_ops swap_ops = {
>  };
>  
>  /**
> - *	load_image - load the image
> - *	@handle and the snapshot handle @snapshot
> - *	(assume there are @nr_pages pages to load)
> - */
> -
> -static int load_image(struct snapshot_handle *snapshot, unsigned int nr_to_read)
> -{
> -	unsigned int m;
> -	int error = 0;
> -	struct timeval start;
> -	struct timeval stop;
> -	struct bio *bio;
> -	int err2;
> -	unsigned nr_pages;
> -
> -	printk(KERN_INFO "PM: Loading image data pages (%u pages) ...     ",
> -		nr_to_read);
> -	m = nr_to_read / 100;
> -	if (!m)
> -		m = 1;
> -	nr_pages = 0;
> -	bio = NULL;
> -	do_gettimeofday(&start);
> -	for ( ; ; ) {
> -		error = sws_io_ops->read_page(data_of(*snapshot), &bio);
> -		if (error)
> -			break;
> -		if (snapshot->sync_read)
> -			error = sws_wait_on_bio_chain(&bio);
> -		if (error)
> -			break;
> -		error = snapshot_write_next(snapshot);
> -		if (error >= 0)
> -			nr_pages++;
> -		if (error <= 0)
> -			break;
> -		if (!(nr_pages % m))
> -			printk("\b\b\b\b%3d%%", nr_pages / m);
> -	}
> -	err2 = sws_wait_on_bio_chain(&bio);
> -	do_gettimeofday(&stop);
> -	if (!error)
> -		error = err2;
> -	if (!error) {
> -		printk("\b\b\b\bdone\n");
> -		snapshot_write_finalize(snapshot);
> -		if (!snapshot_image_loaded(snapshot))
> -			error = -ENODATA;
> -	} else
> -		printk("\n");
> -	swsusp_show_speed(&start, &stop, nr_to_read, "Read");
> -	return error;
> -}
> -
> -/**
> - *	swsusp_read - read the hibernation image.
> - *	@flags_p: flags passed by the "frozen" kernel in the image header should
> - *		  be written into this memeory location
> - */
> -
> -int swsusp_read(unsigned int *flags_p)
> -{
> -	int error;
> -	struct snapshot_handle snapshot;
> -
> -	memset(&snapshot, 0, sizeof(struct snapshot_handle));
> -	error = sws_io_ops->get_reader(flags_p);
> -	if (error)
> -		goto end;
> -	error = snapshot_read_init(&snapshot);
> -	if (!error)
> -		error = load_image(&snapshot, snapshot_get_image_size() - 1);
> -	sws_io_ops->put_reader();
> -end:
> -	if (!error)
> -		pr_debug("PM: Image successfully loaded\n");
> -	else
> -		pr_debug("PM: Error %d resuming\n", error);
> -	return error;
> -}
> -
> -/**
>   *      swsusp_check - Check for swsusp signature in the resume device
>   */
>  
>