[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

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

[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

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: [linux-pm] [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: [linux-pm] [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: [linux-pm] [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: [linux-pm] [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: [linux-pm] [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: [linux-pm] [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 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

[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

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

[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

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
> 

[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

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

[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

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

[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

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

[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

[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

[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

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: [linux-pm] 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: [linux-pm] 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: [linux-pm] 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: [linux-pm] 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

[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

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

[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

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

[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

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

[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

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
> 

[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 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
>   */
>  
> 

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: [linux-pm] [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 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);
>  
>