Switch /dev/snapshot reader to sws_module_ops approach so that we
can transparently rewrite the rest of the snapshot from pages pulling
to their pushing through layers.
Signed-off-by: Jiri Slaby <jslaby@suse.cz>
Cc: Nigel Cunningham <ncunningham@crca.org.au>
Cc: "Rafael J. Wysocki" <rjw@sisk.pl>
---
kernel/power/user.c | 73 +++++++++++++++++++++++++++++++++++++++++++++-----
1 files changed, 65 insertions(+), 8 deletions(-)
diff --git a/kernel/power/user.c b/kernel/power/user.c
index 748567d..1b5d2e1 100644
--- a/kernel/power/user.c
+++ b/kernel/power/user.c
@@ -76,6 +76,7 @@ static DECLARE_BITMAP(to_do_flags, 10);
#define TODO_FINISH 2
#define TODO_CLOSED 3
#define TODO_ERROR 4
+#define TODO_RD_RUNNING 5
static unsigned long user_storage_available(void)
{
@@ -123,12 +124,41 @@ static int put_user_writer(unsigned int flags, int error)
return error;
}
+static int get_user_reader(unsigned int *flags_p)
+{
+ return 0;
+}
+
+static int user_read_page(void *addr, struct bio **bio_chain)
+{
+ int err = 0;
+
+ to_do_buf = addr;
+ wmb();
+ set_bit(TODO_WORK, to_do_flags);
+ wake_up_interruptible(&to_do_wait);
+
+ wait_event(to_do_done, !test_bit(TODO_WORK, to_do_flags) ||
+ (err = test_bit(TODO_CLOSED, to_do_flags)));
+
+ return err ? -EIO : 0;
+}
+
+static int put_user_reader(void)
+{
+ return 0;
+}
+
struct sws_module_ops user_ops = {
.storage_available = user_storage_available,
.get_writer = get_user_writer,
.put_writer = put_user_writer,
.write_page = user_write_page,
+
+ .get_reader = get_user_reader,
+ .put_reader = put_user_reader,
+ .read_page = user_read_page,
};
static void snapshot_writer(struct work_struct *work)
@@ -142,6 +172,22 @@ static void snapshot_writer(struct work_struct *work)
static DECLARE_WORK(snapshot_writer_w, snapshot_writer);
+static void snapshot_reader(struct work_struct *work)
+{
+ int ret;
+
+ set_bit(TODO_RD_RUNNING, to_do_flags);
+ ret = swsusp_read(NULL);
+ if (ret) ...Ok, I guess that now I see what you are doing.... adding interface
layer between /dev/snapshot and core hibernation code.
To recap, 2.6.33 hibernation looks like:
core hibernation
/\
/ \
swsusp /dev/snapshot
swap \
writing -------- read/write/ioctl interface
\
s2disk
and after your patches, we'd get
core hibernation
/\
---------- sws_module_ops interface
/ \
swsusp /dev/snapshot
swap \
writing -------- read/write/ioctl interface
\
s2disk
(Right? Did I understand the patches correctly?)
I have some problems with sws_module_ops interface (handcoded locking
is too ugly to live), but it is better than I expected. But there may
be better solution available, one that does not need two interfaces to
maintain (we can't really get rid of userland interface). What about
this?
core hibernation
\
\
/dev/snapshot
/ \
---------- read/write/ioctl interface
/ \
swsusp s2disk
swap
writing
? That way, we have just one interface, and still keep the advantages
of modularity / defined interfaces.
(You could literary call sys_read() from inside the kernel -- after
set_fs() -- but going to that extreme is probably not neccessary. But
having interface very similar to what /dev/snapshot provides -- with
the same locking rules -- should result in better code.)
Pavel
--
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
--
Hi. On 25/03/10 16:30, Pavel Machek wrote: Just picking up on that bracketed part: Can we flag the userland interface (and uswsusp) as being planned for eventual removal now... or at least agree to work toward that? I'm asking because if we're going to make a go of getting the in-kernel code in much better shape, and we have Rafael, Jiri and I - and you? - all pulling in the same direction to improve it, there's going to come a point (hopefully not too far away) where uswsusp is just making life too difficult, and getting rid of it will be a big help. Regards, Nigel --
Hi again. I realised after sending this that the " - and you? - " was ambiguous. I wasn't meaning to suggest that you might pull in a different direction, but rather uncertainty as to whether you might help with the effort - it's been a while (AFAIR) since you've done any hibernaton patches. Humble apologies for the ambiguity! Nigel --
We're not dropping user space interfaces used by every distro I know of. Rafael --
Hi. So what's your long term plan then? Regards, Nigel --
First, improve the in-kernel thing, second, switch people to it, _then_ remove the s2disk interface (after we're reasonably sure it's not used by any major distro) and _finally_ simplify things after it's been removed. Does that sound reasonable? Rafael --
Hi. Well, that's pretty much what I was thinking too - improve then remove. I was just suggesting that we flag now that this is our plan, so it doesn't come as a surprise to anyone later and we can proceed more quickly than might otherwise be the case. I'm imagining it won't take long to get uswsusp features into the kernel code. Regards, Nigel --
I think it's too early for that at this point. Rafael --
I'd really prefer to keep s2disk interface. It allows advanced stuff like internet suspend resume (yep someone is doing it), crazy stuff like multiple resumes from same image for fast booting (yep, some embedded people are doing that) and might-be-useful stuff like s2both... Pavel -- (english) http://www.livejournal.com/~pavelmachek (cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html --
Hi. Neither of those are impossible with in-kernel code, so I'd argue that there's no need to keep the s2disk interface long-term. Userspace helpers might be necessary for the first one (to manage the network interface), but I already have multiple-resumes-from-the-same-image support in TuxOnice (and more 'crazy stuff' like support for resuming a different image after writing one - that can be used to switch to an initramfs containing the binaries needed to power down a UPS). Regards, Nigel --
The fact that it can be done in kernel -- anything can -- but whether it should. I'd very much like to keep the 'crazy stuff' in userspace. Pavel -- (english) http://www.livejournal.com/~pavelmachek (cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html --
On Friday 02 April 2010, Pavel Machek wrote: IMO the question really boils down to what's the benefit of doing these things in the kernel vs doing them in the user space. If doing them in the kernel reduces the overall complexity of design (including the user space interface involved etc.) I don't really see why not to. Rafael --
The user space interface does things that the in-kernel one doesn't really care for, so I don't think that would be a good thing to do. I admit it's a bit like this right now (snapshot_[read|write]_next() do some things to satisfy the user space interface's needs), but I don't really think it should go any further than that. Moreover, the Jiri's approach allows us to handle other types of storage as well as swap using uniform interface. Rafael --
