2.6.20 regression: suspend to disk no more works

2.6.20 regression: suspend to disk no more works

[Andrey Borzenkov]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

In *the same* configuration STD now fails with "Cannot find swap device". The 
reason is changes in kernel/power/swap.c. In 2.6.19 it did not require valid 
swsusp_resume_device at all - it took first available swap device and saved 
image. Later during resume swsusp_resume_device was set either by command 
line or sysfs and everything worked nicely.

Now swsusp_swap_check() unfortunately checks for swsusp_resume_device at 
*suspend* time:

        res = swap_type_of(swsusp_resume_device, swsusp_resume_block);
        if (res < 0)
                return res;

        root_swap = res;
        resume_bdev = open_by_devnum(swsusp_resume_device, FMODE_WRITE);
        if (IS_ERR(resume_bdev))
                return PTR_ERR(resume_bdev);

but in case of modular driver for swap device this is likely to be undefined. 
This is as of 2.6.20-rc3.

I already have seen these reports. While 'echo a:b > /sys/power/resume' before 
suspend is a workaround, this still breaks perfectly valid setup that worked 
before. Also 'echo a:b > /sys/power/resume' is actually wrong - we are not 
going to resume at this point; but there is no way to just tell kernel "use 
this device for next STD" ... also the error message is misleading, it should 
complaint "no resume device found". Swap is there all right.

- -andrey
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.5 (GNU/Linux)

iD8DBQFFmWSqR6LMutpd94wRAu61AJ9mdbF4W6RNQ848PU0e4n/3MKtNnwCgzwOh
HkZ9e8lHPMH1tRNzCw/2O58=
=HlW4
-----END PGP SIGNATURE-----

Re: 2.6.20 regression: suspend to disk no more works

[Rafael J. Wysocki]

Hi,

On Monday, 1 January 2007 20:44, Andrey Borzenkov wrote:
> In *the same* configuration STD now fails with "Cannot find swap device". The
> reason is changes in kernel/power/swap.c. In 2.6.19 it did not require valid
> swsusp_resume_device at all - it took first available swap device and saved
> image. Later during resume swsusp_resume_device was set either by command
> line or sysfs and everything worked nicely.
> 
> Now swsusp_swap_check() unfortunately checks for swsusp_resume_device at
> *suspend* time:
> 
>         res = swap_type_of(swsusp_resume_device, swsusp_resume_block);
>         if (res < 0)
>                 return res;
> 
>         root_swap = res;
>         resume_bdev = open_by_devnum(swsusp_resume_device, FMODE_WRITE);
>         if (IS_ERR(resume_bdev))
>                 return PTR_ERR(resume_bdev);
> 
> but in case of modular driver for swap device this is likely to be undefined.
> This is as of 2.6.20-rc3.

Actually, if you look at the 2.6.19 code, the call to swap_type_of() is there
in swsusp_swap_check() too.

The problem is with open_by_devnum(), I think, which obviously cannot succeed
if swsusp_resume_device is not set.  I think we should return resume_bdev from
swsusp_swap_check() like in the appended patch (untested).  Pavel?

> I already have seen these reports.

Yes, me too.

> While 'echo a:b > /sys/power/resume' before 
> suspend is a workaround, this still breaks perfectly valid setup that worked
> before. Also 'echo a:b > /sys/power/resume' is actually wrong - we are not
> going to resume at this point; but there is no way to just tell kernel "use
> this device for next STD" ... also the error message is misleading, it should
> complaint "no resume device found". Swap is there all right.

Thanks for the report.

Greetings,
Rafael


---
 include/linux/swap.h |    2 +-
 kernel/power/swap.c  |    9 +++++----
 kernel/power/user.c  |    7 ++++---
 mm/swapfile.c        |    8 +++++++-
 4 files changed, 17 insertions(+), 9 deletions(-)

Index: linux-2.6.20-rc3/include/linux/swap.h
===================================================================
--- linux-2.6.20-rc3.orig/include/linux/swap.h
+++ linux-2.6.20-rc3/include/linux/swap.h
@@ -245,7 +245,7 @@ extern int swap_duplicate(swp_entry_t);
 extern int valid_swaphandles(swp_entry_t, unsigned long *);
 extern void swap_free(swp_entry_t);
 extern void free_swap_and_cache(swp_entry_t);
