Skip to contentSkip to conversation

aio: aio_complete() will never be called in interrupt context

PATCH In review Nikanth Karthikesan 4 participants 9 messages Jun 26, 2008 – Jul 2, 2008

Subsystems (2)
VFS Aio
lore
Share & tools
lore

Nikanth Karthikesan

author
#0 lore raw

PATCH 1/1 aio: aio_complete() will never be called in interrupt context

aio_complete() is never called from interrupt context. It is called from user context or worker threads. Remove disabling interrupts and for kmap_atomic use KM_USER slots instead of KM_IRQ slots.

Signed-off-by Nikanth Karthikesan <[email protected]>

---

fs/aio.c +2 -5
index 7817e8f..27b7181 100644
@@ -893,7 +893,6 @@ int aio_complete(struct kiocb *iocb, long res, long res2)
 	struct aio_ring_info	*info;
 	struct aio_ring	*ring;
 	struct io_event	*event;
-	unsigned long	flags;
 	unsigned long	tail;
 	int		ret;
 

@@ -916,11 +915,9 @@ int aio_complete(struct kiocb *iocb, long res, long res2)
 
 	/* add a completion event to the ring buffer.
 	 * must be done holding ctx->ctx_lock to prevent
-	 * other code from messing with the tail
-	 * pointer since we might be called from irq
-	 * context.
+	 * other code from messing with the tail pointer.
 	 */
-	spin_lock_irqsave(&ctx->ctx_lock, flags);
+	spin_lock(&ctx->ctx_lock);
 
 	if (iocb->ki_run_list.prev && !list_empty(&iocb->ki_run_list))
 		list_del_init(&iocb->ki_run_list);

@@ -932,10 +929,10 @@ int aio_complete(struct kiocb *iocb, long res, long

res2)
if (kiocbIsCancelled(iocb))
goto put_rq;

- ring = kmap_atomic(info->ring_pages[0], KM_IRQ1);
+ ring = kmap_atomic(info->ring_pages[0], KM_USER1);

tail = info->tail;
- event = aio_ring_event(info, tail, KM_IRQ0);
+ event = aio_ring_event(info, tail, KM_USER0);
if (++tail >= info->nr)
tail = 0;

@@ -956,8 +953,8 @@ int aio_complete(struct kiocb *iocb, long res, long res2)
info->tail = tail;
ring->tail = tail;

- put_aio_ring_event(event, KM_IRQ0);
- kunmap_atomic(ring, KM_IRQ1);
+ put_aio_ring_event(event, KM_USER0);
+ kunmap_atomic(ring, KM_USER1);

pr_debug("added to ring %p at [%lu]\n", iocb, tail);

@@ -984,7 +981,7 @@ put_rq:
if (waitqueue_active(&ctx->wait))
wake_up(&ctx->wait);

- spin_unlock_irqrestore(&ctx->ctx_lock, flags);
+ spin_unlock(&ctx->ctx_lock);
return ret;
}

#1 lore raw
Quoted 3 lines · Nikanth Karthikesan

aio_complete() is never called from interrupt context. It is called from user context or worker threads. Remove disabling interrupts and for kmap_atomic use KM_USER slots instead of KM_IRQ slots.

Actually, it can be called from softirq context. See fs/directio.c.

Cheers,

Jeff

author
#2 lore raw
Quoted 7 lines · On Friday 27 June 2008 18:41:03 Jeff Moyer wrote:
Quoted 3 lines · Nikanth Karthikesan

aio_complete() is never called from interrupt context. It is called from user context or worker threads. Remove disabling interrupts and for kmap_atomic use KM_USER slots instead of KM_IRQ slots.

Actually, it can be called from softirq context. See fs/directio.c.

Oh, sorry for my ignorance. Will using KM_SOFTIRQ slots be fine?

Thanks
Nikanth Karthikesan

aio_complete() is never called from hardware interrupt context. Remove disabling interrupts and for kmap_atomic use KM_SOFTIRQ slots instead of KM_IRQ slots.

Signed-off-by Nikanth Karthikesan <[email protected]>

---

