From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753833AbcESGgb (ORCPT ); Thu, 19 May 2016 02:36:31 -0400 Received: from metis.ext.4.pengutronix.de ([92.198.50.35]:49970 "EHLO metis.ext.4.pengutronix.de" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751948AbcESGga (ORCPT ); Thu, 19 May 2016 02:36:30 -0400 Date: Thu, 19 May 2016 08:36:27 +0200 From: Markus Pargmann To: Pranay Srivastava Cc: nbd-general@lists.sourceforge.net, linux-kernel@vger.kernel.org, Greg KH Subject: Re: [PATCH v4 18/18] make nbd device wait for its users in case of timeout Message-ID: <20160519063627.GF19642@pengutronix.de> References: <1462954726-11825-1-git-send-email-pranjas@gmail.com> <1462954726-11825-19-git-send-email-pranjas@gmail.com> <28709789.5mkdsd5mKW@adelgunde> MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha1; protocol="application/pgp-signature"; boundary="oPmsXEqKQNHCSXW7" Content-Disposition: inline In-Reply-To: X-Sent-From: Pengutronix Hildesheim X-URL: http://www.pengutronix.de/ X-IRC: #ptxdist @freenode X-Accept-Language: de,en X-Accept-Content-Type: text/plain X-Uptime: 08:35:25 up 18 days, 15:37, 115 users, load average: 0.71, 1.11, 1.36 User-Agent: Mutt/1.5.24 (2015-08-30) X-SA-Exim-Connect-IP: 2001:67c:670:100:1d::7 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 --oPmsXEqKQNHCSXW7 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline Content-Transfer-Encoding: quoted-printable 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 wro= te: > > 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 > >> --- > >> 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? >=20 > Yup will do. >=20 > > > >> > >> 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. >=20 > Ok. >=20 > > > >> }; > >> > >> #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_INTE= RRUPTIBLE); > >> + 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 =3D 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? >=20 > 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 >=20 > > > > Best Regards, > > > > Markus >=20 > I'll send in a new patch after making changes as per your review. >=20 > Thanks alot!. >=20 > > > >> + nbd_dev->users++; > >> + pr_debug("Opening nbd_dev %s. Active users =3D %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 =3D disk->private_data; > >> + nbd_dev->users--; > >> + pr_debug("Closing nbd_dev %s. Active users =3D %u\n", > >> + disk->disk_name, nbd_dev->users); > >> + if (nbd_dev->users =3D=3D 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 =3D { > >> .owner =3D THIS_MODULE, > >> .ioctl =3D nbd_ioctl, > >> .compat_ioctl =3D nbd_ioctl, > >> + .open =3D nbd_open, > >> + .release =3D 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-555= 5 | >=20 >=20 >=20 > --=20 > ---P.K.S >=20 --=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-5555 | --oPmsXEqKQNHCSXW7 Content-Type: application/pgp-signature; name="signature.asc" -----BEGIN PGP SIGNATURE----- Version: GnuPG v1 iQIcBAEBAgAGBQJXPV7rAAoJEEpcgKtcEGQQQ70QALkBb3TJNKw3TaTnWbMw5EQS hbOyhVYFhq7+G5mtoh3Jdf6fjbQb4egFSwJqdqpLRt/4W2XvEbbIrRUb+0WUQnNs 76627RJhvFFNef2Qlyy5/SxhqG33suQ4cICCNFTU4zBwOhrxFWrggRGNAnndUwRu I080RsklnMYVZbU6i3sB58f730TYtKbJmUakdcdloK3BBIz7zl/zYhTbGIO2sh+b NR1q3PsPLBmblk/Y+M+2JVEsjRFe3GswwqqN4WQ3HZFeYhYSuywItxwGgq1eOAGw mteGmXGrKxmv9Z1YkZbd8GeBDLsF8zJz6ZNh2XEimD06Xziigyq1HALgn9/y2SG0 qHTZdLxC03j7OToSLdlNeKqrA7KNV36jFHpLxzcJjGavBWgncq+7ZTljjtfjfYPA 0PuUlVYDgX73FnvAZCNeq5fajIFbAT3iMZQiBFNQbf0GDIlFS7wgKiAQnkoU3n1d 1fTHyXZEsTy0r6+jUB+RuYAKLlO+c/LkCGz2Q++0AatBONcagSaLYObETcoJjJD6 ZWIgweai9CtT01RG+x3XLHGHC2Xs5hBbDDeJm9gf7rjrxT4e+Qyr//HWy+wE9fcD iPrwYXkjHTBj4es24rRyE+bFaxtXMQsTF6IZ4VtXnewqUlgXrvVLpAaR9ZwaWfp4 Ed54qcZiTgG53rgCw06M =y2gH -----END PGP SIGNATURE----- --oPmsXEqKQNHCSXW7--