Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1430523 > unrolled thread

[PATCH 1/2] nbd: make nbd device wait for its users

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  [PATCH 1/2] nbd: make nbd device wait for its users Markus Pargmann <mpa@pengutronix.de> - 2016-06-24 11:40 +0200
    Re: [PATCH 1/2] nbd: make nbd device wait for its users Pranay Srivastava <pranjas@gmail.com> - 2016-06-24 11:50 +0200
    Re: [Nbd] [PATCH 1/2] nbd: make nbd device wait for its users Eric Blake <eblake@redhat.com> - 2016-06-24 15:50 +0200
    Re: [PATCH 1/2] nbd: make nbd device wait for its users Pranay Srivastava <pranjas@gmail.com> - 2016-06-25 20:00 +0200
      Re: [PATCH 1/2] nbd: make nbd device wait for its users Markus Pargmann <mpa@pengutronix.de> - 2016-06-29 09:00 +0200

#1430523 — [PATCH 1/2] nbd: make nbd device wait for its users

From: "Pranay Kr. Srivastava" <pranjas@gmail.com>

When a timeout occurs or a recv fails, then instead of abruplty killing
nbd block device wait for it's users to finish.

This is more required when filesystem(s) like ext2 or ext3 don't expect
their buffer heads to disappear while the filesystem is mounted.

Each open is counted. The blockdevice is kept open until the last user
closes the block device. This offers the possibility as well to open a
new socket to be used while the filesystems are mounted.

Signed-off-by: Pranay Kr. Srivastava <pranjas@gmail.com>

[mpa: Keep the blockdevice open until all users left]
Signed-off-by: Markus Pargmann <mpa@pengutronix.de>
---
Hi,

I used your patch and changed it a bit based on my ideas. The general
difference is that this keeps the block device open. After all users left, the
device is reset.

The followup patch then restricts access to ioctls after a disconnect. I wanted
to avoid that anyone sets up anything new without all the old users leaving.

Please let me know what you think about this.

Best Regards,

Markus

 drivers/block/nbd.c | 62 ++++++++++++++++++++++++++++++++++++++---------------
 1 file changed, 45 insertions(+), 17 deletions(-)

diff --git a/drivers/block/nbd.c b/drivers/block/nbd.c
index 1efc26bf1d21..620660f3ff0f 100644
--- a/drivers/block/nbd.c
+++ b/drivers/block/nbd.c
@@ -69,6 +69,8 @@ struct nbd_device {
 #if IS_ENABLED(CONFIG_DEBUG_FS)
 	struct dentry *dbg_dir;
 #endif
+	atomic_t users; /* Users that opened the block device */
+	struct block_device *bdev;
 };
 
 #if IS_ENABLED(CONFIG_DEBUG_FS)
@@ -655,9 +657,26 @@ static int nbd_set_socket(struct nbd_device *nbd, struct socket *sock)
 	return ret;
 }
 
+static void nbd_bdev_reset(struct block_device *bdev)
+{
+	set_device_ro(bdev, false);
+	bdev->bd_inode->i_size = 0;
+	if (max_part > 0) {
+		blkdev_reread_part(bdev);
+		bdev->bd_invalidated = 1;
+	}
+}
+
 /* Reset all properties of an NBD device */
 static void nbd_reset(struct nbd_device *nbd)
 {
+	sock_shutdown(nbd);
+	nbd_clear_que(nbd);
+	if (nbd->bdev) {
+		kill_bdev(nbd->bdev);
+		nbd_bdev_reset(nbd->bdev);
+	}
+
 	nbd->disconnect = false;
 	nbd->timedout = false;
 	nbd->blksize = 1024;
@@ -669,16 +688,6 @@ static void nbd_reset(struct nbd_device *nbd)
 	del_timer_sync(&nbd->timeout_timer);
 }
 
