From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752016AbcFNI7V (ORCPT ); Tue, 14 Jun 2016 04:59:21 -0400 Received: from metis.ext.4.pengutronix.de ([92.198.50.35]:59856 "EHLO metis.ext.4.pengutronix.de" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751392AbcFNI7R (ORCPT ); Tue, 14 Jun 2016 04:59:17 -0400 From: Markus Pargmann To: "Pranay Kr. Srivastava" Cc: nbd-general@lists.sourceforge.net, linux-kernel@vger.kernel.org Subject: Re: [PATCH v2 4/5]nbd: make nbd device wait for its users. Date: Tue, 14 Jun 2016 10:59:14 +0200 Message-ID: <2280543.pBfpMHAWFW@adelgunde> User-Agent: KMail/4.14.1 (Linux/4.5.0-0.bpo.2-amd64; KDE/4.14.2; x86_64; ; ) In-Reply-To: <1464863101-16805-5-git-send-email-pranjas@gmail.com> References: <3898019.JRqDjBPssX@adelgunde> <1464863101-16805-1-git-send-email-pranjas@gmail.com> <1464863101-16805-5-git-send-email-pranjas@gmail.com> MIME-Version: 1.0 Content-Type: multipart/signed; boundary="nextPart1759475.z9MndbXCBo"; 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 --nextPart1759475.z9MndbXCBo Content-Transfer-Encoding: quoted-printable Content-Type: text/plain; charset="us-ascii" On Thursday 02 June 2016 13:25:00 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 > Signed-off-by: Pranay Kr. Srivastava > --- > drivers/block/nbd.c | 58 +++++++++++++++++++++++++++++++++++++++++++= ++++++++++ > 1 file changed, 58 insertions(+) >=20 > diff --git a/drivers/block/nbd.c b/drivers/block/nbd.c > index d1d898d..4da40dc 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 > +=09atomic_t inuse; > =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,7 @@ 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 *); > =20 > static inline struct device *nbd_to_dev(struct nbd_device *nbd) > { > @@ -682,6 +686,8 @@ 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); > +=09kref_init(&nbd->users); > =09queue_flag_clear_unlocked(QUEUE_FLAG_DISCARD, nbd->disk->queue); > =09del_timer_sync(&nbd->timeout_timer); > } > @@ -815,6 +821,14 @@ static int __nbd_ioctl(struct block_device *bdev= , struct nbd_device *nbd, > =09=09kthread_stop(thread); > =20 > =09=09sock_shutdown(nbd); > +=09=09/* > +=09=09 * kref_init initializes with ref count as 1, > +=09=09 * nbd_client, or the user-land program executing > +=09=09 * this ioctl will make the refcount to 2[at least] > +=09=09 * so subtracting 2 from refcount. > +=09=09 */ > +=09=09kref_sub(&nbd->users, 2, nbd_kref_release); Why don't you use a kref_put? > +=09=09wait_for_completion(&nbd->user_completion); > =09=09mutex_lock(&nbd->tx_lock); > =09=09nbd_clear_que(nbd); > =09=09kill_bdev(bdev); > @@ -865,13 +879,56 @@ 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); Not indented to opening bracket. > +=09pr_debug("Releasing kref [%s]\n", __func__); > +=09atomic_set(&nbd->inuse, 0); > +=09complete(&nbd->user_completion); > + > +} > + > +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=09atomic_set(&nbd_dev->inuse, 1); > + > +=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) - 1); Indent to opening bracket. > +=09return 0; > +} > + > +static void nbd_release(struct gendisk *disk, fmode_t mode) > +{ > +=09struct nbd_device *nbd_dev =3D disk->private_data; > +=09/* > +=09*kref_init initializes ref count to 1, so we > +=09*we check for refcount to be 2 for a final put. > +=09* > +=09*kref needs to be re-initialized just here as the > +=09*other process holding it must see the ref count as 2. > +=09*/ > +=09if (atomic_read(&nbd_dev->inuse)) > +=09=09kref_put(&nbd_dev->users, nbd_kref_release); What is this inuse atomic for? Everyone that releases the nbd device will need to execute a kref_put(). Best Regards, Markus > + > +=09pr_debug("Closing nbd_dev %s. Active users =3D %u\n", > +=09=09=09disk->disk_name, > +=09=09=09atomic_read(&nbd_dev->users.refcount) - 1); > +} > =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, > @@ -1107,6 +1164,7 @@ static int __init nbd_init(void) > =09=09disk->fops =3D &nbd_fops; > =09=09disk->private_data =3D &nbd_dev[i]; > =09=09sprintf(disk->disk_name, "nbd%d", i); > +=09=09atomic_set(&nbd_dev[i].inuse, 0); > =09=09nbd_reset(&nbd_dev[i]); > =09=09add_disk(disk); > =09} >=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 | --nextPart1759475.z9MndbXCBo 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 iQIcBAABCAAGBQJXX8diAAoJENnm3voMNZulL/oP/1Hlj0rfnIWiqoeTvo6/pYbR gCLYG5W7On0FwlMcupLyS/ob/G9cCxWninDAhzFo9LlVU7mq0/1MbfI1oRhPMff0 o+G4dk3NQQRgzG7tu7aMssEEH32J8yoOygwaEv1y0NSW/ACenNEsmH/vDRS2beUO cr2Xfyr8mReeIN+PduYq42DIC1SeYS1IdtIH32Ll1UtLx85ZXNgbr10VSfHTqaUl TH6stCEyVCdXl3HAUS11t9f8xEYXpu6OQJzbTzESaUo31q0tw2smcK5qenQb+Lgj O4D3UWy+WbT/xG4evxPRAklPbU17yTwD4lLIWsWp3/nWAyTvhKh160/mMm3ubOiV +YNOs2Jjb40F7dgv3CZZEHhxFaSokqC4tzRtesW/Sx97+jWxkfcZIwoo8Fo1i2Ua OFj1ZE5GFOkw72D1bseKGDGBXf1ycBtNqFrCzAi2wqTjHJDRtSHWsxfwKpliRMuR S05YnSqRVI3+g4SrCyGW7hW1WsVcYs4yquiGh0z6m7HsSxqk3J2mVINEwh750pOp 0sxNxlvlmS4egwnmBGB4EYsJXsD6L+As81HZZ2YNicaaAZxjkMrLQ/mwIGVtq3/b YgQGNoP9PwFaN9cW5BE7r1AVFwPzG5W89BenKnQKUAvCSYPfX/EbL+RlXDcwhigC TXFpRDXibWceMiLjDpJu =idje -----END PGP SIGNATURE----- --nextPart1759475.z9MndbXCBo--