-extern int swap_type_of(dev_t, sector_t);
+extern int swap_type_of(dev_t, sector_t, struct block_device **);
 extern unsigned int count_swap_pages(int, int);
 extern sector_t map_swap_page(struct swap_info_struct *, pgoff_t);
 extern sector_t swapdev_block(int, pgoff_t);
Index: linux-2.6.20-rc3/kernel/power/swap.c
===================================================================
--- linux-2.6.20-rc3.orig/kernel/power/swap.c
+++ linux-2.6.20-rc3/kernel/power/swap.c
@@ -165,14 +165,15 @@ static int swsusp_swap_check(void) /* Th
 {
 	int res;
 
-	res = swap_type_of(swsusp_resume_device, swsusp_resume_block);
+	res = swap_type_of(swsusp_resume_device, swsusp_resume_block,
+			&resume_bdev);
 	if (res < 0)
 		return res;
 
 	root_swap = res;
-	resume_bdev = open_by_devnum(swsusp_resume_device, FMODE_WRITE);
-	if (IS_ERR(resume_bdev))
-		return PTR_ERR(resume_bdev);
+	res = blkdev_get(resume_bdev, FMODE_WRITE, O_RDWR);
+	if (res)
+		return res;
 
 	res = set_blocksize(resume_bdev, PAGE_SIZE);
 	if (res < 0)
Index: linux-2.6.20-rc3/mm/swapfile.c
===================================================================
--- linux-2.6.20-rc3.orig/mm/swapfile.c
+++ linux-2.6.20-rc3/mm/swapfile.c
@@ -434,7 +434,7 @@ void free_swap_and_cache(swp_entry_t ent
  *
  * This is needed for the suspend to disk (aka swsusp).
  */