-static void nbd_bdev_reset(struct block_device *bdev)
-{
-	set_device_ro(bdev, false);
-	bdev->bd_inode->i_size = 0;
-	if (max_part > 0) {
-		blkdev_reread_part(bdev);
-		bdev->bd_invalidated = 1;
-	}
-}
-
 static void nbd_parse_flags(struct nbd_device *nbd, struct block_device *bdev)
 {
 	if (nbd->flags & NBD_FLAG_READ_ONLY)
@@ -803,18 +812,11 @@ static int __nbd_ioctl(struct block_device *bdev, struct nbd_device *nbd,
 		mutex_lock(&nbd->tx_lock);
 		nbd->task_recv = NULL;
 
-		sock_shutdown(nbd);
-		nbd_clear_que(nbd);
-		kill_bdev(bdev);
-		nbd_bdev_reset(bdev);
-
 		if (nbd->disconnect) /* user requested, ignore socket errors */
 			error = 0;
 		if (nbd->timedout)
 			error = -ETIMEDOUT;
 
-		nbd_reset(nbd);
-
 		return error;
 	}
 
@@ -853,10 +855,35 @@ static int nbd_ioctl(struct block_device *bdev, fmode_t mode,
 	return error;
 }
 
+static int nbd_open(struct block_device *bdev, fmode_t mode)
+{
+	struct nbd_device *nbd = bdev->bd_disk->private_data;
+
+	atomic_inc(&nbd->users);
+
+	if (!nbd->bdev)
+		nbd->bdev = bdev;
+
+	return 0;
+}
+
+static void nbd_release(struct gendisk *disk, fmode_t mode)
+{
+	struct nbd_device *nbd = disk->private_data;
+
+	if (atomic_dec_and_test(&nbd->users)) {
+		mutex_lock(&nbd->tx_lock);
+		nbd_reset(nbd);
+		mutex_unlock(&nbd->tx_lock);
+	}
+}
+
 static const struct block_device_operations nbd_fops = {
 	.owner =	THIS_MODULE,
 	.ioctl =	nbd_ioctl,
 	.compat_ioctl =	nbd_ioctl,
+	.open =		nbd_open,
+	.release =	nbd_release,
 };
 
 #if IS_ENABLED(CONFIG_DEBUG_FS)
@@ -1087,6 +1114,7 @@ static int __init nbd_init(void)
 		disk->private_data = &nbd_dev[i];
 		sprintf(disk->disk_name, "nbd%d", i);
 		nbd_reset(&nbd_dev[i]);
+		atomic_set(&nbd_dev[i].users, 0);
 		add_disk(disk);
 	}
 
-- 
2.1.4

[toc] | [next] | [standalone]

#1430528

Hey Markus,

