PM: hibernate: Drain trailing zero pages on userspace restore
Subsystems (2)
v2 cover PM: hibernate: Drain trailing zero pages on userspace restore
Hi,
Here's v2 of the patch.
Changes from v1:
- The extra snapshot_write_next() call happens now at the beginning of snapshot_write_finalize(), but only when handle->cur is in the data page region.
- We return -ENODATA if snapshot_write_next() returns > 0 (indicating that the kernel was expecting another page).
- A second patch changes the userspace restore code to return -ENODATA when snapshot_image_loaded() fails.
Regards,
Berto
Alberto Garcia (2):
PM: hibernate: Drain trailing zero pages on userspace restore
PM: hibernate: return -ENODATA if the snapshot image is not loaded
kernel/power/snapshot.c | 11 +++++++++++ kernel/power/user.c | 7 +++++-- 2 files changed, 16 insertions(+), 2 deletions(-)
Signature · 1 line
2.47.3
PATCH v2 1/2 PM: hibernate: Drain trailing zero pages on userspace restore
Commit 005e8dddd497 ("PM: hibernate: don't store zero pages in the image file") added an optimization to skip zero-filled pages in the hibernation image. On restore, zero pages are handled internally by snapshot_write_next() in a loop that processes them without returning to the caller.
With the userspace restore interface, writing the last non-zero page to /dev/snapshot is followed by the SNAPSHOT_ATOMIC_RESTORE ioctl. At this point there are no more calls to snapshot_write_next() so any trailing zero pages are not processed, snapshot_image_loaded() fails because handle->cur is smaller than expected, the ioctl returns -EPERM and the image is not restored.
The in-kernel restore path is not affected by this because the loop in load_image() in swap.c calls snapshot_write_next() until it returns 0. It is this final call that drains any trailing zero pages.
Fixed by calling snapshot_write_next() in snapshot_write_finalize(), giving the kernel the chance to drain any trailing zero pages.
--- kernel/power/snapshot.c | 11 +++++++++++ 1 file changed, 11 insertions(+)
kernel/power/snapshot.c +11 -0
@@ -2855,6 +2855,17 @@ int snapshot_write_finalize(struct snapshot_handle *handle) {int error; +/* +* Call snapshot_write_next() to drain any trailing zero pages, +* but make sure we're in the data page region first. +* This function can return PAGE_SIZE if the kernel was expecting +* another copy page. Return -ENODATA in that situation. +*/ +if (handle->cur > nr_meta_pages +1 ) { + error =snapshot_write_next (handle); +if (error) +return error >0 ? -ENODATA : error; + }copy_last_highmem_page (); error =hibernate_restore_protect_page (handle->buffer);/* Do that only if we have loaded the image entirely */
Signature · 1 line
2.47.3
PATCH v2 2/2 PM: hibernate: return -ENODATA if the snapshot image is not loaded
In reply to Alberto Garciasnapshot_image_loaded() is used in both the in-kernel and the userspace restore path to ensure that the snapshot image has been completely loaded. However the latter path returns -EPERM in such situations, which is meant for cases where the operation is neither write-only nor ready.
This patch updates the check so the returned error code is -ENODATA in both cases.
--- kernel/power/user.c | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-)
kernel/power/user.c +5 -2
@@ -322,11 +322,14 @@ static long snapshot_ioctl(struct file *filp, unsigned int cmd, error =snapshot_write_finalize (&data->handle);if (error)break ; -if (data->mode !=O_WRONLY || !data->frozen || - !snapshot_image_loaded (&data->handle)) { +if (data->mode !=O_WRONLY || !data->frozen) { error = -EPERM ;break ; } +if (!snapshot_image_loaded (&data->handle)) { + error = -ENODATA ; +break ; + } error =hibernation_restore (data->platform_support);break ;
Signature · 1 line
2.47.3
Quoted 18 lines · Alberto Garcia
Commit
005e8dddd497("PM: hibernate: don't store zero pages in the image file") added an optimization to skip zero-filled pages in the hibernation image. On restore, zero pages are handled internally bysnapshot_write_next()in a loop that processes them without returning to the caller.With the userspace restore interface, writing the last non-zero page to
/dev/snapshotis followed by theSNAPSHOT_ATOMIC_RESTOREioctl. At this point there are no more calls tosnapshot_write_next()so any trailing zero pages are not processed,snapshot_image_loaded()fails becausehandle->curis smaller than expected, the ioctl returns-EPERMand the image is not restored.The in-kernel restore path is not affected by this because the loop in
load_image()inswap.ccallssnapshot_write_next()until it returns 0. It is this final call that drains any trailing zero pages.
Quoted 5 lines
Fixed by calling
snapshot_write_next()insnapshot_write_finalize(), giving the kernel the chance to drain any trailing zero pages.Fixes 005e8dddd497 ("PM: hibernate: don't store zero pages in the image file")Signed-off-by Alberto Garcia <[email protected]>
Quoted 29 lines
--- kernel/power/snapshot.c | 11 +++++++++++ 1 file changed, 11 insertions(+)kernel/power/snapshot.c +11
-1@@ -2855,6 +2855,17 @@ int snapshot_write_finalize(struct snapshot_handle *handle) {int error; +/* +* Call snapshot_write_next() to drain any trailing zero pages, +* but make sure we're in the data page region first. +* This function can return PAGE_SIZE if the kernel was expecting +* another copy page. Return -ENODATA in that situation. +*/ +if (handle->cur > nr_meta_pages +1 ) { + error =snapshot_write_next (handle); +if (error) +return error >0 ? -ENODATA : error; + }copy_last_highmem_page (); error =hibernate_restore_protect_page (handle->buffer);/* Do that only if we have loaded the image entirely */ --2.47.3
Re: [PATCH v2 2/2] PM: hibernate: return -ENODATA if the snapshot image is not loaded
In reply to Alberto GarciaQuoted 7 lines · Alberto Garcia
snapshot_image_loaded()is used in both the in-kernel and the userspace restore path to ensure that the snapshot image has been completely loaded. However the latter path returns-EPERMin such situations, which is meant for cases where the operation is neither write-only nor ready.
Quoted 5 lines
This patch updates the check so the returned error code is
-ENODATAin both cases.Suggested-by Brian Geffon <[email protected]>Signed-off-by Alberto Garcia <[email protected]>
Quoted 28 lines
--- kernel/power/user.c | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-)kernel/power/user.c +5
-3@@ -322,11 +322,14 @@ static long snapshot_ioctl(struct file *filp, unsigned int cmd, error =snapshot_write_finalize (&data->handle);if (error)break ; -if (data->mode !=O_WRONLY || !data->frozen || - !snapshot_image_loaded (&data->handle)) { +if (data->mode !=O_WRONLY || !data->frozen) { error = -EPERM ;break ; } +if (!snapshot_image_loaded (&data->handle)) { + error = -ENODATA ; +break ; + } error =hibernation_restore (data->platform_support);break ; --2.47.3
Quoted 20 lines · Brian Geffon
Quoted 18 lines · Alberto Garcia
Commit
005e8dddd497("PM: hibernate: don't store zero pages in the image file") added an optimization to skip zero-filled pages in the hibernation image. On restore, zero pages are handled internally bysnapshot_write_next()in a loop that processes them without returning to the caller.With the userspace restore interface, writing the last non-zero page to
/dev/snapshotis followed by theSNAPSHOT_ATOMIC_RESTOREioctl. At this point there are no more calls tosnapshot_write_next()so any trailing zero pages are not processed,snapshot_image_loaded()fails becausehandle->curis smaller than expected, the ioctl returns-EPERMand the image is not restored.The in-kernel restore path is not affected by this because the loop in
load_image()inswap.ccallssnapshot_write_next()until it returns 0. It is this final call that drains any trailing zero pages.
Quoted 7 lines
Quoted 5 lines
Fixed by calling
snapshot_write_next()insnapshot_write_finalize(), giving the kernel the chance to drain any trailing zero pages.Fixes 005e8dddd497 ("PM: hibernate: don't store zero pages in the image file")Signed-off-by Alberto Garcia <[email protected]>Acked-by Brian Geffon <[email protected]>
Applied as 7.0-rc material, thanks!
Quoted 29 lines
Quoted 29 lines
--- kernel/power/snapshot.c | 11 +++++++++++ 1 file changed, 11 insertions(+)kernel/power/snapshot.c +11
-1@@ -2855,6 +2855,17 @@ int snapshot_write_finalize(struct snapshot_handle *handle) {int error; +/* +* Call snapshot_write_next() to drain any trailing zero pages, +* but make sure we're in the data page region first. +* This function can return PAGE_SIZE if the kernel was expecting +* another copy page. Return -ENODATA in that situation. +*/ +if (handle->cur > nr_meta_pages +1 ) { + error =snapshot_write_next (handle); +if (error) +return error >0 ? -ENODATA : error; + }copy_last_highmem_page (); error =hibernate_restore_protect_page (handle->buffer);/* Do that only if we have loaded the image entirely */ --2.47.3
Re: [PATCH v2 2/2] PM: hibernate: return -ENODATA if the snapshot image is not loaded
In reply to Brian GeffonQuoted 9 lines · Brian Geffon
Quoted 7 lines · Alberto Garcia
snapshot_image_loaded()is used in both the in-kernel and the userspace restore path to ensure that the snapshot image has been completely loaded. However the latter path returns-EPERMin such situations, which is meant for cases where the operation is neither write-only nor ready.
Quoted 7 lines
Quoted 5 lines
This patch updates the check so the returned error code is
-ENODATAin both cases.Suggested-by Brian Geffon <[email protected]>Signed-off-by Alberto Garcia <[email protected]>Acked-by Brian Geffon <[email protected]>
Applied as 7.1 material, thanks!
Quoted 28 lines
Quoted 28 lines
--- kernel/power/user.c | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-)kernel/power/user.c +5
-3@@ -322,11 +322,14 @@ static long snapshot_ioctl(struct file *filp, unsigned int cmd, error =snapshot_write_finalize (&data->handle);if (error)break ; -if (data->mode !=O_WRONLY || !data->frozen || - !snapshot_image_loaded (&data->handle)) { +if (data->mode !=O_WRONLY || !data->frozen) { error = -EPERM ;break ; } +if (!snapshot_image_loaded (&data->handle)) { + error = -ENODATA ; +break ; + } error =hibernation_restore (data->platform_support);break ; --2.47.3