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 <[email protected]> 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 <[email protected]> > >> --- > >> 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 |
signature.asc
Description: PGP signature
