Re: [RFC PATCH] PM: Add a switch for disabling/enabling sync() before suspend

From: Jonas Meurer <jonas@freesources.org>
To: "Rafael J. Wysocki" <rjw@rjwysocki.net>
Cc: linux-pm@vger.kernel.org, Pavel Machek <pavel@ucw.cz>,
	Len Brown <len.brown@intel.com>,
	Tim Dittler <tim.dittler@systemli.org>
Subject: Re: [RFC PATCH] PM: Add a switch for disabling/enabling sync() before suspend
Date: Mon, 14 Oct 2019 19:46:14 +0200	[thread overview]
Message-ID: <063b2b9e-19f1-e67a-1d54-b1a813364bb8@freesources.org> (raw)
In-Reply-To: <2847488.TR0R5COpHM@kreacher>

Hi Rafael and linux-pm maintainers,

thanks a lot for your feedback, it's much appreciated!

Rafael J. Wysocki:
>> This patch adds a run-time switch at `/sys/power/suspend_sync`.
> 
> I'd prefer "sync_on_suspend".

Agreed and changed.

>> The switch allows to enable or disable the final sync() from the suspend.=
>> c
>> Linux Kernel system suspend implementation. This is useful to avoid race
>> conditions if block devices have been suspended before. Be aware that you=
>>
>> have to take care of sync() yourself before suspending the system if you
>> disable it here.
>>
>> Since this is my first patch against the Linux kernel and I don't
>> consider it ready for inclusion yet, I decided to send it to pm-linux
>> and the PM subsystem maintainers only first. Would be very glad if you
>> could take a look and comment on it :)
>>
>> Some questions:
>>
>> * There already is a build-time config flag[2] for en- or disabling the
>>   sync() in suspend.c. Is it acceptable to have both a build-time *and*
>>   a *run-time* switch? Or would a run-time switch have to replace the
>>   build-time switch? If so, a direct question to Rafael, as you added
>>   the build-time flag: Would that be ok for you?
> 
> If there is a run-time knob to disable the syncing, the only reason for
> the config option to be there will be to set the default value of that.

Makes sense. I changed the meaning of the build-time flag accordingly.

>> To give a bit more contect: In Debian, we're currently working[3] on
>> support to suspend unlocked dm-crypt devices before system suspend.
>> During that work, we realized that the final sync() from Linux Kernel
>> system suspend implementation can lead to a dead lock.
> 
> That's also true for FUSE filesystems I think and please note that this isn't
> going to work with hibernation (in which case filesystems are synced
> regardless).

In my opinion, hibernation doesn't matter much. Since the memory is
powered off on hibernation anyway, there's no reason to luksSuspend the
devices beforehands, don't you think so?

> The changes look reasonable to me.

Glad to read. I'll send a second version of the patches as replies soon
after this mail. How do we go on from here? Could you imagine to review
them again and sign them afterwards? Or am I supposed to send them to
the lkml and wait for feedback before getting more signers? As written,
I'm new to the Linux Kernel development process and not sure what's the
logical next step to get the patches merged.

Thanks again for your help!

Cheers
 jonas