Skip to contentSkip to conversation

sys_ioprio_set: don't disable irqs

PATCH In review Oleg Nesterov 3 participants 6 messages Aug 20, 2006 – Aug 22, 2006

Subsystems (2)
VFS Block layer
lore
Share & tools
lore

Oleg Nesterov

author
#0 lore raw

PATCH 1/1 sys_ioprio_set: don't disable irqs

It is not good to disable interrupts while traversing all tasks in the system. As I see it, sys_ioprio_get() doesn't need to cli() at all, sys_ioprio_set() does it for cfq_ioc_set_ioprio() which can do it itself.

Signed-off-by Oleg Nesterov <[email protected]>
block/cfq-iosched.c renamed from block/cfq-iosched.c~5_setpr +3 -3
@@ -1421,14 +1421,14 @@ static inline void changed_ioprio(struct
 }
 
 /*
- * callback from sys_ioprio_set, irqs are disabled
+ * callback from sys_ioprio_set.
  */
 static int cfq_ioc_set_ioprio(struct io_context *ioc, unsigned int ioprio)
 {
 	struct cfq_io_context *cic;
 	struct rb_node *n;
 
-	spin_lock(&cfq_exit_lock);
+	spin_lock_irq(&cfq_exit_lock);
 
 	n = rb_first(&ioc->cic_root);
 	while (n != NULL) {

@@ -1438,7 +1438,7 @@ static int cfq_ioc_set_ioprio(struct io_
 		n = rb_next(n);
 	}
 
-	spin_unlock(&cfq_exit_lock);
+	spin_unlock_irq(&cfq_exit_lock);
 
 	return 0;
 }
fs/ioprio.c renamed from fs/ioprio.c~5_setpr +4 -4
@@ -81,7 +81,7 @@ asmlinkage long sys_ioprio_set(int which
 	}
 
 	ret = -ESRCH;
-	read_lock_irq(&tasklist_lock);
+	read_lock(&tasklist_lock);
 	switch (which) {
 		case IOPRIO_WHO_PROCESS:
 			if (!who)

@@ -124,7 +124,7 @@ free_uid:
 			ret = -EINVAL;
 	}
 
-	read_unlock_irq(&tasklist_lock);
+	read_unlock(&tasklist_lock);
 	return ret;
 }
 

