From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753444AbbKILKG (ORCPT ); Mon, 9 Nov 2015 06:10:06 -0500 Received: from metis.ext.4.pengutronix.de ([92.198.50.35]:44913 "EHLO metis.ext.pengutronix.de" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750788AbbKILKB (ORCPT ); Mon, 9 Nov 2015 06:10:01 -0500 From: Markus Pargmann To: Oleg Nesterov Cc: nbd-general@lists.sourceforge.net, Christoph Hellwig , linux-kernel@vger.kernel.org Subject: Re: [PATCH 0/4] nbd: Remove signal usage Date: Mon, 09 Nov 2015 12:09:50 +0100 Message-ID: <2430905.RrJy3SRkBX@adelgunde> User-Agent: KMail/4.14.1 (Linux/3.16.0-4-amd64; KDE/4.14.2; x86_64; ; ) In-Reply-To: <20151101190500.GA1019@redhat.com> References: <1446133360-30652-1-git-send-email-mpa@pengutronix.de> <20151101190500.GA1019@redhat.com> MIME-Version: 1.0 Content-Type: multipart/signed; boundary="nextPart1622037.B0H8Hf4I5C"; 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 --nextPart1622037.B0H8Hf4I5C Content-Transfer-Encoding: quoted-printable Content-Type: text/plain; charset="us-ascii" Hi Oleg, On Sunday 01 November 2015 20:05:00 Oleg Nesterov wrote: > Hi Markus, >=20 > Sorry again for delay. I was offlist. again. Sorry I hadn't too much time last week. >=20 > On 10/29, Markus Pargmann wrote: > > > > Hi, > > > > this is a try to remove all the signals from NBD. The first patch r= eplaces the > > signals. The other patches are some cleanups I made on the way. > > > > This should solve the kthread_run() problems as well. >=20 > I obviously can't review these changes, I do not understand this code= > enough. But they look good imo. Thanks for having a look. >=20 > However, I do not understand the usage of ->task_recv and ->task_send= . >=20 > pid_show() doesn't even check nbd->task_recv !=3D NULL. Honestly, I s= imply > do not know if it can race with device_remove_file() or not. I think = it > can, but I can be easily wrong... pid_show() should hopefully not be the problem. The 'pid' file which us= es pid_show() is created after task_recv was set and is removed using device_remove_file(). Assuming that there are no open calls to pid_show= () after device_remove_file() was called this setup should not have a race issue= . >=20 > nbd_dbg_tasks_show() looks racy too even if it checks task_recv/task_= send, > at least this needs READ_ONCE() but in theory this is not enough, > task_pid_nr() can read the freed task_struct. Yes it requires a READ_ONCE(). But it is not possible for task_pid_nr t= o use the freed task_struct. With the patches I posted, the send thread is ke= pt alive until kthread_stop() is called. The debugfs files are removed before th= e send thread is stopped. So if there is a task struct it is valid. >=20 > Again, I can easily miss something. But whatever I missed, perhaps th= e > trivial (but uncompiled/untested) patch below makes sense anyway? Yes as we don't need the task_struct anymore to send signals it makes s= ense. Best Regards, Markus >=20 > Oleg. >=20 >=20 > diff --git a/drivers/block/nbd.c b/drivers/block/nbd.c > index f547005..67c1e09 100644 > --- a/drivers/block/nbd.c > +++ b/drivers/block/nbd.c > @@ -63,8 +63,8 @@ struct nbd_device { > =09struct timer_list timeout_timer; > =09/* protects initialization and shutdown of the socket */ > =09spinlock_t sock_lock; > -=09struct task_struct *task_recv; > -=09struct task_struct *task_send; > +=09pid_t task_recv; > +=09pid_t task_send; > =20 > #if IS_ENABLED(CONFIG_DEBUG_FS) > =09struct dentry *dbg_dir; > @@ -392,7 +392,7 @@ static ssize_t pid_show(struct device *dev, > =09struct gendisk *disk =3D dev_to_disk(dev); > =09struct nbd_device *nbd =3D (struct nbd_device *)disk->private_dat= a; > =20 > -=09return sprintf(buf, "%d\n", task_pid_nr(nbd->task_recv)); > +=09return sprintf(buf, "%d\n", nbd->task_recv); > } > =20 > static struct device_attribute pid_attr =3D { > @@ -409,13 +409,13 @@ static int nbd_thread_recv(struct nbd_device *n= bd) > =20 > =09sk_set_memalloc(nbd->sock->sk); > =20 > -=09nbd->task_recv =3D current; > +=09nbd->task_recv =3D task_pid_nr(current); > =20 > =09ret =3D device_create_file(disk_to_dev(nbd->disk), &pid_attr); > =09if (ret) { > =09=09dev_err(disk_to_dev(nbd->disk), "device_create_file failed!\n"= ); > =20 > -=09=09nbd->task_recv =3D NULL; > +=09=09nbd->task_recv =3D 0; > =20 > =09=09return ret; > =09} > @@ -432,7 +432,7 @@ static int nbd_thread_recv(struct nbd_device *nbd= ) > =20 > =09device_remove_file(disk_to_dev(nbd->disk), &pid_attr); > =20 > -=09nbd->task_recv =3D NULL; > +=09nbd->task_recv =3D 0; > =20 > =09return ret; > } > @@ -526,7 +526,7 @@ static int nbd_thread_send(void *data) > =09struct nbd_device *nbd =3D data; > =09struct request *req; > =20 > -=09nbd->task_send =3D current; > +=09nbd->task_send =3D task_pid_nr(current); > =20 > =09set_user_nice(current, MIN_NICE); > =09while (!kthread_should_stop() || !list_empty(&nbd->waiting_queue)= ) { > @@ -549,7 +549,7 @@ static int nbd_thread_send(void *data) > =09=09nbd_handle_req(nbd, req); > =09} > =20 > -=09nbd->task_send =3D NULL; > +=09nbd->task_send =3D 0; > =20 > =09return 0; > } > @@ -827,9 +827,9 @@ static int nbd_dbg_tasks_show(struct seq_file *s,= void *unused) > =09struct nbd_device *nbd =3D s->private; > =20 > =09if (nbd->task_recv) > -=09=09seq_printf(s, "recv: %d\n", task_pid_nr(nbd->task_recv)); > +=09=09seq_printf(s, "recv: %d\n", nbd->task_recv); > =09if (nbd->task_send) > -=09=09seq_printf(s, "send: %d\n", task_pid_nr(nbd->task_send)); > +=09=09seq_printf(s, "send: %d\n", nbd->task_send); > =20 > =09return 0; > } >=20 >=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 | --nextPart1622037.B0H8Hf4I5C 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 iQIcBAABCAAGBQJWQH7+AAoJEEpcgKtcEGQQ6RMP/0PfsWRWGS+Mzm2lsvwOQi7y 0pGuI1cAVFGGZattoxggZ9CjCJRj842gj8t9YqdkGsdSLc+l9f5SHyme2bLlJW+V aLeOOaFprxMwl0nyNsfbzedjHjA1Vno08jLg84uQPIbdEAHJWT7NTPkrlpYaphiF wI8ROknpcvep+75OdeEZJNXxRQoIi2IQAM/pFc0kx3X2zTbtYbujIhtYzJgBDYTB N8O+I1bSKdsKdxT2VTHnURykEprvSrlalbV51GCkikVXgIUjDh7MLIDlooh8pN24 GWLJ7L/VuuXbnclSxQHv1RzjSBIOo186k4810nDyDyh97PEKHQwT9nA0tCF/0D+s lRJqvrtzvEnkEphY9pOyc4zM4iX1DLowi7NwkA+UZK7mHko8T+2AP/H6GiRQhek/ 7zj988fsq/P92fxlE8oncp7iowjQv9egld/sTSKRvmEGeJJxPVD/YtT8IrC6CGI+ blW5A5zjBANIkiUyoXRWgG11ptWuZn9g2iLMm4lk9WPe50JI0ncbv7DCjGhcOQgN ZgsTysXncszoun3sJCoY/bC4RE6jYjXk+VVXGZWZfBHYglqVci8fqO2sEKPuwcgS NHs3q4SLUpJ/GOprQpx6U+9KK51Jhbp43qxumNimSOaiCRHeniNd1V2i2hz22GI7 1cOeO1VF+/ATpmU9pOhu =H6In -----END PGP SIGNATURE----- --nextPart1622037.B0H8Hf4I5C--