Hi, sorry I couldn't fit the review into last week.
On Tuesday 24 May 2016 14:26:27 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. > > Use a kref for users using this. The device will > be released for kref count of 2, not less or more. > > Signed-off-by: Pranay Kr. Srivastava <[email protected]> > --- > drivers/block/nbd.c | 51 > +++++++++++++++++++++++++++++++++++++++++++++++++++ > 1 file changed, 51 insertions(+) > > diff --git a/drivers/block/nbd.c b/drivers/block/nbd.c > index af86c9b..59db890 100644 > --- a/drivers/block/nbd.c > +++ b/drivers/block/nbd.c > @@ -71,6 +71,8 @@ struct nbd_device { > struct dentry *dbg_dir; > #endif > struct work_struct ws_nbd; > + struct kref users; > + struct completion user_completion; > }; > > #if IS_ENABLED(CONFIG_DEBUG_FS) > @@ -674,6 +676,7 @@ static void nbd_reset(struct nbd_device *nbd) > nbd->flags = 0; > nbd->xmit_timeout = 0; > INIT_WORK(&nbd->ws_nbd, nbd_work_func); > + init_completion(&nbd->user_completion); > queue_flag_clear_unlocked(QUEUE_FLAG_DISCARD, nbd->disk->queue); > del_timer_sync(&nbd->timeout_timer); > } > @@ -807,6 +810,7 @@ static int __nbd_ioctl(struct block_device *bdev, struct > nbd_device *nbd, > kthread_stop(thread); > > sock_shutdown(nbd); > + wait_for_completion(&nbd->user_completion); > mutex_lock(&nbd->tx_lock); > nbd_clear_que(nbd); > kill_bdev(bdev); > @@ -858,12 +862,58 @@ static int nbd_ioctl(struct block_device *bdev, fmode_t > mode, > return error; > } > > +static void nbd_kref_release(struct kref *kref_users) > +{ > + struct nbd_device *nbd = container_of(kref_users, struct nbd_device, > + users); > + pr_debug("Releasing kref [%s]\n", __FUNCTION__); > + complete(&nbd->user_completion); > + > +} > + > +static int nbd_open(struct block_device *bdev, fmode_t mode) > +{ > + struct nbd_device *nbd_dev = bdev->bd_disk->private_data; > + > + kref_get(&nbd_dev->users); > + pr_debug("Opening nbd_dev %s. Active users = %u\n", > + bdev->bd_disk->disk_name, > + atomic_read(&nbd_dev->users.refcount) - 1); > + return 0; > +} > + > +static void nbd_release(struct gendisk *disk, fmode_t mode) > +{ > + struct nbd_device *nbd_dev = disk->private_data; > + /* > + *kref_init initializes ref count to 1, so we > + *we check for refcount to be 2 for a final put. > + * > + *kref needs to be re-initialized just here as the > + *other process holding it must see the ref count as 2. > + */ > + kref_put(&nbd_dev->users, nbd_kref_release); > + > + if (atomic_read(&nbd_dev->users.refcount) == 2) { > + kref_sub(&nbd_dev->users, 2, nbd_kref_release); > + kref_init(&nbd_dev->users); > + kref_get(&nbd_dev->users); Reading the refcount directly seems not to be as it supposed to be. Why don't you put a kref_init() and kref_put() call into NBD_DO_IT? This way you don't have to work around the property that kref_init() starts with a refcount of 1 but you can use it. For example: NBD_DO_IT: kref_init() ... kref_put() nbd_thread_recv() and nbd_thread_send(): kref_get() ... kref_put() In nbd_open() you could use kref_get_unless_zero() to avoid opening a not connected device. nbd_release() would then be a very simple kref_put() without checking for 2 and so on. Also there are some checkpatch issues with this patch. Best Regards, Markus > + } > + > + pr_debug("Closing nbd_dev %s. Active users = %u\n", > + disk->disk_name, > + atomic_read(&nbd_dev->users.refcount) - 1); > +} > + > static const struct block_device_operations nbd_fops = { > .owner = THIS_MODULE, > .ioctl = nbd_ioctl, > .compat_ioctl = nbd_ioctl, > + .open = nbd_open, > + .release = nbd_release > }; > > + > static void nbd_work_func(struct work_struct *ws_nbd) > { > struct nbd_device *nbd_dev = container_of(ws_nbd, struct nbd_device, > @@ -1098,6 +1148,7 @@ static int __init nbd_init(void) > disk->first_minor = i << part_shift; > disk->fops = &nbd_fops; > disk->private_data = &nbd_dev[i]; > + kref_init(&nbd_dev[i].users); > sprintf(disk->disk_name, "nbd%d", i); > nbd_reset(&nbd_dev[i]); > add_disk(disk); > -- 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: This is a digitally signed message part.
------------------------------------------------------------------------------ What NetFlow Analyzer can do for you? Monitors network bandwidth and traffic patterns at an interface-level. Reveals which users, apps, and protocols are consuming the most bandwidth. Provides multi-vendor support for NetFlow, J-Flow, sFlow and other flows. Make informed decisions using capacity planning reports. https://ad.doubleclick.net/ddm/clk/305295220;132659582;e
_______________________________________________ Nbd-general mailing list [email protected] https://lists.sourceforge.net/lists/listinfo/nbd-general
