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

Rafael J. Wysocki
2010-03-25T22:21:49Z

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

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

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

Rafael