: do not subtract NR_FILE_MAPPED in minimum_image_size()

Commit Message

Rainer Fiebig Dec. 22, 2017, 10:13 a.m. UTC

Hi!

s2disk/s2both may fail unnecessarily and erratically if NR_FILE_MAPPED
is high - for instance when using VMs with VirtualBox and perhaps VMware
Player. In those situations s2disk becomes unreliable and therefore
unusable.

A typical scenario is: user issues a s2disk and it fails. User issues a
second s2disk immediately after that and it succeeds. And user wonders why.

The problem is caused by minimum_image_size() in snapshot.c. The value
it returns is roughly 100 % too high because NR_FILE_MAPPED is
subtracted in its calculation which is imo wrong. Eventually the number
of preallocated image pages is falsely too low.

This doesn't matter as long as NR_FILE_MAPPED-values are in a normal
range or in 32bit-environments as the code allows for allocation of
additional pages from highmem.

But with the high values generated by VirtualBox-VMs (a 2-GB-VM causes
NR_FILE_MAPPED go up by 2 GB) it may lead to failure in 64bit-systems.

Not subtracting NR_FILE_MAPPED in minimum_image_size() solves the problem.

I've done at least hundreds of successful s2both/s2disk now on an x86_64
system (with and without VirtualBox) which gives me some confidence that
this is right. It has turned s2disk/s2both from unusable into 100% reliable.

As I don't have 32bit-equipment, I could not test it in such
environment, though.

