Re: [PATCH v4 18/18] make nbd device wait for its users in case of timeout

[Markus Pargmann]

Hi,

On Thu, May 12, 2016 at 08:49:00PM +0530, Pranay Srivastava wrote:
> On Thu, May 12, 2016 at 2:49 PM, Markus Pargmann <mpa@xxxxxxxxxxxxxx> wrote:
> > Hi,
> >
> > On Wednesday 11 May 2016 11:18:46 Pranay Kr. Srivastava wrote:
> >> 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.
> >>
> >> The change is described below:
> >> a) Add a users count to nbd_device structure.
> >> b) Add a bit flag to nbd_device structure of unsigned long.
> >>
> >> If the current user count is not 1 then make nbd-client wait
> >> for the in_use bit to be cleared.
> >
> > Thanks, I like this approach much more.
> >
> >>
> >> Signed-off-by: Pranay Kr. Srivastava <pranjas@xxxxxxxxx>
> >> ---
> >>  drivers/block/nbd.c      | 40 ++++++++++++++++++++++++++++++++++++++++
> >>  include/uapi/linux/nbd.h |  1 +
> >>  2 files changed, 41 insertions(+)
> >>
> >> diff --git a/drivers/block/nbd.c b/drivers/block/nbd.c
> >> index 482a3c0..9b024d8 100644
> >> --- a/drivers/block/nbd.c
> >> +++ b/drivers/block/nbd.c
> >> @@ -59,6 +59,7 @@ struct nbd_device {
> >>       int xmit_timeout;
> >>       atomic_t timedout;
> >>       bool disconnect; /* a disconnect has been requested by user */
> >> +     u32 users;
> >
> > Perhaps it is better to use kref for this?
> 
> Yup will do.
> 
> >
> >>
> >>       struct timer_list timeout_timer;
> >>       /* protects initialization and shutdown of the socket */
> >> @@ -69,6 +70,7 @@ struct nbd_device {
> >>  #if IS_ENABLED(CONFIG_DEBUG_FS)
> >>       struct dentry *dbg_dir;
> >>  #endif
> >> +     unsigned long bflags;   /* word size bit flags for use. */
> >
> > Maybe it is better to use a completion instead of a bitfield.
> 
> Ok.
> 
> >
> >>  };
> >>
> >>  #if IS_ENABLED(CONFIG_DEBUG_FS)
> >> @@ -822,6 +824,15 @@ static int __nbd_ioctl(struct block_device *bdev, struct nbd_device *nbd,
> >>               sock_shutdown(nbd);
> >>               mutex_lock(&nbd->tx_lock);
> >>               nbd_clear_que(nbd);
> >> +             /*
> >> +              * Wait for any users currently using
> >> +              * this block device.
> >> +              */
> >> +             mutex_unlock(&nbd->tx_lock);
> >> +             pr_info("Waiting for users to release device %s ...\n",
> >> +                                             bdev->bd_disk->disk_name);
> >> +             wait_on_bit(&nbd->bflags, NBD_BFLAG_INUSE_BIT, TASK_INTERRUPTIBLE);
> >> +             mutex_lock(&nbd->tx_lock);
> >>               kill_bdev(bdev);
> >>               nbd_bdev_reset(bdev);
> >>
> >> @@ -870,10 +881,39 @@ 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_dev = bdev->bd_disk->private_data;
> >
> > Here is a new line missing otherwise checkpatch will probably warn about
> > this?
> >
> > Should we check here if we are connected here? And check whether the
> > connection is about to be closed?
> 
> I don't think it should matter if we are connected or not. We already
> handle that case
> correctly. Requests would fail and those failures will be reported to
> userland. The idea
> here is not to enforce "harsh measures" on the user on failure but to
> report them instead.

I see. Yes that's fine.

Thanks,

Markus

> 
> >
> > Best Regards,
> >
> > Markus
> 
> I'll send in a new patch after making changes as per your review.
> 
> Thanks alot!.
> 
> >
> >> +     nbd_dev->users++;
> >> +     pr_debug("Opening nbd_dev %s. Active users = %u\n",
> >> +                     bdev->bd_disk->disk_name, nbd_dev->users);
> >> +     if (nbd_dev->users > 1)
> >> +     {
> >> +             set_bit(NBD_BFLAG_INUSE_BIT, &nbd_dev->bflags);
> >> +     }
> >> +     return 0;
> >> +}
> >> +
> >> +static void nbd_release(struct gendisk *disk, fmode_t mode)
> >> +{
> >> +     struct nbd_device *nbd_dev = disk->private_data;
> >> +     nbd_dev->users--;
> >> +     pr_debug("Closing nbd_dev %s. Active users = %u\n",
> >> +                     disk->disk_name, nbd_dev->users);
> >> +     if (nbd_dev->users == 1)
> >> +     {
> >> +             clear_bit(NBD_BFLAG_INUSE_BIT, &nbd_dev->bflags);
> >> +             smp_mb();
> >> +             wake_up_bit(&nbd_dev->bflags, NBD_BFLAG_INUSE_BIT);
> >> +     }
> >> +}
> >> +
> >>  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)
> >> diff --git a/include/uapi/linux/nbd.h b/include/uapi/linux/nbd.h
> >> index e08e413..8f3d3f0 100644
> >> --- a/include/uapi/linux/nbd.h
> >> +++ b/include/uapi/linux/nbd.h
> >> @@ -44,6 +44,7 @@ enum {
> >>  /* there is a gap here to match userspace */
> >>  #define NBD_FLAG_SEND_TRIM    (1 << 5) /* send trim/discard */
> >>
> >> +#define NBD_BFLAG_INUSE_BIT  (1) /* bit number for bflags */
> >>  /* userspace doesn't need the nbd_device structure */
> >>
> >>  /* These are sent over the network in the request/reply magic fields */
> >>
> >
> > --
> > 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 |
> 
> 
> 
> -- 
>         ---P.K.S
> 

-- 
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: PGP signature