[RFC 0/4] nbd: root device support

[RFC 0/4] nbd: root device support

[Markus Pargmann]

Hi,

This series adds root device support to nbd. The last patch contains a simple
and small implementation of the nbd-client to make the nbd negotiation. After
the negotiation, the socket is bound to nbd0 so the rootfs can be mounted from
nbd0. The connection parameters are passed as module parameter in the following
format:
	nbd.root_server=[<SERVER_IP>:]<SERVER_PORT>/<EXPORT_NAME>

Patches 2 and 3 are preparation patches for patch 4 which adds nbd-root.

Patch 4 uses blkdev_get_by_dev() at the moment to get the blockdevice for a
specific nbd_device. However the documentation of blkdev_get_by_dev() states
that the function should not be used. I couldn't find any better way to get
the blockdevice so I would appreciate any suggestions on how to do it correctly.

Best regards,

Markus


Markus Pargmann (4):
  nbd: Replace kthread_create with kthread_run
  nbd: Split 'DO_IT' into three functions
  nbd: Create helper functions for ioctls
  nbd: Add support for nbd as root device

 drivers/block/nbd.c | 531 ++++++++++++++++++++++++++++++++++++++++++++--------
 1 file changed, 454 insertions(+), 77 deletions(-)

-- 
2.1.4

[RFC 1/4] nbd: Replace kthread_create with kthread_run

[Markus Pargmann]

kthread_run includes the wake_up_process() call, so instead of
kthread_create() followed by wake_up_process() we can use this macro.

Signed-off-by: Markus Pargmann <mpa@pengutronix.de>
---
 drivers/block/nbd.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/block/nbd.c b/drivers/block/nbd.c
index 4bc2a5cb9935..6bd7df2ff648 100644
--- a/drivers/block/nbd.c
+++ b/drivers/block/nbd.c
@@ -706,13 +706,13 @@ static int __nbd_ioctl(struct block_device *bdev, struct nbd_device *nbd,
 		else
 			blk_queue_flush(nbd->disk->queue, 0);
 
-		thread = kthread_create(nbd_thread, nbd, "%s",
-					nbd->disk->disk_name);
+		thread = kthread_run(nbd_thread, nbd, "%s",
+				     nbd->disk->disk_name);
 		if (IS_ERR(thread)) {
 			mutex_lock(&nbd->tx_lock);
 			return PTR_ERR(thread);
 		}
-		wake_up_process(thread);
+
 		error = nbd_do_it(nbd);
 		kthread_stop(thread);
 
-- 
2.1.4

[RFC 2/4] nbd: Split 'DO_IT' into three functions

[Markus Pargmann]

The DO_IT ioctl does a lot of things to handle the connection. This
patch splits the DO_IT ioctl routine into three functions
nbd_connection_start()/run()/stop(). This makes it easier to read and
allows other functions to call the same code. This patch prepares the
DO_IT code for nbd-root device.

Signed-off-by: Markus Pargmann <mpa@pengutronix.de>
---
 drivers/block/nbd.c | 146 ++++++++++++++++++++++++++++++++--------------------
 1 file changed, 91 insertions(+), 55 deletions(-)

diff --git a/drivers/block/nbd.c b/drivers/block/nbd.c
index 6bd7df2ff648..0ff6ebbec252 100644
--- a/drivers/block/nbd.c
+++ b/drivers/block/nbd.c
@@ -596,6 +596,95 @@ static void do_nbd_request(struct request_queue *q)
 	}
 }
 