On Fri, Jun 24, 2016 at 2:59 PM, Markus Pargmann <mpa@pengutronix.de> wrote:
> From: "Pranay Kr. Srivastava" <pranjas@gmail.com>
>
> When a timeout occurs or a recv fails, then instead of abruplty killing
> nbd block device wait for it's users to finish.
>
> This is more required when filesystem(s) like ext2 or ext3 don't expect
> their buffer heads to disappear while the filesystem is mounted.
>
> Each open is counted. The blockdevice is kept open until the last user
> closes the block device. This offers the possibility as well to open a
> new socket to be used while the filesystems are mounted.
>
> Signed-off-by: Pranay Kr. Srivastava <pranjas@gmail.com>
>
> [mpa: Keep the blockdevice open until all users left]
> Signed-off-by: Markus Pargmann <mpa@pengutronix.de>
> ---
> Hi,
>
> I used your patch and changed it a bit based on my ideas. The general
> difference is that this keeps the block device open. After all users left, the
> device is reset.
>
> The followup patch then restricts access to ioctls after a disconnect. I wanted
> to avoid that anyone sets up anything new without all the old users leaving.
>
> Please let me know what you think about this.
>
> Best Regards,
>
> Markus
>
>  drivers/block/nbd.c | 62 ++++++++++++++++++++++++++++++++++++++---------------
>  1 file changed, 45 insertions(+), 17 deletions(-)
>
> diff --git a/drivers/block/nbd.c b/drivers/block/nbd.c
> index 1efc26bf1d21..620660f3ff0f 100644
> --- a/drivers/block/nbd.c
> +++ b/drivers/block/nbd.c
> @@ -69,6 +69,8 @@ struct nbd_device {
>  #if IS_ENABLED(CONFIG_DEBUG_FS)
>         struct dentry *dbg_dir;
>  #endif
> +       atomic_t users; /* Users that opened the block device */
> +       struct block_device *bdev;
>  };
>
>  #if IS_ENABLED(CONFIG_DEBUG_FS)
> @@ -655,9 +657,26 @@ static int nbd_set_socket(struct nbd_device *nbd, struct socket *sock)
>         return ret;
>  }
>
> +static void nbd_bdev_reset(struct block_device *bdev)
> +{
> +       set_device_ro(bdev, false);
> +       bdev->bd_inode->i_size = 0;
> +       if (max_part > 0) {
> +               blkdev_reread_part(bdev);
> +               bdev->bd_invalidated = 1;
> +       }
> +}
> +
>  /* Reset all properties of an NBD device */
>  static void nbd_reset(struct nbd_device *nbd)
>  {
> +       sock_shutdown(nbd);
> +       nbd_clear_que(nbd);
> +       if (nbd->bdev) {
> +               kill_bdev(nbd->bdev);
> +               nbd_bdev_reset(nbd->bdev);
> +       }
> +
>         nbd->disconnect = false;
>         nbd->timedout = false;
>         nbd->blksize = 1024;
> @@ -669,16 +688,6 @@ static void nbd_reset(struct nbd_device *nbd)
>         del_timer_sync(&nbd->timeout_timer);
>  }
>
> -static void nbd_bdev_reset(struct block_device *bdev)
> -{
> -       set_device_ro(bdev, false);
> -       bdev->bd_inode->i_size = 0;
> -       if (max_part > 0) {
> -               blkdev_reread_part(bdev);
> -               bdev->bd_invalidated = 1;
> -       }
> -}
> -
>  static void nbd_parse_flags(struct nbd_device *nbd, struct block_device *bdev)
>  {
>         if (nbd->flags & NBD_FLAG_READ_ONLY)
> @@ -803,18 +812,11 @@ static int __nbd_ioctl(struct block_device *bdev, struct nbd_device *nbd,
>                 mutex_lock(&nbd->tx_lock);
>                 nbd->task_recv = NULL;
>
> -               sock_shutdown(nbd);
> -               nbd_clear_que(nbd);
> -               kill_bdev(bdev);
> -               nbd_bdev_reset(bdev);
> -
>                 if (nbd->disconnect) /* user requested, ignore socket errors */
>                         error = 0;
>                 if (nbd->timedout)
>                         error = -ETIMEDOUT;
>
> -               nbd_reset(nbd);
> -
>                 return error;
>         }
>
> @@ -853,10 +855,35 @@ static int nbd_ioctl(struct block_device *bdev, fmode_t mode,
>         return error;
>  }
>
> +static int nbd_open(struct block_device *bdev, fmode_t mode)
> +{
> +       struct nbd_device *nbd = bdev->bd_disk->private_data;
> +
> +       atomic_inc(&nbd->users);
> +
> +       if (!nbd->bdev)
> +               nbd->bdev = bdev;
> +
> +       return 0;
> +}
> +
> +static void nbd_release(struct gendisk *disk, fmode_t mode)
> +{
> +       struct nbd_device *nbd = disk->private_data;
> +
> +       if (atomic_dec_and_test(&nbd->users)) {
> +               mutex_lock(&nbd->tx_lock);
> +               nbd_reset(nbd);
> +               mutex_unlock(&nbd->tx_lock);
> +       }
> +}
> +
>  static const struct block_device_operations nbd_fops = {
>         .owner =        THIS_MODULE,
>         .ioctl =        nbd_ioctl,
>         .compat_ioctl = nbd_ioctl,
> +       .open =         nbd_open,
> +       .release =      nbd_release,
>  };
>
>  #if IS_ENABLED(CONFIG_DEBUG_FS)
> @@ -1087,6 +1114,7 @@ static int __init nbd_init(void)
>                 disk->private_data = &nbd_dev[i];
>                 sprintf(disk->disk_name, "nbd%d", i);
>                 nbd_reset(&nbd_dev[i]);
> +               atomic_set(&nbd_dev[i].users, 0);
>                 add_disk(disk);
>         }
>
> --
> 2.1.4
>

I'll be sending out rebased patch today, by next hour or so. There are
modifications to it as well like you've suggested.

-- 
        ---P.K.S

[toc] | [prev] | [next] | [standalone]