@@ -170,7 +170,7 @@ asmlinkage long sys_ioprio_get(int which
 	int ret = -ESRCH;
 	int tmpio;
 
-	read_lock_irq(&tasklist_lock);
+	read_lock(&tasklist_lock);
 	switch (which) {
 		case IOPRIO_WHO_PROCESS:
 			if (!who)

@@ -221,7 +221,7 @@ asmlinkage long sys_ioprio_get(int which
 			ret = -EINVAL;
 	}
 
-	read_unlock_irq(&tasklist_lock);
+	read_unlock(&tasklist_lock);
 	return ret;
 }
author
#1 lore raw

Question: why do we need to disable irqs in exit_io_context() ? Why do we need ->alloc_lock to clear io_context->task ?

In other words, could you explain why the patch below is not correct.

Thanks,

Oleg.

block/ll_rw_blk.c renamed from block/ll_rw_blk.c~6_exit +2 -5
@@ -3580,25 +3580,22 @@ EXPORT_SYMBOL(put_io_context);
 /* Called by the exitting task */
 void exit_io_context(void)
 {
-	unsigned long flags;
 	struct io_context *ioc;
 	struct cfq_io_context *cic;
 
-	local_irq_save(flags);
 	task_lock(current);
 	ioc = current->io_context;
 	current->io_context = NULL;
-	ioc->task = NULL;
 	task_unlock(current);
-	local_irq_restore(flags);
 
+	ioc->task = NULL;
 	if (ioc->aic && ioc->aic->exit)
 		ioc->aic->exit(ioc->aic);
 	if (ioc->cic_root.rb_node != NULL) {
 		cic = rb_entry(rb_first(&ioc->cic_root), struct cfq_io_context, rb_node);
 		cic->exit(ioc);
 	}
- 
+
 	put_io_context(ioc);
 }
#2 lore raw
Quoted 1 line · On Mon, 21 Aug 2006 00:50:34 +0400 Oleg Nesterov <[email protected]> wrote:

Question: why do we need to disable irqs in exit_io_context() ?

iirc it was to prevent IRQ-context code from getting a hold on current->io_context and then playing around with it while it's getting freed.

In practice, a preempt_disable() there would probably suffice (ie: if this CPU is running an ISR, it won't be running exit_io_context as well). But local_irq_disable() is clearer, albeit more expensive.

Quoted 1 line

Why do we need ->alloc_lock to clear io_context->task ?

To prevent races against elv_unregister(), I guess.

Quoted 36 lines

In other words, could you explain why the patch below is not correct.

Thanks,

Oleg.

block/ll_rw_blk.c renamed from block/ll_rw_blk.c~6_exit +2 -5
@@ -3580,25 +3580,22 @@ EXPORT_SYMBOL(put_io_context);
 /* Called by the exitting task */
 void exit_io_context(void)
 {
-	unsigned long flags;
 	struct io_context *ioc;
 	struct cfq_io_context *cic;
 
-	local_irq_save(flags);
 	task_lock(current);
 	ioc = current->io_context;
 	current->io_context = NULL;
-	ioc->task = NULL;
 	task_unlock(current);
-	local_irq_restore(flags);
 
+	ioc->task = NULL;
 	if (ioc->aic && ioc->aic->exit)
 		ioc->aic->exit(ioc->aic);
 	if (ioc->cic_root.rb_node != NULL) {
 		cic = rb_entry(rb_first(&ioc->cic_root), struct cfq_io_context, rb_node);
 		cic->exit(ioc);
 	}
- 
+
 	put_io_context(ioc);
 }
author
#3 lore raw

PATCH 1/1 elv_unregister: fix possible crash on module unload

An exiting task or process which didn't do I/O yet have no io context, elv_unregister() should check it is not NULL.

Signed-off-by Oleg Nesterov <[email protected]>
block/elevator.c renamed from block/elevator.c~8_crash +2 -1
@@ -765,7 +765,8 @@ void elv_unregister(struct elevator_type
 		read_lock(&tasklist_lock);
 		do_each_thread(g, p) {
 			task_lock(p);
-			e->ops.trim(p->io_context);
+			if (p->io_context)
+				e->ops.trim(p->io_context);
 			task_unlock(p);
 		} while_each_thread(g, p);
 		read_unlock(&tasklist_lock);
#4 lore raw

Re: [PATCH] elv_unregister: fix possible crash on module unload

Quoted 8 lines · Oleg Nesterov

An exiting task or process which didn't do I/O yet have no io context, elv_unregister() should check it is not NULL.

Signed-off-by Oleg Nesterov <[email protected]>

block/elevator.c void elv_unregister(struct elevator_type · line 772

Quoted 9 lines
block/elevator.c renamed from block/elevator.c~8_crash +2 -1
@@ -765,7 +765,8 @@ void elv_unregister(struct elevator_type
 		read_lock(&tasklist_lock);
 		do_each_thread(g, p) {
 			task_lock(p);
-			e->ops.trim(p->io_context);
+			if (p->io_context)
+				e->ops.trim(p->io_context);
 			task_unlock(p);
 		} while_each_thread(g, p);
 		read_unlock(&tasklist_lock);

Good catch, applied. Thanks! I wonder why this hasn't been seen on switching io schedulers, which makes me a little suspicious. Did you see it trigger?

Signature · 1 line
Jens Axboe
author
#5 lore raw
In reply to Andrew Morton
Quoted 6 lines · Andrew Morton
Quoted 1 line · On Mon, 21 Aug 2006 00:50:34 +0400 Oleg Nesterov <[email protected]> wrote:

Question: why do we need to disable irqs in exit_io_context() ?

Quoted 7 lines

iirc it was to prevent IRQ-context code from getting a hold on current->io_context and then playing around with it while it's getting freed.

In practice, a preempt_disable() there would probably suffice (ie: if this CPU is running an ISR, it won't be running exit_io_context as well). But local_irq_disable() is clearer, albeit more expensive.

Looks like my understanding of block I/O is even less than nothing :(

irq_disable() can't prevent from IRQ-context code playing with our io_context on other CPUs. But this doesn't matter, we are only changing ioc->task.

What does matter, we are clearing the pointer to it: task_struct->io_context, and IRQ should not look at it, no?

Or... Do you mean it is possible to submit I/O from IRQ on behalf of current ????? In that case current_io_context() will re-instantiate ->io_context after irq_enable(). What is exit_io_context() for then? It is only called from do_exit() when we know the task won't start IO.

(please don't beat a newbie)

Quoted 3 lines
Quoted 1 line

Why do we need ->alloc_lock to clear io_context->task ?

To prevent races against elv_unregister(), I guess.

elv_unregister() takes task_lock(), should see ->io_context == NULL.

Oleg.