-int swap_type_of(dev_t device, sector_t offset)
+int swap_type_of(dev_t device, sector_t offset, struct block_device **bdev_p)
 {
 	struct block_device *bdev = NULL;
 	int i;
@@ -450,6 +450,9 @@ int swap_type_of(dev_t device, sector_t 
 			continue;
 
 		if (!bdev) {
+			if (bdev_p)
+				*bdev_p = sis->bdev;
+
 			spin_unlock(&swap_lock);
 			return i;
 		}
@@ -459,6 +462,9 @@ int swap_type_of(dev_t device, sector_t 
 			se = list_entry(sis->extent_list.next,
 					struct swap_extent, list);
 			if (se->start_block == offset) {
+				if (bdev_p)
+					*bdev_p = sis->bdev;
+
 				spin_unlock(&swap_lock);
 				bdput(bdev);
 				return i;
Index: linux-2.6.20-rc3/kernel/power/user.c
===================================================================
--- linux-2.6.20-rc3.orig/kernel/power/user.c
+++ linux-2.6.20-rc3/kernel/power/user.c
@@ -58,7 +58,7 @@ static int snapshot_open(struct inode *i
 	memset(&data->handle, 0, sizeof(struct snapshot_handle));
 	if ((filp->f_flags & O_ACCMODE) == O_RDONLY) {
 		data->swap = swsusp_resume_device ?
-				swap_type_of(swsusp_resume_device, 0) : -1;
+			swap_type_of(swsusp_resume_device, 0, NULL) : -1;
 		data->mode = O_RDONLY;
 	} else {
 		data->swap = -1;
@@ -327,7 +327,8 @@ static int snapshot_ioctl(struct inode *
 			 * so we need to recode them
 			 */
 			if (old_decode_dev(arg)) {
-				data->swap = swap_type_of(old_decode_dev(arg), 0);
+				data->swap = swap_type_of(old_decode_dev(arg),
+							0, NULL);
 				if (data->swap < 0)
 					error = -ENODEV;
 			} else {
@@ -427,7 +428,7 @@ static int snapshot_ioctl(struct inode *
 			swdev = old_decode_dev(swap_area.dev);
 			if (swdev) {
 				offset = swap_area.offset;
-				data->swap = swap_type_of(swdev, offset);
+				data->swap = swap_type_of(swdev, offset, NULL);
 				if (data->swap < 0)
 					error = -ENODEV;
 			} else {

Re: 2.6.20 regression: suspend to disk no more works

[Andrey Borzenkov]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

On Tuesday 02 January 2007 02:28, Rafael J. Wysocki wrote:
> Hi,
>
> On Monday, 1 January 2007 20:44, Andrey Borzenkov wrote:
> > In *the same* configuration STD now fails with "Cannot find swap device".
> > The reason is changes in kernel/power/swap.c. In 2.6.19 it did not
> > require valid swsusp_resume_device at all - it took first available swap
> > device and saved image. Later during resume swsusp_resume_device was set
> > either by command line or sysfs and everything worked nicely.
> >
> > Now swsusp_swap_check() unfortunately checks for swsusp_resume_device at
> > *suspend* time:
> >
> >         res = swap_type_of(swsusp_resume_device, swsusp_resume_block);
> >         if (res < 0)
> >                 return res;
> >
> >         root_swap = res;
> >         resume_bdev = open_by_devnum(swsusp_resume_device, FMODE_WRITE);
> >         if (IS_ERR(resume_bdev))
> >                 return PTR_ERR(resume_bdev);
> >
> > but in case of modular driver for swap device this is likely to be
> > undefined. This is as of 2.6.20-rc3.
>
> Actually, if you look at the 2.6.19 code, the call to swap_type_of() is
> there in swsusp_swap_check() too.
>
> The problem is with open_by_devnum(), I think, which obviously cannot
> succeed if swsusp_resume_device is not set.  I think we should return
> resume_bdev from swsusp_swap_check() like in the appended patch (untested).
>  Pavel?
>

this works, thank you

- -andrey

> > I already have seen these reports.
>
> Yes, me too.
>
> > While 'echo a:b > /sys/power/resume' before
> > suspend is a workaround, this still breaks perfectly valid setup that
> > worked before. Also 'echo a:b > /sys/power/resume' is actually wrong - we
> > are not going to resume at this point; but there is no way to just tell
> > kernel "use this device for next STD" ... also the error message is
> > misleading, it should complaint "no resume device found". Swap is there
> > all right.
>
> Thanks for the report.
>
> Greetings,
> Rafael
>
>
> ---
>  include/linux/swap.h |    2 +-
>  kernel/power/swap.c  |    9 +++++----
>  kernel/power/user.c  |    7 ++++---
>  mm/swapfile.c        |    8 +++++++-
>  4 files changed, 17 insertions(+), 9 deletions(-)
>
> Index: linux-2.6.20-rc3/include/linux/swap.h
> ===================================================================
> --- linux-2.6.20-rc3.orig/include/linux/swap.h
> +++ linux-2.6.20-rc3/include/linux/swap.h
> @@ -245,7 +245,7 @@ extern int swap_duplicate(swp_entry_t);
>  extern int valid_swaphandles(swp_entry_t, unsigned long *);
>  extern void swap_free(swp_entry_t);
>  extern void free_swap_and_cache(swp_entry_t);
> -extern int swap_type_of(dev_t, sector_t);
> +extern int swap_type_of(dev_t, sector_t, struct block_device **);
>  extern unsigned int count_swap_pages(int, int);
>  extern sector_t map_swap_page(struct swap_info_struct *, pgoff_t);
>  extern sector_t swapdev_block(int, pgoff_t);
> Index: linux-2.6.20-rc3/kernel/power/swap.c
> ===================================================================
> --- linux-2.6.20-rc3.orig/kernel/power/swap.c
> +++ linux-2.6.20-rc3/kernel/power/swap.c
> @@ -165,14 +165,15 @@ static int swsusp_swap_check(void) /* Th
>  {
>  	int res;
>
> -	res = swap_type_of(swsusp_resume_device, swsusp_resume_block);
> +	res = swap_type_of(swsusp_resume_device, swsusp_resume_block,
> +			&resume_bdev);
>  	if (res < 0)
>  		return res;
>
>  	root_swap = res;
> -	resume_bdev = open_by_devnum(swsusp_resume_device, FMODE_WRITE);
> -	if (IS_ERR(resume_bdev))
> -		return PTR_ERR(resume_bdev);
> +	res = blkdev_get(resume_bdev, FMODE_WRITE, O_RDWR);
> +	if (res)
> +		return res;
>
>  	res = set_blocksize(resume_bdev, PAGE_SIZE);
>  	if (res < 0)
> Index: linux-2.6.20-rc3/mm/swapfile.c
> ===================================================================
> --- linux-2.6.20-rc3.orig/mm/swapfile.c
> +++ linux-2.6.20-rc3/mm/swapfile.c
> @@ -434,7 +434,7 @@ void free_swap_and_cache(swp_entry_t ent
>   *
>   * This is needed for the suspend to disk (aka swsusp).
>   */
> -int swap_type_of(dev_t device, sector_t offset)
> +int swap_type_of(dev_t device, sector_t offset, struct block_device
> **bdev_p) {
>  	struct block_device *bdev = NULL;
>  	int i;
> @@ -450,6 +450,9 @@ int swap_type_of(dev_t device, sector_t
>  			continue;
>
>  		if (!bdev) {
> +			if (bdev_p)
> +				*bdev_p = sis->bdev;
> +
>  			spin_unlock(&swap_lock);
>  			return i;
>  		}
> @@ -459,6 +462,9 @@ int swap_type_of(dev_t device, sector_t
>  			se = list_entry(sis->extent_list.next,
>  					struct swap_extent, list);
>  			if (se->start_block == offset) {
> +				if (bdev_p)
> +					*bdev_p = sis->bdev;
> +
>  				spin_unlock(&swap_lock);
>  				bdput(bdev);
>  				return i;
> Index: linux-2.6.20-rc3/kernel/power/user.c
> ===================================================================
> --- linux-2.6.20-rc3.orig/kernel/power/user.c
> +++ linux-2.6.20-rc3/kernel/power/user.c
> @@ -58,7 +58,7 @@ static int snapshot_open(struct inode *i
>  	memset(&data->handle, 0, sizeof(struct snapshot_handle));
>  	if ((filp->f_flags & O_ACCMODE) == O_RDONLY) {
>  		data->swap = swsusp_resume_device ?
> -				swap_type_of(swsusp_resume_device, 0) : -1;
> +			swap_type_of(swsusp_resume_device, 0, NULL) : -1;
>  		data->mode = O_RDONLY;
>  	} else {
>  		data->swap = -1;
> @@ -327,7 +327,8 @@ static int snapshot_ioctl(struct inode *
>  			 * so we need to recode them
>  			 */
>  			if (old_decode_dev(arg)) {
> -				data->swap = swap_type_of(old_decode_dev(arg), 0);
> +				data->swap = swap_type_of(old_decode_dev(arg),
> +							0, NULL);
>  				if (data->swap < 0)
>  					error = -ENODEV;
>  			} else {
> @@ -427,7 +428,7 @@ static int snapshot_ioctl(struct inode *
>  			swdev = old_decode_dev(swap_area.dev);
>  			if (swdev) {
>  				offset = swap_area.offset;
> -				data->swap = swap_type_of(swdev, offset);
> +				data->swap = swap_type_of(swdev, offset, NULL);
>  				if (data->swap < 0)
>  					error = -ENODEV;
>  			} else {
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.5 (GNU/Linux)

iD8DBQFFmhn/R6LMutpd94wRAs/nAJ4kcMD1KidpfQtTwnxYCQTf/ZNEzwCZATSf
6OFWDF/9sui3vvwmwwe6dos=
=fYE3
-----END PGP SIGNATURE-----

Re: 2.6.20 regression: suspend to disk no more works

[Pavel Machek]

Hi!

> > While 'echo a:b > /sys/power/resume' before 
> > suspend is a workaround, this still breaks perfectly valid setup that worked
> > before. Also 'echo a:b > /sys/power/resume' is actually wrong - we are not
> > going to resume at this point; but there is no way to just tell kernel "use
> > this device for next STD" ... also the error message is misleading, it should
> > complaint "no resume device found". Swap is there all right.
> 
> Thanks for the report.

It fixes it for too people, I guess that's ACK... certainly for -mm,
probably for 2.6.20, too.
								Pavel

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