#1430676 — Re: [Nbd] [PATCH 1/2] nbd: make nbd device wait for its users

[Multipart message — attachments visible in raw view] — view raw

On 06/24/2016 03:29 AM, Markus Pargmann wrote:
> From: "Pranay Kr. Srivastava" <pranjas@gmail.com>
> 
> When a timeout occurs or a recv fails, then instead of abruplty killing

s/abruplty/abruptly/

> nbd block device wait for it's users to finish.

s/it's/its/ (remember, "it's" is only usable where "it is" would also be
appropriate)

> 
> This is more required when filesystem(s) like ext2 or ext3 don't expect
> their buffer heads to disappear while the filesystem is mounted.
> 
> Each open is counted. The blockdevice is kept open until the last user
> closes the block device. This offers the possibility as well to open a
> new socket to be used while the filesystems are mounted.
> 
> Signed-off-by: Pranay Kr. Srivastava <pranjas@gmail.com>
> 
> [mpa: Keep the blockdevice open until all users left]
> Signed-off-by: Markus Pargmann <mpa@pengutronix.de>



-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org

[toc] | [prev] | [next] | [standalone]

#1431189

On Fri, Jun 24, 2016 at 2:59 PM, Markus Pargmann <mpa@pengutronix.de> wrote:
> From: "Pranay Kr. Srivastava" <pranjas@gmail.com>
>
> When a timeout occurs or a recv fails, then instead of abruplty killing
> nbd block device wait for it's users to finish.
>
> This is more required when filesystem(s) like ext2 or ext3 don't expect
> their buffer heads to disappear while the filesystem is mounted.
>
> Each open is counted. The blockdevice is kept open until the last user
> closes the block device. This offers the possibility as well to open a
> new socket to be used while the filesystems are mounted.
>
> Signed-off-by: Pranay Kr. Srivastava <pranjas@gmail.com>
>
> [mpa: Keep the blockdevice open until all users left]
> Signed-off-by: Markus Pargmann <mpa@pengutronix.de>
> ---
> Hi,
>
> I used your patch and changed it a bit based on my ideas. The general
> difference is that this keeps the block device open. After all users left, the
> device is reset.
>
> The followup patch then restricts access to ioctls after a disconnect. I wanted
> to avoid that anyone sets up anything new without all the old users leaving.
>
> Please let me know what you think about this.
>
> Best Regards,
>
> Markus
>
>  drivers/block/nbd.c | 62 ++++++++++++++++++++++++++++++++++++++---------------
>  1 file changed, 45 insertions(+), 17 deletions(-)
>
> diff --git a/drivers/block/nbd.c b/drivers/block/nbd.c
> index 1efc26bf1d21..620660f3ff0f 100644
> --- a/drivers/block/nbd.c
> +++ b/drivers/block/nbd.c
> @@ -69,6 +69,8 @@ struct nbd_device {
>  #if IS_ENABLED(CONFIG_DEBUG_FS)
>         struct dentry *dbg_dir;
>  #endif
> +       atomic_t users; /* Users that opened the block device */

We don't need to put bdev in nbd struct. We can get it via gendisk and
bdget/bdput.
> +       struct block_device *bdev;
>  };
>
>  #if IS_ENABLED(CONFIG_DEBUG_FS)
> @@ -655,9 +657,26 @@ static int nbd_set_socket(struct nbd_device *nbd, struct socket *sock)
>         return ret;
>  }
>
> +static void nbd_bdev_reset(struct block_device *bdev)
> +{
> +       set_device_ro(bdev, false);
> +       bdev->bd_inode->i_size = 0;

i_size_write should be better I guess.

> +       if (max_part > 0) {
> +               blkdev_reread_part(bdev);
> +               bdev->bd_invalidated = 1;
> +       }
> +}
> +
>  /* Reset all properties of an NBD device */
>  static void nbd_reset(struct nbd_device *nbd)
>  {
> +       sock_shutdown(nbd);
> +       nbd_clear_que(nbd);
> +       if (nbd->bdev) {
> +               kill_bdev(nbd->bdev);
> +               nbd_bdev_reset(nbd->bdev);
> +       }
> +
>         nbd->disconnect = false;
>         nbd->timedout = false;
>         nbd->blksize = 1024;

I actually forgot to ask about this blksize. We do set_blocksize call
for bdev, but
shouldn't we instead do blkdev_logicial_blocksize?

Mount would set the blocksize depending on the filesystem's block size and the
block device logical block size supported. Should we just get rid of this?

> @@ -669,16 +688,6 @@ static void nbd_reset(struct nbd_device *nbd)
>         del_timer_sync(&nbd->timeout_timer);
>  }
>
> -static void nbd_bdev_reset(struct block_device *bdev)
> -{
> -       set_device_ro(bdev, false);
> -       bdev->bd_inode->i_size = 0;
> -       if (max_part > 0) {
> -               blkdev_reread_part(bdev);
> -               bdev->bd_invalidated = 1;
> -       }
> -}
> -
>  static void nbd_parse_flags(struct nbd_device *nbd, struct block_device *bdev)
>  {
>         if (nbd->flags & NBD_FLAG_READ_ONLY)
> @@ -803,18 +812,11 @@ static int __nbd_ioctl(struct block_device *bdev, struct nbd_device *nbd,

How about disallowing any ioctl until the nbd_device has been reset?
Perhaps, throw error
in open instead of checking here?

>                 mutex_lock(&nbd->tx_lock);
>                 nbd->task_recv = NULL;
>
> -               sock_shutdown(nbd);
> -               nbd_clear_que(nbd);
> -               kill_bdev(bdev);
> -               nbd_bdev_reset(bdev);
> -
>                 if (nbd->disconnect) /* user requested, ignore socket errors */
>                         error = 0;
>                 if (nbd->timedout)
>                         error = -ETIMEDOUT;
>

Doesn't this change the semantics for user space? NBD_DO_IT is
supposed to be over either on error or a
user disconnect not before that right[?].

> -               nbd_reset(nbd);
> -
>                 return error;
>         }
>
> @@ -853,10 +855,35 @@ static int nbd_ioctl(struct block_device *bdev, fmode_t mode,
>         return error;
>  }
>
> +static int nbd_open(struct block_device *bdev, fmode_t mode)
> +{
> +       struct nbd_device *nbd = bdev->bd_disk->private_data;
> +
> +       atomic_inc(&nbd->users);
> +
> +       if (!nbd->bdev)
> +               nbd->bdev = bdev;
> +
> +       return 0;
> +}
> +
> +static void nbd_release(struct gendisk *disk, fmode_t mode)
> +{
> +       struct nbd_device *nbd = disk->private_data;
> +
> +       if (atomic_dec_and_test(&nbd->users)) {
> +               mutex_lock(&nbd->tx_lock);
> +               nbd_reset(nbd);
> +               mutex_unlock(&nbd->tx_lock);
> +       }
> +}
> +
What if the following happens,

1) nbd-client [nbd-c1] attaches the block device