The problem has also been discussed in a bug-thread which I closed
myself because it obviously led to nowhere
(https://bugzilla.kernel.org/show_bug.cgi?id=97201).

Thoughts?

Rainer Fiebig

Comments

Rafael J. Wysocki Jan. 5, 2018, 1:50 p.m. UTC | #1

On Friday, December 22, 2017 11:13:59 AM CET Rainer Fiebig wrote:
> Hi!
> 
> s2disk/s2both may fail unnecessarily and erratically if NR_FILE_MAPPED
> is high - for instance when using VMs with VirtualBox and perhaps VMware
> Player. In those situations s2disk becomes unreliable and therefore
> unusable.
> 
> A typical scenario is: user issues a s2disk and it fails. User issues a
> second s2disk immediately after that and it succeeds. And user wonders why.
> 
> The problem is caused by minimum_image_size() in snapshot.c. The value
> it returns is roughly 100 % too high because NR_FILE_MAPPED is
> subtracted in its calculation which is imo wrong. Eventually the number
> of preallocated image pages is falsely too low.
> 
> This doesn't matter as long as NR_FILE_MAPPED-values are in a normal
> range or in 32bit-environments as the code allows for allocation of
> additional pages from highmem.
> 
> But with the high values generated by VirtualBox-VMs (a 2-GB-VM causes
> NR_FILE_MAPPED go up by 2 GB) it may lead to failure in 64bit-systems.
> 
> Not subtracting NR_FILE_MAPPED in minimum_image_size() solves the problem.
> 
> I've done at least hundreds of successful s2both/s2disk now on an x86_64
> system (with and without VirtualBox) which gives me some confidence that
> this is right. It has turned s2disk/s2both from unusable into 100% reliable.
> 
> As I don't have 32bit-equipment, I could not test it in such
> environment, though.
> 
> The problem has also been discussed in a bug-thread which I closed
> myself because it obviously led to nowhere
> (https://bugzilla.kernel.org/show_bug.cgi?id=97201).
> 
> Thoughts?

Technically, it makes sense to me.

The patch is missing a Signed-off-by: tag which strictly speaking is
necessary for it to be applied, but I have added one for you speculatively.
I need you to confirm that this is correct in accordance with the
the Developer's Certificate of Origin section in
Documentation/process/submitting-patches.rst, however.

Also your e-mail client breaks lines in patches, so I needed to fix that up,
but then it applied for me and I'm going to tentatively queue it up for 4.16.

Thanks,
Rafael


> ___
> 
> --- /kernel/power/snapshot.c
> +++ /kernel/power/snapshot.c
> @@ -1641,8 +1641,7 @@
>   * [number of saveable pages] - [number of pages that can be freed in
> theory]
>   *
>   * where the second term is the sum of (1) reclaimable slab pages, (2)
> active
> - * and (3) inactive anonymous pages, (4) active and (5) inactive file
> pages,
> - * minus mapped file pages.
> + * and (3) inactive anonymous pages, (4) active and (5) inactive file
> pages.
>   */
>  static unsigned long minimum_image_size(unsigned long saveable)
>  {
> @@ -1652,8 +1651,7 @@
>  		+ global_node_page_state(NR_ACTIVE_ANON)
>  		+ global_node_page_state(NR_INACTIVE_ANON)
>  		+ global_node_page_state(NR_ACTIVE_FILE)
> -		+ global_node_page_state(NR_INACTIVE_FILE)
> -		- global_node_page_state(NR_FILE_MAPPED);
> +		+ global_node_page_state(NR_INACTIVE_FILE);
> 
>  	return saveable <= size ? 0 : saveable - size;
>  }
> 
> 
>

Rainer Fiebig Jan. 5, 2018, 5:26 p.m. UTC | #2

Rafael J. Wysocki wrote:
> On Friday, December 22, 2017 11:13:59 AM CET Rainer Fiebig wrote:
>> Hi!
>>
>> s2disk/s2both may fail unnecessarily and erratically if NR_FILE_MAPPED
>> is high - for instance when using VMs with VirtualBox and perhaps VMware
>> Player. In those situations s2disk becomes unreliable and therefore
>> unusable.
>>
>> A typical scenario is: user issues a s2disk and it fails. User issues a
>> second s2disk immediately after that and it succeeds. And user wonders why.
>>
>> The problem is caused by minimum_image_size() in snapshot.c. The value
>> it returns is roughly 100 % too high because NR_FILE_MAPPED is
>> subtracted in its calculation which is imo wrong. Eventually the number
>> of preallocated image pages is falsely too low.
>>
>> This doesn't matter as long as NR_FILE_MAPPED-values are in a normal
>> range or in 32bit-environments as the code allows for allocation of
>> additional pages from highmem.
>>
>> But with the high values generated by VirtualBox-VMs (a 2-GB-VM causes
>> NR_FILE_MAPPED go up by 2 GB) it may lead to failure in 64bit-systems.
>>
>> Not subtracting NR_FILE_MAPPED in minimum_image_size() solves the problem.
>>
>> I've done at least hundreds of successful s2both/s2disk now on an x86_64
>> system (with and without VirtualBox) which gives me some confidence that
>> this is right. It has turned s2disk/s2both from unusable into 100% reliable.
>>
>> As I don't have 32bit-equipment, I could not test it in such
>> environment, though.
>>
>> The problem has also been discussed in a bug-thread which I closed
>> myself because it obviously led to nowhere
>> (https://bugzilla.kernel.org/show_bug.cgi?id=97201).
>>
>> Thoughts?
> 
> Technically, it makes sense to me.
> 
> The patch is missing a Signed-off-by: tag which strictly speaking is
> necessary for it to be applied, but I have added one for you speculatively.
> I need you to confirm that this is correct in accordance with the
> the Developer's Certificate of Origin section in
> Documentation/process/submitting-patches.rst, however.
> 

Very kind of you, thanks. I do confirm that the patch submitted by me is
in accordance with the a/m Developer's Certificate of Origin.

> Also your e-mail client breaks lines in patches, so I needed to fix that up,
Sorry for that!
> but then it applied for me and I'm going to tentatively queue it up for 4.16.
> 
Sounds great!
> Thanks,
> Rafael
> 
Thank you for your time and effort!
Rainer Fiebig
> 
>> ___
>>
>> --- /kernel/power/snapshot.c
>> +++ /kernel/power/snapshot.c
>> @@ -1641,8 +1641,7 @@
>>   * [number of saveable pages] - [number of pages that can be freed in
>> theory]
>>   *
>>   * where the second term is the sum of (1) reclaimable slab pages, (2)
>> active
>> - * and (3) inactive anonymous pages, (4) active and (5) inactive file
>> pages,
>> - * minus mapped file pages.
>> + * and (3) inactive anonymous pages, (4) active and (5) inactive file
>> pages.
>>   */
>>  static unsigned long minimum_image_size(unsigned long saveable)
>>  {
>> @@ -1652,8 +1651,7 @@
>>  		+ global_node_page_state(NR_ACTIVE_ANON)
>>  		+ global_node_page_state(NR_INACTIVE_ANON)
>>  		+ global_node_page_state(NR_ACTIVE_FILE)
>> -		+ global_node_page_state(NR_INACTIVE_FILE)
>> -		- global_node_page_state(NR_FILE_MAPPED);
>> +		+ global_node_page_state(NR_INACTIVE_FILE);
>>
>>  	return saveable <= size ? 0 : saveable - size;
>>  }
>>
>>
>>
> 
>

Rafael J. Wysocki Jan. 7, 2018, 11:41 p.m. UTC | #3

On Friday, January 5, 2018 6:26:25 PM CET Rainer Fiebig wrote:
>  This is an OpenPGP/MIME signed message (RFC 4880 and 3156)
> --mE0npTCo0y6m1aIg1zoUzXqdMqby7xbmy
> Content-Type: multipart/mixed; boundary="f6rtsVlzN2rzdl3eJl68BRUHIV7FJFIDu";
>  protected-headers="v1"
> From: Rainer Fiebig <[e-mail]>
> To: "Rafael J. Wysocki" <[e-mail]>
> Cc: [e-mail], Thorsten Leemhuis <[e-mail]>
> Message-ID: <[e-mail]>
> Subject: Re: [patch]: do not subtract NR_FILE_MAPPED in minimum_image_size()
> References: <[e-mail]>
>  <[e-mail]>
> In-Reply-To: <[e-mail]>
> 
> --f6rtsVlzN2rzdl3eJl68BRUHIV7FJFIDu
> Content-Type: text/plain; charset=UTF-8
> Content-Transfer-Encoding: quoted-printable
> 
> Rafael J. Wysocki wrote:
> > On Friday, December 22, 2017 11:13:59 AM CET Rainer Fiebig wrote:
> >> Hi!
> >>
> >> s2disk/s2both may fail unnecessarily and erratically if NR_FILE_MAPPED=
> 
> >> is high - for instance when using VMs with VirtualBox and perhaps VMwa=
> re
> >> Player. In those situations s2disk becomes unreliable and therefore
> >> unusable.
> >>
> >> A typical scenario is: user issues a s2disk and it fails. User issues =
> a
> >> second s2disk immediately after that and it succeeds. And user wonders=
>  why.
> >>
> >> The problem is caused by minimum_image_size() in snapshot.c. The value=
> 
> >> it returns is roughly 100 % too high because NR_FILE_MAPPED is
> >> subtracted in its calculation which is imo wrong. Eventually the numbe=
> r
> >> of preallocated image pages is falsely too low.
> >>
> >> This doesn't matter as long as NR_FILE_MAPPED-values are in a normal
> >> range or in 32bit-environments as the code allows for allocation of
> >> additional pages from highmem.
> >>
> >> But with the high values generated by VirtualBox-VMs (a 2-GB-VM causes=
> 
> >> NR_FILE_MAPPED go up by 2 GB) it may lead to failure in 64bit-systems.=
> 
> >>
> >> Not subtracting NR_FILE_MAPPED in minimum_image_size() solves the prob=
> lem.
> >>
> >> I've done at least hundreds of successful s2both/s2disk now on an x86_=
> 64
> >> system (with and without VirtualBox) which gives me some confidence th=
> at
> >> this is right. It has turned s2disk/s2both from unusable into 100% rel=
> iable.
> >>
> >> As I don't have 32bit-equipment, I could not test it in such
> >> environment, though.
> >>
> >> The problem has also been discussed in a bug-thread which I closed
> >> myself because it obviously led to nowhere
> >> (https://bugzilla.kernel.org/show_bug.cgi?id=3D97201).
> >>
> >> Thoughts?
> >=20
> > Technically, it makes sense to me.
> >=20
> > The patch is missing a Signed-off-by: tag which strictly speaking is
> > necessary for it to be applied, but I have added one for you speculativ=
> ely.
> > I need you to confirm that this is correct in accordance with the
> > the Developer's Certificate of Origin section in
> > Documentation/process/submitting-patches.rst, however.
> >=20
> 
> Very kind of you, thanks. I do confirm that the patch submitted by me is
> in accordance with the a/m Developer's Certificate of Origin.

OK, thanks!

Applied to my linux-next branch now.

Thanks,
Rafael

Rainer Fiebig Feb. 16, 2018, 4:57 p.m. UTC | #4

Rafael J. Wysocki wrote:
> On Friday, January 5, 2018 6:26:25 PM CET Rainer Fiebig wrote:
>>  This is an OpenPGP/MIME signed message (RFC 4880 and 3156)
>> --mE0npTCo0y6m1aIg1zoUzXqdMqby7xbmy
>> Content-Type: multipart/mixed; boundary="f6rtsVlzN2rzdl3eJl68BRUHIV7FJFIDu";
>>  protected-headers="v1"
>> From: Rainer Fiebig <[e-mail]>
>> To: "Rafael J. Wysocki" <[e-mail]>
>> Cc: [e-mail], Thorsten Leemhuis <[e-mail]>
>> Message-ID: <[e-mail]>
>> Subject: Re: [patch]: do not subtract NR_FILE_MAPPED in minimum_image_size()
>> References: <[e-mail]>
>>  <[e-mail]>
>> In-Reply-To: <[e-mail]>
>>
>> --f6rtsVlzN2rzdl3eJl68BRUHIV7FJFIDu
>> Content-Type: text/plain; charset=UTF-8
>> Content-Transfer-Encoding: quoted-printable
>>
>> Rafael J. Wysocki wrote:
>>> On Friday, December 22, 2017 11:13:59 AM CET Rainer Fiebig wrote:
>>>> Hi!
>>>>
>>>> s2disk/s2both may fail unnecessarily and erratically if NR_FILE_MAPPED=
>>
>>>> is high - for instance when using VMs with VirtualBox and perhaps VMwa=
>> re
>>>> Player. In those situations s2disk becomes unreliable and therefore
>>>> unusable.
>>>>
>>>> A typical scenario is: user issues a s2disk and it fails. User issues =
>> a
>>>> second s2disk immediately after that and it succeeds. And user wonders=
>>  why.
>>>>
>>>> The problem is caused by minimum_image_size() in snapshot.c. The value=
>>
>>>> it returns is roughly 100 % too high because NR_FILE_MAPPED is
>>>> subtracted in its calculation which is imo wrong. Eventually the numbe=
>> r
>>>> of preallocated image pages is falsely too low.
>>>>
>>>> This doesn't matter as long as NR_FILE_MAPPED-values are in a normal
>>>> range or in 32bit-environments as the code allows for allocation of
>>>> additional pages from highmem.
>>>>
>>>> But with the high values generated by VirtualBox-VMs (a 2-GB-VM causes=
>>
>>>> NR_FILE_MAPPED go up by 2 GB) it may lead to failure in 64bit-systems.=
>>
>>>>
>>>> Not subtracting NR_FILE_MAPPED in minimum_image_size() solves the prob=
>> lem.
>>>>
>>>> I've done at least hundreds of successful s2both/s2disk now on an x86_=
>> 64
>>>> system (with and without VirtualBox) which gives me some confidence th=
>> at
>>>> this is right. It has turned s2disk/s2both from unusable into 100% rel=
>> iable.
>>>>
>>>> As I don't have 32bit-equipment, I could not test it in such
>>>> environment, though.
>>>>
>>>> The problem has also been discussed in a bug-thread which I closed
>>>> myself because it obviously led to nowhere
>>>> (https://bugzilla.kernel.org/show_bug.cgi?id=3D97201).
>>>>
>>>> Thoughts?
>>> =20
>>> Technically, it makes sense to me.
>>> =20
>>> The patch is missing a Signed-off-by: tag which strictly speaking is
>>> necessary for it to be applied, but I have added one for you speculativ=
>> ely.
>>> I need you to confirm that this is correct in accordance with the
>>> the Developer's Certificate of Origin section in
>>> Documentation/process/submitting-patches.rst, however.
>>> =20
>>
>> Very kind of you, thanks. I do confirm that the patch submitted by me is
>> in accordance with the a/m Developer's Certificate of Origin.
> 
> OK, thanks!
> 
> Applied to my linux-next branch now.
> 
> Thanks,
> Rafael
> 

Just tried 4.16.0-rc1 (gcc-7.3.0).
Fortunately, the VirtualBox kernel-modules compiled (quite often not the case with new kernel-versions),
so I could test it with high nr_mapped.

Works as expected, all else nice so far.

Thanks and so long!

Rainer Fiebig

Patch

--- /kernel/power/snapshot.c
+++ /kernel/power/snapshot.c
@@ -1641,8 +1641,7 @@ 
  * [number of saveable pages] - [number of pages that can be freed in
theory]
  *
  * where the second term is the sum of (1) reclaimable slab pages, (2)
active
- * and (3) inactive anonymous pages, (4) active and (5) inactive file
pages,
- * minus mapped file pages.
+ * and (3) inactive anonymous pages, (4) active and (5) inactive file