Re: [RFC][PATCH -mm] PM: Do not sync from within the freezer during suspend to RAM

Re: [RFC][PATCH -mm] PM: Do not sync from within the freezer during suspend to RAM

[Nigel Cunningham]

[-- Attachment #1.1: Type: text/plain, Size: 1359 bytes --]

Hi.

On Thursday 05 July 2007 00:58:58 Rafael J. Wysocki wrote:
> From: Rafael J. Wysocki <rjw@sisk.pl>
> 
> The syncing of filesystems from within the freezer in not needed for suspend 
to
> RAM.  Change freeze_processes() so that it doesn't execute sys_sync() and
> introduce the "syncing" version of it to be called from the hibernation code
> paths.
> 
> Signed-off-by: Rafael J. Wysocki <rjw@sisk.pl>
> ---
>  include/linux/freezer.h |   14 ++++++++++++--
>  kernel/power/disk.c     |    2 +-
>  kernel/power/main.c     |    6 ++++++
>  kernel/power/process.c  |    8 +++++---
>  kernel/power/user.c     |    2 +-
>  5 files changed, 25 insertions(+), 7 deletions(-)

Looks ok, except that I wonder if you want the following fragment. It looks to 
me (looking at rc6) like with this code, you'll currently call sys_sync twice 
when suspending to ram. Maybe I'm misreading it. Also, shouldn't it be done 
after taking the mutex?

Regards,

Nigel

> @@ -231,6 +232,11 @@ static int enter_state(suspend_state_t s
>  
>  	if (!valid_state(state))
>  		return -ENODEV;
> +
> +	printk("Syncing filesystems ... ");
> +	sys_sync();
> +	printk("done.\n");
> +
>  	if (!mutex_trylock(&pm_mutex))
>  		return -EBUSY;
>  


-- 
Nigel, Michelle and Alisdair Cunningham
5 Mitchell Street
Cobden 3266
Victoria, Australia

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

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

Re: [RFC][PATCH -mm] PM: Do not sync from within the freezer during suspend to RAM

[Pavel Machek]

On Thu 2007-07-05 08:48:15, Nigel Cunningham wrote:
> Hi.
> 
> On Thursday 05 July 2007 00:58:58 Rafael J. Wysocki wrote:
> > From: Rafael J. Wysocki <rjw@sisk.pl>
> > 
> > The syncing of filesystems from within the freezer in not needed for suspend 
> to
> > RAM.  Change freeze_processes() so that it doesn't execute sys_sync() and
> > introduce the "syncing" version of it to be called from the hibernation code
> > paths.
> > 
> > Signed-off-by: Rafael J. Wysocki <rjw@sisk.pl>
> > ---
> >  include/linux/freezer.h |   14 ++++++++++++--
> >  kernel/power/disk.c     |    2 +-
> >  kernel/power/main.c     |    6 ++++++
> >  kernel/power/process.c  |    8 +++++---
> >  kernel/power/user.c     |    2 +-
> >  5 files changed, 25 insertions(+), 7 deletions(-)
> 
> Looks ok, except that I wonder if you want the following fragment. It looks to 
> me (looking at rc6) like with this code, you'll currently call sys_sync twice 
> when suspending to ram. Maybe I'm misreading it. Also, shouldn't it be done 
> after taking the mutex?

sys_sync() should be okay to call, mutex or not.
									Pavel



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

Re: [RFC][PATCH -mm] PM: Do not sync from within the freezer during suspend to RAM

[Nigel Cunningham]

[-- Attachment #1.1: Type: text/plain, Size: 1570 bytes --]

Hi.

On Thursday 05 July 2007 08:49:42 Pavel Machek wrote:
> On Thu 2007-07-05 08:48:15, Nigel Cunningham wrote:
> > Hi.
> > 
> > On Thursday 05 July 2007 00:58:58 Rafael J. Wysocki wrote:
> > > From: Rafael J. Wysocki <rjw@sisk.pl>
> > > 
> > > The syncing of filesystems from within the freezer in not needed for 
suspend 
> > to
> > > RAM.  Change freeze_processes() so that it doesn't execute sys_sync() 
and
> > > introduce the "syncing" version of it to be called from the hibernation 
code
> > > paths.
> > > 
> > > Signed-off-by: Rafael J. Wysocki <rjw@sisk.pl>
> > > ---
> > >  include/linux/freezer.h |   14 ++++++++++++--
> > >  kernel/power/disk.c     |    2 +-
> > >  kernel/power/main.c     |    6 ++++++
> > >  kernel/power/process.c  |    8 +++++---
> > >  kernel/power/user.c     |    2 +-
> > >  5 files changed, 25 insertions(+), 7 deletions(-)
> > 
> > Looks ok, except that I wonder if you want the following fragment. It 
looks to 
> > me (looking at rc6) like with this code, you'll currently call sys_sync 
twice 
> > when suspending to ram. Maybe I'm misreading it. Also, shouldn't it be 
done 
> > after taking the mutex?
> 
> sys_sync() should be okay to call, mutex or not.

Yeah. That wasn't my point, sorry. Calling sys_sync is pointless if you're 
going to fail to take the mutex. It makes more sense to know you've got it 
before you start doing things related to actually suspending.

Regards,

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

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

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

Re: [RFC][PATCH -mm] PM: Do not sync from within the freezer during suspend to RAM

[Rafael J. Wysocki]

On Thursday, 5 July 2007 00:52, Nigel Cunningham wrote:
> Hi.
> 
> On Thursday 05 July 2007 08:49:42 Pavel Machek wrote:
> > On Thu 2007-07-05 08:48:15, Nigel Cunningham wrote:
> > > Hi.
> > > 
> > > On Thursday 05 July 2007 00:58:58 Rafael J. Wysocki wrote:
> > > > From: Rafael J. Wysocki <rjw@sisk.pl>
> > > > 
> > > > The syncing of filesystems from within the freezer in not needed for 
> suspend 
> > > to
> > > > RAM.  Change freeze_processes() so that it doesn't execute sys_sync() 
> and
> > > > introduce the "syncing" version of it to be called from the hibernation 
> code
> > > > paths.
> > > > 
> > > > Signed-off-by: Rafael J. Wysocki <rjw@sisk.pl>
> > > > ---
> > > >  include/linux/freezer.h |   14 ++++++++++++--
> > > >  kernel/power/disk.c     |    2 +-
> > > >  kernel/power/main.c     |    6 ++++++
> > > >  kernel/power/process.c  |    8 +++++---
> > > >  kernel/power/user.c     |    2 +-
> > > >  5 files changed, 25 insertions(+), 7 deletions(-)
> > > 
> > > Looks ok, except that I wonder if you want the following fragment. It 
> looks to 
> > > me (looking at rc6) like with this code, you'll currently call sys_sync 
> twice 
> > > when suspending to ram. Maybe I'm misreading it. Also, shouldn't it be 
> done 
> > > after taking the mutex?
> > 
> > sys_sync() should be okay to call, mutex or not.
> 
> Yeah. That wasn't my point, sorry. Calling sys_sync is pointless if you're 
> going to fail to take the mutex. It makes more sense to know you've got it 
> before you start doing things related to actually suspending.

Well, that's a valid point, I'll move it under the mutex.

And why do you think it will be called twice?

Greetings,
Rafael


-- 
"Premature optimization is the root of all evil." - Donald Knuth

Re: [RFC][PATCH -mm] PM: Do not sync from within the freezer during suspend to RAM

[Nigel Cunningham]

[-- Attachment #1.1: Type: text/plain, Size: 2006 bytes --]

Hi.

On Thursday 05 July 2007 21:28:49 Rafael J. Wysocki wrote:
> On Thursday, 5 July 2007 00:52, Nigel Cunningham wrote:
> > On Thursday 05 July 2007 08:49:42 Pavel Machek wrote:
> > > On Thu 2007-07-05 08:48:15, Nigel Cunningham wrote:
> > > > On Thursday 05 July 2007 00:58:58 Rafael J. Wysocki wrote:
> > > > > The syncing of filesystems from within the freezer in not needed for 
> > suspend 
> > > > to
> > > > > RAM.  Change freeze_processes() so that it doesn't execute 
sys_sync() 
> > and
> > > > > introduce the "syncing" version of it to be called from the 
hibernation 
> > code
> > > > > paths.
> > > > > 
> > > > > Signed-off-by: Rafael J. Wysocki <rjw@sisk.pl>
> > > > > ---
> > > > >  include/linux/freezer.h |   14 ++++++++++++--
> > > > >  kernel/power/disk.c     |    2 +-
> > > > >  kernel/power/main.c     |    6 ++++++
> > > > >  kernel/power/process.c  |    8 +++++---
> > > > >  kernel/power/user.c     |    2 +-
> > > > >  5 files changed, 25 insertions(+), 7 deletions(-)
> > > > 
> > > > Looks ok, except that I wonder if you want the following fragment. It 
> > looks to 
> > > > me (looking at rc6) like with this code, you'll currently call 
sys_sync 
> > twice 
> > > > when suspending to ram. Maybe I'm misreading it. Also, shouldn't it be 
> > done 
> > > > after taking the mutex?
> > > 
> > > sys_sync() should be okay to call, mutex or not.
> > 
> > Yeah. That wasn't my point, sorry. Calling sys_sync is pointless if you're 
> > going to fail to take the mutex. It makes more sense to know you've got it 
> > before you start doing things related to actually suspending.
> 
> Well, that's a valid point, I'll move it under the mutex.

Okee doke.
 
> And why do you think it will be called twice?

Hmm. Looking again now, I can't see why I thought that. Maybe I just hadn't 
had enough caffeine today at that stage :)

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

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

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

Re: [RFC][PATCH -mm] PM: Do not sync filesystems from within the freezer

[Pavel Machek]

Hi!

> From: Rafael J. Wysocki <rjw@sisk.pl>
> 
> The syncing of filesystems from within the freezer is generally not needed.
> Change freeze_processes() so that it doesn't execute sys_sync() and make the
> suspend and hibernation code path sync filesystems independently of the freezer.

Yes, we can do that, but ... why?

Does it actually fix FUSE? Miklos claims sync is nop on FUSE...?
									Pavel

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

Re: [RFC][PATCH -mm] PM: Do not sync filesystems from within the freezer

[Rafael J. Wysocki]

On Friday, 6 July 2007 00:00, Pavel Machek wrote:
> Hi!
> 
> > From: Rafael J. Wysocki <rjw@sisk.pl>
> > 
> > The syncing of filesystems from within the freezer is generally not needed.
> > Change freeze_processes() so that it doesn't execute sys_sync() and make the
> > suspend and hibernation code path sync filesystems independently of the freezer.
> 
> Yes, we can do that, but ... why?

I think that sync and the freezer are different things and shouldn't be mixed in
such a way as they are now.

> Does it actually fix FUSE?

It should prevent the freezer from deadlocking.

> Miklos claims sync is nop on FUSE...? 

In that case there shouldn't be any deadlock, but a freezer failure. :-)

Greetings,
Rafael


-- 
"Premature optimization is the root of all evil." - Donald Knuth

Re: [RFC][PATCH -mm] PM: Do not sync filesystems from within the freezer

[Nigel Cunningham]

[-- Attachment #1.1: Type: text/plain, Size: 1524 bytes --]

Hi.

On Friday 06 July 2007 17:02:53 Rafael J. Wysocki wrote:
> On Friday, 6 July 2007 00:00, Pavel Machek wrote:
> > Hi!
> > 
> > > From: Rafael J. Wysocki <rjw@sisk.pl>
> > > 
> > > The syncing of filesystems from within the freezer is generally not 
needed.
> > > Change freeze_processes() so that it doesn't execute sys_sync() and make 
the
> > > suspend and hibernation code path sync filesystems independently of the 
freezer.
> > 
> > Yes, we can do that, but ... why?
> 
> I think that sync and the freezer are different things and shouldn't be 
mixed in
> such a way as they are now.
> 
> > Does it actually fix FUSE?
> 
> It should prevent the freezer from deadlocking.

That's not the same thing. It's like saying "My footbrake grabs so I'll use 
the handbrake all the time. Take the stone out of the brake pad! :)

> > Miklos claims sync is nop on FUSE...? 
> 
> In that case there shouldn't be any deadlock, but a freezer failure. :-)

Isn't this scary? I'm agreeing with Pavel and the two of us seem to be 
disagreeing with everyone else!

To get more serious and practical though, I think the solution is to fuzz the 
userspace/kernelspace distinction. What we really want to do is freeze things 
that submit I/O, then sync, then freeze anything that processes I/O and needs 
to be frozen. In effect, redefine fuse processes as freezeable kernel 
threads.

Regards,

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

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

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

Re: [RFC][PATCH -mm] PM: Do not sync filesystems from within the freezer

[Miklos Szeredi]

> To get more serious and practical though, I think the solution is to
> fuzz the userspace/kernelspace distinction. What we really want to
> do is freeze things that submit I/O, then sync, then freeze anything
> that processes I/O and needs to be frozen. In effect, redefine fuse
> processes as freezeable kernel threads.

Another myth, that has been debunked already.  The problem is: how do
you define fuse processes?  There's no theoretical or even practial
way to do that.

Miklos

Re: [RFC][PATCH -mm] PM: Do not sync filesystems from within the freezer

[Nigel Cunningham]

[-- Attachment #1.1: Type: text/plain, Size: 837 bytes --]

Hi.

On Friday 06 July 2007 17:13:27 Miklos Szeredi wrote:
> > To get more serious and practical though, I think the solution is to
> > fuzz the userspace/kernelspace distinction. What we really want to
> > do is freeze things that submit I/O, then sync, then freeze anything
> > that processes I/O and needs to be frozen. In effect, redefine fuse
> > processes as freezeable kernel threads.
> 
> Another myth, that has been debunked already.  The problem is: how do
> you define fuse processes?  There's no theoretical or even practial
> way to do that.

No theoretical or practical way?! I'll freely admit to being quite ignorant 
about fuse, but surely there's some way by which they can be distinguished.

Regards,

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

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

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

Re: [RFC][PATCH -mm] PM: Do not sync filesystems from within the freezer

[Miklos Szeredi]

> > > To get more serious and practical though, I think the solution is to
> > > fuzz the userspace/kernelspace distinction. What we really want to
> > > do is freeze things that submit I/O, then sync, then freeze anything
> > > that processes I/O and needs to be frozen. In effect, redefine fuse
> > > processes as freezeable kernel threads.
> > 
> > Another myth, that has been debunked already.  The problem is: how do
> > you define fuse processes?  There's no theoretical or even practial
> > way to do that.
> 
> No theoretical or practical way?! I'll freely admit to being quite ignorant 
> about fuse, but surely there's some way by which they can be distinguished.

How?  OK, there are some tasks, that read and/or write /dev/fuse. And
there are some that just communicate in some way with the above.

These could all be considered "fuse tasks", but those that don't do
I/O on /dev/fuse are indistinguishable from non-fuse tasks.

And for example sshfs does have such a thread, which is in the reply
chain, yet never communicates directly with the fuse kernel module.

Miklos

possible solution for problem 2 (was Re: [RFC][PATCH -mm] PM: Do not sync filesystems from within the freezer)

[Pavel Machek]

Hi!

> > > > To get more serious and practical though, I think the solution is to
> > > > fuzz the userspace/kernelspace distinction. What we really want to
> > > > do is freeze things that submit I/O, then sync, then freeze anything
> > > > that processes I/O and needs to be frozen. In effect, redefine fuse
> > > > processes as freezeable kernel threads.
> > > 
> > > Another myth, that has been debunked already.  The problem is: how do
> > > you define fuse processes?  There's no theoretical or even practial
> > > way to do that.
> > 
> > No theoretical or practical way?! I'll freely admit to being quite ignorant 
> > about fuse, but surely there's some way by which they can be distinguished.
> 
> How?  OK, there are some tasks, that read and/or write /dev/fuse. And
> there are some that just communicate in some way with the above.

(Not that I'm advocating this, but:)

You could ask fused to identify tasks involved in fuse handling. "Hey,
fused, please give me list of PIDs".

Yes, that would be beyond ugly.

(I guess I'm advocating this:)

We probably can do variation on this. Notify fuse early that suspend
is comming, so we can wait for all the fused requests to be flushed
(/fusectl/*/waiting going to 0), and then just trap tasks trying to
communicate with fuse in a sane place (i.e. not a place where VFS
locks are held).

...I'm not saying this is a nice solution, but it should
work... right...? And should work for hibernation, too...?

(at this point, we still have problem 1: something in s2ram path
causes fuse to communicate with its frozen fused. It is not sync, so
what is it?)
									Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

Re: [RFC][PATCH -mm] PM: Do not sync filesystems from within the freezer

[Benjamin Herrenschmidt]

On Fri, 2007-07-06 at 09:13 +0200, Miklos Szeredi wrote:
> 
> Another myth, that has been debunked already.  The problem is: how do
> you define fuse processes?  There's no theoretical or even practial
> way to do that.

It could if they told the kernel via some black magic ...

But that still suck. The freezer sucks :-)

Ben.

Re: [RFC][PATCH -mm] PM: Do not sync filesystems from within the freezer

[Miklos Szeredi]

> On Fri, 2007-07-06 at 09:13 +0200, Miklos Szeredi wrote:
> > 
> > Another myth, that has been debunked already.  The problem is: how do
> > you define fuse processes?  There's no theoretical or even practial
> > way to do that.
> 
> It could if they told the kernel via some black magic ...
> 
> But that still suck. The freezer sucks :-)

Yeah, and it wouldn't work in practice, since the auxilary tasks might
be part of a library which is not even aware of being used by a "fuse
task".

Miklos

Re: [RFC][PATCH -mm] PM: Do not sync filesystems from within the freezer

[Nigel Cunningham]

[-- Attachment #1.1: Type: text/plain, Size: 1034 bytes --]

Hi.

On Friday 06 July 2007 19:05:53 Miklos Szeredi wrote:
> > On Fri, 2007-07-06 at 09:13 +0200, Miklos Szeredi wrote:
> > > 
> > > Another myth, that has been debunked already.  The problem is: how do
> > > you define fuse processes?  There's no theoretical or even practial
> > > way to do that.
> > 
> > It could if they told the kernel via some black magic ...
> > 
> > But that still suck. The freezer sucks :-)
> 
> Yeah, and it wouldn't work in practice, since the auxilary tasks might
> be part of a library which is not even aware of being used by a "fuse
> task".

This is why I think the whole concept of filesystems in userspace is broken. 
Trying to shift things that need special privilege and special handling to 
userspace is just asking for trouble. You can say it's the kernel code's 
fault, but then you have to explain why it's only fuse (yeah ok, and XFS) 
that have problems.

Regards,

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

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

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

Re: [RFC][PATCH -mm] PM: Do not sync filesystems from within the freezer

[Miklos Szeredi]

> > > On Fri, 2007-07-06 at 09:13 +0200, Miklos Szeredi wrote:
> > > > 
> > > > Another myth, that has been debunked already.  The problem is: how do
> > > > you define fuse processes?  There's no theoretical or even practial
> > > > way to do that.
> > > 
> > > It could if they told the kernel via some black magic ...
> > > 
> > > But that still suck. The freezer sucks :-)
> > 
> > Yeah, and it wouldn't work in practice, since the auxilary tasks might
> > be part of a library which is not even aware of being used by a "fuse
> > task".
> 
> This is why I think the whole concept of filesystems in userspace is broken. 
> Trying to shift things that need special privilege and special handling to 
> userspace is just asking for trouble.

I'm not claiming fuse doesn't introduce problems.  I've been tackling
those problems for a number of years now.  Suspend/hibernate is just
the next thing that needs looking at.

Just blaming either suspend or fuse for these problems is unproductive
and stupid.  The problems _are_ solvable if not always simply.

> You can say it's the kernel code's fault, but then you have to
> explain why it's only fuse (yeah ok, and XFS) that have problems.

I'm not faulting anythig.  My opinion, is that the freezer is not
needed for suspend, and removing it will not just solve the problems
with fuse but several others.  It probably needs a lot of work, but
hey, that's why we are here.

The patch posted by Rafael, which lets the freezer skip uninterrupible
tasks is a step in the right direction.

Miklos

Re: [RFC][PATCH -mm] PM: Do not sync filesystems from within the freezer

[Rafael J. Wysocki]

On Friday, 6 July 2007 11:31, Miklos Szeredi wrote:
> > > > On Fri, 2007-07-06 at 09:13 +0200, Miklos Szeredi wrote:
> > > > > 
> > > > > Another myth, that has been debunked already.  The problem is: how do
> > > > > you define fuse processes?  There's no theoretical or even practial
> > > > > way to do that.
> > > > 
> > > > It could if they told the kernel via some black magic ...
> > > > 
> > > > But that still suck. The freezer sucks :-)
> > > 
> > > Yeah, and it wouldn't work in practice, since the auxilary tasks might
> > > be part of a library which is not even aware of being used by a "fuse
> > > task".
> > 
> > This is why I think the whole concept of filesystems in userspace is broken. 
> > Trying to shift things that need special privilege and special handling to 
> > userspace is just asking for trouble.
> 
> I'm not claiming fuse doesn't introduce problems.  I've been tackling
> those problems for a number of years now.  Suspend/hibernate is just
> the next thing that needs looking at.
> 
> Just blaming either suspend or fuse for these problems is unproductive
> and stupid.

Well, the "stupid" part is a bit offensive, don't you think?

> The problems _are_ solvable if not always simply. 

Agreed.

> > You can say it's the kernel code's fault, but then you have to
> > explain why it's only fuse (yeah ok, and XFS) that have problems.
> 
> I'm not faulting anythig.  My opinion, is that the freezer is not
> needed for suspend, and removing it will not just solve the problems
> with fuse but several others.  It probably needs a lot of work, but
> hey, that's why we are here.
> 
> The patch posted by Rafael, which lets the freezer skip uninterrupible
> tasks is a step in the right direction.

Thanks.

BTW, I think that this is just less radical than the change proposed by Matthew
and it should address the underlying problems.

Greetings,
Rafael


-- 
"Premature optimization is the root of all evil." - Donald Knuth