2) mount this nbd device

3) somebody goes crazy and does nbd-client -d <nbd_dev>

Step 3) would cause a DISCONNECT and a CLEAR_SOCK ioctl to be fired,
but in CLEAR_SOCK there's a call to kill_bdev, so I guess that needs
to go as well[?].

>  static const struct block_device_operations nbd_fops = {
>         .owner =        THIS_MODULE,
>         .ioctl =        nbd_ioctl,
>         .compat_ioctl = nbd_ioctl,
> +       .open =         nbd_open,
> +       .release =      nbd_release,
>  };
>
>  #if IS_ENABLED(CONFIG_DEBUG_FS)
> @@ -1087,6 +1114,7 @@ static int __init nbd_init(void)
>                 disk->private_data = &nbd_dev[i];
>                 sprintf(disk->disk_name, "nbd%d", i);
>                 nbd_reset(&nbd_dev[i]);
> +               atomic_set(&nbd_dev[i].users, 0);
>                 add_disk(disk);
>         }
>
> --
> 2.1.4
>

-- 
        ---P.K.S

[toc] | [prev] | [next] | [standalone]

#1433490

[Multipart message — attachments visible in raw view] — view raw

Hi,

On Saturday 25 June 2016 23:22:06 Pranay Srivastava wrote:
> On Fri, Jun 24, 2016 at 2:59 PM, Markus Pargmann <mpa@pengutronix.de> wrote:
> > From: "Pranay Kr. Srivastava" <pranjas@gmail.com>
> >
> > When a timeout occurs or a recv fails, then instead of abruplty killing
> > nbd block device wait for it's users to finish.
> >
> > This is more required when filesystem(s) like ext2 or ext3 don't expect
> > their buffer heads to disappear while the filesystem is mounted.
> >
> > Each open is counted. The blockdevice is kept open until the last user
> > closes the block device. This offers the possibility as well to open a
> > new socket to be used while the filesystems are mounted.
> >
> > Signed-off-by: Pranay Kr. Srivastava <pranjas@gmail.com>
> >
> > [mpa: Keep the blockdevice open until all users left]
> > Signed-off-by: Markus Pargmann <mpa@pengutronix.de>
> > ---
> > Hi,
> >
> > I used your patch and changed it a bit based on my ideas. The general
> > difference is that this keeps the block device open. After all users left, the
> > device is reset.
> >
> > The followup patch then restricts access to ioctls after a disconnect. I wanted
> > to avoid that anyone sets up anything new without all the old users leaving.
> >
> > Please let me know what you think about this.
> >
> > Best Regards,
> >
> > Markus
> >
> >  drivers/block/nbd.c | 62 ++++++++++++++++++++++++++++++++++++++---------------
> >  1 file changed, 45 insertions(+), 17 deletions(-)
> >
> > diff --git a/drivers/block/nbd.c b/drivers/block/nbd.c
> > index 1efc26bf1d21..620660f3ff0f 100644
> > --- a/drivers/block/nbd.c
> > +++ b/drivers/block/nbd.c
> > @@ -69,6 +69,8 @@ struct nbd_device {
> >  #if IS_ENABLED(CONFIG_DEBUG_FS)
> >         struct dentry *dbg_dir;
> >  #endif
> > +       atomic_t users; /* Users that opened the block device */
> 
> We don't need to put bdev in nbd struct. We can get it via gendisk and
> bdget/bdput.

Indeed, thanks.

> > +       struct block_device *bdev;
> >  };
> >
> >  #if IS_ENABLED(CONFIG_DEBUG_FS)
> > @@ -655,9 +657,26 @@ static int nbd_set_socket(struct nbd_device *nbd, struct socket *sock)
> >         return ret;
> >  }
> >
> > +static void nbd_bdev_reset(struct block_device *bdev)
> > +{
> > +       set_device_ro(bdev, false);
> > +       bdev->bd_inode->i_size = 0;
> 
> i_size_write should be better I guess.

Yes, but that is a separate patch. This code is just moved

> 
> > +       if (max_part > 0) {
> > +               blkdev_reread_part(bdev);
> > +               bdev->bd_invalidated = 1;
> > +       }
> > +}
> > +
> >  /* Reset all properties of an NBD device */
> >  static void nbd_reset(struct nbd_device *nbd)
> >  {
> > +       sock_shutdown(nbd);
> > +       nbd_clear_que(nbd);
> > +       if (nbd->bdev) {
> > +               kill_bdev(nbd->bdev);
> > +               nbd_bdev_reset(nbd->bdev);
> > +       }
> > +
> >         nbd->disconnect = false;
> >         nbd->timedout = false;
> >         nbd->blksize = 1024;
> 
> I actually forgot to ask about this blksize. We do set_blocksize call
> for bdev, but
> shouldn't we instead do blkdev_logicial_blocksize?
> 
> Mount would set the blocksize depending on the filesystem's block size and the
> block device logical block size supported. Should we just get rid of this?

I think we should perhaps set the logical blocksize as well. But that's
a separate topic as well.

> 
> > @@ -669,16 +688,6 @@ static void nbd_reset(struct nbd_device *nbd)
> >         del_timer_sync(&nbd->timeout_timer);
> >  }
> >
> > -static void nbd_bdev_reset(struct block_device *bdev)
> > -{
> > -       set_device_ro(bdev, false);
> > -       bdev->bd_inode->i_size = 0;
> > -       if (max_part > 0) {
> > -               blkdev_reread_part(bdev);
> > -               bdev->bd_invalidated = 1;
> > -       }
> > -}
> > -
> >  static void nbd_parse_flags(struct nbd_device *nbd, struct block_device *bdev)
> >  {
> >         if (nbd->flags & NBD_FLAG_READ_ONLY)
> > @@ -803,18 +812,11 @@ static int __nbd_ioctl(struct block_device *bdev, struct nbd_device *nbd,
> 
> How about disallowing any ioctl until the nbd_device has been reset?
> Perhaps, throw error
> in open instead of checking here?

This is a comment to patch 2?!
The idea was to not allow any control over the nbd device unless some
clear instructions for a cleanup are send, for example CLEAR_SOCK. After
that nothing will work.

> 
> >                 mutex_lock(&nbd->tx_lock);
> >                 nbd->task_recv = NULL;
> >
> > -               sock_shutdown(nbd);
> > -               nbd_clear_que(nbd);
> > -               kill_bdev(bdev);
> > -               nbd_bdev_reset(bdev);
> > -
> >                 if (nbd->disconnect) /* user requested, ignore socket errors */
> >                         error = 0;
> >                 if (nbd->timedout)
> >                         error = -ETIMEDOUT;
> >
> 
> Doesn't this change the semantics for user space? NBD_DO_IT is
> supposed to be over either on error or a
> user disconnect not before that right[?].

Yes it does. But it will automatically cleanup the remaining pieces. For
example for the standard nbd-client implementation a CLEAR_SOCK is
called afterwards which will end up with the same result as with the
previous code, killing all bdev. Otherwise after all block device users
left, the cleanup is done. If someone tries to use the nbd before it was
cleaned up, it will return -EBUSY with an error message indicating the
reason.

> 
> > -               nbd_reset(nbd);
> > -
> >                 return error;
> >         }
> >
> > @@ -853,10 +855,35 @@ static int nbd_ioctl(struct block_device *bdev, fmode_t mode,
> >         return error;
> >  }
> >
> > +static int nbd_open(struct block_device *bdev, fmode_t mode)
> > +{
> > +       struct nbd_device *nbd = bdev->bd_disk->private_data;
> > +
> > +       atomic_inc(&nbd->users);
> > +
> > +       if (!nbd->bdev)
> > +               nbd->bdev = bdev;
> > +
> > +       return 0;
> > +}
> > +
> > +static void nbd_release(struct gendisk *disk, fmode_t mode)
> > +{
> > +       struct nbd_device *nbd = disk->private_data;
> > +
> > +       if (atomic_dec_and_test(&nbd->users)) {
> > +               mutex_lock(&nbd->tx_lock);
> > +               nbd_reset(nbd);
> > +               mutex_unlock(&nbd->tx_lock);
> > +       }
> > +}
> > +
> What if the following happens,
> 
> 1) nbd-client [nbd-c1] attaches the block device
> 
> 2) mount this nbd device
> 
> 3) somebody goes crazy and does nbd-client -d <nbd_dev>
> 
> Step 3) would cause a DISCONNECT and a CLEAR_SOCK ioctl to be fired,
> but in CLEAR_SOCK there's a call to kill_bdev, so I guess that needs
> to go as well[?].

No, this is exactly what I wanted to happen. CLEAR_SOCK is an ioctl that
should ensure that the socket connection is gone.

I would prefer a client that has a "--force" option. Depending on this
option CLEAR_SOCK is called or not. This way the user can decide what
happens on disconnect or when the nbd device is still busy.

Best Regards,

Markus

> 
> >  static const struct block_device_operations nbd_fops = {
> >         .owner =        THIS_MODULE,
> >         .ioctl =        nbd_ioctl,
> >         .compat_ioctl = nbd_ioctl,
> > +       .open =         nbd_open,
> > +       .release =      nbd_release,
> >  };
> >
> >  #if IS_ENABLED(CONFIG_DEBUG_FS)
> > @@ -1087,6 +1114,7 @@ static int __init nbd_init(void)
> >                 disk->private_data = &nbd_dev[i];
> >                 sprintf(disk->disk_name, "nbd%d", i);
> >                 nbd_reset(&nbd_dev[i]);
> > +               atomic_set(&nbd_dev[i].users, 0);
> >                 add_disk(disk);
> >         }
> >
> > --
> > 2.1.4
> >
> 
> 

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

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web