[PATCH] nbd: correct disconnect behavior

[PATCH] nbd: correct disconnect behavior

[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).

Signed-off-by: Paul Clements <paul.clements@steeleye.com>
---

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

diff --git a/drivers/block/nbd.c b/drivers/block/nbd.c
index 7fecc78..8b7664d 100644
--- a/drivers/block/nbd.c
+++ b/drivers/block/nbd.c
@@ -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;
 	}
 
diff --git a/include/linux/nbd.h b/include/linux/nbd.h
index 4871170..ae4981e 100644
--- a/include/linux/nbd.h
+++ b/include/linux/nbd.h
@@ -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

Re: [PATCH] nbd: correct disconnect behavior

[Rob Landley]

On 06/19/2013 04:09:18 PM, Paul Clements wrote:
> 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 <paul.clements@steeleye.com>

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 <rob@landley.net>

Rob

Re: [PATCH] nbd: correct disconnect behavior

[Andrew Morton]

On Wed, 19 Jun 2013 17:09:18 -0400 (EDT) Paul Clements <paul.clements@steeleye.com> wrote:

> 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.

> --- a/drivers/block/nbd.c
> +++ b/drivers/block/nbd.c
> @@ -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;
>  	}

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?

> --- a/include/linux/nbd.h
> +++ b/include/linux/nbd.h
> @@ -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 ;)

Re: [PATCH] nbd: correct disconnect behavior

[Andrew Morton]

On Thu, 27 Jun 2013 18:20:37 -0400 Paul Clements <paul.clements@us.sios.com> wrote:

> > --- a/drivers/block/nbd.c
> > > +++ b/drivers/block/nbd.c
> > > @@ -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;
> > >       }
> >
> > 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:
> 
> 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?

> > 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.

Re: [PATCH] nbd: correct disconnect behavior

[Paul Clements]

On Thu, Jun 27, 2013 at 6:28 PM, Andrew Morton
<akpm@linux-foundation.org> wrote:
> On Thu, 27 Jun 2013 18:20:37 -0400 Paul Clements <paul.clements@us.sios.com> 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.

> 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).

>> > 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 :)

Re: [PATCH] nbd: correct disconnect behavior

[Rob Landley]

On 06/26/2013 06:21:07 PM, Andrew Morton wrote:
> On Wed, 19 Jun 2013 17:09:18 -0400 (EDT) Paul Clements  
> <paul.clements@steeleye.com> wrote:
> 
> > 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.
> 
> > --- a/drivers/block/nbd.c
> > +++ b/drivers/block/nbd.c
> > @@ -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;
> >  	}
> 
> 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.)

> 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?
> > --- a/include/linux/nbd.h
> > +++ b/include/linux/nbd.h
> > @@ -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

Re: [PATCH] nbd: correct disconnect behavior

[Pavel Machek]

Hi!

> >> --- a/include/linux/nbd.h
> >> +++ b/include/linux/nbd.h
> >> @@ -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?)

Its useful as documentation, and while it is probably int,
faster/smaller code can probably generated in some cases.
									Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

Re: [PATCH] nbd: correct disconnect behavior

[Paul Clements]

On Wed, Jun 26, 2013 at 7:21 PM, Andrew Morton
<akpm@linux-foundation.org> wrote:
>
> On Wed, 19 Jun 2013 17:09:18 -0400 (EDT) Paul Clements <paul.clements@steeleye.com> wrote:
>
> 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.


That is a good idea. It would be useful in stable.


> > --- a/drivers/block/nbd.c
> > +++ b/drivers/block/nbd.c
> > @@ -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;
> >       }
>
> 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:

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.


>
> > --- a/include/linux/nbd.h
> > +++ b/include/linux/nbd.h
> > @@ -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 ;)


Hey, maybe I want to be able to compile with gcc 2.7.2 ? :)