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

[Markus Pargmann]

Hi,

On Saturday 25 June 2016 23:22:06 Pranay Srivastava wrote:
> On Fri, Jun 24, 2016 at 2:59 PM, Markus Pargmann <mpa@xxxxxxxxxxxxxx> wrote:
> > From: "Pranay Kr. Srivastava" <pranjas@xxxxxxxxx>
> >
> > 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@xxxxxxxxx>
> >
> > [mpa: Keep the blockdevice open until all users left]
> > Signed-off-by: Markus Pargmann <mpa@xxxxxxxxxxxxxx>
> > ---
> > 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 |

Attachment: signature.asc
Description: This is a digitally signed message part.