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

[Jonas Meurer <jonas@freesources.org>]

Hi Rafael and linux-pm maintainers,

sorry for the noise, but again: is there a chance to get a brief review
of my patchset?

Probably it was a bad idea to rename the build-time flag, right? Should
I revert that part of the patch?

Cheers,
 jonas

Jonas Meurer:
> 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
> 
>