nbd: correct disconnect behavior

PATCH In review Paul Clements 5 participants 7 messages Jun 19, 2013 – Aug 26, 2013 1 message not in the archive

Subsystems (2)

Block layer Network block device (NBD)

lore

Share & tools

lore

Paul Clements

author

2013-06-19 21:09 UTC #0 lore raw

PATCH 1/1 nbd: correct disconnect behavior

Currently, when a disconnect is requested by the user (via NBD_DISCONNECT ioctl) the return from NBD_DO_IT is undefined (it is usually one of several error codes). This means that nbd-client does not know if a manual disconnect was performed or whether a network error occurred. Because of this, nbd-client's persist mode (which tries to reconnect after error, but not after manual disconnect) does not always work correctly.

This change fixes this by causing NBD_DO_IT to always return 0 if a user requests a disconnect. This means that nbd-client can correctly either persist the connection (if an error occurred) or disconnect (if the user requested it).

Signed-off-by Paul Clements <[email protected]>

---

 drivers/block/nbd.c |    5 +++++
 include/linux/nbd.h |    1 +
 2 files changed, 6 insertions(+)

drivers/block/nbd.c +5 -0

index 7fecc78..8b7664d 100644
@@ -623,6 +623,8 @@ static int __nbd_ioctl(struct block_device *bdev, struct nbd_device *nbd,
 		if (!nbd->sock)
 			return -EINVAL;
 
+		nbd->disconnect = 1;
+
 		nbd_send_req(nbd, &sreq);
                 return 0;
 	}

