[Suspend2][ 0/9] Extents support.

[Suspend2][ 0/9] Extents support.

[Nigel Cunningham]

Add Suspend2 extent support. Extents are used for storing the lists
of blocks to which the image will be written, and are stored in the
image header for use at resume time.

[Suspend2][ 1/9] [Suspend2] Extents header.

[Nigel Cunningham]

Add the header of the extents.c. Extents are used to record the storage
that is used - both swap_entry_ts and sector offsets in devices.

Signed-off-by: Nigel Cunningham <nigel@suspend2.net>

 kernel/power/extent.c |   19 +++++++++++++++++++
 1 files changed, 19 insertions(+), 0 deletions(-)

diff --git a/kernel/power/extent.c b/kernel/power/extent.c
new file mode 100644
index 0000000..4027bb1
--- /dev/null
+++ b/kernel/power/extent.c
@@ -0,0 +1,19 @@
+/* 
+ * kernel/power/extent.c
+ * 
+ * (C) 2003-2006 Nigel Cunningham <nigel@suspend2.net>
+ *
+ * Distributed under GPLv2.
+ * 
+ * These functions encapsulate the manipulation of storage metadata. For
+ * pageflags, we use dynamically allocated bitmaps.
+ */
+
+#include <linux/module.h>
+#include <linux/suspend.h>
+#include "modules.h"
+#include "extent.h"
+#include "ui.h"
+
+int suspend_extents_allocated = 0;
+

--
Nigel Cunningham		nigel at suspend2 dot net

[Suspend2][ 2/9] [Suspend2] Extent allocation routines.

[Nigel Cunningham]

Add routines to get and put extents.

Signed-off-by: Nigel Cunningham <nigel@suspend2.net>

 kernel/power/extent.c |   30 ++++++++++++++++++++++++++++++
 1 files changed, 30 insertions(+), 0 deletions(-)

diff --git a/kernel/power/extent.c b/kernel/power/extent.c
index 4027bb1..2fb6a23 100644
--- a/kernel/power/extent.c
+++ b/kernel/power/extent.c
@@ -17,3 +17,33 @@
 
 int suspend_extents_allocated = 0;
 
+/* suspend_get_extent
+ *
+ * Returns a free extent. May fail, returning NULL instead.
+ */
+static struct extent *suspend_get_extent(void)
+{
+	struct extent *result;
+	
+	if (!(result = kmalloc(sizeof(struct extent), GFP_ATOMIC)))
+		return NULL;
+
+	suspend_extents_allocated++;
+	result->minimum = result->maximum = 0;
+	result->next = NULL;
+
+	return result;
+}
+
+/* suspend_put_extent.
+ *
+ * Frees an extent. Assumes unlinking is done by the caller.
+ */
+void suspend_put_extent(struct extent *extent)
+{
+	BUG_ON(!extent);
+
+	kfree(extent);
+	suspend_extents_allocated--;
+}
+

--
Nigel Cunningham		nigel at suspend2 dot net

[Suspend2][ 3/9] [Suspend2] Free a whole extent chain.

[Nigel Cunningham]

Add a routine to free an entire extent chain at once.

Signed-off-by: Nigel Cunningham <nigel@suspend2.net>

 kernel/power/extent.c |   23 +++++++++++++++++++++++
 1 files changed, 23 insertions(+), 0 deletions(-)

diff --git a/kernel/power/extent.c b/kernel/power/extent.c
index 2fb6a23..ab19509 100644
--- a/kernel/power/extent.c
+++ b/kernel/power/extent.c
@@ -47,3 +47,26 @@ void suspend_put_extent(struct extent *e
 	suspend_extents_allocated--;
 }
 
+/* suspend_put_extent_chain.
+ *
+ * Frees a whole chain of extents.
+ */
+void suspend_put_extent_chain(struct extent_chain *chain)
+{
+	struct extent *this;
+
+	this = chain->first;
+
+	while(this) {
+		struct extent *next = this->next;
+		kfree(this);
+		chain->frees++;
+		suspend_extents_allocated --;
+		this = next;
+	}
+	
+	BUG_ON(chain->frees != chain->allocs);
+	chain->first = chain->last = chain->last_touched = NULL;
+	chain->size = chain->allocs = chain->frees = 0;
+}
+

--
Nigel Cunningham		nigel at suspend2 dot net

[Suspend2][ 4/9] [Suspend2] Add extent to extent chain.

[Nigel Cunningham]

Add a new extent, possibly merging it with existing extents.

Signed-off-by: Nigel Cunningham <nigel@suspend2.net>

 kernel/power/extent.c |   71 +++++++++++++++++++++++++++++++++++++++++++++++++
 1 files changed, 71 insertions(+), 0 deletions(-)

diff --git a/kernel/power/extent.c b/kernel/power/extent.c
index ab19509..31fcb65 100644
--- a/kernel/power/extent.c
+++ b/kernel/power/extent.c
@@ -70,3 +70,74 @@ void suspend_put_extent_chain(struct ext
 	chain->size = chain->allocs = chain->frees = 0;
 }
 
+/* 
+ * suspend_add_to_extent_chain
+ *
+ * Add an extent to an existing chain.
+ */
+int suspend_add_to_extent_chain(struct extent_chain *chain, 
+		unsigned long minimum, unsigned long maximum)
+{
+	struct extent *new_extent = NULL, *start_at;
+
+	/* Find the right place in the chain */
+	start_at = (chain->last_touched && 
+		    (chain->last_touched->minimum < minimum)) ?
+		chain->last_touched : NULL;
+
+	if (!start_at && chain->first && chain->first->minimum < minimum)
+		start_at = chain->first;
+
+	while (start_at && start_at->next && start_at->next->minimum < minimum)
+		start_at = start_at->next;
+
+	if (start_at && start_at->maximum == (minimum - 1)) {
+		start_at->maximum = maximum;
+
+		/* Merge with the following one? */
+		if (start_at->next &&
+		    start_at->maximum + 1 == start_at->next->minimum) {
+			struct extent *to_free = start_at->next;
+			start_at->maximum = start_at->next->maximum;
+			start_at->next = start_at->next->next;
+			chain->frees++;
+			suspend_put_extent(to_free);
+		}
+
+		chain->last_touched = start_at;
+		chain->size+= (maximum - minimum + 1);
+
+		return 0;
+	}
+
+	new_extent = suspend_get_extent();
+	if (!new_extent) {
+		printk("Error unable to append a new extent to the chain.\n");
+		return 2;
+	}
+
+	chain->allocs++;
+	chain->size+= (maximum - minimum + 1);
+	new_extent->minimum = minimum;
+	new_extent->maximum = maximum;
+	new_extent->next = NULL;
+
+	chain->last_touched = new_extent;
+
+	if (start_at) {
+		struct extent *next = start_at->next;
+		start_at->next = new_extent;
+		new_extent->next = next;
+		if (!next)
+			chain->last = new_extent;
+	} else {
+		if (chain->first) {
+			new_extent->next = chain->first;
+			chain->first = new_extent;
+		} else
+			chain->last = chain->first = new_extent;
+	}
+
+	return 0;
+}
+

--
Nigel Cunningham		nigel at suspend2 dot net

[Suspend2][ 5/9] [Suspend2] Serialise extent chains.

[Nigel Cunningham]

Add routines for storing extent chains in an image header.

Signed-off-by: Nigel Cunningham <nigel@suspend2.net>

 kernel/power/extent.c |   65 +++++++++++++++++++++++++++++++++++++++++++++++++
 1 files changed, 65 insertions(+), 0 deletions(-)

diff --git a/kernel/power/extent.c b/kernel/power/extent.c
index 31fcb65..f7db014 100644
--- a/kernel/power/extent.c
+++ b/kernel/power/extent.c
@@ -141,3 +141,68 @@ int suspend_add_to_extent_chain(struct e
 	return 0;
 }
 
+/* suspend_serialise_extent_chain
+ *
+ * Write a chain in the image.
+ */
+int suspend_serialise_extent_chain(struct suspend_module_ops *owner,
+		struct extent_chain *chain)
+{
+	struct extent *this;
+	int ret, i = 0;
+	
+	if ((ret = suspend_active_writer->rw_header_chunk(WRITE, owner,
+		(char *) chain,
+		3 * sizeof(int))))
+		return ret;
+
+	this = chain->first;
+	while (this) {
+		if ((ret = suspend_active_writer->rw_header_chunk(WRITE, owner,
+				(char *) this,
+				2 * sizeof(unsigned long))))
+			return ret;
+		this = this->next;
+		i++;
+	}
+
+	if (i != (chain->allocs - chain->frees)) {
+		printk(KERN_EMERG "Saved %d extents but chain metadata says there should be %d-%d.\n",
+				i, chain->allocs, chain->frees);
+		BUG();
+	}
+
+	return ret;
+}
+
+/* suspend_load_extent_chain
+ *
+ * Read back a chain saved in the image.
+ */
+int suspend_load_extent_chain(struct extent_chain *chain)
+{
+	struct extent *this, *last = NULL;
+	int i, ret;
+
+	if (!(ret = suspend_active_writer->rw_header_chunk(READ, NULL,
+		(char *) chain,
+		3 * sizeof(int))))
+		return ret;
+
+	for (i = 0; i < (chain->allocs - chain->frees); i++) {
+		this = kmalloc(sizeof(struct extent), GFP_ATOMIC);
+		BUG_ON(!this); /* Shouldn't run out of memory trying this! */
+		this->next = NULL;
+		if (!(ret = suspend_active_writer->rw_header_chunk(READ, NULL,
+				(char *) this, 2 * sizeof(unsigned long))))
+			return ret;
+		if (last)
+			last->next = this;
+		else
+			chain->first = this;
+		last = this;
+	}
+	chain->last = last;
+	return ret;
+}
+

--
Nigel Cunningham		nigel at suspend2 dot net

[Suspend2][ 6/9] [Suspend2] Get next extent in an extent state.

[Nigel Cunningham]

Add a routine for getting the next device and block to which a page will be
written. We are essentially iterating through a series of one (filewriter)
or more (swapwriter) devices, each having a chain of sectors in which the
image is stored.

Signed-off-by: Nigel Cunningham <nigel@suspend2.net>

 kernel/power/extent.c |   43 +++++++++++++++++++++++++++++++++++++++++++
 1 files changed, 43 insertions(+), 0 deletions(-)

diff --git a/kernel/power/extent.c b/kernel/power/extent.c
index f7db014..8cbb48a 100644
--- a/kernel/power/extent.c
+++ b/kernel/power/extent.c
@@ -206,3 +206,46 @@ int suspend_load_extent_chain(struct ext
 	return ret;
 }
 
+/* suspend_extent_state_next
+ *
+ * Given a state, progress to the next valid entry. We may begin in an
+ * invalid state, as we do when invoked after extent_state_goto_start below.
+ *
+ * When using compression and expected_compression > 0, we allocate fewer
+ * swap entries, so we can validly run out of data to return.
+ */
+unsigned long suspend_extent_state_next(struct extent_iterate_state *state)
+{
+	if (state->current_chain > state->num_chains)
+		return 0;
+
+	if (state->current_extent) {
+		if (state->current_offset == state->current_extent->maximum) {
+			if (state->current_extent->next) {
+				state->current_extent = state->current_extent->next;
+				state->current_offset = state->current_extent->minimum;
+			} else {
+				state->current_extent = NULL;
+				state->current_offset = 0;
+			}
+		} else
+			state->current_offset++;
+	}
+
+	while(!state->current_extent) {
+		int chain_num = ++(state->current_chain);
+
+		if (chain_num > state->num_chains)
+			return 0;
+
+		state->current_extent = (state->chains + chain_num)->first;
+
+		if (!state->current_extent)
+			continue;
+
+		state->current_offset = state->current_extent->minimum;
+	}
+
+	return state->current_offset;
+}
+

--
Nigel Cunningham		nigel at suspend2 dot net

[Suspend2][ 7/9] [Suspend2] Extent state to the start.

[Nigel Cunningham]

Reset an extent state to the start of the data.

Signed-off-by: Nigel Cunningham <nigel@suspend2.net>

 kernel/power/extent.c |   11 +++++++++++
 1 files changed, 11 insertions(+), 0 deletions(-)

diff --git a/kernel/power/extent.c b/kernel/power/extent.c
index 8cbb48a..248e4de 100644
--- a/kernel/power/extent.c
+++ b/kernel/power/extent.c
@@ -249,3 +249,14 @@ unsigned long suspend_extent_state_next(
 	return state->current_offset;
 }
 
+/* suspend_extent_state_goto_start
+ *
+ * Find the first valid value in a group of chains.
+ */
+void suspend_extent_state_goto_start(struct extent_iterate_state *state)
+{
+	state->current_chain = -1;
+	state->current_extent = NULL;
+	state->current_offset = 0;
+}
+

--
Nigel Cunningham		nigel at suspend2 dot net

[Suspend2][ 8/9] [Suspend2] Extent state save and restore.

[Nigel Cunningham]

