From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752724AbcF2HPG (ORCPT ); Wed, 29 Jun 2016 03:15:06 -0400 Received: from mail-wm0-f66.google.com ([74.125.82.66]:33890 "EHLO mail-wm0-f66.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751173AbcF2HPD (ORCPT ); Wed, 29 Jun 2016 03:15:03 -0400 MIME-Version: 1.0 In-Reply-To: <13976789.IcbFSv43Dd@adelgunde> References: <1962682.lYRJ5o9hTF@adelgunde> <1466762976-12648-1-git-send-email-pranjas@gmail.com> <1466762976-12648-4-git-send-email-pranjas@gmail.com> <13976789.IcbFSv43Dd@adelgunde> From: Pranay Srivastava Date: Wed, 29 Jun 2016 12:45:00 +0530 Message-ID: Subject: Re: [PATCH 3/3]nbd: make nbd device wait for its users To: Markus Pargmann Cc: nbd-general@lists.sourceforge.net, linux-kernel@vger.kernel.org, Wouter Verhelst Content-Type: text/plain; charset=UTF-8 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Hi On Wed, Jun 29, 2016 at 12:36 PM, Markus Pargmann wrote: > Hi, > > On Friday 24 June 2016 13:09:36 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. >> >> Each open of a nbd device is refcounted, while >> the userland program [nbd-client] doing the >> NBD_DO_IT ioctl would now wait for any other users >> of this device before invalidating the nbd device. >> >> A timedout or a disconnected device, if in use, can't >> be used until it has been resetted. The resetting happens >> when all tasks having this bdev open closes this bdev. > > Sorry, but this patch is unreadable. You are changing so many unrelated > whitespaces, lines, comments (that you introduced yourself in a previous > patch) and unrelated code. Please keep only the things that are Yes you are right. > necessary for this single patch. Everything else can go into different > patches. Also it would be good to run checkpatch sometimes. > > Also using your own atomic implementation instead of kref would be good > perhaps. Although I thought kref would work at the beginning but it > seems not to. I was able to make it work and it turned out to be simpler. Do you think you can go over this patch again just for the kref part? Should I resend this patch again? > > Best Regards, > > Markus > >> >> Signed-off-by: Pranay Kr. Srivastava >> --- >> drivers/block/nbd.c | 124 ++++++++++++++++++++++++++++++++++++++++------------ >> 1 file changed, 96 insertions(+), 28 deletions(-) >> >> diff --git a/drivers/block/nbd.c b/drivers/block/nbd.c >> index 9223b09..0587bbd 100644 >> --- a/drivers/block/nbd.c >> +++ b/drivers/block/nbd.c >> @@ -70,10 +70,13 @@ struct nbd_device { >> #if IS_ENABLED(CONFIG_DEBUG_FS) >> struct dentry *dbg_dir; >> #endif >> + >> /* >> - *This is specifically for calling sock_shutdown, for now. >> - */ >> + *This is specifically for calling sock_shutdown, for now. >> + */ >> struct work_struct ws_shutdown; >> + struct kref users; >> + struct completion user_completion; >> }; >> >> #if IS_ENABLED(CONFIG_DEBUG_FS) >> @@ -104,6 +107,8 @@ static DEFINE_SPINLOCK(nbd_lock); >> * Shutdown function for nbd_dev work struct. >> */ >> static void nbd_ws_func_shutdown(struct work_struct *); >> +static void nbd_kref_release(struct kref *); >> +static int nbd_size_clear(struct nbd_device *, struct block_device *); >> >> static inline struct device *nbd_to_dev(struct nbd_device *nbd) >> { >> @@ -129,7 +134,7 @@ static const char *nbdcmd_to_ascii(int cmd) >> >> static int nbd_size_clear(struct nbd_device *nbd, struct block_device *bdev) >> { >> - bdev->bd_inode->i_size = 0; >> + i_size_write(bdev->bd_inode, 0); >> set_capacity(nbd->disk, 0); >> kobject_uevent(&nbd_to_dev(nbd)->kobj, KOBJ_CHANGE); >> >> @@ -141,7 +146,7 @@ static void nbd_size_update(struct nbd_device *nbd, struct block_device *bdev) >> if (!nbd_is_connected(nbd)) >> return; >> >> - bdev->bd_inode->i_size = nbd->bytesize; >> + i_size_write(bdev->bd_inode, nbd->bytesize); >> set_capacity(nbd->disk, nbd->bytesize >> 9); >> kobject_uevent(&nbd_to_dev(nbd)->kobj, KOBJ_CHANGE); >> } >> @@ -150,11 +155,9 @@ static int nbd_size_set(struct nbd_device *nbd, struct block_device *bdev, >> int blocksize, int nr_blocks) >> { >> int ret; >> - >> ret = set_blocksize(bdev, blocksize); >> if (ret) >> return ret; >> - >> nbd->blksize = blocksize; >> nbd->bytesize = (loff_t)blocksize * (loff_t)nr_blocks; >> >> @@ -202,14 +205,19 @@ static void nbd_xmit_timeout(unsigned long arg) >> { >> struct nbd_device *nbd = (struct nbd_device *)arg; >> >> + if (nbd->timedout) >> + return; >> + >> if (list_empty(&nbd->queue_head)) >> return; >> + >> nbd->timedout = true; >> - schedule_work(&nbd->ws_shutdown); >> + >> /* >> * Make sure sender thread sees nbd->timedout. >> */ >> smp_wmb(); >> + schedule_work(&nbd->ws_shutdown); >> wake_up(&nbd->waiting_wq); >> dev_err(nbd_to_dev(nbd), "Connection timed out, shutting down connection\n"); >> } >> @@ -476,8 +484,6 @@ static int nbd_thread_recv(struct nbd_device *nbd, struct block_device *bdev) >> nbd_end_request(nbd, req); >> } >> >> - nbd_size_clear(nbd, bdev); >> - >> device_remove_file(disk_to_dev(nbd->disk), &dev_attr_pid); >> >> nbd->task_recv = NULL; >> @@ -580,8 +586,8 @@ static int nbd_thread_send(void *data) >> while (!kthread_should_stop() || !list_empty(&nbd->waiting_queue)) { >> /* wait for something to do */ >> wait_event_interruptible(nbd->waiting_wq, >> - kthread_should_stop() || >> - !list_empty(&nbd->waiting_queue)); >> + kthread_should_stop() || >> + !list_empty(&nbd->waiting_queue)); >> >> /* extract request */ >> if (list_empty(&nbd->waiting_queue)) >> @@ -589,11 +595,11 @@ static int nbd_thread_send(void *data) >> >> spin_lock_irq(&nbd->queue_lock); >> req = list_entry(nbd->waiting_queue.next, struct request, >> - queuelist); >> + queuelist); >> list_del_init(&req->queuelist); >> spin_unlock_irq(&nbd->queue_lock); >> >> - nbd_handle_req(nbd, req); >> + /* handle request */ >> if (nbd->timedout) { >> req->errors++; >> nbd_end_request(nbd, req); >> @@ -654,12 +660,13 @@ static int nbd_set_socket(struct nbd_device *nbd, struct socket *sock) >> int ret = 0; >> >> spin_lock(&nbd->sock_lock); >> - if (nbd->sock) >> + >> + if (nbd->sock || nbd->timedout) >> ret = -EBUSY; >> else >> nbd->sock = sock; >> - spin_unlock(&nbd->sock_lock); >> >> + spin_unlock(&nbd->sock_lock); >> return ret; >> } >> >> @@ -674,6 +681,7 @@ static void nbd_reset(struct nbd_device *nbd) >> nbd->flags = 0; >> nbd->xmit_timeout = 0; >> INIT_WORK(&nbd->ws_shutdown, nbd_ws_func_shutdown); >> + init_completion(&nbd->user_completion); >> queue_flag_clear_unlocked(QUEUE_FLAG_DISCARD, nbd->disk->queue); >> del_timer_sync(&nbd->timeout_timer); >> } >> @@ -708,6 +716,9 @@ static void nbd_dev_dbg_close(struct nbd_device *nbd); >> static int __nbd_ioctl(struct block_device *bdev, struct nbd_device *nbd, >> unsigned int cmd, unsigned long arg) >> { >> + if (nbd->timedout || nbd->disconnect) >> + return -EBUSY; >> + >> switch (cmd) { >> case NBD_DISCONNECT: { >> struct request sreq; >> @@ -737,7 +748,6 @@ static int __nbd_ioctl(struct block_device *bdev, struct nbd_device *nbd, >> nbd_clear_que(nbd); >> BUG_ON(!list_empty(&nbd->queue_head)); >> BUG_ON(!list_empty(&nbd->waiting_queue)); >> - kill_bdev(bdev); >> return 0; >> >> case NBD_SET_SOCK: { >> @@ -756,7 +766,6 @@ static int __nbd_ioctl(struct block_device *bdev, struct nbd_device *nbd, >> >> case NBD_SET_BLKSIZE: { >> loff_t bsize = div_s64(nbd->bytesize, arg); >> - >> return nbd_size_set(nbd, bdev, arg, bsize); >> } >> >> @@ -808,22 +817,29 @@ static int __nbd_ioctl(struct block_device *bdev, struct nbd_device *nbd, >> error = nbd_thread_recv(nbd, bdev); >> nbd_dev_dbg_close(nbd); >> kthread_stop(thread); >> - sock_shutdown(nbd); >> >> - mutex_lock(&nbd->tx_lock); >> - nbd->task_recv = NULL; >> - >> - nbd_clear_que(nbd); >> - kill_bdev(bdev); >> - nbd_bdev_reset(bdev); >> + sock_shutdown(nbd); >> >> if (nbd->disconnect) /* user requested, ignore socket errors */ >> error = 0; >> if (nbd->timedout) >> error = -ETIMEDOUT; >> >> - nbd_reset(nbd); >> + mutex_lock(&nbd->tx_lock); >> + nbd_clear_que(nbd); >> + nbd->disconnect = true; /* To kill bdev*/ >> + mutex_unlock(&nbd->tx_lock); >> + cancel_work_sync(&nbd->ws_shutdown); >> + kref_put(&nbd->users, nbd_kref_release); >> + wait_for_completion(&nbd->user_completion); >> >> + mutex_lock(&bdev->bd_mutex); >> + if (!kref_get_unless_zero(&nbd->users)) >> + kref_init(&nbd->users); >> + mutex_unlock(&bdev->bd_mutex); >> + >> + mutex_lock(&nbd->tx_lock); >> + nbd_reset(nbd); >> return error; >> } >> >> @@ -861,19 +877,71 @@ 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); >> + schedule_work(&nbd->ws_shutdown); >> +} >> + >> +static int nbd_open(struct block_device *bdev, fmode_t mode) >> +{ >> + struct nbd_device *nbd_dev = bdev->bd_disk->private_data; >> + >> + if (!kref_get_unless_zero(&nbd_dev->users)) >> + kref_init(&nbd_dev->users); >> + >> + pr_debug("Opening nbd_dev %s. Active users = %u\n", >> + bdev->bd_disk->disk_name, >> + atomic_read(&nbd_dev->users.refcount) >> + ); >> + return 0; >> +} >> + >> +static void nbd_release(struct gendisk *disk, fmode_t mode) >> +{ >> + struct nbd_device *nbd_dev = disk->private_data; >> + >> + kref_put(&nbd_dev->users, nbd_kref_release); >> + >> + pr_debug("Closing nbd_dev %s. Active users = %u\n", >> + disk->disk_name, >> + atomic_read(&nbd_dev->users.refcount) >> + ); >> +} >> >> 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_ws_func_shutdown(struct work_struct *ws_nbd) >> { >> struct nbd_device *nbd_dev = container_of(ws_nbd, struct nbd_device, >> - ws_shutdown); >> - >> - sock_shutdown(nbd_dev); >> + ws_shutdown); >> + struct block_device *bdev = bdget(part_devt( >> + dev_to_part(nbd_to_dev(nbd_dev)) >> + ) >> + ); >> + BUG_ON(!bdev); >> + if (nbd_dev->timedout) >> + sock_shutdown(nbd_dev); >> + >> + if (nbd_dev->disconnect) { >> + mutex_lock(&nbd_dev->tx_lock); >> + nbd_dev->task_recv = NULL; >> + nbd_clear_que(nbd_dev); >> + kill_bdev(bdev); >> + nbd_bdev_reset(bdev); >> + mutex_unlock(&nbd_dev->tx_lock); >> + nbd_size_clear(nbd_dev, bdev); >> + complete(&nbd_dev->user_completion); >> + } >> + bdput(bdev); >> } >> >> #if IS_ENABLED(CONFIG_DEBUG_FS) >> > > -- > 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