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(+)
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
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 <[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-
On Wed, 19 Jun 2013 17:09:18 -0400 (EDT) Paul Clements <[email protected]> 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 ;)
On Wed, Jun 26, 2013 at 7:21 PM, Andrew Morton
<[email protected]> wrote:
>
> On Wed, 19 Jun 2013 17:09:18 -0400 (EDT) Paul Clements <[email protected]> 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 ? :)
On Thu, 27 Jun 2013 18:20:37 -0400 Paul Clements <[email protected]> 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.
On Thu, Jun 27, 2013 at 6:28 PM, Andrew Morton
<[email protected]> wrote:
> 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.
> 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 :)
On 06/26/2013 06:21:07 PM, Andrew Morton wrote:
> On Wed, 19 Jun 2013 17:09:18 -0400 (EDT) Paul Clements
> <[email protected]> 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-
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