Re: [PATCH 0/1] swsusp: fix breakage with swap on LVM

Re: [PATCH 0/1] swsusp: fix breakage with swap on LVM

[Rafael J. Wysocki]

On Thursday 16 February 2006 17:13, Pavel Machek wrote:
> -rc3 version looks ok, and we probably want it in asap. -mm
> version looks a bit long... --p

That's because it adds a new function + comment.

I think it's not a good idea to remake mm/swapfile.c:swap_type_of()
in a -rc3-like fashion, because it is called by the userland interface for
a different purpose and should not return non-error for the argument
being zero.

Greetings,
Rafael

Re: [PATCH 0/1] swsusp: fix breakage with swap on LVM

[Rafael J. Wysocki]

On Thursday 16 February 2006 22:51, Rafael J. Wysocki wrote:
> On Thursday 16 February 2006 17:13, Pavel Machek wrote:
> > -rc3 version looks ok, and we probably want it in asap. -mm
> > version looks a bit long... --p
> 
> That's because it adds a new function + comment.
> 
> I think it's not a good idea to remake mm/swapfile.c:swap_type_of()
> in a -rc3-like fashion, because it is called by the userland interface for
> a different purpose and should not return non-error for the argument
> being zero.

Well, alternatively I can change the userland interface. :-)


Signed-off-by: Rafael J. Wysocki <rjw@sisk.pl>
---
 kernel/power/user.c |   13 +++++++++----
 mm/swapfile.c       |    6 ++++--
 2 files changed, 13 insertions(+), 6 deletions(-)

