From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752509AbcF2HH1 (ORCPT ); Wed, 29 Jun 2016 03:07:27 -0400 Received: from metis.ext.4.pengutronix.de ([92.198.50.35]:44724 "EHLO metis.ext.4.pengutronix.de" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751057AbcF2HHZ (ORCPT ); Wed, 29 Jun 2016 03:07:25 -0400 From: Markus Pargmann To: "Pranay Kr. Srivastava" Cc: nbd-general@lists.sourceforge.net, linux-kernel@vger.kernel.org, w@uter.be Subject: Re: [PATCH 3/3]nbd: make nbd device wait for its users Date: Wed, 29 Jun 2016 09:06:53 +0200 Message-ID: <13976789.IcbFSv43Dd@adelgunde> User-Agent: KMail/4.14.1 (Linux/4.6.0-0.bpo.1-amd64; KDE/4.14.2; x86_64; ; ) In-Reply-To: <1466762976-12648-4-git-send-email-pranjas@gmail.com> References: <1962682.lYRJ5o9hTF@adelgunde> <1466762976-12648-1-git-send-email-pranjas@gmail.com> <1466762976-12648-4-git-send-email-pranjas@gmail.com> MIME-Version: 1.0 Content-Type: multipart/signed; boundary="nextPart2070064.kvbYd4iSzx"; micalg="pgp-sha256"; protocol="application/pgp-signature" X-SA-Exim-Connect-IP: 2001:67c:670:100:a61f:72ff:fe68:75ba X-SA-Exim-Mail-From: mpa@pengutronix.de X-SA-Exim-Scanned: No (on metis.ext.pengutronix.de); SAEximRunCond expanded to false X-PTX-Original-Recipient: linux-kernel@vger.kernel.org Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org --nextPart2070064.kvbYd4iSzx Content-Transfer-Encoding: quoted-printable Content-Type: text/plain; charset="us-ascii" 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. >=20 > This is more required when filesystem(s) like > ext2 or ext3 don't expect their buffer heads to > disappear while the filesystem is mounted. >=20 > 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. >=20 > 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 previou= s patch) and unrelated code. Please keep only the things that are 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. Best Regards, Markus >=20 > Signed-off-by: Pranay Kr. Srivastava > --- > drivers/block/nbd.c | 124 ++++++++++++++++++++++++++++++++++++++++--= =2D--------- > 1 file changed, 96 insertions(+), 28 deletions(-) >=20 > 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) > =09struct dentry *dbg_dir; > #endif > + > =09/* > -=09*This is specifically for calling sock_shutdown, for now. > -=09*/ > +=09 *This is specifically for calling sock_shutdown, for now. > +=09 */ > =09struct work_struct ws_shutdown; > +=09struct kref users; > +=09struct completion user_completion; > }; > =20 > #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 *= ); > =20 > static inline struct device *nbd_to_dev(struct nbd_device *nbd) > { > @@ -129,7 +134,7 @@ static const char *nbdcmd_to_ascii(int cmd) > =20 > static int nbd_size_clear(struct nbd_device *nbd, struct block_devic= e *bdev) > { > -=09bdev->bd_inode->i_size =3D 0; > +=09i_size_write(bdev->bd_inode, 0); > =09set_capacity(nbd->disk, 0); > =09kobject_uevent(&nbd_to_dev(nbd)->kobj, KOBJ_CHANGE); > =20 > @@ -141,7 +146,7 @@ static void nbd_size_update(struct nbd_device *nb= d, struct block_device *bdev) > =09if (!nbd_is_connected(nbd)) > =09=09return; > =20 > -=09bdev->bd_inode->i_size =3D nbd->bytesize; > +=09i_size_write(bdev->bd_inode, nbd->bytesize); > =09set_capacity(nbd->disk, nbd->bytesize >> 9); > =09kobject_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, > =09=09=09int blocksize, int nr_blocks) > { > =09int ret; > - > =09ret =3D set_blocksize(bdev, blocksize); > =09if (ret) > =09=09return ret; > - > =09nbd->blksize =3D blocksize; > =09nbd->bytesize =3D (loff_t)blocksize * (loff_t)nr_blocks; > =20 > @@ -202,14 +205,19 @@ static void nbd_xmit_timeout(unsigned long arg)= > { > =09struct nbd_device *nbd =3D (struct nbd_device *)arg; > =20 > +=09if (nbd->timedout) > +=09=09return; > + > =09if (list_empty(&nbd->queue_head)) > =09=09return; > + > =09nbd->timedout =3D true; > -=09schedule_work(&nbd->ws_shutdown); > + > =09/* > =09 * Make sure sender thread sees nbd->timedout. > =09 */ > =09smp_wmb(); > +=09schedule_work(&nbd->ws_shutdown); > =09wake_up(&nbd->waiting_wq); > =09dev_err(nbd_to_dev(nbd), "Connection timed out, shutting down con= nection\n"); > } > @@ -476,8 +484,6 @@ static int nbd_thread_recv(struct nbd_device *nbd= , struct block_device *bdev) > =09=09nbd_end_request(nbd, req); > =09} > =20 > -=09nbd_size_clear(nbd, bdev); > - > =09device_remove_file(disk_to_dev(nbd->disk), &dev_attr_pid); > =20 > =09nbd->task_recv =3D NULL; > @@ -580,8 +586,8 @@ static int nbd_thread_send(void *data) > =09while (!kthread_should_stop() || !list_empty(&nbd->waiting_queue)= ) { > =09=09/* wait for something to do */ > =09=09wait_event_interruptible(nbd->waiting_wq, > -=09=09=09=09kthread_should_stop() || > -=09=09=09=09!list_empty(&nbd->waiting_queue)); > +=09=09=09=09=09 kthread_should_stop() || > +=09=09=09=09=09 !list_empty(&nbd->waiting_queue)); > =20 > =09=09/* extract request */ > =09=09if (list_empty(&nbd->waiting_queue)) > @@ -589,11 +595,11 @@ static int nbd_thread_send(void *data) > =20 > =09=09spin_lock_irq(&nbd->queue_lock); > =09=09req =3D list_entry(nbd->waiting_queue.next, struct request, > -=09=09=09=09queuelist); > +=09=09=09=09 queuelist); > =09=09list_del_init(&req->queuelist); > =09=09spin_unlock_irq(&nbd->queue_lock); > =20 > -=09=09nbd_handle_req(nbd, req); > +=09=09/* handle request */ > =09=09if (nbd->timedout) { > =09=09=09req->errors++; > =09=09=09nbd_end_request(nbd, req); > @@ -654,12 +660,13 @@ static int nbd_set_socket(struct nbd_device *nb= d, struct socket *sock) > =09int ret =3D 0; > =20 > =09spin_lock(&nbd->sock_lock); > -=09if (nbd->sock) > + > +=09if (nbd->sock || nbd->timedout) > =09=09ret =3D -EBUSY; > =09else > =09=09nbd->sock =3D sock; > -=09spin_unlock(&nbd->sock_lock); > =20 > +=09spin_unlock(&nbd->sock_lock); > =09return ret; > } > =20 > @@ -674,6 +681,7 @@ static void nbd_reset(struct nbd_device *nbd) > =09nbd->flags =3D 0; > =09nbd->xmit_timeout =3D 0; > =09INIT_WORK(&nbd->ws_shutdown, nbd_ws_func_shutdown); > +=09init_completion(&nbd->user_completion); > =09queue_flag_clear_unlocked(QUEUE_FLAG_DISCARD, nbd->disk->queue); > =09del_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, > =09=09 unsigned int cmd, unsigned long arg) > { > +=09if (nbd->timedout || nbd->disconnect) > +=09=09return -EBUSY; > + > =09switch (cmd) { > =09case NBD_DISCONNECT: { > =09=09struct request sreq; > @@ -737,7 +748,6 @@ static int __nbd_ioctl(struct block_device *bdev,= struct nbd_device *nbd, > =09=09nbd_clear_que(nbd); > =09=09BUG_ON(!list_empty(&nbd->queue_head)); > =09=09BUG_ON(!list_empty(&nbd->waiting_queue)); > -=09=09kill_bdev(bdev); > =09=09return 0; > =20 > =09case NBD_SET_SOCK: { > @@ -756,7 +766,6 @@ static int __nbd_ioctl(struct block_device *bdev,= struct nbd_device *nbd, > =20 > =09case NBD_SET_BLKSIZE: { > =09=09loff_t bsize =3D div_s64(nbd->bytesize, arg); > - > =09=09return nbd_size_set(nbd, bdev, arg, bsize); > =09} > =20 > @@ -808,22 +817,29 @@ static int __nbd_ioctl(struct block_device *bde= v, struct nbd_device *nbd, > =09=09error =3D nbd_thread_recv(nbd, bdev); > =09=09nbd_dev_dbg_close(nbd); > =09=09kthread_stop(thread); > -=09=09sock_shutdown(nbd); > =20 > -=09=09mutex_lock(&nbd->tx_lock); > -=09=09nbd->task_recv =3D NULL; > - > -=09=09nbd_clear_que(nbd); > -=09=09kill_bdev(bdev); > -=09=09nbd_bdev_reset(bdev); > +=09=09sock_shutdown(nbd); > =20 > =09=09if (nbd->disconnect) /* user requested, ignore socket errors *= / > =09=09=09error =3D 0; > =09=09if (nbd->timedout) > =09=09=09error =3D -ETIMEDOUT; > =20 > -=09=09nbd_reset(nbd); > +=09=09mutex_lock(&nbd->tx_lock); > +=09=09nbd_clear_que(nbd); > +=09=09nbd->disconnect =3D true; /* To kill bdev*/ > +=09=09mutex_unlock(&nbd->tx_lock); > +=09=09cancel_work_sync(&nbd->ws_shutdown); > +=09=09kref_put(&nbd->users, nbd_kref_release); > +=09=09wait_for_completion(&nbd->user_completion); > =20 > +=09=09mutex_lock(&bdev->bd_mutex); > +=09=09if (!kref_get_unless_zero(&nbd->users)) > +=09=09=09kref_init(&nbd->users); > +=09=09mutex_unlock(&bdev->bd_mutex); > + > +=09=09mutex_lock(&nbd->tx_lock); > +=09=09nbd_reset(nbd); > =09=09return error; > =09} > =20 > @@ -861,19 +877,71 @@ static int nbd_ioctl(struct block_device *bdev,= fmode_t mode, > =20 > =09return error; > } > +static void nbd_kref_release(struct kref *kref_users) > +{ > +=09struct nbd_device *nbd =3D container_of(kref_users, struct nbd_de= vice, > +=09=09=09users); > +=09schedule_work(&nbd->ws_shutdown); > +} > + > +static int nbd_open(struct block_device *bdev, fmode_t mode) > +{ > +=09struct nbd_device *nbd_dev =3D bdev->bd_disk->private_data; > + > +=09if (!kref_get_unless_zero(&nbd_dev->users)) > +=09=09kref_init(&nbd_dev->users); > + > +=09pr_debug("Opening nbd_dev %s. Active users =3D %u\n", > +=09=09=09bdev->bd_disk->disk_name, > +=09=09=09atomic_read(&nbd_dev->users.refcount) > +=09=09); > +=09return 0; > +} > + > +static void nbd_release(struct gendisk *disk, fmode_t mode) > +{ > +=09struct nbd_device *nbd_dev =3D disk->private_data; > + > +=09kref_put(&nbd_dev->users, nbd_kref_release); > + > +=09pr_debug("Closing nbd_dev %s. Active users =3D %u\n", > +=09=09=09disk->disk_name, > +=09=09=09atomic_read(&nbd_dev->users.refcount) > +=09=09); > +} > =20 > static const struct block_device_operations nbd_fops =3D { > =09.owner =3D=09THIS_MODULE, > =09.ioctl =3D=09nbd_ioctl, > =09.compat_ioctl =3D=09nbd_ioctl, > +=09.open =3D=09=09nbd_open, > +=09.release =3D=09nbd_release > }; > =20 > + > static void nbd_ws_func_shutdown(struct work_struct *ws_nbd) > { > =09struct nbd_device *nbd_dev =3D container_of(ws_nbd, struct nbd_de= vice, > -=09=09=09ws_shutdown); > - > -=09sock_shutdown(nbd_dev); > +=09=09=09=09=09=09=09ws_shutdown); > +=09struct block_device *bdev =3D bdget(part_devt( > +=09=09=09=09=09=09dev_to_part(nbd_to_dev(nbd_dev)) > +=09=09=09=09=09=09) > +=09=09=09=09=09); > +=09BUG_ON(!bdev); > +=09if (nbd_dev->timedout) > +=09=09sock_shutdown(nbd_dev); > + > +=09if (nbd_dev->disconnect) { > +=09=09mutex_lock(&nbd_dev->tx_lock); > +=09=09nbd_dev->task_recv =3D NULL; > +=09=09nbd_clear_que(nbd_dev); > +=09=09kill_bdev(bdev); > +=09=09nbd_bdev_reset(bdev); > +=09=09mutex_unlock(&nbd_dev->tx_lock); > +=09=09nbd_size_clear(nbd_dev, bdev); > +=09=09complete(&nbd_dev->user_completion); > +=09} > +=09bdput(bdev); > } > =20 > #if IS_ENABLED(CONFIG_DEBUG_FS) >=20 =2D-=20 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-555= 5 | --nextPart2070064.kvbYd4iSzx Content-Type: application/pgp-signature; name="signature.asc" Content-Description: This is a digitally signed message part. Content-Transfer-Encoding: 7Bit -----BEGIN PGP SIGNATURE----- Version: GnuPG v2 iQIcBAABCAAGBQJXc3ONAAoJENnm3voMNZul0voP/1iYA44rUkLJ1WhbLthSEoag 3qXLmsu1R8iWDr1W+Gcw8qQnyU32xkXGCDpKQlLS11BY4R4ZuQqV5ulzYe1OikKg RT5m4w6JDW3BqWcx3nHOR4SxNo4OmI7LwTakHY8xfcjj7IytTXEk5A/TDjU6t5/N obfLEUz4sSpr7kbFCd7h5Gnso3YGM5pBS+adSlD4RQOAoYlFGI0LgTTd68OBPq+S rSpZqBiwvhtxZapKzVDLByM+GBeotsNbLohwVZu2mfEFzXPcvqe4xV73oqkxAXrr 8FCdWqBI5dsDVg3286U+efHoIOi5LFbKAMEF6ALQeVh95UEYzVYs/nw3h6Eoh0Dq yMy92IjSWa3fDWdqDOC1k86f33fgVRmgA0rzB5Auw9o2TuMrUPhbjzhccst74AO+ LOJdDasSmI1AB2k9alhPgv6CcFCN33HodNwOfGTvGv6lgySdjAkMAB88LXGXvYyg Gv5DaHnpX8ZMIdkRu/WqgWw7/k6L15LiZA6q/X6CA3tVQRrE0eK7HtxZqTXgEDEq k7nTlsJYMhtws9gm8K5P4cMjCdEiGR8K3a9WBOpMT+XXS/xqZ9NozwRVt2FYDHNW 2lIyGM4jotPSWQofUeN83c34efKuP+tgxQvDwBPhfTCKPIvQtr+On/mslVYMEo8t dOzx0CNsBW29PRYXiZDy =65/o -----END PGP SIGNATURE----- --nextPart2070064.kvbYd4iSzx--