Add support for remembering a position and restoring it later. This is used
to divide the storage into streams (the header, and two parts to the data
proper).

Signed-off-by: Nigel Cunningham <nigel@suspend2.net>

 kernel/power/extent.c |   47 +++++++++++++++++++++++++++++++++++++++++++++++
 1 files changed, 47 insertions(+), 0 deletions(-)

diff --git a/kernel/power/extent.c b/kernel/power/extent.c
index 248e4de..a3b9569 100644
--- a/kernel/power/extent.c
+++ b/kernel/power/extent.c
@@ -260,3 +260,50 @@ void suspend_extent_state_goto_start(str
 	state->current_offset = 0;
 }
 
+/* suspend_extent_start_save
+ *
+ * Given a state and a struct extent_state_store, save the crreutn
+ * position in a format that can be used with relocated chains (at
+ * resume time).
+ */
+void suspend_extent_state_save(struct extent_iterate_state *state,
+		struct extent_iterate_saved_state *saved_state)
+{
+	struct extent *extent;
+
+	saved_state->chain_num = state->current_chain;
+	saved_state->extent_num = 0;
+	saved_state->offset = state->current_offset;
+
+	if (saved_state->chain_num == -1)
+		return;
+	
+	extent = (state->chains + state->current_chain)->first;
+
+	while (extent != state->current_extent) {
+		saved_state->extent_num++;
+		extent = extent->next;
+	}
+}
+
+/* suspend_extent_start_restore
+ *
+ * Restore the position saved by extent_state_save.
+ */
+void suspend_extent_state_restore(struct extent_iterate_state *state,
+		struct extent_iterate_saved_state *saved_state)
+{
+	int posn = saved_state->extent_num;
+
+	if (saved_state->chain_num == -1) {
+		suspend_extent_state_goto_start(state);
+		return;
+	}
+
+	state->current_chain = saved_state->chain_num;
+	state->current_extent = (state->chains + state->current_chain)->first;
+	state->current_offset = saved_state->offset;
+
+	while (posn--)
+		state->current_extent = state->current_extent->next;
+}

--
Nigel Cunningham		nigel at suspend2 dot net

[Suspend2][ 9/9] [Suspend2] Extent header.

[Nigel Cunningham]

Add the header file for extents.

Signed-off-by: Nigel Cunningham <nigel@suspend2.net>

 kernel/power/extent.h |   82 +++++++++++++++++++++++++++++++++++++++++++++++++
 1 files changed, 82 insertions(+), 0 deletions(-)

diff --git a/kernel/power/extent.h b/kernel/power/extent.h
new file mode 100644
index 0000000..f68ca55
--- /dev/null
+++ b/kernel/power/extent.h
@@ -0,0 +1,82 @@
+/*
+ * kernel/power/extent.h
+ *
+ * Copyright (C) 2004-2006 Nigel Cunningham <nigel@suspend2.net>
+ *
+ * This file is released under the GPLv2.
+ *
+ * It contains declarations related to extents. Extents are
+ * suspend's method of storing some of the metadata for the image.
+ * See extent.c for more info.
+ *
+ */
+
+#include "modules.h"
+
+#ifndef EXTENT_H
+#define EXTENT_H
+struct extent_chain {
+	int size; /* size of the extent ie sum (max-min+1) */
+	int allocs, frees;
+	char *name;
+	struct extent *first, *last, *last_touched;
+};
+
+/*
+ * We rely on extents not fitting evenly into a page.
+ * The last four bytes are used to store the number
+ * of the page, to make saving & reloading pages simpler.
+ */
+struct extent {
+	unsigned long minimum, maximum;
+	struct extent *next;
+};
+
+struct extent_iterate_state {
+	struct extent_chain *chains;
+	int num_chains;
+	int current_chain;
+	struct extent *current_extent;
+	unsigned long current_offset;
+};
+
+struct extent_iterate_saved_state {
+	int chain_num;
+	int extent_num;
+	unsigned long offset;
+};
+
+#define suspend_extent_state_eof(state) ((state)->num_chains < (state)->current_chain)
+
+#define suspend_extent_for_each(extent_chain, extentpointer, value) \
+if ((extent_chain)->first) \
+	for ((extentpointer) = (extent_chain)->first, (value) = \
+			(extentpointer)->minimum; \
+	     ((extentpointer) && ((extentpointer)->next || (value) <= \
+				 (extentpointer)->maximum)); \
+	     (((value) == (extentpointer)->maximum) ? \
+		((extentpointer) = (extentpointer)->next, (value) = \
+		 ((extentpointer) ? (extentpointer)->minimum : 0)) : \
+			(value)++))
+
+extern int suspend_extents_allocated;
+void suspend_put_extent_chain(struct extent_chain *chain);
+int suspend_add_to_extent_chain(struct extent_chain *chain, 
+		unsigned long minimum, unsigned long maximum);
+int suspend_serialise_extent_chain(struct suspend_module_ops *owner,
+		struct extent_chain *chain);
+int suspend_load_extent_chain(struct extent_chain *chain);
+
+/* swap_entry_to_extent_val & extent_val_to_swap_entry: 
+ * We are putting offset in the low bits so consecutive swap entries
+ * make consecutive extent values */
+#define swap_entry_to_extent_val(swp_entry) (swp_entry.val)
+#define extent_val_to_swap_entry(val) (swp_entry_t) { (val) }
+
+void suspend_extent_state_save(struct extent_iterate_state *state,
+		struct extent_iterate_saved_state *saved_state);
+void suspend_extent_state_restore(struct extent_iterate_state *state,
+		struct extent_iterate_saved_state *saved_state);
+void suspend_extent_state_goto_start(struct extent_iterate_state *state);
+unsigned long suspend_extent_state_next(struct extent_iterate_state *state);
+#endif

--
Nigel Cunningham		nigel at suspend2 dot net

Re: [Suspend2][ 0/9] Extents support.

[Rafael J. Wysocki]

On Monday 26 June 2006 18:54, Nigel Cunningham wrote:
> 
> Add Suspend2 extent support. Extents are used for storing the lists
> of blocks to which the image will be written, and are stored in the
> image header for use at resume time.

Could you please put all of the changes in kernel/power/extents.c into one
patch?  It's quite difficult to review them now, at least for me.

Well, I think similar remarks will apply to the other series of patches too, so
I won't repeat them if you don't mind.  [Also I won't be able to have a look
at the other patches today.  I'll do my best to review them in the next couple
of days.]

Greetings,
Rafael

Re: [Suspend2][ 0/9] Extents support.

[Nigel Cunningham]

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

Hi.

On Tuesday 27 June 2006 07:20, Rafael J. Wysocki wrote:
> On Monday 26 June 2006 18:54, Nigel Cunningham wrote:
> > Add Suspend2 extent support. Extents are used for storing the lists
> > of blocks to which the image will be written, and are stored in the
> > image header for use at resume time.
>
> Could you please put all of the changes in kernel/power/extents.c into one
> patch?  It's quite difficult to review them now, at least for me.

I spent a long time splitting them up because I was asked in previous 
iterations to break them into manageable chunks. How about if I were to email 
you the individual files off line, so as to not send the same amount again?

> Well, I think similar remarks will apply to the other series of patches
> too, so I won't repeat them if you don't mind.  [Also I won't be able to
> have a look at the other patches today.  I'll do my best to review them in
> the next couple of days.]

Thanks. No rush.

Regards,

Nigel
-- 
See http://www.suspend2.net for Howtos, FAQs, mailing
lists, wiki and bugzilla info.