fs/aio.c +7 -10
Index: linux-2.6/fs/aio.c
===================================================================
@@ -901,7 +901,6 @@ int aio_complete(struct kiocb *iocb, lon
 	struct aio_ring_info	*info;
 	struct aio_ring	*ring;
 	struct io_event	*event;
-	unsigned long	flags;
 	unsigned long	tail;
 	int		ret;
 

@@ -924,11 +923,9 @@ int aio_complete(struct kiocb *iocb, lon
 
 	/* add a completion event to the ring buffer.
 	 * must be done holding ctx->ctx_lock to prevent
-	 * other code from messing with the tail
-	 * pointer since we might be called from irq
-	 * context.
+	 * other code from messing with the tail pointer.
 	 */
-	spin_lock_irqsave(&ctx->ctx_lock, flags);
+	spin_lock(&ctx->ctx_lock);
 
 	if (iocb->ki_run_list.prev && !list_empty(&iocb->ki_run_list))
 		list_del_init(&iocb->ki_run_list);

@@ -940,10 +937,10 @@ int aio_complete(struct kiocb *iocb, lon
 	if (kiocbIsCancelled(iocb))
 		goto put_rq;
 
-	ring = kmap_atomic(info->ring_pages[0], KM_IRQ1);
+	ring = kmap_atomic(info->ring_pages[0], KM_SOFTIRQ1);
 
 	tail = info->tail;
-	event = aio_ring_event(info, tail, KM_IRQ0);
+	event = aio_ring_event(info, tail, KM_SOFTIRQ0);
 	if (++tail >= info->nr)
 		tail = 0;
 

@@ -964,8 +961,8 @@ int aio_complete(struct kiocb *iocb, lon
 	info->tail = tail;
 	ring->tail = tail;
 
-	put_aio_ring_event(event, KM_IRQ0);
-	kunmap_atomic(ring, KM_IRQ1);
+	put_aio_ring_event(event, KM_SOFTIRQ0);
+	kunmap_atomic(ring, KM_SOFTIRQ1);
 
 	pr_debug("added to ring %p at [%lu]\n", iocb, tail);
 

@@ -992,7 +989,7 @@ put_rq:
 	if (waitqueue_active(&ctx->wait))
 		wake_up(&ctx->wait);
 
-	spin_unlock_irqrestore(&ctx->ctx_lock, flags);
+	spin_unlock(&ctx->ctx_lock);
 	return ret;
 }
#3 lore raw
In reply to Nikanth Karthikesan
Quoted 3 lines · Nikanth Karthikesan

aio_complete() is never called from interrupt context. It is called from user context or worker threads. Remove disabling interrupts and for kmap_atomic use KM_USER slots instead of KM_IRQ slots.

How are you testing these patches?

- z

author
#4 lore raw
Quoted 7 lines · On Monday 30 June 2008 23:13:26 Zach Brown wrote:
Quoted 3 lines · Nikanth Karthikesan

aio_complete() is never called from interrupt context. It is called from user context or worker threads. Remove disabling interrupts and for kmap_atomic use KM_USER slots instead of KM_IRQ slots.

How are you testing these patches?

I did not do any specific testing. Just ran multiple instances of a file-copy utility that uses aio and a bit of Chris Mason's aio-stress.

Thanks
Nikanth karthikesan

p.s: Is the archives of the linux-aio list available somewhere? The URL in the mail footer seems to be unavailable.

#5 lore raw
Quoted 3 lines

I did not do any specific testing. Just ran multiple instances of a file-copy utility that uses aio and a bit of Chris Mason's aio-stress.

Are you running these tests under a kernel with lockdep enabled?

Quoted 2 lines

p.s: Is the archives of the linux-aio list available somewhere? The URL in the mail footer seems to be unavailable.

The very first google hit for 'linux-aio mailing list archive' works:

http://marc.info/?l=linux-aio

- z

author
#6 lore raw
Quoted 5 lines · On Wednesday 02 July 2008 01:20:42 Zach Brown wrote:
Quoted 2 lines

I did not do any specific testing. Just ran multiple instances of a file-copy utility that uses aio and a bit of Chris Mason's aio-stress.

Are you running these tests under a kernel with lockdep enabled?

Yes. I have CONFIG_LOCKDEP_SUPPORT=y and did not get any warning messages.

Quoted 7 lines
Quoted 2 lines

p.s: Is the archives of the linux-aio list available somewhere? The URL in the mail footer seems to be unavailable.

The very first google hit for 'linux-aio mailing list archive' works:

http://marc.info/?l=linux-aio

Thanks and sorry for the noise.

But it would be nice if the link in the mail footer(www.kvack.org/aio) is changed to an active link or removed. The link redirects to www.kvack.org/~blah/aio/ which returns 404

Thanks
Nikanth Karthikesan

#7 lore raw
Quoted 2 lines

Yes. I have CONFIG_LOCKDEP_SUPPORT=y and did not get any warning messages.

You should mention details like this in the patch descriptions.

Quoted 3 lines

But it would be nice if the link in the mail footer(www.kvack.org/aio) is changed to an active link or removed. The link redirects to www.kvack.org/~blah/aio/ which returns 404

Yeah, it's perhaps not the best that this is still hosted over on Ben's personal boxes. I wonder if we should move to linux-aio@vger..

- z

maintainer
#8 lore raw
Quoted 2 lines · Zach Brown

Yeah, it's perhaps not the best that this is still hosted over on Ben's personal boxes. I wonder if we should move to linux-aio@vger..

Just tell me where to point it to...

-ben

Signature · 2 lines
"Time is of no importance, Mr. President, only life is important."
Don't Email: <[email protected]>.