@@ -654,6 +656,7 @@ static int __nbd_ioctl(struct block_device *bdev, struct nbd_device *nbd,
 				nbd->sock = SOCKET_I(inode);
 				if (max_part > 0)
 					bdev->bd_invalidated = 1;
+				nbd->disconnect = 0; /* we're connected now */
 				return 0;
 			} else {
 				fput(file);

@@ -742,6 +745,8 @@ static int __nbd_ioctl(struct block_device *bdev, struct nbd_device *nbd,
 		set_capacity(nbd->disk, 0);
 		if (max_part > 0)
 			ioctl_by_bdev(bdev, BLKRRPART, 0);
+		if (nbd->disconnect) /* user requested, ignore socket errors */
+			return 0;
 		return nbd->harderror;
 	}

include/linux/nbd.h +1 -0

index 4871170..ae4981e 100644
@@ -41,6 +41,7 @@ struct nbd_device {
 	u64 bytesize;
 	pid_t pid; /* pid of nbd-client, if attached */
 	int xmit_timeout;
+	int disconnect; /* a disconnect has been requested by user */
 };
 
 #endif

Rob Landley

Acked-by on v1 1/1

2013-06-21 03:56 UTC #1 lore raw

Quoted 7 lines · Paul Clements

Currently, when a disconnect is requested by the user (via NBD_DISCONNECT ioctl) the return from NBD_DO_IT is undefined (it is usually one of several error codes). This means that nbd-client does not know if a manual disconnect was performed or whether a network error occurred. Because of this, nbd-client's persist mode (which tries to reconnect after error, but not after manual disconnect) does not always work correctly.

Quoted 6 lines

This change fixes this by causing NBD_DO_IT to always return 0 if a user requests a disconnect. This means that nbd-client can correctly either persist the connection (if an error occurred) or disconnect (if the user requested it).
Signed-off-by Paul Clements <[email protected]>

Is _that_ what it was?

(Guy who wrote the busybox NBD client and never did quite understand the disconnect/reconnect behavior.)

Acked-by Rob Landley <[email protected]>

Rob

Andrew Morton

2013-06-26 23:21 UTC #2 lore raw

In reply to Paul Clements

Quoted 11 lines · On Wed, 19 Jun 2013 17:09:18 -0400 (EDT) Paul Clements…

Currently, when a disconnect is requested by the user (via NBD_DISCONNECT ioctl) the return from NBD_DO_IT is undefined (it is usually one of several error codes). This means that nbd-client does not know if a manual disconnect was performed or whether a network error occurred. Because of this, nbd-client's persist mode (which tries to reconnect after error, but not after manual disconnect) does not always work correctly.
This change fixes this by causing NBD_DO_IT to always return 0 if a user requests a disconnect. This means that nbd-client can correctly either persist the connection (if an error occurred) or disconnect (if the user requested it).

This sounds like something which users of 3.10 and earlier kernels might want, so I added the Cc:stable tag. Please let me know if you disagree.

Quoted 20 lines

drivers/block/nbd.c +3 -0

@@ -623,6 +623,8 @@ static int __nbd_ioctl(struct block_device *bdev, struct nbd_device *nbd,
 		if (!nbd->sock)
 			return -EINVAL;
 
+		nbd->disconnect = 1;
+
 		nbd_send_req(nbd, &sreq);
                 return 0;
 	}

@@ -654,6 +656,7 @@ static int __nbd_ioctl(struct block_device *bdev, struct nbd_device *nbd,
 				nbd->sock = SOCKET_I(inode);
 				if (max_part > 0)
 					bdev->bd_invalidated = 1;
+				nbd->disconnect = 0; /* we're connected now */
 				return 0;
 			} else {
 				fput(file);

drivers/block/nbd.c static int __nbd_ioctl(struct block_device *bdev, struct nbd_device *nbd, · line 751

Quoted 7 lines

drivers/block/nbd.c +2 -0

@@ -742,6 +745,8 @@ static int __nbd_ioctl(struct block_device *bdev, struct nbd_device *nbd,
 		set_capacity(nbd->disk, 0);
 		if (max_part > 0)
 			ioctl_by_bdev(bdev, BLKRRPART, 0);
+		if (nbd->disconnect) /* user requested, ignore socket errors */
+			return 0;
 		return nbd->harderror;
 	}

hm, how does nbd work... Hard to tell as nothing seems to be documented anywhere :(

afacit the code assumes that the user will run ioctl(NBD_DISCONNECT) and then ioctl(NBD_DO_IT) and then ioctl(NBD_SET_SOCK), yes? Does this change mean that if userspace calls the ioctls in an other-than-expected order, Weird Things will happen? Would it be safer to clear ->disconnect in NBD_DO_IT?

include/linux/nbd.h struct nbd_device { · line 45

Quoted 8 lines

include/linux/nbd.h +1 -0

@@ -41,6 +41,7 @@ struct nbd_device {
 	u64 bytesize;
 	pid_t pid; /* pid of nbd-client, if attached */
 	int xmit_timeout;
+	int disconnect; /* a disconnect has been requested by user */
 };

The cool kids are using bool lately ;)

Andrew Morton

2013-06-27 22:28 UTC #4 lore raw

In reply to a message not in the archive

Quoted 48 lines · On Thu, 27 Jun 2013 18:20:37 -0400 Paul Clements <[email protected]>…

Quoted 28 lines
--- a/drivers/block/nbd.c
Review static int __nbd_ioctl(struct block_device *bdev,
Quoted 2 lines
+++ b/drivers/block/nbd.c
(quoted diff) +0 -0
@@ -623,6 +623,8 @@ static int __nbd_ioctl(struct block_device *bdev,
struct nbd_device *nbd,
drivers/block/nbd.c static int __nbd_ioctl(struct block_device *bdev,
Quoted 9 lines
drivers/block/nbd.c +2 -0
              if (!nbd->sock)
                      return -EINVAL;
 
+             nbd->disconnect = 1;
+
              nbd_send_req(nbd, &sreq);
                 return 0;
      }
@@ -654,6 +656,7 @@ static int __nbd_ioctl(struct block_device *bdev,
struct nbd_device *nbd,
drivers/block/nbd.c static int __nbd_ioctl(struct block_device *bdev, struct nbd_device *nbd,
Quoted 4 lines
drivers/block/nbd.c +1 -0
                              nbd->sock = SOCKET_I(inode);
                              if (max_part > 0)
                                      bdev->bd_invalidated = 1;
+                             nbd->disconnect = 0; /* we're connected
now */
drivers/block/nbd.c static int __nbd_ioctl(struct block_device *bdev,
Quoted 4 lines
drivers/block/nbd.c +0 -0
                              return 0;
                      } else {
                              fput(file);
@@ -742,6 +745,8 @@ static int __nbd_ioctl(struct block_device *bdev,
struct nbd_device *nbd,
drivers/block/nbd.c static int __nbd_ioctl(struct block_device *bdev, struct nbd_device *nbd,
Quoted 4 lines
drivers/block/nbd.c +1 -0
              set_capacity(nbd->disk, 0);
              if (max_part > 0)
                      ioctl_by_bdev(bdev, BLKRRPART, 0);
+             if (nbd->disconnect) /* user requested, ignore socket
Quoted 8 lines
errors */
drivers/block/nbd.c static int __nbd_ioctl(struct block_device *bdev, struct nbd_device *nbd, · line 751
Quoted 3 lines
drivers/block/nbd.c +1 -0
+                     return 0;
              return nbd->harderror;
      }
hm, how does nbd work... Hard to tell as nothing seems to be documented anywhere :(
I think most people look at the nbd-client to see. :)
But I will work on some docs for the ioctls...
Briefly, though, it goes something like this:
nbd-client and nbd-server negotiate parameters, including the server telling the client how big the export is
Then the nbd-client does a series of ioctls to set up the device:

Quoted 8 lines

NBD_SET_SIZE - set the size of the device
NBD_SET_SOCK - tell the kernel which socket to use NBD_DO_IT - this ioctl lasts for the life of the device and causes nbd-client to go into a receive loop, handling I/O replies from the nbd-server
NBD_DISCONNECT, meanwhile, can be called from a separate thread (usually "nbd-client -d" calls it). This requests a disconnect of the device.

OK, but. "Would it be safer to clear ->disconnect in NBD_DO_IT?"

If not safer, would it be cleaner?

Quoted 4 lines

Quoted 2 lines
The cool kids are using bool lately ;)
Hey, maybe I want to be able to compile with gcc 2.7.2 ? :)

Sob, I miss 2.7.2. It was a good 50% faster than the new improved models. But I don't think this yearning makes us cool.

Paul Clements

2013-06-27 23:25 UTC #5 lore raw

Quoted 2 lines · Andrew Morton

On Thu, 27 Jun 2013 18:20:37 -0400 Paul Clements <[email protected]> wrote:
OK, but. "Would it be safer to clear ->disconnect in NBD_DO_IT?"

About the same in terms of safety. Both ioctls have to be called to set up the device and neither can be called again, until the device is reset.

Quoted 1 line

If not safer, would it be cleaner?

I don't know, seems like a toss up. NBD_SET_SOCK is the earliest place that the socket might conceivably be usable, so I wanted to clear disconnect there (e.g., in case an alternate/new version of NBD_DO_IT, as has been discussed, is ever implemented).

Quoted 7 lines

Quoted 4 lines
Quoted 2 lines
The cool kids are using bool lately ;)
Hey, maybe I want to be able to compile with gcc 2.7.2 ? :)
Sob, I miss 2.7.2. It was a good 50% faster than the new improved models. But I don't think this yearning makes us cool.

No, I think it just makes us old :)

Rob Landley

2013-07-02 07:19 UTC #6 lore raw

In reply to Andrew Morton

Quoted 53 lines · Andrew Morton

Quoted 1 line · On Wed, 19 Jun 2013 17:09:18 -0400 (EDT) Paul Clements…
Currently, when a disconnect is requested by the user (via
NBD_DISCONNECT
Quoted 4 lines
ioctl) the return from NBD_DO_IT is undefined (it is usually one of several error codes). This means that nbd-client does not know if a manual disconnect was performed or whether a network error occurred. Because of this, nbd-client's persist mode (which tries to
reconnect after
Quoted 1 line
error, but not after manual disconnect) does not always work
correctly.
Quoted 2 lines
This change fixes this by causing NBD_DO_IT to always return 0 if a
user
Quoted 1 line
requests a disconnect. This means that nbd-client can correctly
either
Quoted 1 line
persist the connection (if an error occurred) or disconnect (if the
user
Quoted 1 line
requested it).
This sounds like something which users of 3.10 and earlier kernels might want, so I added the Cc:stable tag. Please let me know if you disagree.
drivers/block/nbd.c static int __nbd_ioctl(struct block_device
Quoted 3 lines
drivers/block/nbd.c +0 -0
@@ -623,6 +623,8 @@ static int __nbd_ioctl(struct block_device  
*bdev, struct nbd_device *nbd,
drivers/block/nbd.c static int __nbd_ioctl(struct block_device
Quoted 9 lines
drivers/block/nbd.c +2 -0
 		if (!nbd->sock)
 			return -EINVAL;
 
+		nbd->disconnect = 1;
+
 		nbd_send_req(nbd, &sreq);
                 return 0;
 	}
@@ -654,6 +656,7 @@ static int __nbd_ioctl(struct block_device  
*bdev, struct nbd_device *nbd,
drivers/block/nbd.c static int __nbd_ioctl(struct block_device *bdev, struct nbd_device *nbd,
Quoted 4 lines
drivers/block/nbd.c +1 -0
 				nbd->sock = SOCKET_I(inode);
 				if (max_part > 0)
 					bdev->bd_invalidated = 1;
+				nbd->disconnect = 0; /* we're connected  
now */
drivers/block/nbd.c static int __nbd_ioctl(struct block_device
Quoted 4 lines
drivers/block/nbd.c +0 -0
 				return 0;
 			} else {
 				fput(file);
@@ -742,6 +745,8 @@ static int __nbd_ioctl(struct block_device  
*bdev, struct nbd_device *nbd,
drivers/block/nbd.c static int __nbd_ioctl(struct block_device *bdev, struct nbd_device *nbd,
Quoted 4 lines
drivers/block/nbd.c +1 -0
 		set_capacity(nbd->disk, 0);
 		if (max_part > 0)
 			ioctl_by_bdev(bdev, BLKRRPART, 0);
+		if (nbd->disconnect) /* user requested, ignore socket  
errors */

Quoted 6 lines

drivers/block/nbd.c static int __nbd_ioctl(struct block_device *bdev, struct nbd_device *nbd, · line 751
Quoted 3 lines
drivers/block/nbd.c +1 -0
+			return 0;
 		return nbd->harderror;
 	}
hm, how does nbd work... Hard to tell as nothing seems to be documented anywhere :(

I wrote the busybox version, which might be a bit simpler:

http://git.busybox.net/busybox/tree/networking/nbd-client.c

(Sorry about the #ifdefs, they're not mine.)

Quoted 5 lines

afacit the code assumes that the user will run ioctl(NBD_DISCONNECT) and then ioctl(NBD_DO_IT) and then ioctl(NBD_SET_SOCK), yes? Does this change mean that if userspace calls the ioctls in an other-than-expected order, Weird Things will happen? Would it be safer to clear ->disconnect in NBD_DO_IT?

Quoted 10 lines

include/linux/nbd.h struct nbd_device { · line 45
Quoted 8 lines
include/linux/nbd.h +1 -0
@@ -41,6 +41,7 @@ struct nbd_device {
 	u64 bytesize;
 	pid_t pid; /* pid of nbd-client, if attached */
 	int xmit_timeout;
+	int disconnect; /* a disconnect has been requested by user */
 };
The cool kids are using bool lately ;)

No, they're not. The C++ guys and stuffy old ex-cobol types are, and think it helps. (Does any architecture anywhere _not_ use int for bool?)

Rob

Pavel Machek

2013-08-26 12:55 UTC #7 lore raw

Hi!

Quoted 11 lines

Quoted 10 lines
include/linux/nbd.h struct nbd_device { · line 45
Quoted 8 lines
include/linux/nbd.h +1 -0
@@ -41,6 +41,7 @@ struct nbd_device {
 	u64 bytesize;
 	pid_t pid; /* pid of nbd-client, if attached */
 	int xmit_timeout;
+	int disconnect; /* a disconnect has been requested by user */
 };
The cool kids are using bool lately ;)

Quoted 3 lines

No, they're not. The C++ guys and stuffy old ex-cobol types are, and think it helps. (Does any architecture anywhere _not_ use int for bool?)

Its useful as documentation, and while it is probably int, faster/smaller code can probably generated in some cases.
Pavel

Signature · 2 lines

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