[-- Attachment #2: Type: application/pgp-signature, Size: 189 bytes --]

Re: [Suspend2][ 0/9] Extents support.

[Jens Axboe]

On Tue, Jun 27 2006, Nigel Cunningham wrote:
> Hi.
> 
> On Tuesday 27 June 2006 07:20, Rafael J. Wysocki wrote:
> > On Monday 26 June 2006 18:54, Nigel Cunningham wrote:
> > > Add Suspend2 extent support. Extents are used for storing the lists
> > > of blocks to which the image will be written, and are stored in the
> > > image header for use at resume time.
> >
> > Could you please put all of the changes in kernel/power/extents.c into one
> > patch?  It's quite difficult to review them now, at least for me.
> 
> I spent a long time splitting them up because I was asked in previous 
> iterations to break them into manageable chunks. How about if I were to email 
> you the individual files off line, so as to not send the same amount again?

Managable chunks means logical changes go together, one function per
diff is really extreme and unreviewable. Support for extents is one
logical change, so it's one patch. Unless of course you have to do some
preparatory patches, then you'd do those separately.

I must admit I thought you were kidding when I read through this extents
patch series, having a single patch just adding includes!

-- 
Jens Axboe

Re: [Suspend2][ 0/9] Extents support.

[Nigel Cunningham]

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

Hi.

On Tuesday 27 June 2006 15:36, Jens Axboe wrote:
> On Tue, Jun 27 2006, Nigel Cunningham wrote:
> > Hi.
> >
> > On Tuesday 27 June 2006 07:20, Rafael J. Wysocki wrote:
> > > On Monday 26 June 2006 18:54, Nigel Cunningham wrote:
> > > > Add Suspend2 extent support. Extents are used for storing the lists
> > > > of blocks to which the image will be written, and are stored in the
> > > > image header for use at resume time.
> > >
> > > Could you please put all of the changes in kernel/power/extents.c into
> > > one patch?  It's quite difficult to review them now, at least for me.
> >
> > I spent a long time splitting them up because I was asked in previous
> > iterations to break them into manageable chunks. How about if I were to
> > email you the individual files off line, so as to not send the same
> > amount again?
>
> Managable chunks means logical changes go together, one function per
> diff is really extreme and unreviewable. Support for extents is one
> logical change, so it's one patch. Unless of course you have to do some
> preparatory patches, then you'd do those separately.
>
> I must admit I thought you were kidding when I read through this extents
> patch series, having a single patch just adding includes!

Sorry for fluffing it up. I'm pretty inexperienced, but I'm trying to follow 
CodingStyle and all the other advice. If I'd known I'd misunderstood what was 
wanted, I probably could have submitted this months ago. Oh well. Live and 
learn. What would you have me do at this point?

Regards,

Nigel

-- 
See http://www.suspend2.net for Howtos, FAQs, mailing
lists, wiki and bugzilla info.

[-- Attachment #2: Type: application/pgp-signature, Size: 189 bytes --]

Re: [Suspend2][ 0/9] Extents support.

[Jens Axboe]

On Tue, Jun 27 2006, Nigel Cunningham wrote:
> Hi.
> 
> On Tuesday 27 June 2006 15:36, Jens Axboe wrote:
> > On Tue, Jun 27 2006, Nigel Cunningham wrote:
> > > Hi.
> > >
> > > On Tuesday 27 June 2006 07:20, Rafael J. Wysocki wrote:
> > > > On Monday 26 June 2006 18:54, Nigel Cunningham wrote:
> > > > > Add Suspend2 extent support. Extents are used for storing the lists
> > > > > of blocks to which the image will be written, and are stored in the
> > > > > image header for use at resume time.
> > > >
> > > > Could you please put all of the changes in kernel/power/extents.c into
> > > > one patch?  It's quite difficult to review them now, at least for me.
> > >
> > > I spent a long time splitting them up because I was asked in previous
> > > iterations to break them into manageable chunks. How about if I were to
> > > email you the individual files off line, so as to not send the same
> > > amount again?
> >
> > Managable chunks means logical changes go together, one function per
> > diff is really extreme and unreviewable. Support for extents is one
> > logical change, so it's one patch. Unless of course you have to do some
> > preparatory patches, then you'd do those separately.
> >
> > I must admit I thought you were kidding when I read through this extents
> > patch series, having a single patch just adding includes!
> 
> Sorry for fluffing it up. I'm pretty inexperienced, but I'm trying to
> follow CodingStyle and all the other advice. If I'd known I'd
> misunderstood what was wanted, I probably could have submitted this
> months ago. Oh well. Live and learn. What would you have me do at this
> point?

Split up your patches differently, and not in so many steps. Ideally
each step should work and compile, with each introducing some sort of
functionality. Each patch should be reviewable on its own.

-- 
Jens Axboe

Re: [Suspend2][ 0/9] Extents support.

[Greg KH]

On Tue, Jun 27, 2006 at 03:39:26PM +1000, Nigel Cunningham wrote:
> Hi.
> 
> On Tuesday 27 June 2006 15:36, Jens Axboe wrote:
> > On Tue, Jun 27 2006, Nigel Cunningham wrote:
> > > Hi.
> > >
> > > On Tuesday 27 June 2006 07:20, Rafael J. Wysocki wrote:
> > > > On Monday 26 June 2006 18:54, Nigel Cunningham wrote:
> > > > > Add Suspend2 extent support. Extents are used for storing the lists
> > > > > of blocks to which the image will be written, and are stored in the
> > > > > image header for use at resume time.
> > > >
> > > > Could you please put all of the changes in kernel/power/extents.c into
> > > > one patch? ?It's quite difficult to review them now, at least for me.
> > >
> > > I spent a long time splitting them up because I was asked in previous
> > > iterations to break them into manageable chunks. How about if I were to
> > > email you the individual files off line, so as to not send the same
> > > amount again?
> >
> > Managable chunks means logical changes go together, one function per
> > diff is really extreme and unreviewable. Support for extents is one
> > logical change, so it's one patch. Unless of course you have to do some
> > preparatory patches, then you'd do those separately.
> >
> > I must admit I thought you were kidding when I read through this extents
> > patch series, having a single patch just adding includes!
> 
> Sorry for fluffing it up. I'm pretty inexperienced, but I'm trying to follow 
> CodingStyle and all the other advice. If I'd known I'd misunderstood what was 
> wanted, I probably could have submitted this months ago. Oh well. Live and 
> learn. What would you have me do at this point?

Please break things up into logical steps to solve the problem, and try
it again.

Oh, and as a meta-comment, why /proc?  You know that's not acceptable,
right?

thanks,

greg k-h

Re: [Suspend2][ 0/9] Extents support.

[Nigel Cunningham]

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

Hi.

On Tuesday 27 June 2006 17:06, Greg KH wrote:
> Oh, and as a meta-comment, why /proc?  You know that's not acceptable,
> right?

Partly because when I did consider switching to /sys, I found it to be 
incomprehensible (even with the LWN articles and Documentation/ files). 
Jonathan's articles and LCA presentation did help me start to get a better 
grip, but then it just didn't seem to be worth the effort. I have two simple 
relatively simple routines that handle all my proc entries at the moment, so 
that adding a new entry is just a matter of adding an element in an array of 
structs (saying what variable is being read/written, what type, min/max 
values and side effect routines, eg). It looked to me like changing to sysfs 
was going to require me to have a separate routine for every sysfs entry, 
even though they'd all have those some basic features. Maybe I'm just 
ignorant. Please tell me I am and point me in the right direction.

Regards,

Nigel
-- 
See http://www.suspend2.net for Howtos, FAQs, mailing
lists, wiki and bugzilla info.

[-- Attachment #2: Type: application/pgp-signature, Size: 189 bytes --]

Re: [Suspend2][ 0/9] Extents support.

[Nigel Cunningham]

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

Hi.

On Tuesday 27 June 2006 17:05, Jens Axboe wrote:
> On Tue, Jun 27 2006, Nigel Cunningham wrote:
> > On Tuesday 27 June 2006 15:36, Jens Axboe wrote:
> > > On Tue, Jun 27 2006, Nigel Cunningham wrote:
> > > > On Tuesday 27 June 2006 07:20, Rafael J. Wysocki wrote:
> > > > > On Monday 26 June 2006 18:54, Nigel Cunningham wrote:
> > > > > > Add Suspend2 extent support. Extents are used for storing the
> > > > > > lists of blocks to which the image will be written, and are
> > > > > > stored in the image header for use at resume time.
> > > > >
> > > > > Could you please put all of the changes in kernel/power/extents.c
> > > > > into one patch?  It's quite difficult to review them now, at least
> > > > > for me.
> > > >
> > > > I spent a long time splitting them up because I was asked in previous
> > > > iterations to break them into manageable chunks. How about if I were
> > > > to email you the individual files off line, so as to not send the
> > > > same amount again?
> > >
> > > Managable chunks means logical changes go together, one function per
> > > diff is really extreme and unreviewable. Support for extents is one
> > > logical change, so it's one patch. Unless of course you have to do some
> > > preparatory patches, then you'd do those separately.
> > >
> > > I must admit I thought you were kidding when I read through this
> > > extents patch series, having a single patch just adding includes!
> >
> > Sorry for fluffing it up. I'm pretty inexperienced, but I'm trying to
> > follow CodingStyle and all the other advice. If I'd known I'd
> > misunderstood what was wanted, I probably could have submitted this
> > months ago. Oh well. Live and learn. What would you have me do at this
> > point?
>
> Split up your patches differently, and not in so many steps. Ideally
> each step should work and compile, with each introducing some sort of
> functionality. Each patch should be reviewable on its own.

The difficulty I have there is that suspending to disk doesn't seem to me to 
be something where you can add a bit at a time like that. I do have proc 
entries that allow you to say "Just freeze the processes and prepare the 
metadata, then free it and exit" (freezer_test) and "Do everything but 
actually writing the image and doing the atomic copy, then exit 
(test_filter_speed), for diagnosing problems and tuning the configuration, 
but if I start were to start again with nothing, I'd only write the dynamic 
pageflags code to start with and submit it (giving you lib/dyn_pageflags.c 
and kernel/power/pageflags.c), then the refrigerator changes and the extent 
code and so on. I guess what I'm trying to say is that I'm not mutating 
swsusp into suspend2 here, and I don't think I can. Suspend2 is a 
reimplementation of swsusp, not a series of incremental modifications. It 
uses completely different methods for writing the image, storing the metadata 
and so on. Until recently, the only thing it shared with swsusp was the 
refrigerator and driver model calls, and even now the sharing of lowlevel 
code is only a tiny fraction of all that is done.

Could I ask what might be a dumb question in this regard - why isn't Reiser4 
going through the same process? Is it an indication that I shouldn't have 
submitted these patches and should have just asked Andrew to take Suspend2 
into mm, or is there something different between Reiser4 and Suspend2 that 
I'm missing?

Regards,

Nigel

-- 
See http://www.suspend2.net for Howtos, FAQs, mailing
lists, wiki and bugzilla info.

[-- Attachment #2: Type: application/pgp-signature, Size: 189 bytes --]

Re: [Suspend2][ 0/9] Extents support.

[Greg KH]

On Tue, Jun 27, 2006 at 05:27:30PM +1000, Nigel Cunningham wrote:
> Hi.
> 
> On Tuesday 27 June 2006 17:06, Greg KH wrote:
> > Oh, and as a meta-comment, why /proc?  You know that's not acceptable,
> > right?
> 
> Partly because when I did consider switching to /sys, I found it to be 
> incomprehensible (even with the LWN articles and Documentation/ files). 
> Jonathan's articles and LCA presentation did help me start to get a better 
> grip, but then it just didn't seem to be worth the effort. I have two simple 
> relatively simple routines that handle all my proc entries at the moment, so 
> that adding a new entry is just a matter of adding an element in an array of 
> structs (saying what variable is being read/written, what type, min/max 
> values and side effect routines, eg). It looked to me like changing to sysfs 
> was going to require me to have a separate routine for every sysfs entry, 
> even though they'd all have those some basic features. Maybe I'm just 
> ignorant. Please tell me I am and point me in the right direction.

Well, as your stuff does not have anything to do with "processes",
putting it in /proc is not acceptable.

sysfs is one value per file, and if that matches up to what you need,
then it should be fine to use.

You do need to have some kind of function for every sysfs entry, but you
can group common ones together (as the hwmon drivers do.)

As you will not have a backing "device" to attach your files to, you
will probably need to deal with "raw" kobjects, and the learning curve
for how to create files in sysfs with them is unfortunatly a bit steep.
But there is lots of working examples in the kernel that do this (block
devices, md, driver core, etc.), there's plenty of code to copy from to
get it to work.

And if that doesn't look like fun, you can always just use create a new
filesystem (only 200 lines of code), or use debugfs.

good luck,

greg k-h

Re: [Suspend2][ 0/9] Extents support.

[Jens Axboe]

On Tue, Jun 27 2006, Nigel Cunningham wrote:
> Hi.
> 
> On Tuesday 27 June 2006 17:05, Jens Axboe wrote:
> > On Tue, Jun 27 2006, Nigel Cunningham wrote:
> > > On Tuesday 27 June 2006 15:36, Jens Axboe wrote:
> > > > On Tue, Jun 27 2006, Nigel Cunningham wrote:
> > > > > On Tuesday 27 June 2006 07:20, Rafael J. Wysocki wrote:
> > > > > > On Monday 26 June 2006 18:54, Nigel Cunningham wrote:
> > > > > > > Add Suspend2 extent support. Extents are used for storing the
> > > > > > > lists of blocks to which the image will be written, and are
> > > > > > > stored in the image header for use at resume time.
> > > > > >
> > > > > > Could you please put all of the changes in kernel/power/extents.c
> > > > > > into one patch?  It's quite difficult to review them now, at least
> > > > > > for me.
> > > > >
> > > > > I spent a long time splitting them up because I was asked in previous
> > > > > iterations to break them into manageable chunks. How about if I were
> > > > > to email you the individual files off line, so as to not send the
> > > > > same amount again?
> > > >
> > > > Managable chunks means logical changes go together, one function per
> > > > diff is really extreme and unreviewable. Support for extents is one
> > > > logical change, so it's one patch. Unless of course you have to do some
> > > > preparatory patches, then you'd do those separately.
> > > >
> > > > I must admit I thought you were kidding when I read through this
> > > > extents patch series, having a single patch just adding includes!
> > >
> > > Sorry for fluffing it up. I'm pretty inexperienced, but I'm trying to
> > > follow CodingStyle and all the other advice. If I'd known I'd
> > > misunderstood what was wanted, I probably could have submitted this
> > > months ago. Oh well. Live and learn. What would you have me do at this
> > > point?
> >
> > Split up your patches differently, and not in so many steps. Ideally
> > each step should work and compile, with each introducing some sort of
> > functionality. Each patch should be reviewable on its own.
> 
> The difficulty I have there is that suspending to disk doesn't seem to
> me to be something where you can add a bit at a time like that. I do
> have proc entries that allow you to say "Just freeze the processes and
> prepare the metadata, then free it and exit" (freezer_test) and "Do
> everything but actually writing the image and doing the atomic copy,
> then exit (test_filter_speed), for diagnosing problems and tuning the
> configuration, but if I start were to start again with nothing, I'd
> only write the dynamic pageflags code to start with and submit it
> (giving you lib/dyn_pageflags.c and kernel/power/pageflags.c), then
> the refrigerator changes and the extent code and so on. I guess what
> I'm trying to say is that I'm not mutating swsusp into suspend2 here,
> and I don't think I can. Suspend2 is a reimplementation of swsusp, not
> a series of incremental modifications. It uses completely different
> methods for writing the image, storing the metadata and so on. Until
> recently, the only thing it shared with swsusp was the refrigerator
> and driver model calls, and even now the sharing of lowlevel code is
> only a tiny fraction of all that is done.

You can't split up what isn't composed of multiple things, of course. I
didn't review your patches (sorry), but if you have changes outside of
suspend2 itself, then you need to split these out. You could submit
those patches seperately.

Now I haven't followed the suspend2 vs swsusp debate very closely, but
it seems to me that your biggest problem with getting this merged is
getting consensus on where exactly this is going. Nobody wants two
different suspend modules in the kernel. So there are two options -
suspend2 is deemed the way to go, and it gets merged and replaces
swsusp. Or the other way around - people like swsusp more, and you are
doomed to maintain suspend2 outside the tree.

> Could I ask what might be a dumb question in this regard - why isn't
> Reiser4 going through the same process? Is it an indication that I
> shouldn't have submitted these patches and should have just asked
> Andrew to take Suspend2 into mm, or is there something different
> between Reiser4 and Suspend2 that I'm missing?

That's not a dumb question at all. reiser4 hasn't been merged for years,
so you probably don't want to look at that as an example :-)

reiser4 is pretty much a separate entity, so it doesn't make sense so
split that up much for submission. Core kernel changes (as always) need
to be split, of course.

Sorry I cannot be of more help.

-- 
Jens Axboe

Re: [Suspend2][ 0/9] Extents support.

[Greg KH]

On Tue, Jun 27, 2006 at 09:59:06AM +0200, Jens Axboe wrote:
> Now I haven't followed the suspend2 vs swsusp debate very closely, but
> it seems to me that your biggest problem with getting this merged is
> getting consensus on where exactly this is going. Nobody wants two
> different suspend modules in the kernel. So there are two options -
> suspend2 is deemed the way to go, and it gets merged and replaces
> swsusp. Or the other way around - people like swsusp more, and you are
> doomed to maintain suspend2 outside the tree.

Actually, there's a third option that is looking like the way forward,
doing all of this from userspace and having no suspend-to-disk in the
kernel tree at all.

Pavel and others have a working implementation and are slowly moving
toward adding all of the "bright and shiny" features that is in suspend2
to it (encryption, progress screens, abort by pressing a key, etc.) so
that there is no loss of functionality.

So I don't really see the future of suspend2 because of this...

thanks,

greg k-h

Re: [Suspend2][ 0/9] Extents support.

[Jens Axboe]

On Tue, Jun 27 2006, Greg KH wrote:
> On Tue, Jun 27, 2006 at 09:59:06AM +0200, Jens Axboe wrote:
> > Now I haven't followed the suspend2 vs swsusp debate very closely, but
> > it seems to me that your biggest problem with getting this merged is
> > getting consensus on where exactly this is going. Nobody wants two
> > different suspend modules in the kernel. So there are two options -
> > suspend2 is deemed the way to go, and it gets merged and replaces
> > swsusp. Or the other way around - people like swsusp more, and you are
> > doomed to maintain suspend2 outside the tree.
> 
> Actually, there's a third option that is looking like the way forward,
> doing all of this from userspace and having no suspend-to-disk in the
> kernel tree at all.

Yeah, but isn't that already in progress and swsusp being migrated that
way? So really option #2.

> Pavel and others have a working implementation and are slowly moving
> toward adding all of the "bright and shiny" features that is in suspend2
> to it (encryption, progress screens, abort by pressing a key, etc.) so
> that there is no loss of functionality.
> 
> So I don't really see the future of suspend2 because of this...

Well, it sure looks slim..

-- 
Jens Axboe

Re: [Suspend2][ 0/9] Extents support.

[Nigel Cunningham]

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

Hi.

On Tuesday 27 June 2006 18:12, Greg KH wrote:
> On Tue, Jun 27, 2006 at 09:59:06AM +0200, Jens Axboe wrote:
> > Now I haven't followed the suspend2 vs swsusp debate very closely, but
> > it seems to me that your biggest problem with getting this merged is
> > getting consensus on where exactly this is going. Nobody wants two
> > different suspend modules in the kernel. So there are two options -
> > suspend2 is deemed the way to go, and it gets merged and replaces
> > swsusp. Or the other way around - people like swsusp more, and you are
> > doomed to maintain suspend2 outside the tree.
>
> Actually, there's a third option that is looking like the way forward,
> doing all of this from userspace and having no suspend-to-disk in the
> kernel tree at all.
>
> Pavel and others have a working implementation and are slowly moving
> toward adding all of the "bright and shiny" features that is in suspend2
> to it (encryption, progress screens, abort by pressing a key, etc.) so
> that there is no loss of functionality.
>
> So I don't really see the future of suspend2 because of this...

But what Rafael and Pavel are doing is really only moving the highest level of 
controlling logic to userspace (ok, and maybe compression and encryption 
too). Everything important (freezing other processes, atomic copy and the 
guts of the I/O) is still done by the kernel.

And there _is_ loss of functionality - uswsusp still doesn't support writing a 
full image of memory, writing to multiple swap devices (partitions or files), 
or writing to ordinary files. They're getting the low hanging fruit, but when 
it comes to these parts of the problem, they're going to require either smoke 
and very good mirrors (eg the swap prefetching trick), or simply refuse to 
implement them.

If we take the problem one step further, and begin to think about 
checkpointing, they're in even bigger trouble. I'll freely admit that I'd 
have to redesign the way I store data so that random parts of the image could 
be replaced, have hooks in mm to be able to learn what pages need have 
changed and would also need filesystem support to handle that part of the 
problem, but I'd at least be working in the right domain.

I don't want to demean Rafael and Pavels' work for a moment. I've benefited 
from Pavel's help of Gabor in the beginning, and a little bit since he forked 
and merged in 2.5. But it seems to me that uswsusp is a short trip down a 
dead end road. It just doesn't have a future beyond being an interesting hack 
that proves you can safely run a program in userspace while snapshotting. 
Suspending to disk belongs in the kernel. That is shown clearly by the fact 
that uswsusp continues to use kernel code to do the really critical tasks, 
rather than being some super privileged userspace program that does them 
itself from userspace.

Regards,

Nigel
-- 
See http://www.suspend2.net for Howtos, FAQs, mailing
lists, wiki and bugzilla info.

[-- Attachment #2: Type: application/pgp-signature, Size: 189 bytes --]

Re: [Suspend2][ 0/9] Extents support.

[Nigel Cunningham]

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

Hi.

On Tuesday 27 June 2006 17:59, Jens Axboe wrote:
> On Tue, Jun 27 2006, Nigel Cunningham wrote:
> > On Tuesday 27 June 2006 17:05, Jens Axboe wrote:
> > > On Tue, Jun 27 2006, Nigel Cunningham wrote:
> > > > On Tuesday 27 June 2006 15:36, Jens Axboe wrote:
> > > > > On Tue, Jun 27 2006, Nigel Cunningham wrote:
> > > > > > On Tuesday 27 June 2006 07:20, Rafael J. Wysocki wrote:
> > > > > > > On Monday 26 June 2006 18:54, Nigel Cunningham wrote:
> > > > > > > > Add Suspend2 extent support. Extents are used for storing the
> > > > > > > > lists of blocks to which the image will be written, and are
> > > > > > > > stored in the image header for use at resume time.
> > > > > > >
> > > > > > > Could you please put all of the changes in
> > > > > > > kernel/power/extents.c into one patch?  It's quite difficult to
> > > > > > > review them now, at least for me.
> > > > > >
> > > > > > I spent a long time splitting them up because I was asked in
> > > > > > previous iterations to break them into manageable chunks. How
> > > > > > about if I were to email you the individual files off line, so as
> > > > > > to not send the same amount again?
> > > > >
> > > > > Managable chunks means logical changes go together, one function
> > > > > per diff is really extreme and unreviewable. Support for extents is
> > > > > one logical change, so it's one patch. Unless of course you have to
> > > > > do some preparatory patches, then you'd do those separately.
> > > > >
> > > > > I must admit I thought you were kidding when I read through this
> > > > > extents patch series, having a single patch just adding includes!
> > > >
> > > > Sorry for fluffing it up. I'm pretty inexperienced, but I'm trying to
> > > > follow CodingStyle and all the other advice. If I'd known I'd
> > > > misunderstood what was wanted, I probably could have submitted this
> > > > months ago. Oh well. Live and learn. What would you have me do at
> > > > this point?
> > >
> > > Split up your patches differently, and not in so many steps. Ideally
> > > each step should work and compile, with each introducing some sort of
> > > functionality. Each patch should be reviewable on its own.
> >
> > The difficulty I have there is that suspending to disk doesn't seem to
> > me to be something where you can add a bit at a time like that. I do
> > have proc entries that allow you to say "Just freeze the processes and
> > prepare the metadata, then free it and exit" (freezer_test) and "Do
> > everything but actually writing the image and doing the atomic copy,
> > then exit (test_filter_speed), for diagnosing problems and tuning the
> > configuration, but if I start were to start again with nothing, I'd
> > only write the dynamic pageflags code to start with and submit it
> > (giving you lib/dyn_pageflags.c and kernel/power/pageflags.c), then
> > the refrigerator changes and the extent code and so on. I guess what
> > I'm trying to say is that I'm not mutating swsusp into suspend2 here,
> > and I don't think I can. Suspend2 is a reimplementation of swsusp, not
> > a series of incremental modifications. It uses completely different
> > methods for writing the image, storing the metadata and so on. Until
> > recently, the only thing it shared with swsusp was the refrigerator
> > and driver model calls, and even now the sharing of lowlevel code is
> > only a tiny fraction of all that is done.
>
> You can't split up what isn't composed of multiple things, of course. I
> didn't review your patches (sorry), but if you have changes outside of
> suspend2 itself, then you need to split these out. You could submit
> those patches seperately.

Ok. That I can certainly do, and I can post one patch per file in a logical 
order.

> Now I haven't followed the suspend2 vs swsusp debate very closely, but
> it seems to me that your biggest problem with getting this merged is
> getting consensus on where exactly this is going. Nobody wants two
> different suspend modules in the kernel. So there are two options -
> suspend2 is deemed the way to go, and it gets merged and replaces
> swsusp. Or the other way around - people like swsusp more, and you are
> doomed to maintain suspend2 outside the tree.

Generally, I agree, although my understanding of Rafael and Pavel's mindset is 
that swsusp is a dead dog and uswsusp is the way they want to see things go. 
swsusp is only staying for backwards compatability. If that's the case, 
perhaps we can just replace swsusp with Suspend2 and let them have their 
existing interface for uswsusp. Still not ideal, I agree, but it would be 
progress.

> > Could I ask what might be a dumb question in this regard - why isn't
> > Reiser4 going through the same process? Is it an indication that I
> > shouldn't have submitted these patches and should have just asked
> > Andrew to take Suspend2 into mm, or is there something different
> > between Reiser4 and Suspend2 that I'm missing?
>
> That's not a dumb question at all. reiser4 hasn't been merged for years,
> so you probably don't want to look at that as an example :-)
>
> reiser4 is pretty much a separate entity, so it doesn't make sense so
> split that up much for submission. Core kernel changes (as always) need
> to be split, of course.

Ok. I didn't remember when reiser4 first appeared or how it got into -mm. I 
guess I'm picturing suspend2 as similar because I'm thinking of Suspend2 as 
pretty much a separate subsystem - it's interface to the rest of the kernel 
can be defined in terms of freeze|thaw_processes, drivers_suspend|resume, 
bmap|submit_bio and I'm imagining the Reiser4 would have a similar sort of 
limited set of functions it uses for interfacing with the rest of the kernel.

Regards,

Nigel

-- 
See http://www.suspend2.net for Howtos, FAQs, mailing
lists, wiki and bugzilla info.

[-- Attachment #2: Type: application/pgp-signature, Size: 189 bytes --]

Re: [Suspend2][ 0/9] Extents support.

[Nigel Cunningham]

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

Hi.

On Tuesday 27 June 2006 17:53, Greg KH wrote:
> Well, as your stuff does not have anything to do with "processes",
> putting it in /proc is not acceptable.
>
> sysfs is one value per file, and if that matches up to what you need,
> then it should be fine to use.

It does.

> You do need to have some kind of function for every sysfs entry, but you
> can group common ones together (as the hwmon drivers do.)

Ok. I'll take a look.

> As you will not have a backing "device" to attach your files to, you
> will probably need to deal with "raw" kobjects, and the learning curve
> for how to create files in sysfs with them is unfortunatly a bit steep.
> But there is lots of working examples in the kernel that do this (block
> devices, md, driver core, etc.), there's plenty of code to copy from to
> get it to work.
>
> And if that doesn't look like fun, you can always just use create a new
> filesystem (only 200 lines of code), or use debugfs.
>
> good luck,

Ok. I'll give it a go. Thanks for the pointers, Greg.

Regards,

Nigel
-- 
See http://www.suspend2.net for Howtos, FAQs, mailing
lists, wiki and bugzilla info.

[-- Attachment #2: Type: application/pgp-signature, Size: 189 bytes --]

Re: [Suspend2][ 0/9] Extents support.

[Rafael J. Wysocki]

Hi,

On Tuesday 27 June 2006 11:07, Nigel Cunningham wrote:
> On Tuesday 27 June 2006 17:59, Jens Axboe wrote:
> > On Tue, Jun 27 2006, Nigel Cunningham wrote:
> > > On Tuesday 27 June 2006 17:05, Jens Axboe wrote:
> > > > On Tue, Jun 27 2006, Nigel Cunningham wrote:
> > > > > On Tuesday 27 June 2006 15:36, Jens Axboe wrote:
> > > > > > On Tue, Jun 27 2006, Nigel Cunningham wrote:
> > > > > > > On Tuesday 27 June 2006 07:20, Rafael J. Wysocki wrote:
> > > > > > > > On Monday 26 June 2006 18:54, Nigel Cunningham wrote:
> > > > > > > > > Add Suspend2 extent support. Extents are used for storing the
> > > > > > > > > lists of blocks to which the image will be written, and are
> > > > > > > > > stored in the image header for use at resume time.
> > > > > > > >
> > > > > > > > Could you please put all of the changes in
> > > > > > > > kernel/power/extents.c into one patch?  It's quite difficult to
> > > > > > > > review them now, at least for me.
> > > > > > >
> > > > > > > I spent a long time splitting them up because I was asked in
> > > > > > > previous iterations to break them into manageable chunks. How
> > > > > > > about if I were to email you the individual files off line, so as
> > > > > > > to not send the same amount again?
> > > > > >
> > > > > > Managable chunks means logical changes go together, one function
> > > > > > per diff is really extreme and unreviewable. Support for extents is
> > > > > > one logical change, so it's one patch. Unless of course you have to
> > > > > > do some preparatory patches, then you'd do those separately.
> > > > > >
> > > > > > I must admit I thought you were kidding when I read through this
> > > > > > extents patch series, having a single patch just adding includes!
> > > > >
> > > > > Sorry for fluffing it up. I'm pretty inexperienced, but I'm trying to
> > > > > follow CodingStyle and all the other advice. If I'd known I'd
> > > > > misunderstood what was wanted, I probably could have submitted this
> > > > > months ago. Oh well. Live and learn. What would you have me do at
> > > > > this point?
> > > >
> > > > Split up your patches differently, and not in so many steps. Ideally
> > > > each step should work and compile, with each introducing some sort of
> > > > functionality. Each patch should be reviewable on its own.
> > >
> > > The difficulty I have there is that suspending to disk doesn't seem to
> > > me to be something where you can add a bit at a time like that. I do
> > > have proc entries that allow you to say "Just freeze the processes and
> > > prepare the metadata, then free it and exit" (freezer_test) and "Do
> > > everything but actually writing the image and doing the atomic copy,
> > > then exit (test_filter_speed), for diagnosing problems and tuning the
> > > configuration, but if I start were to start again with nothing, I'd
> > > only write the dynamic pageflags code to start with and submit it
> > > (giving you lib/dyn_pageflags.c and kernel/power/pageflags.c), then
> > > the refrigerator changes and the extent code and so on. I guess what
> > > I'm trying to say is that I'm not mutating swsusp into suspend2 here,
> > > and I don't think I can. Suspend2 is a reimplementation of swsusp, not
> > > a series of incremental modifications. It uses completely different
> > > methods for writing the image, storing the metadata and so on. Until
> > > recently, the only thing it shared with swsusp was the refrigerator
> > > and driver model calls, and even now the sharing of lowlevel code is
> > > only a tiny fraction of all that is done.
> >
> > You can't split up what isn't composed of multiple things, of course. I
> > didn't review your patches (sorry), but if you have changes outside of
> > suspend2 itself, then you need to split these out. You could submit
> > those patches seperately.
> 
> Ok. That I can certainly do, and I can post one patch per file in a logical 
> order.
> 
> > Now I haven't followed the suspend2 vs swsusp debate very closely, but
> > it seems to me that your biggest problem with getting this merged is
> > getting consensus on where exactly this is going. Nobody wants two
> > different suspend modules in the kernel. So there are two options -
> > suspend2 is deemed the way to go, and it gets merged and replaces
> > swsusp. Or the other way around - people like swsusp more, and you are
> > doomed to maintain suspend2 outside the tree.
> 
> Generally, I agree, although my understanding of Rafael and Pavel's mindset is 
> that swsusp is a dead dog and uswsusp is the way they want to see things go. 
> swsusp is only staying for backwards compatability. If that's the case, 
> perhaps we can just replace swsusp with Suspend2 and let them have their 
> existing interface for uswsusp. Still not ideal, I agree, but it would be 
> progress.

Well, ususpend needs some core functionality to be provided by the kernel,
like freezing/thawing processes (this is also used by the STR), snapshotting
the system memory.  These should be shared with the in-kernel suspend,
be it swsusp or suspend2.
 
> > > Could I ask what might be a dumb question in this regard - why isn't
> > > Reiser4 going through the same process? Is it an indication that I
> > > shouldn't have submitted these patches and should have just asked
> > > Andrew to take Suspend2 into mm, or is there something different
> > > between Reiser4 and Suspend2 that I'm missing?
> >
> > That's not a dumb question at all. reiser4 hasn't been merged for years,
> > so you probably don't want to look at that as an example :-)
> >
> > reiser4 is pretty much a separate entity, so it doesn't make sense so
> > split that up much for submission. Core kernel changes (as always) need
> > to be split, of course.
> 
> Ok. I didn't remember when reiser4 first appeared or how it got into -mm. I 
> guess I'm picturing suspend2 as similar because I'm thinking of Suspend2 as 
> pretty much a separate subsystem - it's interface to the rest of the kernel 
> can be defined in terms of freeze|thaw_processes, drivers_suspend|resume, 
> bmap|submit_bio and I'm imagining the Reiser4 would have a similar sort of 
> limited set of functions it uses for interfacing with the rest of the kernel.

IMHO it's close enough to this, but reiser4 is considered as just another
filesystem and there are many of thses in the kernel already.  Still there
are some controversies around reiser4 as well.

Greetings,
Rafael

Re: [Suspend2][ 0/9] Extents support.

[Nigel Cunningham]

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

Hi.

On Tuesday 27 June 2006 19:26, Rafael J. Wysocki wrote:
> > > Now I haven't followed the suspend2 vs swsusp debate very closely, but
> > > it seems to me that your biggest problem with getting this merged is
> > > getting consensus on where exactly this is going. Nobody wants two
> > > different suspend modules in the kernel. So there are two options -
> > > suspend2 is deemed the way to go, and it gets merged and replaces
> > > swsusp. Or the other way around - people like swsusp more, and you are
> > > doomed to maintain suspend2 outside the tree.
> >
> > Generally, I agree, although my understanding of Rafael and Pavel's
> > mindset is that swsusp is a dead dog and uswsusp is the way they want to
> > see things go. swsusp is only staying for backwards compatability. If
> > that's the case, perhaps we can just replace swsusp with Suspend2 and let
> > them have their existing interface for uswsusp. Still not ideal, I agree,
> > but it would be progress.
>
> Well, ususpend needs some core functionality to be provided by the kernel,
> like freezing/thawing processes (this is also used by the STR),
> snapshotting the system memory.  These should be shared with the in-kernel
> suspend, be it swsusp or suspend2.

If I modify suspend2 so that from now on it replaces swsusp, using noresume, 
resume= and echo disk > /sys/power/state in a way that's backward compatible 
with swsusp and doesn't interfere with uswsusp support, would you be happy? 
IIRC, Pavel has said in the past he wishes I'd just do that, but he's not you 
of course.

Regards,

Nigel
-- 
See http://www.suspend2.net for Howtos, FAQs, mailing
lists, wiki and bugzilla info.

[-- Attachment #2: Type: application/pgp-signature, Size: 189 bytes --]

Re: [Suspend2][ 0/9] Extents support.

[Rafael J. Wysocki]

Hi,

On Tuesday 27 June 2006 11:35, Nigel Cunningham wrote:
> On Tuesday 27 June 2006 19:26, Rafael J. Wysocki wrote:
> > > > Now I haven't followed the suspend2 vs swsusp debate very closely, but
> > > > it seems to me that your biggest problem with getting this merged is
> > > > getting consensus on where exactly this is going. Nobody wants two
> > > > different suspend modules in the kernel. So there are two options -
> > > > suspend2 is deemed the way to go, and it gets merged and replaces
> > > > swsusp. Or the other way around - people like swsusp more, and you are
> > > > doomed to maintain suspend2 outside the tree.
> > >
> > > Generally, I agree, although my understanding of Rafael and Pavel's
> > > mindset is that swsusp is a dead dog and uswsusp is the way they want to
> > > see things go. swsusp is only staying for backwards compatability. If
> > > that's the case, perhaps we can just replace swsusp with Suspend2 and let
> > > them have their existing interface for uswsusp. Still not ideal, I agree,
> > > but it would be progress.
> >
> > Well, ususpend needs some core functionality to be provided by the kernel,
> > like freezing/thawing processes (this is also used by the STR),
> > snapshotting the system memory.  These should be shared with the in-kernel
> > suspend, be it swsusp or suspend2.
> 
> If I modify suspend2 so that from now on it replaces swsusp, using noresume, 
> resume= and echo disk > /sys/power/state in a way that's backward compatible 
> with swsusp and doesn't interfere with uswsusp support, would you be happy? 
> IIRC, Pavel has said in the past he wishes I'd just do that, but he's not you 
> of course.

That depends on how it's done.  For sure, I wouldn't like it to be done in the
"everything at once" manner.

Greetings,
Rafael

Re: [Suspend2][ 0/9] Extents support.

[Nigel Cunningham]

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

Hi.

On Wednesday 28 June 2006 08:19, Rafael J. Wysocki wrote:
> Hi,
>
> On Tuesday 27 June 2006 11:35, Nigel Cunningham wrote:
> > On Tuesday 27 June 2006 19:26, Rafael J. Wysocki wrote:
> > > > > Now I haven't followed the suspend2 vs swsusp debate very closely,
> > > > > but it seems to me that your biggest problem with getting this
> > > > > merged is getting consensus on where exactly this is going. Nobody
> > > > > wants two different suspend modules in the kernel. So there are two
> > > > > options - suspend2 is deemed the way to go, and it gets merged and
> > > > > replaces swsusp. Or the other way around - people like swsusp more,
> > > > > and you are doomed to maintain suspend2 outside the tree.
> > > >
> > > > Generally, I agree, although my understanding of Rafael and Pavel's
> > > > mindset is that swsusp is a dead dog and uswsusp is the way they want
> > > > to see things go. swsusp is only staying for backwards compatability.
> > > > If that's the case, perhaps we can just replace swsusp with Suspend2
> > > > and let them have their existing interface for uswsusp. Still not
> > > > ideal, I agree, but it would be progress.
> > >
> > > Well, ususpend needs some core functionality to be provided by the
> > > kernel, like freezing/thawing processes (this is also used by the STR),
> > > snapshotting the system memory.  These should be shared with the
> > > in-kernel suspend, be it swsusp or suspend2.
> >
> > If I modify suspend2 so that from now on it replaces swsusp, using
> > noresume, resume= and echo disk > /sys/power/state in a way that's
> > backward compatible with swsusp and doesn't interfere with uswsusp
> > support, would you be happy? IIRC, Pavel has said in the past he wishes
> > I'd just do that, but he's not you of course.
>
> That depends on how it's done.  For sure, I wouldn't like it to be done in
> the "everything at once" manner.

I'm not sure I get what you're saying. Do you mean you'd prefer them to 
coexist for a time in mainline? If so, I'd point out that suspend2 uses 
different parameters at the moment precisely so they can coexist, so that 
wouldn't be any change.

Regards,

Nigel
-- 
See http://www.suspend2.net for Howtos, FAQs, mailing
lists, wiki and bugzilla info.

[-- Attachment #2: Type: application/pgp-signature, Size: 189 bytes --]

Re: [Suspend2][ 0/9] Extents support.

[Rahul Karnik]

On 6/27/06, Nigel Cunningham <nigel@suspend2.net> wrote:
> Suspend2 is a
> reimplementation of swsusp, not a series of incremental modifications. It
> uses completely different methods for writing the image, storing the metadata
> and so on. Until recently, the only thing it shared with swsusp was the
> refrigerator and driver model calls, and even now the sharing of lowlevel
> code is only a tiny fraction of all that is done.

This is something I don't understand. Why can you not submit patches
that simply do things like "change method for writing image" and
reduce the difference between suspend2 and mainline? It may be more
work, but I think you will find that incremental changes are a lot
easier for people to review and merge.

Right now, it seems like Linus and Andrew have only two choices:

1. Ignore your submission
2. Merge all of suspend2

We have had complete reworks of entire subsystems in the 2.6 series
without problems, simply because the effort was taken to make the
changes incrementally.

> Could I ask what might be a dumb question in this regard - why isn't Reiser4
> going through the same process?

reiser4 has been in -mm for quite a while without being merged into
mainline. I don't think you want the same fate for suspend2.

Finally, I just want to say I appreciate all the effort you (and Pavel
and Rafael) are putting into suspend-to-disk. It is critical
functionality for me and I think we can get both a well functioning
and maintainable implementation.

Thanks,
Rahul
-- 
Rahul Karnik
rahul@genebrew.com

Re: [Suspend2][ 0/9] Extents support.

[Nigel Cunningham]

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

Hi.

On Wednesday 28 June 2006 21:28, Rahul Karnik wrote:
> On 6/27/06, Nigel Cunningham <nigel@suspend2.net> wrote:
> > Suspend2 is a
> > reimplementation of swsusp, not a series of incremental modifications. It
> > uses completely different methods for writing the image, storing the
> > metadata and so on. Until recently, the only thing it shared with swsusp
> > was the refrigerator and driver model calls, and even now the sharing of
> > lowlevel code is only a tiny fraction of all that is done.
>
> This is something I don't understand. Why can you not submit patches
> that simply do things like "change method for writing image" and
> reduce the difference between suspend2 and mainline? It may be more
> work, but I think you will find that incremental changes are a lot
> easier for people to review and merge.

It's because it's all so interconnected. Adding the modular infrastructure is 
useless without something to use the modules. Changing to use the pageflags 
functionality requires modifications in both the preparation of the image and 
in the I/O. There are bits that could be done incrementally, but they're 
minor. I did start with the same codebase that Pavel forked, but then did 
substantial rewrites in going from the betas to 1.0 and to 2.0.

Thanks for the email.

Nigel
-- 
See http://www.suspend2.net for Howtos, FAQs, mailing
lists, wiki and bugzilla info.

[-- Attachment #2: Type: application/pgp-signature, Size: 189 bytes --]

Re: [Suspend2][ 0/9] Extents support.

[Olivier Galibert]

On Wed, Jun 28, 2006 at 07:28:38AM -0400, Rahul Karnik wrote:
> This is something I don't understand. Why can you not submit patches
> that simply do things like "change method for writing image" and

Because the systematic answer of the suspend maintainer to that is
"you should write the image in userspace", to follow your example.


> reduce the difference between suspend2 and mainline? It may be more
> work, but I think you will find that incremental changes are a lot
> easier for people to review and merge.

Pavel wants everything in userspace.  Nigel wants everything but the
chrome in kernelspace.  Other kernels maintainers are split on the
issue, or don't care apart from "it should have been working for
years, damnit".  Andrew plainly said he doesn't know what's best, and
Linus seems to be very careful not to talk about it.

What should have happened is that suspend2 should have been merged
years ago, and then now transiting more of it to userspace or not
could be experimented, starting on a decently working base.  Pavel's
NIH-ing prevented that though.

In any case, don't use software suspend on a machine with data you
care about, and especially not uswsusp.  Side effect of userspace code
is that nobody from outside the project reviews it, and it's way too
young to have any kind of confidence in it.

  OG.

Re: [Suspend2][ 0/9] Extents support.

[Pekka Enberg]

Hi Nigel,

On 6/28/06, Nigel Cunningham <nigel@suspend2.net> wrote:
> It's because it's all so interconnected. Adding the modular infrastructure is
> useless without something to use the modules. Changing to use the pageflags
> functionality requires modifications in both the preparation of the image and
> in the I/O. There are bits that could be done incrementally, but they're
> minor. I did start with the same codebase that Pavel forked, but then did
> substantial rewrites in going from the betas to 1.0 and to 2.0.

Hmm, so, if you leave out the controversial in-kernel stuff like, user
interface bits, "extensible API", compression, and crypto, are you
saying there's nothing in suspend2 that can be merged separately?

                                        Pekka

Re: [Suspend2][ 0/9] Extents support.

[Pavel Machek]

Hi!

> Pavel wants everything in userspace.  Nigel wants everything but the
> chrome in kernelspace.  Other kernels maintainers are split on the
> issue, or don't care apart from "it should have been working for
> years, damnit".  Andrew plainly said he doesn't know what's best, and
> Linus seems to be very careful not to talk about it.
> 
> What should have happened is that suspend2 should have been merged
> years ago, and then now transiting more of it to userspace or not
> could be experimented, starting on a decently working base.  Pavel's
> NIH-ing prevented that though.
> 
> In any case, don't use software suspend on a machine with data you
> care about, and especially not uswsusp.  Side effect of userspace code
> is that nobody from outside the project reviews it, and it's way too
> young to have any kind of confidence in it.

Well, swsusp is *way* better reviewed than suspend2, because it is
<10% of code size, and it is in kernel already (plus Rafael did the
reviews/rewriting).

Your point about uswsusp is "interesting", but notice that noone ever
reviewed suspend2, either. So... if you care about your data, swsusp
is currently the _safest_ way to go. (It may not have the _nicest_
progress indication :-).
								Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

Re: [Suspend2][ 0/9] Extents support.

[Pavel Machek]

Hi!

> > > Now I haven't followed the suspend2 vs swsusp debate very closely, but
> > > it seems to me that your biggest problem with getting this merged is
> > > getting consensus on where exactly this is going. Nobody wants two
> > > different suspend modules in the kernel. So there are two options -
> > > suspend2 is deemed the way to go, and it gets merged and replaces
> > > swsusp. Or the other way around - people like swsusp more, and you are
> > > doomed to maintain suspend2 outside the tree.
> >
> > Actually, there's a third option that is looking like the way forward,
> > doing all of this from userspace and having no suspend-to-disk in the
> > kernel tree at all.
> >
> > Pavel and others have a working implementation and are slowly moving
> > toward adding all of the "bright and shiny" features that is in suspend2
> > to it (encryption, progress screens, abort by pressing a key, etc.) so
> > that there is no loss of functionality.
> >
> > So I don't really see the future of suspend2 because of this...
> 
> But what Rafael and Pavel are doing is really only moving the highest level of 
> controlling logic to userspace (ok, and maybe compression and encryption 
> too). Everything important (freezing other processes, atomic copy and the 
> guts of the I/O) is still done by the kernel.

Can you do the same and move compression/encryption to userspace, too?

And actually that "highest level" covers >50% of suspend2 code. That
would be around 7K lines of code removed from kernel if you did the
same, and suspend2 patch would be half the size...

> And there _is_ loss of functionality - uswsusp still doesn't support writing a 
> full image of memory, writing to multiple swap devices (partitions or files), 
> or writing to ordinary files. They're getting the low hanging fruit,
> but when 

That's userspace problem. Of course we are after low-hanging fruit,
first, but uswsusp design (AND CURRENT KERNEL PARTS) allow you to get
all the fruit with the right userspace.

> it comes to these parts of the problem, they're going to require either smoke 
> and very good mirrors (eg the swap prefetching trick), or simply refuse to 
> implement them.

:-) I like the mirrors idea. Anyway Rafael *did* get code for saving
whole memory at one point, but it looked quite dangerous to me. It was
~300 lines. I'm sure it can be resurrected.

> If we take the problem one step further, and begin to think about 
> checkpointing, they're in even bigger trouble. I'll freely admit that I'd 
> have to redesign the way I store data so that random parts of the image could 
> be replaced, have hooks in mm to be able to learn what pages need have 
> changed and would also need filesystem support to handle that part of the 
> problem, but I'd at least be working in the right domain.

Could you explain? I do not get the checkpointing remark.
									Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

Re: [Suspend2][ 0/9] Extents support.

[Nigel Cunningham]

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

Hi.

On Thursday 29 June 2006 07:11, Pavel Machek wrote:
> Hi!
>
> > > > Now I haven't followed the suspend2 vs swsusp debate very closely,
> > > > but it seems to me that your biggest problem with getting this merged
> > > > is getting consensus on where exactly this is going. Nobody wants two
> > > > different suspend modules in the kernel. So there are two options -
> > > > suspend2 is deemed the way to go, and it gets merged and replaces
> > > > swsusp. Or the other way around - people like swsusp more, and you
> > > > are doomed to maintain suspend2 outside the tree.
> > >
> > > Actually, there's a third option that is looking like the way forward,
> > > doing all of this from userspace and having no suspend-to-disk in the
> > > kernel tree at all.
> > >
> > > Pavel and others have a working implementation and are slowly moving
> > > toward adding all of the "bright and shiny" features that is in
> > > suspend2 to it (encryption, progress screens, abort by pressing a key,
> > > etc.) so that there is no loss of functionality.
> > >
> > > So I don't really see the future of suspend2 because of this...
> >
> > But what Rafael and Pavel are doing is really only moving the highest
> > level of controlling logic to userspace (ok, and maybe compression and
> > encryption too). Everything important (freezing other processes, atomic
> > copy and the guts of the I/O) is still done by the kernel.
>
> Can you do the same and move compression/encryption to userspace, too?
>
> And actually that "highest level" covers >50% of suspend2 code. That
> would be around 7K lines of code removed from kernel if you did the
> same, and suspend2 patch would be half the size...

That's not true. The compression and encryption support add ~1000 lines, as 
you pointed out the other day. If I moved compression and encryption support 
to userspace, I'd remove 1000 lines and:

- add more code for getting the pages copied to and from userspace
- require the user to get and build $LIBRARIES for doing the compression
- require the user to get and build $HELPER for doing the interface to 
Suspend2
- fail to leverage the perfectly good cryptoapi routines that are already 
there
- slow the whole process down because I'd now have a copy to userspace for 
every page being compressed/encrypted and a copy from userspace for every 
output page.
- make life more complicated for distro maintainers and users because they'd 
have another set of dependencies to worry about and mess with.

I'm not saying it's impossible. I'm just saying it would make suspending more 
complicated, at least potentially slower and more of a pain for everyone. On 
top of that, imagine if someone comes along and writes their own wizz-bang 
compression module, then complains (without telling you they did it) that 
Suspend2 trashed their hard disk when the bug is actually in their code. It's 
another way in which your kernel can be tainted :)

> > And there _is_ loss of functionality - uswsusp still doesn't support
> > writing a full image of memory, writing to multiple swap devices
> > (partitions or files), or writing to ordinary files. They're getting the
> > low hanging fruit, but when
>
> That's userspace problem. Of course we are after low-hanging fruit,
> first, but uswsusp design (AND CURRENT KERNEL PARTS) allow you to get
> all the fruit with the right userspace.
>
> > it comes to these parts of the problem, they're going to require either
> > smoke and very good mirrors (eg the swap prefetching trick), or simply
> > refuse to implement them.
> >
> :-) I like the mirrors idea. Anyway Rafael *did* get code for saving
>
> whole memory at one point, but it looked quite dangerous to me. It was
> ~300 lines. I'm sure it can be resurrected.
>
> > If we take the problem one step further, and begin to think about
> > checkpointing, they're in even bigger trouble. I'll freely admit that I'd
> > have to redesign the way I store data so that random parts of the image
> > could be replaced, have hooks in mm to be able to learn what pages need
> > have changed and would also need filesystem support to handle that part
> > of the problem, but I'd at least be working in the right domain.
>
> Could you explain? I do not get the checkpointing remark.

Sure. Suspending to disk is a pretty similar problem to checkpointing, except 
that you want to continue running afterwards, keep the image and modify it 
from time to time based on the changes in memory (having a checkpointing 
filesystem too, of course). My point is that modifying uswsusp to do 
checkpointing would be far harder precisely because you've pushed the highest 
level logic to userspace. It would be far more complicated, if not impossible 
for you to make the adjustments to do checkpointing.

Regards,

Nigel

-- 
See http://www.suspend2.net for Howtos, FAQs, mailing
lists, wiki and bugzilla info.

[-- Attachment #2: Type: application/pgp-signature, Size: 189 bytes --]

Re: [Suspend2][ 0/9] Extents support.

[Rafael J. Wysocki]

Hi,

On Wednesday 28 June 2006 01:47, Nigel Cunningham wrote:
> On Wednesday 28 June 2006 08:19, Rafael J. Wysocki wrote:
> > On Tuesday 27 June 2006 11:35, Nigel Cunningham wrote:
> > > On Tuesday 27 June 2006 19:26, Rafael J. Wysocki wrote:
> > > > > > Now I haven't followed the suspend2 vs swsusp debate very closely,
> > > > > > but it seems to me that your biggest problem with getting this
> > > > > > merged is getting consensus on where exactly this is going. Nobody
> > > > > > wants two different suspend modules in the kernel. So there are two
> > > > > > options - suspend2 is deemed the way to go, and it gets merged and
> > > > > > replaces swsusp. Or the other way around - people like swsusp more,
> > > > > > and you are doomed to maintain suspend2 outside the tree.
> > > > >
> > > > > Generally, I agree, although my understanding of Rafael and Pavel's
> > > > > mindset is that swsusp is a dead dog and uswsusp is the way they want
> > > > > to see things go. swsusp is only staying for backwards compatability.
> > > > > If that's the case, perhaps we can just replace swsusp with Suspend2
> > > > > and let them have their existing interface for uswsusp. Still not
> > > > > ideal, I agree, but it would be progress.
> > > >
> > > > Well, ususpend needs some core functionality to be provided by the
> > > > kernel, like freezing/thawing processes (this is also used by the STR),
> > > > snapshotting the system memory.  These should be shared with the
> > > > in-kernel suspend, be it swsusp or suspend2.
> > >
> > > If I modify suspend2 so that from now on it replaces swsusp, using
> > > noresume, resume= and echo disk > /sys/power/state in a way that's
> > > backward compatible with swsusp and doesn't interfere with uswsusp
> > > support, would you be happy? IIRC, Pavel has said in the past he wishes
> > > I'd just do that, but he's not you of course.
> >
> > That depends on how it's done.  For sure, I wouldn't like it to be done in
> > the "everything at once" manner.
> 
> I'm not sure I get what you're saying. Do you mean you'd prefer them to 
> coexist for a time in mainline? If so, I'd point out that suspend2 uses 
> different parameters at the moment precisely so they can coexist, so that 
> wouldn't be any change.

No, I'd like it to be done in small steps.  Actually as small as possible, so
that it's always clear what we are going to do and why.

If you want to start right now, please submit a bdevs freezing patch without
any non-essential additions.

Greetings,
Rafael

Re: [Suspend2][ 0/9] Extents support.

[Rafael J. Wysocki]

Hi,

On Wednesday 28 June 2006 14:42, Nigel Cunningham wrote:
> On Wednesday 28 June 2006 21:28, Rahul Karnik wrote:
> > On 6/27/06, Nigel Cunningham <nigel@suspend2.net> wrote:
> > > Suspend2 is a
> > > reimplementation of swsusp, not a series of incremental modifications. It
> > > uses completely different methods for writing the image, storing the
> > > metadata and so on. Until recently, the only thing it shared with swsusp
> > > was the refrigerator and driver model calls, and even now the sharing of
> > > lowlevel code is only a tiny fraction of all that is done.
> >
> > This is something I don't understand. Why can you not submit patches
> > that simply do things like "change method for writing image" and
> > reduce the difference between suspend2 and mainline? It may be more
> > work, but I think you will find that incremental changes are a lot
> > easier for people to review and merge.
> 
> It's because it's all so interconnected.

But it does not have to be.  Please try to untangle it so that you can submit
it in smaller pieces, one piece at a time.  Otherwise you require someone
to review all of your code and understand it at once, which is a huge task
and I don't think there's anyone with time resources needed for doing this.

Greetings,
Rafael

Re: [Suspend2][ 0/9] Extents support.

[Pavel Machek]

Hi!

> > > > So I don't really see the future of suspend2 because of this...
> > >
> > > But what Rafael and Pavel are doing is really only moving the highest
> > > level of controlling logic to userspace (ok, and maybe compression and
> > > encryption too). Everything important (freezing other processes, atomic
> > > copy and the guts of the I/O) is still done by the kernel.
> >
> > Can you do the same and move compression/encryption to userspace, too?
> >
> > And actually that "highest level" covers >50% of suspend2 code. That
> > would be around 7K lines of code removed from kernel if you did the
> > same, and suspend2 patch would be half the size...
> 
> That's not true. The compression and encryption support add ~1000 lines, as 
> you pointed out the other day. If I moved compression and encryption support 
> to userspace, I'd remove 1000 lines and:
> 
> - add more code for getting the pages copied to and from userspace

No, if your main loop is already in userspace, you do not need to add
any more code. And you'd save way more than 1000 lines:

* encryption/compression can be removed

* but that means that writer plugins/filters can be removed

* if you do compress/encrypt in userspace, you can remove that ugly
netlink thingie, and just display progress in userspace, too

...and then, image writing can be moved to userspace...

* swapfile support

* partition support

* plus their plugin infrastructure.

> > > If we take the problem one step further, and begin to think about
> > > checkpointing, they're in even bigger trouble. I'll freely admit that I'd
> > > have to redesign the way I store data so that random parts of the image
> > > could be replaced, have hooks in mm to be able to learn what pages need
> > > have changed and would also need filesystem support to handle that part
> > > of the problem, but I'd at least be working in the right domain.
> >
> > Could you explain? I do not get the checkpointing remark.
> 
> Sure. Suspending to disk is a pretty similar problem to checkpointing, except 
> that you want to continue running afterwards, keep the image and modify it 
> from time to time based on the changes in memory (having a checkpointing 
> filesystem too, of course). My point is that modifying uswsusp to do 
> checkpointing would be far harder precisely because you've pushed the highest 
> level logic to userspace. It would be far more complicated, if not impossible 
> for you to make the adjustments to do checkpointing.

Aha, that's probably better done with Xen, anyway :-).
									Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

Re: [Suspend2][ 0/9] Extents support.

[Nigel Cunningham]

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

Hi.

On Thursday 29 June 2006 08:44, Pavel Machek wrote:
> Hi!
>
> > > > > So I don't really see the future of suspend2 because of this...
> > > >
> > > > But what Rafael and Pavel are doing is really only moving the highest
> > > > level of controlling logic to userspace (ok, and maybe compression
> > > > and encryption too). Everything important (freezing other processes,
> > > > atomic copy and the guts of the I/O) is still done by the kernel.
> > >
> > > Can you do the same and move compression/encryption to userspace, too?
> > >
> > > And actually that "highest level" covers >50% of suspend2 code. That
> > > would be around 7K lines of code removed from kernel if you did the
> > > same, and suspend2 patch would be half the size...
> >
> > That's not true. The compression and encryption support add ~1000 lines,
> > as you pointed out the other day. If I moved compression and encryption
> > support to userspace, I'd remove 1000 lines and:
> >
> > - add more code for getting the pages copied to and from userspace
>
> No, if your main loop is already in userspace, you do not need to add
> any more code. And you'd save way more than 1000 lines:
>
> * encryption/compression can be removed
>
> * but that means that writer plugins/filters can be removed
>
> * if you do compress/encrypt in userspace, you can remove that ugly
> netlink thingie, and just display progress in userspace, too
>
> ...and then, image writing can be moved to userspace...
>
> * swapfile support
>
> * partition support
>
> * plus their plugin infrastructure.

That's going way beyond your inital suggestion. And you haven't responded to 
the other points (which have instead been deleted).

> > > > If we take the problem one step further, and begin to think about
> > > > checkpointing, they're in even bigger trouble. I'll freely admit that
> > > > I'd have to redesign the way I store data so that random parts of the
> > > > image could be replaced, have hooks in mm to be able to learn what
> > > > pages need have changed and would also need filesystem support to
> > > > handle that part of the problem, but I'd at least be working in the
> > > > right domain.
> > >
> > > Could you explain? I do not get the checkpointing remark.
> >
> > Sure. Suspending to disk is a pretty similar problem to checkpointing,
> > except that you want to continue running afterwards, keep the image and
> > modify it from time to time based on the changes in memory (having a
> > checkpointing filesystem too, of course). My point is that modifying
> > uswsusp to do checkpointing would be far harder precisely because you've
> > pushed the highest level logic to userspace. It would be far more
> > complicated, if not impossible for you to make the adjustments to do
> > checkpointing.
>
> Aha, that's probably better done with Xen, anyway :-).

Well, if you're going to put it in the too hard basket, it will have to be.

Nigel
-- 
See http://www.suspend2.net for Howtos, FAQs, mailing
lists, wiki and bugzilla info.

[-- Attachment #2: Type: application/pgp-signature, Size: 189 bytes --]

Re: [Suspend2][ 0/9] Extents support.

[Nigel Cunningham]

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

Hi.

On Thursday 29 June 2006 08:35, Rafael J. Wysocki wrote:
> On Wednesday 28 June 2006 01:47, Nigel Cunningham wrote:
> > On Wednesday 28 June 2006 08:19, Rafael J. Wysocki wrote:
> > > On Tuesday 27 June 2006 11:35, Nigel Cunningham wrote:
> > > > On Tuesday 27 June 2006 19:26, Rafael J. Wysocki wrote:
> > > > > > > Now I haven't followed the suspend2 vs swsusp debate very
> > > > > > > closely, but it seems to me that your biggest problem with
> > > > > > > getting this merged is getting consensus on where exactly this
> > > > > > > is going. Nobody wants two different suspend modules in the
> > > > > > > kernel. So there are two options - suspend2 is deemed the way
> > > > > > > to go, and it gets merged and replaces swsusp. Or the other way
> > > > > > > around - people like swsusp more, and you are doomed to
> > > > > > > maintain suspend2 outside the tree.
> > > > > >
> > > > > > Generally, I agree, although my understanding of Rafael and
> > > > > > Pavel's mindset is that swsusp is a dead dog and uswsusp is the
> > > > > > way they want to see things go. swsusp is only staying for
> > > > > > backwards compatability. If that's the case, perhaps we can just
> > > > > > replace swsusp with Suspend2 and let them have their existing
> > > > > > interface for uswsusp. Still not ideal, I agree, but it would be
> > > > > > progress.
> > > > >
> > > > > Well, ususpend needs some core functionality to be provided by the
> > > > > kernel, like freezing/thawing processes (this is also used by the
> > > > > STR), snapshotting the system memory.  These should be shared with
> > > > > the in-kernel suspend, be it swsusp or suspend2.
> > > >
> > > > If I modify suspend2 so that from now on it replaces swsusp, using
> > > > noresume, resume= and echo disk > /sys/power/state in a way that's
> > > > backward compatible with swsusp and doesn't interfere with uswsusp
> > > > support, would you be happy? IIRC, Pavel has said in the past he
> > > > wishes I'd just do that, but he's not you of course.
> > >
> > > That depends on how it's done.  For sure, I wouldn't like it to be done
> > > in the "everything at once" manner.
> >
> > I'm not sure I get what you're saying. Do you mean you'd prefer them to
> > coexist for a time in mainline? If so, I'd point out that suspend2 uses
> > different parameters at the moment precisely so they can coexist, so that
> > wouldn't be any change.
>
> No, I'd like it to be done in small steps.  Actually as small as possible,
> so that it's always clear what we are going to do and why.
>
> If you want to start right now, please submit a bdevs freezing patch
> without any non-essential additions.

Well, I admit that I'd far prefer to work with you than Pavel, but aren't we 
still just going to reach a point where you say "But that should be done in 
userspace" and I say "But I disagree that userspace is the right place for 
suspend to disk" and we stalemate? What then? It seems that I've already 
wasted a lot of effort listening to requests to split Suspend2 up into 
digestable chunks for review, only to find that they're not the digestable 
chunks people wanted to digest. I don't want to spent my time slicing and 
dicing in another way, only to find that the customer doesn't like that meal 
presentation either.

Can't we instead do what we started to do with the pageflags support? That is, 
look at the files one at a time, beginning with those that implement the more 
primitive functions, such as pageflags and extents and the like, verifying 
that they're implemented ok. Then progress to the higher level functions and 
check that they combine the primitives together okay, and so on. Or (if you 
prefer), do the reverse, beginning with suspend.c and progressing down into 
the details?

I guess the other question is, at the end of this, do you have to understand 
perfectly and completely how Suspend2 works? If not, are there things I could 
do to help? (Straight off, it occurs to me that I should get around to 
completing and updating the Documentation - it might help you a lot).

Regards,

Nigel
-- 
See http://www.suspend2.net for Howtos, FAQs, mailing
lists, wiki and bugzilla info.

[-- Attachment #2: Type: application/pgp-signature, Size: 189 bytes --]

Re: [Suspend2][ 0/9] Extents support.

[Nigel Cunningham]

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

Hi.

On Thursday 29 June 2006 00:42, Pekka Enberg wrote:
> Hi Nigel,
>
> On 6/28/06, Nigel Cunningham <nigel@suspend2.net> wrote:
> > It's because it's all so interconnected. Adding the modular
> > infrastructure is useless without something to use the modules. Changing
> > to use the pageflags functionality requires modifications in both the
> > preparation of the image and in the I/O. There are bits that could be
> > done incrementally, but they're minor. I did start with the same codebase
> > that Pavel forked, but then did substantial rewrites in going from the
> > betas to 1.0 and to 2.0.
>
> Hmm, so, if you leave out the controversial in-kernel stuff like, user
> interface bits, "extensible API", compression, and crypto, are you
> saying there's nothing in suspend2 that can be merged separately?

My point was that the architecture of Suspend2 is fundamentally different to 
that of swsusp. Suspend2 features could potentially be added to swsusp, but 
it would require a lot of work on swsusp. I've worked hard to make Suspend2 
clean and ready for merging. I'm not claiming it's perfect (we've already 
seen a few cleanups I missed), but I fail to see why I should be made to do 
even more work on the old version when I've already said that getting from it 
to Suspend2 involved substantial rewrites. Sure, I know where I'd be headed, 
but it would be a huge waste of time and effort.

Regards,

Nigel
-- 
See http://www.suspend2.net for Howtos, FAQs, mailing
lists, wiki and bugzilla info.

[-- Attachment #2: Type: application/pgp-signature, Size: 189 bytes --]

Re: [Suspend2][ 0/9] Extents support.

[Martin J. Bligh]

> That's not true. The compression and encryption support add ~1000 lines, as 
> you pointed out the other day. If I moved compression and encryption support 
> to userspace, I'd remove 1000 lines and:
> 
> - add more code for getting the pages copied to and from userspace
> - require the user to get and build $LIBRARIES for doing the compression
> - require the user to get and build $HELPER for doing the interface to 
> Suspend2
> - fail to leverage the perfectly good cryptoapi routines that are already 
> there
> - slow the whole process down because I'd now have a copy to userspace for 
> every page being compressed/encrypted and a copy from userspace for every 
> output page.
> - make life more complicated for distro maintainers and users because they'd 
> have another set of dependencies to worry about and mess with.
> 
> I'm not saying it's impossible. I'm just saying it would make suspending more 
> complicated, at least potentially slower and more of a pain for everyone.

Thanks for actually thinking through about the implications of pushing
yet another subsystem out into userspace. Most people don't seem to
bother ;-(

M.

Re: [Suspend2][ 0/9] Extents support.

[Pekka Enberg]

On 6/29/06, Nigel Cunningham <nigel@suspend2.net> wrote:
> Sure, I know where I'd be headed, but it would be a huge waste of time and effort.

Perhaps to you Nigel.  For the rest of us reviewing your patches, it's
much better.  I suspect it would be better for the users down the road
as well.  I don't know if you realize it, but what you're doing now
is, "here's a big chunck of code, take it or leave it".  And at least
historically people have had hard time doing getting stuff merged like
that.

                                                           Pekka

Re: [Suspend2][ 0/9] Extents support.

[Nigel Cunningham]

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

Hi.

On Thursday 29 June 2006 15:19, Pekka Enberg wrote:
> On 6/29/06, Nigel Cunningham <nigel@suspend2.net> wrote:
> > Sure, I know where I'd be headed, but it would be a huge waste of time
> > and effort.
>
> Perhaps to you Nigel.  For the rest of us reviewing your patches, it's
> much better.  I suspect it would be better for the users down the road
> as well.  I don't know if you realize it, but what you're doing now
> is, "here's a big chunck of code, take it or leave it".  And at least
> historically people have had hard time doing getting stuff merged like
> that.

I did try really hard not to do that (big chunk of code, take it or leave it). 
That's why it's split up into so many little patches. The problem seems to be 
that it's not split up in the way some people wanted, rather than not split 
up at all. I want to make it easier on you guys, but it just seems to me like 
regardless of what I do, it's not the right thing.

I can understand wanting small changes to swsusp to transform it into 
suspend2, but I also understand that I've spent approximately 5 years of 
developing from the point Pavel forked the code base until today, and part of 
that has been two complete reworkings of the way in which the data is stored 
and the thing operates - irreducible complexity that just doesn't fit into 
the incremental change model. So I'm trying to do what seems to me to be the 
next best thing. Having arranged functions that deal with particular parts of 
the system into individual files, I've broken the files up into logical parts 
and submitted them in groups. If we consider the more primitive parts first, 
then move to the increasingly abstract operations (or vice versa), I think 
we'll have a good approach with what's already done.

Regards,

Nigel
-- 
See http://www.suspend2.net for Howtos, FAQs, mailing
lists, wiki and bugzilla info.

[-- Attachment #2: Type: application/pgp-signature, Size: 189 bytes --]

Re: [Suspend2][ 0/9] Extents support.

[Rafael J. Wysocki]

Hi,

On Thursday 29 June 2006 01:26, Nigel Cunningham wrote:
> On Thursday 29 June 2006 08:35, Rafael J. Wysocki wrote:
> > On Wednesday 28 June 2006 01:47, Nigel Cunningham wrote:
> > > On Wednesday 28 June 2006 08:19, Rafael J. Wysocki wrote:
> > > > On Tuesday 27 June 2006 11:35, Nigel Cunningham wrote:
> > > > > On Tuesday 27 June 2006 19:26, Rafael J. Wysocki wrote:
> > > > > > > > Now I haven't followed the suspend2 vs swsusp debate very
> > > > > > > > closely, but it seems to me that your biggest problem with
> > > > > > > > getting this merged is getting consensus on where exactly this
> > > > > > > > is going. Nobody wants two different suspend modules in the
> > > > > > > > kernel. So there are two options - suspend2 is deemed the way
> > > > > > > > to go, and it gets merged and replaces swsusp. Or the other way
> > > > > > > > around - people like swsusp more, and you are doomed to
> > > > > > > > maintain suspend2 outside the tree.
> > > > > > >
> > > > > > > Generally, I agree, although my understanding of Rafael and
> > > > > > > Pavel's mindset is that swsusp is a dead dog and uswsusp is the
> > > > > > > way they want to see things go. swsusp is only staying for
> > > > > > > backwards compatability. If that's the case, perhaps we can just
> > > > > > > replace swsusp with Suspend2 and let them have their existing
> > > > > > > interface for uswsusp. Still not ideal, I agree, but it would be
> > > > > > > progress.
> > > > > >
> > > > > > Well, ususpend needs some core functionality to be provided by the
> > > > > > kernel, like freezing/thawing processes (this is also used by the
> > > > > > STR), snapshotting the system memory.  These should be shared with
> > > > > > the in-kernel suspend, be it swsusp or suspend2.
> > > > >
> > > > > If I modify suspend2 so that from now on it replaces swsusp, using
> > > > > noresume, resume= and echo disk > /sys/power/state in a way that's
> > > > > backward compatible with swsusp and doesn't interfere with uswsusp
> > > > > support, would you be happy? IIRC, Pavel has said in the past he
> > > > > wishes I'd just do that, but he's not you of course.
> > > >
> > > > That depends on how it's done.  For sure, I wouldn't like it to be done
> > > > in the "everything at once" manner.
> > >
> > > I'm not sure I get what you're saying. Do you mean you'd prefer them to
> > > coexist for a time in mainline? If so, I'd point out that suspend2 uses
> > > different parameters at the moment precisely so they can coexist, so that
> > > wouldn't be any change.
> >
> > No, I'd like it to be done in small steps.  Actually as small as possible,
> > so that it's always clear what we are going to do and why.
> >
> > If you want to start right now, please submit a bdevs freezing patch
> > without any non-essential additions.
> 
> Well, I admit that I'd far prefer to work with you than Pavel, but aren't we 
> still just going to reach a point where you say "But that should be done in 
> userspace" and I say "But I disagree that userspace is the right place for 
> suspend to disk" and we stalemate? What then? It seems that I've already 
> wasted a lot of effort listening to requests to split Suspend2 up into 
> digestable chunks for review, only to find that they're not the digestable 
> chunks people wanted to digest. I don't want to spent my time slicing and 
> dicing in another way, only to find that the customer doesn't like that meal 
> presentation either.

I haven't said anything about the userspace, so far.  I'm just talking about
what in my opinion can be done to merge some parts of your code early,
which may also help you, because once it gets merged you'll have less code
to maintain.

Now the freezer is the part we must share, so it's better if we start from it,
IMO, and it's quite clear to me that the freezing of bdevs will help fix the
XFS test case.  [It may also be helpful in other ways, but that's not obvious
right now.]  However it is not clear to me that the other changes to the
freezer that you currently have in your patch are correct, so it's better
to consider them separately.

> Can't we instead do what we started to do with the pageflags support? That is, 
> look at the files one at a time, beginning with those that implement the more 
> primitive functions, such as pageflags and extents and the like, verifying 
> that they're implemented ok. Then progress to the higher level functions and 
> check that they combine the primitives together okay, and so on. Or (if you 
> prefer), do the reverse, beginning with suspend.c and progressing down into 
> the details?

We could do a review the way you describe, although I think that the low-level
thing should be considered along with the high-level parts where they are
used, but I see two problems here.

First, it would take a lot of time and the kernel is being developed
continuously, so there's the danger that the code reviewed at the beginning
will have to be redone to reflect the changes in the kernel and it would have
to be reviewed once again.  For this reason it would be more efficient to try
to merge the code as soon as it's been reviewed and we are comfortable
with it.

Second, there may be something in the code that happens to be unmergeable,
whatever the reason (eg. someone violently opposes it etc.).  Then, we'd have
to drop it and that may affect the code that has already been reviewed, so
this code will have to be redone etc.  We can avoid this issue if the patches
are arranged so that we can merge relatively small chunks of them as soon as
everyone is comfortable with them.

As I said before, I think the freezer changes should go first.  The next thing
to try is the "dynamic pageflags" or generally using bitmaps for page
management as it would give us a clear advantage over the current
implementation as far as the utilisation of memory is concerned.

> I guess the other question is, at the end of this, do you have to understand 
> perfectly and completely how Suspend2 works? If not, are there things I could 
> do to help? (Straight off, it occurs to me that I should get around to 
> completing and updating the Documentation - it might help you a lot).

I think I know the general idea well enough and the details are in the code.
I'll ask if I have any doubts. :-)

Greetings,
Rafael

Re: [Suspend2][ 0/9] Extents support.

[Rafael J. Wysocki]

Hi,

On Thursday 29 June 2006 07:44, Nigel Cunningham wrote:
> On Thursday 29 June 2006 15:19, Pekka Enberg wrote:
> > On 6/29/06, Nigel Cunningham <nigel@suspend2.net> wrote:
> > > Sure, I know where I'd be headed, but it would be a huge waste of time
> > > and effort.
> >
> > Perhaps to you Nigel.  For the rest of us reviewing your patches, it's
> > much better.  I suspect it would be better for the users down the road
> > as well.  I don't know if you realize it, but what you're doing now
> > is, "here's a big chunck of code, take it or leave it".  And at least
> > historically people have had hard time doing getting stuff merged like
> > that.
> 
> I did try really hard not to do that (big chunk of code, take it or leave it). 
> That's why it's split up into so many little patches. The problem seems to be 
> that it's not split up in the way some people wanted, rather than not split 
> up at all. I want to make it easier on you guys, but it just seems to me like 
> regardless of what I do, it's not the right thing.

I think the problem is that you want it merged all at once, and it's too much
code for doing so.  The splitting is a separate thing - previously the patches
were too big, now they are too small, but from the reviewer's point of view
it's about the same: you can't get a grip on what's going on and why.

> I can understand wanting small changes to swsusp to transform it into 
> suspend2, but I also understand that I've spent approximately 5 years of 
> developing from the point Pavel forked the code base until today, and part of 
> that has been two complete reworkings of the way in which the data is stored 
> and the thing operates - irreducible complexity that just doesn't fit into 
> the incremental change model. So I'm trying to do what seems to me to be the 
> next best thing. Having arranged functions that deal with particular parts of 
> the system into individual files, I've broken the files up into logical parts 
> and submitted them in groups. If we consider the more primitive parts first, 
> then move to the increasingly abstract operations (or vice versa), I think 
> we'll have a good approach with what's already done.

No.  The additional work on your part _is_ _needed_ so that _other_ _people_
may feel comfortable with your code in the kernel.  Now, apparently, they are
not, for various reasons, and you're just refusing to help them.

Greetings,
Rafael

Re: [Suspend2][ 0/9] Extents support.

[Pavel Machek]

Hi!

> > > That's not true. The compression and encryption support add ~1000 lines,
> > > as you pointed out the other day. If I moved compression and encryption
> > > support to userspace, I'd remove 1000 lines and:
> > >
> > > - add more code for getting the pages copied to and from userspace
> >
> > No, if your main loop is already in userspace, you do not need to add
> > any more code. And you'd save way more than 1000 lines:
> >
> > * encryption/compression can be removed
> >
> > * but that means that writer plugins/filters can be removed
> >
> > * if you do compress/encrypt in userspace, you can remove that ugly
> > netlink thingie, and just display progress in userspace, too
> >
> > ...and then, image writing can be moved to userspace...
> >
> > * swapfile support
> >
> > * partition support
> >
> > * plus their plugin infrastructure.
> 
> That's going way beyond your inital suggestion. And you haven't responded to 
> the other points (which have instead been deleted).

Well, you were arguing that in uswsusp only "thin layer" is in
userspace, and I think I've just shown you it is more thick than you
think.

Of course I deleted your other points, or did you really want me to
argue with "copying from userspace is slow"? Hint: memory is 3GB/sec,
while disk is 50MB/sec.
									Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

suspend2 merge [was Re: [Suspend2][ 0/9] Extents support.]

[Pavel Machek]

Hi!

> > > It's because it's all so interconnected. Adding the modular
> > > infrastructure is useless without something to use the modules. Changing
> > > to use the pageflags functionality requires modifications in both the
> > > preparation of the image and in the I/O. There are bits that could be
> > > done incrementally, but they're minor. I did start with the same codebase
> > > that Pavel forked, but then did substantial rewrites in going from the
> > > betas to 1.0 and to 2.0.
> >
> > Hmm, so, if you leave out the controversial in-kernel stuff like, user
> > interface bits, "extensible API", compression, and crypto, are you
> > saying there's nothing in suspend2 that can be merged separately?
> 
> My point was that the architecture of Suspend2 is fundamentally different to 
> that of swsusp. Suspend2 features could potentially be added to swsusp, but 
> it would require a lot of work on swsusp. I've worked hard to make

That is how kernel development works, I'm afraid. It is better to let
someone do lot of work than to have to merge 14000 lines in one go.

It sucks if that "someone" is you, I guess.

Kernel development works by evolution, unless there's really good
reason not to. And no "swap file support for suspend" is not good
enough reason. Maybe suspend2 was designed to be nice, fast, and full
of features; unfortunately it was not designed to be
mergeable. Oops. I believe you even stated that you do not want it
merged at one point.

There are two possible ways forward:

1. suspend2 stays out of tree, pretty much forever. (Or as long as you
are interested).

2. suspend2 gets complete rewrite, using code that is already present
in kernel to maximum extent. Yes, you need to do "evolution", merge
quickly, and make clear improvement with each patch. You also have to
explain why your patch is neccessary -- why current code can not do
the job. Fortunately for you,all the neccessary support is already in
tree. Only piece missing is "save whole image", and Rafael actually
has patch for that -- but that patch was deemed too dangerous.

Now, I'm sorry you wasted lots of work splitting patches
function-by-function, but watching lkml for a while would probably
tell you how mergeable patches look, and I believe I was pretty clear
that splitting suspend2 is not the _only_ requirement to get it
merged.
									Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

Re: [Suspend2][ 0/9] Extents support.

[Pavel Machek]

Hi!

> > I'm not sure I get what you're saying. Do you mean you'd prefer them to 
> > coexist for a time in mainline? If so, I'd point out that suspend2 uses 
> > different parameters at the moment precisely so they can coexist, so that 
> > wouldn't be any change.
> 
> No, I'd like it to be done in small steps.  Actually as small as possible, so
> that it's always clear what we are going to do and why.
> 
> If you want to start right now, please submit a bdevs freezing patch without
> any non-essential additions.

Actually that's quite a good plan. Or better yet fix XFS so that bdev
freezing is not neccessary. Or maybe _force someone_ to fix XFS...
									Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

Re: suspend2 merge [was Re: [Suspend2][ 0/9] Extents support.]

[Dumitru Ciobarcianu]

On Fri, 2006-06-30 at 19:55 +0200, Pavel Machek wrote:
> Now, I'm sorry you wasted lots of work splitting patches
> function-by-function, but watching lkml for a while would probably
> tell you how mergeable patches look, and I believe I was pretty clear
> that splitting suspend2 is not the _only_ requirement to get it
> merged.


The other requirement is to be implemented in userspace ?
(sorry, couldn't resisist...)

-- 
Cioby