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@...1897...> 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@...17...> > >> --- > >> 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