Index: linux-2.6.16-rc3-mm1/mm/swapfile.c
===================================================================
--- linux-2.6.16-rc3-mm1.orig/mm/swapfile.c
+++ linux-2.6.16-rc3-mm1/mm/swapfile.c
@@ -428,14 +428,16 @@ int swap_type_of(dev_t device)
 {
 	int i;
 
-	if (!device)
-		return -EINVAL;
 	spin_lock(&swap_lock);
 	for (i = 0; i < nr_swapfiles; i++) {
 		struct inode *inode;
 
 		if (!(swap_info[i].flags & SWP_WRITEOK))
 			continue;
+		if (!device) {
+			spin_unlock(&swap_lock);
+			return i;
+		}
 		inode = swap_info->swap_file->f_dentry->d_inode;
 		if (S_ISBLK(inode->i_mode) &&
 		    device == MKDEV(imajor(inode), iminor(inode))) {
Index: linux-2.6.16-rc3-mm1/kernel/power/user.c
===================================================================
--- linux-2.6.16-rc3-mm1.orig/kernel/power/user.c
+++ linux-2.6.16-rc3-mm1/kernel/power/user.c
@@ -51,7 +51,7 @@ static int snapshot_open(struct inode *i
 	filp->private_data = data;
 	memset(&data->handle, 0, sizeof(struct snapshot_handle));
 	if ((filp->f_flags & O_ACCMODE) == O_RDONLY) {
-		data->swap = swap_type_of(swsusp_resume_device);
+		data->swap = swsusp_resume_device ? swap_type_of(swsusp_resume_device) : -1;
 		data->mode = O_RDONLY;
 	} else {
 		data->swap = -1;
@@ -252,9 +252,14 @@ static int snapshot_ioctl(struct inode *
 			 * User space encodes device types as two-byte values,
 			 * so we need to recode them
 			 */
-			data->swap = swap_type_of(old_decode_dev(arg));
-			if (data->swap < 0)
-				error = -ENODEV;
+			if (old_decode_dev(arg)) {
+				data->swap = swap_type_of(old_decode_dev(arg));
+				if (data->swap < 0)
+					error = -ENODEV;
+			} else {
+				data->swap = -1;
+				error = -EINVAL;
+			}
 		} else {
 			error = -EPERM;
 		}

Re: [PATCH 0/1] swsusp: fix breakage with swap on LVM

[Pavel Machek]

On Thu 16-02-06 23:41:16, Rafael J. Wysocki wrote:
> On Thursday 16 February 2006 22:51, Rafael J. Wysocki wrote:
> > On Thursday 16 February 2006 17:13, Pavel Machek wrote:
> > > -rc3 version looks ok, and we probably want it in asap. -mm
> > > version looks a bit long... --p
> > 
> > That's because it adds a new function + comment.
> > 
> > I think it's not a good idea to remake mm/swapfile.c:swap_type_of()
> > in a -rc3-like fashion, because it is called by the userland interface for
> > a different purpose and should not return non-error for the argument
> > being zero.
> 
> Well, alternatively I can change the userland interface. :-)

ACK.


-- 
64 bytes from 195.113.31.123: icmp_seq=28 ttl=51 time=448769.1 ms         

Re: [PATCH 0/1] swsusp: fix breakage with swap on LVM

[Rafael J. Wysocki]

On Saturday 18 February 2006 15:51, Pavel Machek wrote:
> On Thu 16-02-06 23:41:16, Rafael J. Wysocki wrote:
> > On Thursday 16 February 2006 22:51, Rafael J. Wysocki wrote:
> > > On Thursday 16 February 2006 17:13, Pavel Machek wrote:
> > > > -rc3 version looks ok, and we probably want it in asap. -mm
> > > > version looks a bit long... --p
> > > 
> > > That's because it adds a new function + comment.
> > > 
> > > I think it's not a good idea to remake mm/swapfile.c:swap_type_of()
> > > in a -rc3-like fashion, because it is called by the userland interface for
> > > a different purpose and should not return non-error for the argument
> > > being zero.
> > 
> > Well, alternatively I can change the userland interface. :-)
> 
> ACK.

OK

Andrew, could you replace the
swsusp-separate-swap-writing-reading-code-rev-2-fix-breakage-with-swap-on-lvm.patch
with this one, please?  [Appended once again for convenience.]

Rafael
---

Restore the compatibility with the older code and make it possible to
suspend if the kernel command line doesn't contain the "resume=" argument.

Signed-off-by: Rafael J. Wysocki <rjw@sisk.pl>
Acked-by: Pavel Machek <pavel@suse.cz>
---
 kernel/power/user.c |   13 +++++++++----
 mm/swapfile.c       |    6 ++++--
 2 files changed, 13 insertions(+), 6 deletions(-)

Index: linux-2.6.16-rc3-mm1/mm/swapfile.c
===================================================================
--- linux-2.6.16-rc3-mm1.orig/mm/swapfile.c
+++ linux-2.6.16-rc3-mm1/mm/swapfile.c
@@ -428,14 +428,16 @@ int swap_type_of(dev_t device)
 {
 	int i;
 
-	if (!device)
-		return -EINVAL;
 	spin_lock(&swap_lock);
 	for (i = 0; i < nr_swapfiles; i++) {
 		struct inode *inode;
 
 		if (!(swap_info[i].flags & SWP_WRITEOK))
 			continue;
+		if (!device) {
+			spin_unlock(&swap_lock);
+			return i;
+		}
 		inode = swap_info->swap_file->f_dentry->d_inode;
 		if (S_ISBLK(inode->i_mode) &&
 		    device == MKDEV(imajor(inode), iminor(inode))) {
Index: linux-2.6.16-rc3-mm1/kernel/power/user.c
===================================================================
--- linux-2.6.16-rc3-mm1.orig/kernel/power/user.c
+++ linux-2.6.16-rc3-mm1/kernel/power/user.c
@@ -51,7 +51,7 @@ static int snapshot_open(struct inode *i
 	filp->private_data = data;
 	memset(&data->handle, 0, sizeof(struct snapshot_handle));
 	if ((filp->f_flags & O_ACCMODE) == O_RDONLY) {
-		data->swap = swap_type_of(swsusp_resume_device);
+		data->swap = swsusp_resume_device ? swap_type_of(swsusp_resume_device) : -1;
 		data->mode = O_RDONLY;
 	} else {
 		data->swap = -1;
@@ -252,9 +252,14 @@ static int snapshot_ioctl(struct inode *
 			 * User space encodes device types as two-byte values,
 			 * so we need to recode them
 			 */
-			data->swap = swap_type_of(old_decode_dev(arg));
-			if (data->swap < 0)
-				error = -ENODEV;
+			if (old_decode_dev(arg)) {
+				data->swap = swap_type_of(old_decode_dev(arg));
+				if (data->swap < 0)
+					error = -ENODEV;
+			} else {
+				data->swap = -1;
+				error = -EINVAL;
+			}
 		} else {
 			error = -EPERM;
 		}

[PATCH 0/1] swsusp: fix breakage with swap on LVM

[Rafael J. Wysocki]

Hi,

In the following two replies to this message there are two versions of
the same fix, one of which applies to 2.6.16-rc3 and the second to the recent
-mm.

The fix is needed to restore the compatibility with the older code that
allowed to suspend even if the kernel command line didn't contain the
"resume=" argument.  This feature is necessary so that swsusp can work
in the default Fedora setup where the swap partition is on an LVM.

I consider the first patch, against 2.6.16-rc3, as an urgent fix that should
go in 2.6.16, if possible.  It has been tested by Dave and evidently fixes the
Fedora issue.  Unfortunately it doesn't apply to the recent -mm, because
in -mm the code in question is split between mm/swapfile.c and
kernel/power/swap.c, so the other patch is needed.

Please apply.

Greetings,
Rafael