Hi Oleg, On Sunday 01 November 2015 20:05:00 Oleg Nesterov wrote: > Hi Markus, > > Sorry again for delay. I was offlist. again. Sorry I hadn't too much time last week. > > On 10/29, Markus Pargmann wrote: > > > > Hi, > > > > this is a try to remove all the signals from NBD. The first patch replaces the > > signals. The other patches are some cleanups I made on the way. > > > > This should solve the kthread_run() problems as well. > > I obviously can't review these changes, I do not understand this code > enough. But they look good imo. Thanks for having a look. > > However, I do not understand the usage of ->task_recv and ->task_send. > > pid_show() doesn't even check nbd->task_recv != NULL. Honestly, I simply > 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 uses 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. > > 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 to use the freed task_struct. With the patches I posted, the send thread is kept alive until kthread_stop() is called. The debugfs files are removed before the send thread is stopped. So if there is a task struct it is valid. > > Again, I can easily miss something. But whatever I missed, perhaps the > trivial (but uncompiled/untested) patch below makes sense anyway? Yes as we don't need the task_struct anymore to send signals it makes sense. Best Regards, Markus > > Oleg. > > > 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 { > struct timer_list timeout_timer; > /* protects initialization and shutdown of the socket */ > spinlock_t sock_lock; > - struct task_struct *task_recv; > - struct task_struct *task_send; > + pid_t task_recv; > + pid_t task_send; > > #if IS_ENABLED(CONFIG_DEBUG_FS) > struct dentry *dbg_dir; > @@ -392,7 +392,7 @@ static ssize_t pid_show(struct device *dev, > struct gendisk *disk = dev_to_disk(dev); > struct nbd_device *nbd = (struct nbd_device *)disk->private_data; > > - return sprintf(buf, "%d\n", task_pid_nr(nbd->task_recv)); > + return sprintf(buf, "%d\n", nbd->task_recv); > } > > static struct device_attribute pid_attr = { > @@ -409,13 +409,13 @@ static int nbd_thread_recv(struct nbd_device *nbd) > > sk_set_memalloc(nbd->sock->sk); > > - nbd->task_recv = current; > + nbd->task_recv = task_pid_nr(current); > > ret = device_create_file(disk_to_dev(nbd->disk), &pid_attr); > if (ret) { > dev_err(disk_to_dev(nbd->disk), "device_create_file failed!\n"); > > - nbd->task_recv = NULL; > + nbd->task_recv = 0; > > return ret; > } > @@ -432,7 +432,7 @@ static int nbd_thread_recv(struct nbd_device *nbd) > > device_remove_file(disk_to_dev(nbd->disk), &pid_attr); > > - nbd->task_recv = NULL; > + nbd->task_recv = 0; > > return ret; > } > @@ -526,7 +526,7 @@ static int nbd_thread_send(void *data) > struct nbd_device *nbd = data; > struct request *req; > > - nbd->task_send = current; > + nbd->task_send = task_pid_nr(current); > > set_user_nice(current, MIN_NICE); > while (!kthread_should_stop() || !list_empty(&nbd->waiting_queue)) { > @@ -549,7 +549,7 @@ static int nbd_thread_send(void *data) > nbd_handle_req(nbd, req); > } > > - nbd->task_send = NULL; > + nbd->task_send = 0; > > return 0; > } > @@ -827,9 +827,9 @@ static int nbd_dbg_tasks_show(struct seq_file *s, void *unused) > struct nbd_device *nbd = s->private; > > if (nbd->task_recv) > - seq_printf(s, "recv: %d\n", task_pid_nr(nbd->task_recv)); > + seq_printf(s, "recv: %d\n", nbd->task_recv); > if (nbd->task_send) > - seq_printf(s, "send: %d\n", task_pid_nr(nbd->task_send)); > + seq_printf(s, "send: %d\n", nbd->task_send); > > return 0; > } > > -- 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 |
Attachment:
signature.asc
Description: This is a digitally signed message part.