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

[Markus Pargmann <mpa@pengutronix.de>]

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