+static int nbd_connection_start(struct block_device *bdev,
+				struct nbd_device *nbd)
+{
+	if (nbd->flags & NBD_FLAG_READ_ONLY)
+		set_device_ro(bdev, true);
+	if (nbd->flags & NBD_FLAG_SEND_TRIM)
+		queue_flag_set_unlocked(QUEUE_FLAG_DISCARD, nbd->disk->queue);
+	if (nbd->flags & NBD_FLAG_SEND_FLUSH)
+		blk_queue_flush(nbd->disk->queue, REQ_FLUSH);
+	else
+		blk_queue_flush(nbd->disk->queue, 0);
+	return 0;
+}
+
+static int nbd_connection_run(struct nbd_device *nbd)
+{
+	struct task_struct *thread;
+	int error;
+
+	thread = kthread_run(nbd_thread, nbd, "%s", nbd->disk->disk_name);
+	if (IS_ERR(thread)) {
+		mutex_lock(&nbd->tx_lock);
+		return PTR_ERR(thread);
+	}
+
+	error = nbd_do_it(nbd);
+
+	kthread_stop(thread);
+
+	return error;
+}
+
+/*
+ * Call with tx_lock held
+ */
+static int nbd_connection_stop(struct block_device *bdev,
+			       struct nbd_device *nbd)
+{
+	struct socket *sock;
+
+	sock_shutdown(nbd, 0);
+	sock = nbd->sock;
+	nbd->sock = NULL;
+	nbd_clear_que(nbd);
+	dev_warn(disk_to_dev(nbd->disk), "queue cleared\n");
+	kill_bdev(bdev);
+	queue_flag_clear_unlocked(QUEUE_FLAG_DISCARD, nbd->disk->queue);
+	set_device_ro(bdev, false);
+	if (sock)
+		sockfd_put(sock);
+
+	nbd->flags = 0;
+	nbd->bytesize = 0;
+	bdev->bd_inode->i_size = 0;
+	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;
+}
+
+/*
+ * Call with tx_lock held
+ */
+static int nbd_connection_handler(struct block_device *bdev,
+				  struct nbd_device *nbd)
+{
+	int error;
+
+	mutex_unlock(&nbd->tx_lock);
+
+	error = nbd_connection_start(bdev, nbd);
+	if (error)
+		return error;
+
+	error = nbd_connection_run(nbd);
+
+	mutex_lock(&nbd->tx_lock);
+	if (error)
+		return error;
+
+	error = nbd_connection_stop(bdev, nbd);
+
+	return error;
+}
+
 /* Must be called with tx_lock held */
 
 static int __nbd_ioctl(struct block_device *bdev, struct nbd_device *nbd,
@@ -684,61 +773,8 @@ static int __nbd_ioctl(struct block_device *bdev, struct nbd_device *nbd,
 		set_capacity(nbd->disk, nbd->bytesize >> 9);
 		return 0;
 
-	case NBD_DO_IT: {
-		struct task_struct *thread;
-		struct socket *sock;
-		int error;
-
-		if (nbd->pid)
-			return -EBUSY;
-		if (!nbd->sock)
-			return -EINVAL;
-
-		mutex_unlock(&nbd->tx_lock);
-
-		if (nbd->flags & NBD_FLAG_READ_ONLY)
-			set_device_ro(bdev, true);
-		if (nbd->flags & NBD_FLAG_SEND_TRIM)
-			queue_flag_set_unlocked(QUEUE_FLAG_DISCARD,
-				nbd->disk->queue);
-		if (nbd->flags & NBD_FLAG_SEND_FLUSH)
-			blk_queue_flush(nbd->disk->queue, REQ_FLUSH);
-		else
-			blk_queue_flush(nbd->disk->queue, 0);
-
-		thread = kthread_run(nbd_thread, nbd, "%s",
-				     nbd->disk->disk_name);
-		if (IS_ERR(thread)) {
-			mutex_lock(&nbd->tx_lock);
-			return PTR_ERR(thread);
-		}
-
-		error = nbd_do_it(nbd);
-		kthread_stop(thread);
-
-		mutex_lock(&nbd->tx_lock);
-		if (error)
-			return error;
-		sock_shutdown(nbd, 0);
-		sock = nbd->sock;
-		nbd->sock = NULL;
-		nbd_clear_que(nbd);
-		dev_warn(disk_to_dev(nbd->disk), "queue cleared\n");
-		kill_bdev(bdev);
-		queue_flag_clear_unlocked(QUEUE_FLAG_DISCARD, nbd->disk->queue);
-		set_device_ro(bdev, false);
-		if (sock)
-			sockfd_put(sock);
-		nbd->flags = 0;
-		nbd->bytesize = 0;
-		bdev->bd_inode->i_size = 0;
-		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;
-	}
+	case NBD_DO_IT:
+		return nbd_connection_handler(bdev, nbd);
 
 	case NBD_CLEAR_QUE:
 		/*
-- 
2.1.4

Re: [RFC 2/4] nbd: Split 'DO_IT' into three functions

[Paul Clements]

Markus,

This refactor looks OK with the exception of one thing...

On Tue, Jan 13, 2015 at 8:44 AM, Markus Pargmann <mpa@pengutronix.de> wrote:

>  /* Must be called with tx_lock held */
>
>  static int __nbd_ioctl(struct block_device *bdev, struct nbd_device *nbd,
> @@ -684,61 +773,8 @@ static int __nbd_ioctl(struct block_device *bdev, struct nbd_device *nbd,
>                 set_capacity(nbd->disk, nbd->bytesize >> 9);
>                 return 0;
>
> -       case NBD_DO_IT: {
> -               struct task_struct *thread;
> -               struct socket *sock;
> -               int error;
> -
> -               if (nbd->pid)
> -                       return -EBUSY;
> -               if (!nbd->sock)
> -                       return -EINVAL;
>

You seem to have done away with these checks. Was that inadvertent or
was there a reason for that? The pid check is necessary to prevent two
instances of NBD_DO_IT from running. Without the sock check you'll get
a null pointer deref in nbd_do_it.

Thanks,
Paul

Re: [RFC 2/4] nbd: Split 'DO_IT' into three functions

[Markus Pargmann]

[-- Attachment #1: Type: text/plain, Size: 1800 bytes --]

Hi Paul,

On Mon, Jan 26, 2015 at 11:16:12AM -0500, Paul Clements wrote:
> Markus,
> 
> This refactor looks OK with the exception of one thing...
> 
> On Tue, Jan 13, 2015 at 8:44 AM, Markus Pargmann <mpa@pengutronix.de> wrote:
> 
> >  /* Must be called with tx_lock held */
> >
> >  static int __nbd_ioctl(struct block_device *bdev, struct nbd_device *nbd,
> > @@ -684,61 +773,8 @@ static int __nbd_ioctl(struct block_device *bdev, struct nbd_device *nbd,
> >                 set_capacity(nbd->disk, nbd->bytesize >> 9);
> >                 return 0;
> >
> > -       case NBD_DO_IT: {
> > -               struct task_struct *thread;
> > -               struct socket *sock;
> > -               int error;
> > -
> > -               if (nbd->pid)
> > -                       return -EBUSY;
> > -               if (!nbd->sock)
> > -                       return -EINVAL;
> >
> 
> You seem to have done away with these checks. Was that inadvertent or
> was there a reason for that? The pid check is necessary to prevent two
> instances of NBD_DO_IT from running. Without the sock check you'll get
> a null pointer deref in nbd_do_it.

Thanks, no there is no reason, it got dropped somewhere. These checks
should defenitely be there. Will fix it for the next version.

I also fixed a lot of error handling, style, format and documentation in
the other patches. Will send the patch series soon, but thanks for the
review so far.

Best regards,

Markus

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

[RFC 3/4] nbd: Create helper functions for ioctls

[Markus Pargmann]

This patch prepares the nbd code for the nbd-root device patch. It
moves the ioctl code into separate functions so they can be called
directly. The patch creates nbd_set_total_size(), nbd_set_blksize(),
nbd_set_sock() and nbd_set_timeout().

Signed-off-by: Markus Pargmann <mpa@pengutronix.de>
---
 drivers/block/nbd.c | 79 ++++++++++++++++++++++++++++++++++++++---------------
 1 file changed, 57 insertions(+), 22 deletions(-)

diff --git a/drivers/block/nbd.c b/drivers/block/nbd.c
index 0ff6ebbec252..11f7644be111 100644
--- a/drivers/block/nbd.c
+++ b/drivers/block/nbd.c
@@ -596,6 +596,55 @@ static void do_nbd_request(struct request_queue *q)
 	}
 }
 
+/*
+ * Call with tx_lock held
+ */
+static void nbd_set_total_size(struct block_device *bdev,
+			       struct nbd_device *nbd, size_t size)
+{
+	nbd->bytesize = size;
+	bdev->bd_inode->i_size = size;
+	set_blocksize(bdev, nbd->blksize);
+	set_capacity(nbd->disk, size >> 9);
+}
+
+/*
+ * Call with tx_lock held
+ */
+static void nbd_set_blksize(struct block_device *bdev, struct nbd_device *nbd,
+			    size_t blksize)
+{
+	u64 new_bytesize;
+
+	nbd->blksize = blksize;
+	bdev->bd_inode->i_size = nbd->bytesize;
+	set_blocksize(bdev, nbd->blksize);
+
+	new_bytesize = nbd->bytesize & ~(nbd->blksize-1);
+	if (new_bytesize != nbd->bytesize)
+		nbd_set_total_size(bdev, nbd, new_bytesize);
+}
+
+/*
+ * Call with tx_lock held
+ */
+static void nbd_set_sock(struct block_device *bdev, struct nbd_device *nbd,
+			 struct socket *sock)
+{
+	nbd->sock = sock;
+	if (max_part > 0)
+		bdev->bd_invalidated = 1;
+	nbd->disconnect = 0; /* we're connected now */
+}
+
+/*
+ * Call with tx_lock held
+ */
+static void nbd_set_timeout(struct nbd_device *nbd, int timeout)
+{
+	nbd->xmit_timeout = timeout * HZ;
+}
+
 static int nbd_connection_start(struct block_device *bdev,
 				struct nbd_device *nbd)
 {
@@ -733,33 +782,22 @@ static int __nbd_ioctl(struct block_device *bdev, struct nbd_device *nbd,
 		if (nbd->sock)
 			return -EBUSY;
 		sock = sockfd_lookup(arg, &err);
-		if (sock) {
-			nbd->sock = sock;
-			if (max_part > 0)
-				bdev->bd_invalidated = 1;
-			nbd->disconnect = 0; /* we're connected now */
-			return 0;
-		}
-		return -EINVAL;
+		if (!sock)
+			return -EINVAL;
+
+		nbd_set_sock(bdev, nbd, sock);
 	}
 
 	case NBD_SET_BLKSIZE:
-		nbd->blksize = arg;
-		nbd->bytesize &= ~(nbd->blksize-1);
-		bdev->bd_inode->i_size = nbd->bytesize;
-		set_blocksize(bdev, nbd->blksize);
-		set_capacity(nbd->disk, nbd->bytesize >> 9);
+		nbd_set_blksize(bdev, nbd, arg);
 		return 0;
 
 	case NBD_SET_SIZE:
-		nbd->bytesize = arg & ~(nbd->blksize-1);
-		bdev->bd_inode->i_size = nbd->bytesize;
-		set_blocksize(bdev, nbd->blksize);
-		set_capacity(nbd->disk, nbd->bytesize >> 9);
+		nbd_set_total_size(bdev, nbd, arg & ~(nbd->blksize-1));
 		return 0;
 
 	case NBD_SET_TIMEOUT:
-		nbd->xmit_timeout = arg * HZ;
+		nbd_set_timeout(nbd, arg);
 		return 0;
 
 	case NBD_SET_FLAGS:
@@ -767,10 +805,7 @@ static int __nbd_ioctl(struct block_device *bdev, struct nbd_device *nbd,
 		return 0;
 
 	case NBD_SET_SIZE_BLOCKS:
-		nbd->bytesize = ((u64) arg) * nbd->blksize;
-		bdev->bd_inode->i_size = nbd->bytesize;
-		set_blocksize(bdev, nbd->blksize);
-		set_capacity(nbd->disk, nbd->bytesize >> 9);
+		nbd_set_total_size(bdev, nbd, ((u64)arg) * nbd->blksize);
 		return 0;
 
 	case NBD_DO_IT:
-- 
2.1.4

Re: [RFC 3/4] nbd: Create helper functions for ioctls

[Paul Clements]

Markus,

Comments below...

On Tue, Jan 13, 2015 at 8:44 AM, Markus Pargmann <mpa@pengutronix.de> wrote:
> This patch prepares the nbd code for the nbd-root device patch. It
> moves the ioctl code into separate functions so they can be called
> directly. The patch creates nbd_set_total_size(), nbd_set_blksize(),
> nbd_set_sock() and nbd_set_timeout().
>
> Signed-off-by: Markus Pargmann <mpa@pengutronix.de>
> ---
>  drivers/block/nbd.c | 79 ++++++++++++++++++++++++++++++++++++++---------------
>  1 file changed, 57 insertions(+), 22 deletions(-)
>
> diff --git a/drivers/block/nbd.c b/drivers/block/nbd.c
> index 0ff6ebbec252..11f7644be111 100644
> --- a/drivers/block/nbd.c
> +++ b/drivers/block/nbd.c
> @@ -596,6 +596,55 @@ static void do_nbd_request(struct request_queue *q)
>         }
>  }
>
> +/*
> + * Call with tx_lock held
> + */
> +static void nbd_set_total_size(struct block_device *bdev,
> +                              struct nbd_device *nbd, size_t size)
> +{
> +       nbd->bytesize = size;
> +       bdev->bd_inode->i_size = size;
> +       set_blocksize(bdev, nbd->blksize);
> +       set_capacity(nbd->disk, size >> 9);
> +}
> +
> +/*
> + * Call with tx_lock held
> + */
> +static void nbd_set_blksize(struct block_device *bdev, struct nbd_device *nbd,
> +                           size_t blksize)
> +{
> +       u64 new_bytesize;
> +
> +       nbd->blksize = blksize;
> +       bdev->bd_inode->i_size = nbd->bytesize;
> +       set_blocksize(bdev, nbd->blksize);
> +
> +       new_bytesize = nbd->bytesize & ~(nbd->blksize-1);
> +       if (new_bytesize != nbd->bytesize)
> +               nbd_set_total_size(bdev, nbd, new_bytesize);

could just do set_capacity here to save a few cycles, but not a big
deal either way...


> +}
> +
> +/*
> + * Call with tx_lock held
> + */
> +static void nbd_set_sock(struct block_device *bdev, struct nbd_device *nbd,
> +                        struct socket *sock)
> +{
> +       nbd->sock = sock;
> +       if (max_part > 0)
> +               bdev->bd_invalidated = 1;
> +       nbd->disconnect = 0; /* we're connected now */
> +}
> +
> +/*
> + * Call with tx_lock held
> + */
> +static void nbd_set_timeout(struct nbd_device *nbd, int timeout)
> +{
> +       nbd->xmit_timeout = timeout * HZ;
> +}
> +
>  static int nbd_connection_start(struct block_device *bdev,
>                                 struct nbd_device *nbd)
>  {
> @@ -733,33 +782,22 @@ static int __nbd_ioctl(struct block_device *bdev, struct nbd_device *nbd,
>                 if (nbd->sock)
>                         return -EBUSY;
>                 sock = sockfd_lookup(arg, &err);
> -               if (sock) {
> -                       nbd->sock = sock;
> -                       if (max_part > 0)
> -                               bdev->bd_invalidated = 1;
> -                       nbd->disconnect = 0; /* we're connected now */
> -                       return 0;
> -               }
> -               return -EINVAL;
> +               if (!sock)
> +                       return -EINVAL;
> +
> +               nbd_set_sock(bdev, nbd, sock);

need a return 0 here


>         }
>
>         case NBD_SET_BLKSIZE:
> -               nbd->blksize = arg;
> -               nbd->bytesize &= ~(nbd->blksize-1);
> -               bdev->bd_inode->i_size = nbd->bytesize;
> -               set_blocksize(bdev, nbd->blksize);
> -               set_capacity(nbd->disk, nbd->bytesize >> 9);
> +               nbd_set_blksize(bdev, nbd, arg);
>                 return 0;
>
>         case NBD_SET_SIZE:
> -               nbd->bytesize = arg & ~(nbd->blksize-1);
> -               bdev->bd_inode->i_size = nbd->bytesize;
> -               set_blocksize(bdev, nbd->blksize);
> -               set_capacity(nbd->disk, nbd->bytesize >> 9);
> +               nbd_set_total_size(bdev, nbd, arg & ~(nbd->blksize-1));
>                 return 0;

Might consider deprecating this one...it's pretty useless these days,
as it has the same function as NBD_SET_SIZE_BLOCKS, but has a smaller
device size limit (at least on some platforms)... nbd-client does not
use it anymore...


>         case NBD_SET_TIMEOUT:
> -               nbd->xmit_timeout = arg * HZ;
> +               nbd_set_timeout(nbd, arg);
>                 return 0;
>
>         case NBD_SET_FLAGS:
> @@ -767,10 +805,7 @@ static int __nbd_ioctl(struct block_device *bdev, struct nbd_device *nbd,
>                 return 0;
>
>         case NBD_SET_SIZE_BLOCKS:
> -               nbd->bytesize = ((u64) arg) * nbd->blksize;
> -               bdev->bd_inode->i_size = nbd->bytesize;
> -               set_blocksize(bdev, nbd->blksize);
> -               set_capacity(nbd->disk, nbd->bytesize >> 9);
> +               nbd_set_total_size(bdev, nbd, ((u64)arg) * nbd->blksize);
>                 return 0;
>
>         case NBD_DO_IT:

Re: [RFC 3/4] nbd: Create helper functions for ioctls

[Markus Pargmann]

[-- Attachment #1: Type: text/plain, Size: 5125 bytes --]

Hi Paul,

On Wed, Jan 28, 2015 at 04:04:28PM -0500, Paul Clements wrote:
> Markus,
> 
> Comments below...
> 
> On Tue, Jan 13, 2015 at 8:44 AM, Markus Pargmann <mpa@pengutronix.de> wrote:
> > This patch prepares the nbd code for the nbd-root device patch. It
> > moves the ioctl code into separate functions so they can be called
> > directly. The patch creates nbd_set_total_size(), nbd_set_blksize(),
> > nbd_set_sock() and nbd_set_timeout().
> >
> > Signed-off-by: Markus Pargmann <mpa@pengutronix.de>
> > ---
> >  drivers/block/nbd.c | 79 ++++++++++++++++++++++++++++++++++++++---------------
> >  1 file changed, 57 insertions(+), 22 deletions(-)
> >
> > diff --git a/drivers/block/nbd.c b/drivers/block/nbd.c
> > index 0ff6ebbec252..11f7644be111 100644
> > --- a/drivers/block/nbd.c
> > +++ b/drivers/block/nbd.c
> > @@ -596,6 +596,55 @@ static void do_nbd_request(struct request_queue *q)
> >         }
> >  }
> >
> > +/*
> > + * Call with tx_lock held
> > + */
> > +static void nbd_set_total_size(struct block_device *bdev,
> > +                              struct nbd_device *nbd, size_t size)
> > +{
> > +       nbd->bytesize = size;
> > +       bdev->bd_inode->i_size = size;
> > +       set_blocksize(bdev, nbd->blksize);
> > +       set_capacity(nbd->disk, size >> 9);
> > +}
> > +
> > +/*
> > + * Call with tx_lock held
> > + */
> > +static void nbd_set_blksize(struct block_device *bdev, struct nbd_device *nbd,
> > +                           size_t blksize)
> > +{
> > +       u64 new_bytesize;
> > +
> > +       nbd->blksize = blksize;
> > +       bdev->bd_inode->i_size = nbd->bytesize;
> > +       set_blocksize(bdev, nbd->blksize);
> > +
> > +       new_bytesize = nbd->bytesize & ~(nbd->blksize-1);
> > +       if (new_bytesize != nbd->bytesize)
> > +               nbd_set_total_size(bdev, nbd, new_bytesize);
> 
> could just do set_capacity here to save a few cycles, but not a big
> deal either way...

Yes, however I will rework this again because set_blocksize returns
error values. So these functions should make error handling.

> 
> 
> > +}
> > +
> > +/*
> > + * Call with tx_lock held
> > + */
> > +static void nbd_set_sock(struct block_device *bdev, struct nbd_device *nbd,
> > +                        struct socket *sock)
> > +{
> > +       nbd->sock = sock;
> > +       if (max_part > 0)
> > +               bdev->bd_invalidated = 1;
> > +       nbd->disconnect = 0; /* we're connected now */
> > +}
> > +
> > +/*
> > + * Call with tx_lock held
> > + */
> > +static void nbd_set_timeout(struct nbd_device *nbd, int timeout)
> > +{
> > +       nbd->xmit_timeout = timeout * HZ;
> > +}
> > +
> >  static int nbd_connection_start(struct block_device *bdev,
> >                                 struct nbd_device *nbd)
> >  {
> > @@ -733,33 +782,22 @@ static int __nbd_ioctl(struct block_device *bdev, struct nbd_device *nbd,
> >                 if (nbd->sock)
> >                         return -EBUSY;
> >                 sock = sockfd_lookup(arg, &err);
> > -               if (sock) {
> > -                       nbd->sock = sock;
> > -                       if (max_part > 0)
> > -                               bdev->bd_invalidated = 1;
> > -                       nbd->disconnect = 0; /* we're connected now */
> > -                       return 0;
> > -               }
> > -               return -EINVAL;
> > +               if (!sock)
> > +                       return -EINVAL;
> > +
> > +               nbd_set_sock(bdev, nbd, sock);
> 
> need a return 0 here

Yes, fixed.

> 
> 
> >         }
> >
> >         case NBD_SET_BLKSIZE:
> > -               nbd->blksize = arg;
> > -               nbd->bytesize &= ~(nbd->blksize-1);
> > -               bdev->bd_inode->i_size = nbd->bytesize;
> > -               set_blocksize(bdev, nbd->blksize);
> > -               set_capacity(nbd->disk, nbd->bytesize >> 9);
> > +               nbd_set_blksize(bdev, nbd, arg);
> >                 return 0;
> >
> >         case NBD_SET_SIZE:
> > -               nbd->bytesize = arg & ~(nbd->blksize-1);
> > -               bdev->bd_inode->i_size = nbd->bytesize;
> > -               set_blocksize(bdev, nbd->blksize);
> > -               set_capacity(nbd->disk, nbd->bytesize >> 9);
> > +               nbd_set_total_size(bdev, nbd, arg & ~(nbd->blksize-1));
> >                 return 0;
> 
> Might consider deprecating this one...it's pretty useless these days,
> as it has the same function as NBD_SET_SIZE_BLOCKS, but has a smaller
> device size limit (at least on some platforms)... nbd-client does not
> use it anymore...

Yes I will consider this. I am not sure yet as it isn't much overhead to
have this additional ioctl.

Thanks,

Markus

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

[RFC 4/4] nbd: Add support for nbd as root device

[Markus Pargmann]

Adding support to nbd to use it as a root device. This code essentially
provides a minimal nbd-client implementation within the kernel. It opens
a socket and makes the negotiation with the server. Afterwards it passes
the socket to the normal nbd-code to handle the connection.

The arguments for the server are passed via module parameter. The
module parameter has the format
'[<SERVER_IP>:]<SERVER_PORT>/<EXPORT_NAME>'.
SERVER_IP is optional. If it is not available it will use the
root_server_addr transmitted through DHCP.

Based on those arguments, the connection to the server is established
and is connected to the nbd0 device. The rootdevice therefore is
root=/dev/nbd0.

Signed-off-by: Markus Pargmann <mpa@pengutronix.de>
---
 drivers/block/nbd.c | 306 ++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 306 insertions(+)

diff --git a/drivers/block/nbd.c b/drivers/block/nbd.c
index 11f7644be111..ac881ae3c15a 100644
--- a/drivers/block/nbd.c
+++ b/drivers/block/nbd.c
@@ -32,12 +32,17 @@
 #include <net/sock.h>
 #include <linux/net.h>
 #include <linux/kthread.h>
+#include <net/ipconfig.h>
+#include <linux/in.h>
+#include <linux/tcp.h>
 
 #include <asm/uaccess.h>
 #include <asm/types.h>
 
 #include <linux/nbd.h>
 
+#define ADDR_NONE cpu_to_be32(INADDR_NONE)
+
 #define NBD_MAGIC 0x68797548
 
 #ifdef NDEBUG
@@ -71,6 +76,20 @@ static int max_part;
  */
 static DEFINE_SPINLOCK(nbd_lock);
 
+static const char nbd_magic[] = "NBDMAGIC";
+static const u64 nbd_opts_magic = 0x49484156454F5054LL;
+
+/* Options used for the kernel driver */
+#define NBD_OPT_EXPORT_NAME 1
+
+#define NBD_DEFAULT_BLOCKSIZE 1024
+
+extern __be32 root_nfs_parse_addr(char *name);
+
+static __be32 nbd_server_addr = ADDR_NONE;
+static __be32 nbd_server_port;
+static char nbd_server_export[128] = "";
+
 #ifndef NDEBUG
 static const char *ioctl_cmd_to_ascii(int cmd)
 {
@@ -105,6 +124,52 @@ static const char *nbdcmd_to_ascii(int cmd)
 }
 #endif /* NDEBUG */
 
+/*
+ * Parse format "[<SERVER_IP>:]<SERVER_PORT>/<EXPORT_NAME>"
+ */
+static int nbd_server_addr_set(const char *val, const struct kernel_param *kp)
+{
+	char *export;
+	u16 port;
+	int ret;
+	char buf[128];
+
+	strncpy(buf, val, 128);
+
+	nbd_server_addr = root_nfs_parse_addr(buf);
+
+	if (*buf == '\0') {
+		ret = -EINVAL;
+		goto free_buf;
+	}
+	export = strchr(buf, '/');
+	if (!export || *(export + 1) == '\0') {
+		ret = -EINVAL;
+		goto free_buf;
+	}
+	*export = '\0';
+	++export;
+
+	ret = kstrtou16(buf, 10, &port);
+	if (ret)
+		goto free_buf;
+
+	memmove(buf, export, strlen(export) + 1);
+
+	nbd_server_port = htons(port);
+	strcpy(nbd_server_export, export);
+
+	return 0;
+
+free_buf:
+	kfree(buf);
+	return ret;
+}
+
+static const struct kernel_param_ops nbd_server_addr_ops = {
+	.set = nbd_server_addr_set,
+};
+
 static void nbd_end_request(struct request *req)
 {
 	int error = req->errors ? -EIO : 0;
@@ -856,6 +921,245 @@ static const struct block_device_operations nbd_fops =
 	.ioctl =	nbd_ioctl,
 };
 
+static int nbd_connect(struct socket **socket)
+{
+	struct socket *sock;
+	struct sockaddr_in sockaddr;
+	int err;
+	char val;
+
+	err = sock_create_kern(AF_INET, SOCK_STREAM, IPPROTO_TCP, &sock);
+	if (err < 0)
+		return err;
+
+	sockaddr.sin_family = AF_INET;
+	sockaddr.sin_addr.s_addr = root_server_addr;
+	sockaddr.sin_port = nbd_server_port;
+
+	val = 1;
+	sock->ops->setsockopt(sock, IPPROTO_TCP, TCP_NODELAY, &val,
+			      sizeof(val));
+
+	err = sock->ops->connect(sock, (struct sockaddr *)&sockaddr,
+				 sizeof(sockaddr), 0);
+	if (err < 0)
+		return err;
+
+	*socket = sock;
+
+	return 0;
+}
+
+static int nbd_connection_negotiate(struct nbd_device *nbd, char *export_name,
+				    size_t *rsize, u16 *nflags)
+{
+	char buf[256];
+	int ret;
+	u64 magic;
+	u16 flags;
+	u32 client_flags;
+	u32 opt;
+	u32 name_len;
+	u64 nbd_size;
+
+	ret = sock_xmit(nbd, 0, buf, 8, 0);
+	if (ret < 0)
+		return ret;
+
+	if (strncmp(buf, nbd_magic, 8))
+		return -EINVAL;
+
+	ret = sock_xmit(nbd, 0, &magic, sizeof(magic), 0);
+	if (ret < 0)
+		return ret;
+	magic = be64_to_cpu(magic);
+
+	if (magic != nbd_opts_magic)
+		return -EINVAL;
+
+	ret = sock_xmit(nbd, 0, &flags, sizeof(flags), 0);
+	if (ret < 0)
+		return ret;
+
+	*nflags = ntohs(flags);
+
+	client_flags = 0;
+
+	ret = sock_xmit(nbd, 1, &client_flags, sizeof(client_flags), 0);
+	if (ret < 0)
+		return ret;
+
+	magic = cpu_to_be64(nbd_opts_magic);
+	ret = sock_xmit(nbd, 1, &magic, sizeof(magic), 0);
+	if (ret < 0)
+		return ret;
+
+	opt = htonl(NBD_OPT_EXPORT_NAME);
+	ret = sock_xmit(nbd, 1, &opt, sizeof(opt), 0);
+	if (ret < 0)
+		return ret;
+
+	name_len = strlen(export_name);
+	name_len = htonl(name_len);
+	ret = sock_xmit(nbd, 1, &name_len, sizeof(name_len), 0);
+	if (ret < 0)
+		return ret;
+
+	ret = sock_xmit(nbd, 1, export_name, strlen(export_name), 0);
+	if (ret < 0)
+		return ret;
+
+	ret = sock_xmit(nbd, 0, &nbd_size, sizeof(nbd_size), 0);
+	if (ret < 0)
+		return ret;
+	nbd_size = be64_to_cpu(nbd_size);
+
+	ret = sock_xmit(nbd, 0, &flags, sizeof(flags), 0);
+	if (ret < 0)
+		return ret;
+	*nflags |= ntohs(flags);
+
+	ret = sock_xmit(nbd, 0, buf, 124, 0);
+	if (ret < 0)
+		return ret;
+
+	*rsize = nbd_size;
+
+	return 0;
+}
+
+struct nbd_bdev {
+	struct block_device *bdev;
+	struct nbd_device *nbd;
+};
+
+static int nbd_connection_handler_thread(void *data)
+{
+	struct nbd_bdev *nbd_bdev = data;
+	struct nbd_device *nbd = nbd_bdev->nbd;
+	int ret;
+
+	mutex_lock(&nbd->tx_lock);
+	ret = nbd_connection_handler(nbd_bdev->bdev, nbd_bdev->nbd);
+	mutex_unlock(&nbd->tx_lock);
+
+	return ret;
+}
+
+static int nbd_bind_connection(struct block_device *bdev,
+			       struct nbd_device *nbd, struct socket *sock,
+			       size_t rsize, u32 flags)
+{
+	struct nbd_bdev *nbd_bdev;
+	struct task_struct *thread;
+
+	nbd_bdev = kmalloc(sizeof(*nbd_bdev), GFP_KERNEL);
+	if (!nbd_bdev)
+		return -ENOMEM;
+
+	nbd_bdev->bdev = bdev;
+	nbd_bdev->nbd = nbd;
+
+	mutex_lock(&nbd->tx_lock);
+
+	nbd->flags = flags;
+
+	nbd_set_blksize(bdev, nbd, 4096);
+	nbd_set_total_size(bdev, nbd, rsize);
+	nbd_set_blksize(bdev, nbd, NBD_DEFAULT_BLOCKSIZE);
+
+	/*
+	 *mutex_unlock(&nbd->tx_lock);
+	 *nbd_ioctl(bdev, 0, NBD_CLEAR_SOCK, 0);
+	 *mutex_lock(&nbd->tx_lock);
+	 */
+
+	nbd_set_sock(bdev, nbd, sock);
+	nbd_set_timeout(nbd, 2);
+	mutex_unlock(&nbd->tx_lock);
+
+	thread = kthread_run(nbd_connection_handler_thread, nbd_bdev,
+			     "nbd_connection_handler");
+	if (IS_ERR(thread))
+		return PTR_ERR(thread);
+
+	return 0;
+}
+
+static int nbd_setup_bdev(struct nbd_device *nbd, size_t rsize, u16 flags)
+{
+	struct block_device *bdev;
+	int ret;
+
+	bdev = blkdev_get_by_dev(disk_devt(nbd->disk),
+				 FMODE_READ | FMODE_WRITE, nbd->sock);
+	if (IS_ERR(bdev))
+		return PTR_ERR(bdev);
+
+	ret = nbd_bind_connection(bdev, nbd, nbd->sock, rsize, flags);
+
+	return ret;
+}
+
+/*
+ * nbd_root - Called to initialize a root nbd device for booting from nbd0
+ */
+static int nbd_root(void)
+{
+	struct nbd_device *nbd;
+	struct socket *sock;
+	int ret;
+	size_t rsize;
+	u16 flags;
+
+	/* No kernel argument was given, or there were errors parsing it */
+	if (nbd_server_port == 0)
+		return 0;
+
+	if (!strlen(nbd_server_export)) {
+		pr_err("NBD-root: Missing export name\n");
+		return -EINVAL;
+	}
+
+	if (nbd_server_addr == ADDR_NONE) {
+		if (root_server_addr == ADDR_NONE) {
+			pr_err("NBD-root: Failed to find server address\n");
+			return -EINVAL;
+		}
+		nbd_server_addr = root_server_addr;
+	}
+
+	ret = nbd_connect(&sock);
+	if (ret) {
+		pr_err("NBD-root: nbd_connect failed %d\n", ret);
+		return ret;
+	}
+
+	nbd = &nbd_dev[0];
+	nbd->sock = sock;
+
+	ret = nbd_connection_negotiate(nbd, nbd_server_export, &rsize, &flags);
+	if (ret) {
+		pr_err("NBD-root: nbd_connection_negotiate failed %d\n", ret);
+		goto remove_sock;
+	}
+
+	ret = nbd_setup_bdev(nbd, rsize, flags);
+	if (ret) {
+		pr_err("NBD-root: nbd_setup_bdev failed %d\n", ret);
+		goto remove_sock;
+	}
+
+	return 0;
+
+remove_sock:
+	nbd->sock = NULL;
+	return ret;
+}
+
+/* We need this in late_initcall_sync to be sure that the network is setup */
+late_initcall_sync(nbd_root);
+
 /*
  * And here should be modules and kernel interface 
  *  (Just smiley confuses emacs :-)
@@ -991,6 +1295,8 @@ module_param(nbds_max, int, 0444);
 MODULE_PARM_DESC(nbds_max, "number of network block devices to initialize (default: 16)");
 module_param(max_part, int, 0444);
 MODULE_PARM_DESC(max_part, "number of partitions per device (default: 0)");
+module_param_cb(root_server, &nbd_server_addr_ops, NULL, 0);
+MODULE_PARM_DESC(root_server, "root server address for rootfs on a nbd. Format is [<SERVER_IP>:]<SERVER_PORT>/<EXPORT_NAME>.");
 #ifndef NDEBUG
 module_param(debugflags, int, 0644);
 MODULE_PARM_DESC(debugflags, "flags for controlling debug output");
-- 
2.1.4

Re: [RFC 4/4] nbd: Add support for nbd as root device

[Markus Pargmann]

[-- Attachment #1: Type: text/plain, Size: 3513 bytes --]

On Tue, Jan 13, 2015 at 02:44:25PM +0100, Markus Pargmann wrote:
> Adding support to nbd to use it as a root device. This code essentially
> provides a minimal nbd-client implementation within the kernel. It opens
> a socket and makes the negotiation with the server. Afterwards it passes
> the socket to the normal nbd-code to handle the connection.
> 
> The arguments for the server are passed via module parameter. The
> module parameter has the format
> '[<SERVER_IP>:]<SERVER_PORT>/<EXPORT_NAME>'.
> SERVER_IP is optional. If it is not available it will use the
> root_server_addr transmitted through DHCP.
> 
> Based on those arguments, the connection to the server is established
> and is connected to the nbd0 device. The rootdevice therefore is
> root=/dev/nbd0.
> 
> Signed-off-by: Markus Pargmann <mpa@pengutronix.de>
> ---
>  drivers/block/nbd.c | 306 ++++++++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 306 insertions(+)
> 
> diff --git a/drivers/block/nbd.c b/drivers/block/nbd.c
> index 11f7644be111..ac881ae3c15a 100644
> --- a/drivers/block/nbd.c
> +++ b/drivers/block/nbd.c
> @@ -32,12 +32,17 @@
>  #include <net/sock.h>
>  #include <linux/net.h>
>  #include <linux/kthread.h>
> +#include <net/ipconfig.h>
> +#include <linux/in.h>
> +#include <linux/tcp.h>
>  
>  #include <asm/uaccess.h>
>  #include <asm/types.h>
>  
>  #include <linux/nbd.h>
>  
> +#define ADDR_NONE cpu_to_be32(INADDR_NONE)
> +
>  #define NBD_MAGIC 0x68797548
>  
>  #ifdef NDEBUG
> @@ -71,6 +76,20 @@ static int max_part;
>   */
>  static DEFINE_SPINLOCK(nbd_lock);
>  
> +static const char nbd_magic[] = "NBDMAGIC";
> +static const u64 nbd_opts_magic = 0x49484156454F5054LL;
> +
> +/* Options used for the kernel driver */
> +#define NBD_OPT_EXPORT_NAME 1
> +
> +#define NBD_DEFAULT_BLOCKSIZE 1024
> +
> +extern __be32 root_nfs_parse_addr(char *name);
> +
> +static __be32 nbd_server_addr = ADDR_NONE;
> +static __be32 nbd_server_port;
> +static char nbd_server_export[128] = "";
> +
>  #ifndef NDEBUG
>  static const char *ioctl_cmd_to_ascii(int cmd)
>  {
> @@ -105,6 +124,52 @@ static const char *nbdcmd_to_ascii(int cmd)
>  }
>  #endif /* NDEBUG */
>  
> +/*
> + * Parse format "[<SERVER_IP>:]<SERVER_PORT>/<EXPORT_NAME>"
> + */
> +static int nbd_server_addr_set(const char *val, const struct kernel_param *kp)
> +{
> +	char *export;
> +	u16 port;
> +	int ret;
> +	char buf[128];
> +
> +	strncpy(buf, val, 128);
> +
> +	nbd_server_addr = root_nfs_parse_addr(buf);
> +
> +	if (*buf == '\0') {
> +		ret = -EINVAL;
> +		goto free_buf;
> +	}
> +	export = strchr(buf, '/');
> +	if (!export || *(export + 1) == '\0') {
> +		ret = -EINVAL;
> +		goto free_buf;
> +	}
> +	*export = '\0';
> +	++export;
> +
> +	ret = kstrtou16(buf, 10, &port);
> +	if (ret)
> +		goto free_buf;
> +
> +	memmove(buf, export, strlen(export) + 1);

This memmove() is a leftover from a previous version, it is removed for
the next series version. I also added some documentation patches that
describe this feature in Documentation/blockdev/nbd.txt. I will send it
at the end of this week.

Best regards,

Markus

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [Nbd] [RFC 4/4] nbd: Add support for nbd as root device

[Wouter Verhelst]

On Tue, Jan 13, 2015 at 02:44:25PM +0100, Markus Pargmann wrote:
> Adding support to nbd to use it as a root device. This code essentially
> provides a minimal nbd-client implementation within the kernel. It opens
> a socket and makes the negotiation with the server. Afterwards it passes
> the socket to the normal nbd-code to handle the connection.
> 
> The arguments for the server are passed via module parameter. The
> module parameter has the format
> '[<SERVER_IP>:]<SERVER_PORT>/<EXPORT_NAME>'.
> SERVER_IP is optional. If it is not available it will use the
> root_server_addr transmitted through DHCP.
> 
> Based on those arguments, the connection to the server is established
> and is connected to the nbd0 device. The rootdevice therefore is
> root=/dev/nbd0.

Not that I'm opposed to this, but you do realize that doing nbd-client
from initramfs or similar is possible, right? Most initramfs
implementations these days support it.

[...]
-- 
It is easy to love a country that is famous for chocolate and beer

  -- Barack Obama, speaking in Brussels, Belgium, 2014-03-26

Re: [Nbd] [RFC 4/4] nbd: Add support for nbd as root device

[Markus Pargmann]

[-- Attachment #1: Type: text/plain, Size: 1958 bytes --]

Hi,

On Fri, Jan 30, 2015 at 12:42:54AM +0100, Wouter Verhelst wrote:
> On Tue, Jan 13, 2015 at 02:44:25PM +0100, Markus Pargmann wrote:
> > Adding support to nbd to use it as a root device. This code essentially
> > provides a minimal nbd-client implementation within the kernel. It opens
> > a socket and makes the negotiation with the server. Afterwards it passes
> > the socket to the normal nbd-code to handle the connection.
> > 
> > The arguments for the server are passed via module parameter. The
> > module parameter has the format
> > '[<SERVER_IP>:]<SERVER_PORT>/<EXPORT_NAME>'.
> > SERVER_IP is optional. If it is not available it will use the
> > root_server_addr transmitted through DHCP.
> > 
> > Based on those arguments, the connection to the server is established
> > and is connected to the nbd0 device. The rootdevice therefore is
> > root=/dev/nbd0.
> 
> Not that I'm opposed to this, but you do realize that doing nbd-client
> from initramfs or similar is possible, right? Most initramfs
> implementations these days support it.

Yes, that was the first idea how to implement a complete netboot for an
embedded ARM device. However, an initramfs is at least around 1MB in
size which has to be loaded using tftp. As the essential nbd-client
connection setup and negotiation is quite small I decided to go with
nbd-root support.

Also it is quite useful to have nbd-root support much like nfsroot
directly built-in for debugging purposes. It has the big advantage of
booting/testing read-only filesystem images for embedded systems without
the need for an initramfs.

Best regards,

Markus

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [Nbd] [RFC 4/4] nbd: Add support for nbd as root device

[Wouter Verhelst]

On Fri, Jan 30, 2015 at 09:04:00AM +0100, Markus Pargmann wrote:
> Hi,
> 
> On Fri, Jan 30, 2015 at 12:42:54AM +0100, Wouter Verhelst wrote:
> > Not that I'm opposed to this, but you do realize that doing nbd-client
> > from initramfs or similar is possible, right? Most initramfs
> > implementations these days support it.
> 
> Yes, that was the first idea how to implement a complete netboot for an
> embedded ARM device. However, an initramfs is at least around 1MB in
> size which has to be loaded using tftp. As the essential nbd-client
> connection setup and negotiation is quite small I decided to go with
> nbd-root support.
> 
> Also it is quite useful to have nbd-root support much like nfsroot
> directly built-in for debugging purposes. It has the big advantage of
> booting/testing read-only filesystem images for embedded systems without
> the need for an initramfs.

Fair enough, just thought I'd point it out.

When looking at your patch set, two things pop out which you should
probably look at:
- What will happen if someone boots with root-on-NBD in your scheme and
  later does a pivot_root() followed by an NBD_DISCONNECT ioctl on the
  device?
- When a connection is started by nbd-client, the kernel creates a "pid"
  file in sysfs, which contains the PID of the client and which the
  client (when called with -c, or in other cases) uses to verify whether
  a device is connected. At first glance, your patch does not do this,
  which could cause confusion.

I must admit I haven't checked your patch very well (my kernel fu isn't
that advanced) so I might have missed something, but I'd rather point
it out now than have to fix the pieces afterwards ;-)

-- 
It is easy to love a country that is famous for chocolate and beer

  -- Barack Obama, speaking in Brussels, Belgium, 2014-03-26

Re: [Nbd] [RFC 4/4] nbd: Add support for nbd as root device

[Markus Pargmann]

[-- Attachment #1: Type: text/plain, Size: 2716 bytes --]

On Fri, Jan 30, 2015 at 06:30:14PM +0100, Wouter Verhelst wrote:
> On Fri, Jan 30, 2015 at 09:04:00AM +0100, Markus Pargmann wrote:
> > Hi,
> > 
> > On Fri, Jan 30, 2015 at 12:42:54AM +0100, Wouter Verhelst wrote:
> > > Not that I'm opposed to this, but you do realize that doing nbd-client
> > > from initramfs or similar is possible, right? Most initramfs
> > > implementations these days support it.
> > 
> > Yes, that was the first idea how to implement a complete netboot for an
> > embedded ARM device. However, an initramfs is at least around 1MB in
> > size which has to be loaded using tftp. As the essential nbd-client
> > connection setup and negotiation is quite small I decided to go with
> > nbd-root support.
> > 
> > Also it is quite useful to have nbd-root support much like nfsroot
> > directly built-in for debugging purposes. It has the big advantage of
> > booting/testing read-only filesystem images for embedded systems without
> > the need for an initramfs.
> 
> Fair enough, just thought I'd point it out.
> 
> When looking at your patch set, two things pop out which you should
> probably look at:
> - What will happen if someone boots with root-on-NBD in your scheme and
>   later does a pivot_root() followed by an NBD_DISCONNECT ioctl on the
>   device?

Good point. I will look if it works or fix it otherwise.

> - When a connection is started by nbd-client, the kernel creates a "pid"
>   file in sysfs, which contains the PID of the client and which the
>   client (when called with -c, or in other cases) uses to verify whether
>   a device is connected. At first glance, your patch does not do this,
>   which could cause confusion.

I am actually not quite happy to expose the pid of the task that is
doing the receive handling through sysfs. As it is already in the code,
we can't simply remove it. But I think this should be managed by
userspace if it is necessary at some point. It seems like the pid is
only used for the connection status?

Maybe it would be better to expose the connection status directly and
deprecate the 'pid' file.

> 
> I must admit I haven't checked your patch very well (my kernel fu isn't
> that advanced) so I might have missed something, but I'd rather point
> it out now than have to fix the pieces afterwards ;-)

Yes better to discuss it before things break :).

Thanks,

Markus

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [Nbd] [RFC 4/4] nbd: Add support for nbd as root device

[Andreas Klauer]

On Sat, Jan 31, 2015 at 08:52:18AM -0500, Paul Clements wrote:
>    The pid is also used to break a hung connection. See nbd_xmit_timeout.

I'm using the pid to find the first free NBD device...

If it's going away I guess I'll have to stick to /proc/partitions...?
Although last time I tried, it did not work too well.

(discussion on the NBD list, March 2014, "How to find a free NBD device")

Regards
Andreas

Re: [Nbd] [RFC 4/4] nbd: Add support for nbd as root device

[Markus Pargmann]

[-- Attachment #1: Type: text/plain, Size: 1138 bytes --]

Hi,

On Sat, Jan 31, 2015 at 03:43:16PM +0100, Andreas Klauer wrote:
> On Sat, Jan 31, 2015 at 08:52:18AM -0500, Paul Clements wrote:
> >    The pid is also used to break a hung connection. See nbd_xmit_timeout.
> 
> I'm using the pid to find the first free NBD device...
> 
> If it's going away I guess I'll have to stick to /proc/partitions...?
> Although last time I tried, it did not work too well.

This was just an idea I am thinking about.
If it is deprecated, it would still be around for a while. And my first
idea was to replace it with other information that are not internal,
like 'connected' which would be a boolean type.

> 
> (discussion on the NBD list, March 2014, "How to find a free NBD device")

Yes some better non-racy way to find a free nbd would be good.

Best regards,

Markus

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [RFC 4/4] nbd: Add support for nbd as root device

[Markus Pargmann]

[-- Attachment #1: Type: text/plain, Size: 3169 bytes --]

On Sat, Jan 31, 2015 at 08:52:18AM -0500, Paul Clements wrote:
> On Saturday, January 31, 2015, Markus Pargmann <mpa@pengutronix.de> wrote:
> 
> > On Fri, Jan 30, 2015 at 06:30:14PM +0100, Wouter Verhelst wrote:
> > > On Fri, Jan 30, 2015 at 09:04:00AM +0100, Markus Pargmann wrote:
> > > > Hi,
> > > >
> > > > On Fri, Jan 30, 2015 at 12:42:54AM +0100, Wouter Verhelst wrote:
> > > > > Not that I'm opposed to this, but you do realize that doing
> > nbd-client
> > > > > from initramfs or similar is possible, right? Most initramfs
> > > > > implementations these days support it.
> > > >
> > > > Yes, that was the first idea how to implement a complete netboot for an
> > > > embedded ARM device. However, an initramfs is at least around 1MB in
> > > > size which has to be loaded using tftp. As the essential nbd-client
> > > > connection setup and negotiation is quite small I decided to go with
> > > > nbd-root support.
> > > >
> > > > Also it is quite useful to have nbd-root support much like nfsroot
> > > > directly built-in for debugging purposes. It has the big advantage of
> > > > booting/testing read-only filesystem images for embedded systems
> > without
> > > > the need for an initramfs.
> > >
> > > Fair enough, just thought I'd point it out.
> > >
> > > When looking at your patch set, two things pop out which you should
> > > probably look at:
> > > - What will happen if someone boots with root-on-NBD in your scheme and
> > >   later does a pivot_root() followed by an NBD_DISCONNECT ioctl on the
> > >   device?
> >
> > Good point. I will look if it works or fix it otherwise.
> >
> > > - When a connection is started by nbd-client, the kernel creates a "pid"
> > >   file in sysfs, which contains the PID of the client and which the
> > >   client (when called with -c, or in other cases) uses to verify whether
> > >   a device is connected. At first glance, your patch does not do this,
> > >   which could cause confusion.
> >
> > I am actually not quite happy to expose the pid of the task that is
> > doing the receive handling through sysfs. As it is already in the code,
> > we can't simply remove it. But I think this should be managed by
> > userspace if it is necessary at some point. It seems like the pid is
> > only used for the connection status?
> >
> >
> The pid is also used to break a hung connection. See nbd_xmit_timeout.

I think nbd_xmit_timeout only uses task structs and is within the
kernel. There is only a printk which uses the pid.

> Also, see Michal Belcyk's patch for a further improvement to this. They are
> both using pids to kill the hung threads.

Yes there are some occurances of the nbd->pid field, but it seems it is
not essential for the patch. The timeout issues are still on my todo to
reproduce and fix them.

Thanks,

Markus

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [Nbd] [RFC 4/4] nbd: Add support for nbd as root device

[H. Peter Anvin]

On 01/30/2015 12:04 AM, Markus Pargmann wrote:
> Yes, that was the first idea how to implement a complete netboot for an
> embedded ARM device. However, an initramfs is at least around 1MB in
> size...

No, it's not.

	-hpa

Re: [Nbd] [RFC 4/4] nbd: Add support for nbd as root device

[Markus Pargmann]

[-- Attachment #1: Type: text/plain, Size: 715 bytes --]

Hi,

On Fri, Jan 30, 2015 at 10:10:55AM -0800, H. Peter Anvin wrote:
> On 01/30/2015 12:04 AM, Markus Pargmann wrote:
> > Yes, that was the first idea how to implement a complete netboot for an
> > embedded ARM device. However, an initramfs is at least around 1MB in
> > size...
> 
> No, it's not.

Compiling nbd-client statically gives me a binary that is around 900kB.

Regards,

Markus

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]