[PATCH 1/2] AIO: Fix reference counting in io_cancel and aio_cancel_all

[PATCH 1/2] AIO: Fix reference counting in io_cancel and aio_cancel_all

[Sergey Temerkhanov]

[-- Attachment #1: Type: Text/Plain, Size: 269 bytes --]

io_cancel() and aio_cancel_all() have inconsistent reference counting 
(ki_users field of struct kiocb) which leads to unkillable processes upon  
io_cancel() or io_destroy() syscalls. This patch fixes the undesired behavior.

Regards, Sergey Temerkhanov, Cifronic ZAO

[-- Attachment #2: aio-fix.patch --]
[-- Type: text/x-patch, Size: 2630 bytes --]

diff -r b290b47c0880 fs/aio.c
--- a/fs/aio.c	Tue Mar 16 13:51:13 2010 +0300
+++ b/fs/aio.c	Tue Mar 16 14:11:36 2010 +0300
@@ -63,6 +63,8 @@
 static void aio_kick_handler(struct work_struct *);
 static void aio_queue_work(struct kioctx *);
 
+static void aio_cancel_all(struct kioctx *ctx);
+
 /* aio_setup
  *	Creates the slab caches used by the aio routines, panic on
  *	failure as this is done early during the boot sequence.
@@ -315,33 +317,6 @@
 	return ctx;
 }
 
-/* aio_cancel_all
- *	Cancels all outstanding aio requests on an aio context.  Used 
- *	when the processes owning a context have all exited to encourage 
- *	the rapid destruction of the kioctx.
- */
-static void aio_cancel_all(struct kioctx *ctx)
-{
-	int (*cancel)(struct kiocb *, struct io_event *);
-	struct io_event res;
-	spin_lock_irq(&ctx->ctx_lock);
-	ctx->dead = 1;
-	while (!list_empty(&ctx->active_reqs)) {
-		struct list_head *pos = ctx->active_reqs.next;
-		struct kiocb *iocb = list_kiocb(pos);
-		list_del_init(&iocb->ki_list);
-		cancel = iocb->ki_cancel;
-		kiocbSetCancelled(iocb);
-		if (cancel) {
-			iocb->ki_users++;
-			spin_unlock_irq(&ctx->ctx_lock);
-			cancel(iocb, &res);
-			spin_lock_irq(&ctx->ctx_lock);
-		}
-	}
-	spin_unlock_irq(&ctx->ctx_lock);
-}
-
 static void wait_for_all_aios(struct kioctx *ctx)
 {
 	struct task_struct *tsk = current;
@@ -1038,6 +1013,36 @@
 	return ret;
 }
 
+/* aio_cancel_all
+ *	Cancels all outstanding aio requests on an aio context.  Used 
+ *	when the processes owning a context have all exited to encourage 
+ *	the rapid destruction of the kioctx.
+ */
+static void aio_cancel_all(struct kioctx *ctx)
+{
+	int (*cancel)(struct kiocb *, struct io_event *);
+	struct io_event res;
+	spin_lock_irq(&ctx->ctx_lock);
+	ctx->dead = 1;
+	while (!list_empty(&ctx->active_reqs)) {
+		struct list_head *pos = ctx->active_reqs.next;
+		struct kiocb *iocb = list_kiocb(pos);
+		list_del_init(&iocb->ki_list);
+		cancel = iocb->ki_cancel;
+		kiocbSetCancelled(iocb);
+		if (cancel) {
+			iocb->ki_users++;
+			spin_unlock_irq(&ctx->ctx_lock);
+			cancel(iocb, &res);
+			spin_lock_irq(&ctx->ctx_lock);
+			dprintk("aio_cancel_all: iocb->ki_users: %d\n", iocb->ki_users);
+			__aio_put_req(ctx, iocb);
+		}
+		__aio_put_req(ctx, iocb);
+	}
+	spin_unlock_irq(&ctx->ctx_lock);
+}
+
 /* aio_read_evt
  *	Pull an event off of the ioctx's event ring.  Returns the number of 
  *	events fetched (0 or 1 ;-)
@@ -1787,9 +1792,11 @@
 			if (copy_to_user(result, &tmp, sizeof(tmp)))
 				ret = -EFAULT;
 		}
+		aio_put_req(kiocb);
 	} else
 		ret = -EINVAL;
 
+	aio_put_req(kiocb);
 	put_ioctx(ctx);
 
 	return ret;

[PATCH 2/2] AIO: Change pr_debug statements to dprintk in fs/aio.c

[Sergey Temerkhanov]

[-- Attachment #1: Type: Text/Plain, Size: 150 bytes --]

Some functions in fs/aio.c use pr_debug and another use dprintk statements. 
This patch makes it uniform.


Regards, Sergey Temerkhanov, Cifronic ZAO

[-- Attachment #2: aio-debug.patch --]
[-- Type: text/x-patch, Size: 3934 bytes --]

diff -r 5bbe95a762e1 fs/aio.c
--- a/fs/aio.c	Tue Mar 16 14:11:37 2010 +0300
+++ b/fs/aio.c	Tue Mar 16 14:11:44 2010 +0300
@@ -76,7 +76,7 @@
 
 	aio_wq = create_workqueue("aio");
 
-	pr_debug("aio_setup: sizeof(struct page) = %d\n", (int)sizeof(struct page));
+	dprintk("aio_setup: sizeof(struct page) = %d\n", (int)sizeof(struct page));
 
 	return 0;
 }
@@ -221,7 +221,7 @@
 	aio_free_ring(ctx);
 	mmdrop(ctx->mm);
 	ctx->mm = NULL;
-	pr_debug("__put_ioctx: freeing %p\n", ctx);
+	dprintk("__put_ioctx: freeing %p\n", ctx);
 	call_rcu(&ctx->rcu_head, ctx_rcu_free);
 }
 
@@ -247,7 +247,7 @@
 	/* Prevent overflows */
 	if ((nr_events > (0x10000000U / sizeof(struct io_event))) ||
 	    (nr_events > (0x10000000U / sizeof(struct kiocb)))) {
-		pr_debug("ENOMEM: nr_events too high\n");
+		dprintk("ENOMEM: nr_events too high\n");
 		return ERR_PTR(-EINVAL);
 	}
 
@@ -468,6 +468,7 @@
 		kfree(req->ki_iovec);
 	kmem_cache_free(kiocb_cachep, req);
 	ctx->reqs_active--;
+	dprintk("really_put_req: req->ki_users: %d\n", req->ki_users);
 
 	if (unlikely(!ctx->reqs_active && ctx->dead))
 		wake_up(&ctx->wait);
@@ -503,12 +504,14 @@
  */
 static int __aio_put_req(struct kioctx *ctx, struct kiocb *req)
 {
-	dprintk(KERN_DEBUG "aio_put(%p): f_count=%ld\n",
+	dprintk("aio_put(%p): f_count=%ld\n",
 		req, atomic_long_read(&req->ki_filp->f_count));
 
 	assert_spin_locked(&ctx->ctx_lock);
 
 	req->ki_users--;
+	dprintk("__aio_put_req: req: %p, req->ki_users: %d\n",
+		req, req->ki_users);
 	BUG_ON(req->ki_users < 0);
 	if (likely(req->ki_users))
 		return 0;
@@ -954,6 +957,7 @@
 	 * cancelled requests don't get events, userland was given one
 	 * when the event got cancelled.
 	 */
+	dprintk("kiocbIsCancelled(iocb): %d\n", kiocbIsCancelled(iocb));
 	if (kiocbIsCancelled(iocb))
 		goto put_rq;
 
@@ -984,7 +988,7 @@
 	put_aio_ring_event(event, KM_IRQ0);
 	kunmap_atomic(ring, KM_IRQ1);
 
-	pr_debug("added to ring %p at [%lu]\n", iocb, tail);
+	dprintk("added to ring %p at [%lu]\n", iocb, tail);
 
 	/*
 	 * Check if the user asked us to deliver the result through an
@@ -1187,6 +1191,7 @@
 	}
 
 	while (likely(i < nr)) {
+		dprintk("aio: i: %d, nr: %ld\n", i, nr);
 		add_wait_queue_exclusive(&ctx->wait, &wait);
 		do {
 			set_task_state(tsk, TASK_INTERRUPTIBLE);
@@ -1199,6 +1204,7 @@
 				ret = -EINVAL;
 				break;
 			}
+			dprintk("aio: to.timed_out: %d\n", to.timed_out);
 			if (to.timed_out)	/* Only check after read evt */
 				break;
 			/* Try to only show up in io wait if there are ops
@@ -1294,7 +1300,7 @@
 
 	ret = -EINVAL;
 	if (unlikely(ctx || nr_events == 0)) {
-		pr_debug("EINVAL: io_setup: ctx %lu nr_events %u\n",
+		dprintk("EINVAL: io_setup: ctx %lu nr_events %u\n",
 		         ctx, nr_events);
 		goto out;
 	}
@@ -1327,7 +1333,7 @@
 		io_destroy(ioctx);
 		return 0;
 	}
-	pr_debug("EINVAL: io_destroy: invalid context id\n");
+	dprintk("EINVAL: io_destroy: invalid context id\n");
 	return -EINVAL;
 }
 
@@ -1586,7 +1592,7 @@
 
 	/* enforce forwards compatibility on users */
 	if (unlikely(iocb->aio_reserved1 || iocb->aio_reserved2)) {
-		pr_debug("EINVAL: io_submit: reserve field set\n");
+		dprintk("EINVAL: io_submit: reserve field set\n");
 		return -EINVAL;
 	}
 
@@ -1596,7 +1602,7 @@
 	    (iocb->aio_nbytes != (size_t)iocb->aio_nbytes) ||
 	    ((ssize_t)iocb->aio_nbytes < 0)
 	   )) {
-		pr_debug("EINVAL: io_submit: overflow check\n");
+		dprintk("EINVAL: io_submit: overflow check\n");
 		return -EINVAL;
 	}
 
@@ -1690,7 +1696,7 @@
 
 	ctx = lookup_ioctx(ctx_id);
 	if (unlikely(!ctx)) {
-		pr_debug("EINVAL: io_submit: invalid context id\n");
+		dprintk("EINVAL: io_submit: invalid context id\n");
 		return -EINVAL;
 	}
 
@@ -1780,7 +1786,7 @@
 
 	if (NULL != cancel) {
 		struct io_event tmp;
-		pr_debug("calling cancel\n");
+		dprintk("calling cancel\n");
 		memset(&tmp, 0, sizeof(tmp));
 		tmp.obj = (u64)(unsigned long)kiocb->ki_obj.user;
 		tmp.data = kiocb->ki_user_data;

Re: [PATCH 2/2] AIO: Change pr_debug statements to dprintk in fs/aio.c

[Jeff Moyer]

Sergey Temerkhanov <temerkhanov@cifronik.ru> writes:

> Some functions in fs/aio.c use pr_debug and another use dprintk statements. 
> This patch makes it uniform.

This is a good idea, but I'd rather use pr_debug everywhere, since
(depending on the kernel config) those can be enabled at run-time.

Cheers,
Jeff

> Regards, Sergey Temerkhanov, Cifronic ZAO
> diff -r 5bbe95a762e1 fs/aio.c
> --- a/fs/aio.c	Tue Mar 16 14:11:37 2010 +0300
> +++ b/fs/aio.c	Tue Mar 16 14:11:44 2010 +0300
> @@ -76,7 +76,7 @@
>  
>  	aio_wq = create_workqueue("aio");
>  
> -	pr_debug("aio_setup: sizeof(struct page) = %d\n", (int)sizeof(struct page));
> +	dprintk("aio_setup: sizeof(struct page) = %d\n", (int)sizeof(struct page));
>  
>  	return 0;
>  }
> @@ -221,7 +221,7 @@
>  	aio_free_ring(ctx);
>  	mmdrop(ctx->mm);
>  	ctx->mm = NULL;
> -	pr_debug("__put_ioctx: freeing %p\n", ctx);
> +	dprintk("__put_ioctx: freeing %p\n", ctx);
>  	call_rcu(&ctx->rcu_head, ctx_rcu_free);
>  }
>  
> @@ -247,7 +247,7 @@
>  	/* Prevent overflows */
>  	if ((nr_events > (0x10000000U / sizeof(struct io_event))) ||
>  	    (nr_events > (0x10000000U / sizeof(struct kiocb)))) {
> -		pr_debug("ENOMEM: nr_events too high\n");
> +		dprintk("ENOMEM: nr_events too high\n");
>  		return ERR_PTR(-EINVAL);
>  	}
>  
> @@ -468,6 +468,7 @@
>  		kfree(req->ki_iovec);
>  	kmem_cache_free(kiocb_cachep, req);
>  	ctx->reqs_active--;
> +	dprintk("really_put_req: req->ki_users: %d\n", req->ki_users);
>  
>  	if (unlikely(!ctx->reqs_active && ctx->dead))
>  		wake_up(&ctx->wait);
> @@ -503,12 +504,14 @@
>   */
>  static int __aio_put_req(struct kioctx *ctx, struct kiocb *req)
>  {
> -	dprintk(KERN_DEBUG "aio_put(%p): f_count=%ld\n",
> +	dprintk("aio_put(%p): f_count=%ld\n",
>  		req, atomic_long_read(&req->ki_filp->f_count));
>  
>  	assert_spin_locked(&ctx->ctx_lock);
>  
>  	req->ki_users--;
> +	dprintk("__aio_put_req: req: %p, req->ki_users: %d\n",
> +		req, req->ki_users);
>  	BUG_ON(req->ki_users < 0);
>  	if (likely(req->ki_users))
>  		return 0;
> @@ -954,6 +957,7 @@
>  	 * cancelled requests don't get events, userland was given one
>  	 * when the event got cancelled.
>  	 */
> +	dprintk("kiocbIsCancelled(iocb): %d\n", kiocbIsCancelled(iocb));
>  	if (kiocbIsCancelled(iocb))
>  		goto put_rq;
>  
> @@ -984,7 +988,7 @@
>  	put_aio_ring_event(event, KM_IRQ0);
>  	kunmap_atomic(ring, KM_IRQ1);
>  
> -	pr_debug("added to ring %p at [%lu]\n", iocb, tail);
> +	dprintk("added to ring %p at [%lu]\n", iocb, tail);
>  
>  	/*
>  	 * Check if the user asked us to deliver the result through an
> @@ -1187,6 +1191,7 @@
>  	}
>  
>  	while (likely(i < nr)) {
> +		dprintk("aio: i: %d, nr: %ld\n", i, nr);
>  		add_wait_queue_exclusive(&ctx->wait, &wait);
>  		do {
>  			set_task_state(tsk, TASK_INTERRUPTIBLE);
> @@ -1199,6 +1204,7 @@
>  				ret = -EINVAL;
>  				break;
>  			}
> +			dprintk("aio: to.timed_out: %d\n", to.timed_out);
>  			if (to.timed_out)	/* Only check after read evt */
>  				break;
>  			/* Try to only show up in io wait if there are ops
> @@ -1294,7 +1300,7 @@
>  
>  	ret = -EINVAL;
>  	if (unlikely(ctx || nr_events == 0)) {
> -		pr_debug("EINVAL: io_setup: ctx %lu nr_events %u\n",
> +		dprintk("EINVAL: io_setup: ctx %lu nr_events %u\n",
>  		         ctx, nr_events);
>  		goto out;
>  	}
> @@ -1327,7 +1333,7 @@
>  		io_destroy(ioctx);
>  		return 0;
>  	}
> -	pr_debug("EINVAL: io_destroy: invalid context id\n");
> +	dprintk("EINVAL: io_destroy: invalid context id\n");
>  	return -EINVAL;
>  }
>  
> @@ -1586,7 +1592,7 @@
>  
>  	/* enforce forwards compatibility on users */
>  	if (unlikely(iocb->aio_reserved1 || iocb->aio_reserved2)) {
> -		pr_debug("EINVAL: io_submit: reserve field set\n");
> +		dprintk("EINVAL: io_submit: reserve field set\n");
>  		return -EINVAL;
>  	}
>  
> @@ -1596,7 +1602,7 @@
>  	    (iocb->aio_nbytes != (size_t)iocb->aio_nbytes) ||
>  	    ((ssize_t)iocb->aio_nbytes < 0)
>  	   )) {
> -		pr_debug("EINVAL: io_submit: overflow check\n");
> +		dprintk("EINVAL: io_submit: overflow check\n");
>  		return -EINVAL;
>  	}
>  
> @@ -1690,7 +1696,7 @@
>  
>  	ctx = lookup_ioctx(ctx_id);
>  	if (unlikely(!ctx)) {
> -		pr_debug("EINVAL: io_submit: invalid context id\n");
> +		dprintk("EINVAL: io_submit: invalid context id\n");
>  		return -EINVAL;
>  	}
>  
> @@ -1780,7 +1786,7 @@
>  
>  	if (NULL != cancel) {
>  		struct io_event tmp;
> -		pr_debug("calling cancel\n");
> +		dprintk("calling cancel\n");
>  		memset(&tmp, 0, sizeof(tmp));
>  		tmp.obj = (u64)(unsigned long)kiocb->ki_obj.user;
>  		tmp.data = kiocb->ki_user_data;

Re: [PATCH 1/2] AIO: Fix reference counting in io_cancel and aio_cancel_all

[Jeff Moyer]

Sergey Temerkhanov <temerkhanov@cifronik.ru> writes:

> io_cancel() and aio_cancel_all() have inconsistent reference counting 
> (ki_users field of struct kiocb) which leads to unkillable processes upon  
> io_cancel() or io_destroy() syscalls. This patch fixes the undesired behavior.

Hi, Sergey,

Thanks for the patch.  Would you mind resubmitting it to make it a bit
easier to review?  The way you've done things, it is difficult to tell
if you just moved the aio_cancel_all function or if you moved it and
made changes.  Please have the patch that moves it separated out from
other changes, or at least mention in the changelog that the function
was unchanged.  Next, if you could tell what sorts of testing you've
performed, that would be great.  Finally, it would be a good idea to CC
linux-aio@kvack.org on aio patches.

Thanks!
Jeff

>
> Regards, Sergey Temerkhanov, Cifronic ZAO
> diff -r b290b47c0880 fs/aio.c
> --- a/fs/aio.c	Tue Mar 16 13:51:13 2010 +0300
> +++ b/fs/aio.c	Tue Mar 16 14:11:36 2010 +0300
> @@ -63,6 +63,8 @@
>  static void aio_kick_handler(struct work_struct *);
>  static void aio_queue_work(struct kioctx *);
>  
> +static void aio_cancel_all(struct kioctx *ctx);
> +
>  /* aio_setup
>   *	Creates the slab caches used by the aio routines, panic on
>   *	failure as this is done early during the boot sequence.
> @@ -315,33 +317,6 @@
>  	return ctx;
>  }
>  
> -/* aio_cancel_all
> - *	Cancels all outstanding aio requests on an aio context.  Used 
> - *	when the processes owning a context have all exited to encourage 
> - *	the rapid destruction of the kioctx.
> - */
> -static void aio_cancel_all(struct kioctx *ctx)
> -{
> -	int (*cancel)(struct kiocb *, struct io_event *);
> -	struct io_event res;
> -	spin_lock_irq(&ctx->ctx_lock);
> -	ctx->dead = 1;
> -	while (!list_empty(&ctx->active_reqs)) {
> -		struct list_head *pos = ctx->active_reqs.next;
> -		struct kiocb *iocb = list_kiocb(pos);
> -		list_del_init(&iocb->ki_list);
> -		cancel = iocb->ki_cancel;
> -		kiocbSetCancelled(iocb);
> -		if (cancel) {
> -			iocb->ki_users++;
> -			spin_unlock_irq(&ctx->ctx_lock);
> -			cancel(iocb, &res);
> -			spin_lock_irq(&ctx->ctx_lock);
> -		}
> -	}
> -	spin_unlock_irq(&ctx->ctx_lock);
> -}
> -
>  static void wait_for_all_aios(struct kioctx *ctx)
>  {
>  	struct task_struct *tsk = current;
> @@ -1038,6 +1013,36 @@
>  	return ret;
>  }
>  
> +/* aio_cancel_all
> + *	Cancels all outstanding aio requests on an aio context.  Used 
> + *	when the processes owning a context have all exited to encourage 
> + *	the rapid destruction of the kioctx.
> + */
> +static void aio_cancel_all(struct kioctx *ctx)
> +{
> +	int (*cancel)(struct kiocb *, struct io_event *);
> +	struct io_event res;
> +	spin_lock_irq(&ctx->ctx_lock);
> +	ctx->dead = 1;
> +	while (!list_empty(&ctx->active_reqs)) {
> +		struct list_head *pos = ctx->active_reqs.next;
> +		struct kiocb *iocb = list_kiocb(pos);
> +		list_del_init(&iocb->ki_list);
> +		cancel = iocb->ki_cancel;
> +		kiocbSetCancelled(iocb);
> +		if (cancel) {
> +			iocb->ki_users++;
> +			spin_unlock_irq(&ctx->ctx_lock);
> +			cancel(iocb, &res);
> +			spin_lock_irq(&ctx->ctx_lock);
> +			dprintk("aio_cancel_all: iocb->ki_users: %d\n", iocb->ki_users);
> +			__aio_put_req(ctx, iocb);
> +		}
> +		__aio_put_req(ctx, iocb);
> +	}
> +	spin_unlock_irq(&ctx->ctx_lock);
> +}
> +
>  /* aio_read_evt
>   *	Pull an event off of the ioctx's event ring.  Returns the number of 
>   *	events fetched (0 or 1 ;-)
> @@ -1787,9 +1792,11 @@
>  			if (copy_to_user(result, &tmp, sizeof(tmp)))
>  				ret = -EFAULT;
>  		}
> +		aio_put_req(kiocb);
>  	} else
>  		ret = -EINVAL;
>  
> +	aio_put_req(kiocb);
>  	put_ioctx(ctx);
>  
>  	return ret;

Re: [PATCH resubmit 2/3] AIO: Fix reference counting in io_cancel and aio_cancel_all

[Сергей Темерханов]

[-- Attachment #1: Type: Text/Plain, Size: 1049 bytes --]

On Tuesday 16 March 2010 17:46:10 Jeff Moyer wrote:
> Sergey Temerkhanov <temerkhanov@cifronik.ru> writes:
> > io_cancel() and aio_cancel_all() have inconsistent reference counting
> > (ki_users field of struct kiocb) which leads to unkillable processes upon
> > io_cancel() or io_destroy() syscalls. This patch fixes the undesired
> > behavior.
> 
> Hi, Sergey,
> 
> Thanks for the patch.  Would you mind resubmitting it to make it a bit
> easier to review?  The way you've done things, it is difficult to tell
> if you just moved the aio_cancel_all function or if you moved it and
> made changes.  Please have the patch that moves it separated out from
> other changes, or at least mention in the changelog that the function
> was unchanged.  Next, if you could tell what sorts of testing you've
> performed, that would be great.  Finally, it would be a good idea to CC
> linux-aio@kvack.org on aio patches.
> 
> Thanks!
> Jeff
> 

This patch fixes reference counting in io_cancel() and aio_cancel_all()

Regards, Sergey Temerkhanov, Cifronic ZAO

[-- Attachment #2: aio-fix.patch --]
[-- Type: text/x-patch, Size: 535 bytes --]

diff -r 74c172a99cdb fs/aio.c
--- a/fs/aio.c	Tue Mar 16 18:55:59 2010 +0300
+++ b/fs/aio.c	Tue Mar 16 19:08:49 2010 +0300
@@ -1035,7 +1035,9 @@
 			spin_unlock_irq(&ctx->ctx_lock);
 			cancel(iocb, &res);
 			spin_lock_irq(&ctx->ctx_lock);
+			__aio_put_req(req);
 		}
+		__aio_put_req(req);
 	}
 	spin_unlock_irq(&ctx->ctx_lock);
 }
@@ -1789,9 +1791,11 @@
 			if (copy_to_user(result, &tmp, sizeof(tmp)))
 				ret = -EFAULT;
 		}
+		aio_put_req(req);
 	} else
 		ret = -EINVAL;
 
+	aio_put_req(req);
 	put_ioctx(ctx);
 
 	return ret;

Re: [PATCH resubmit 3/3] AIO: Change dprintk statements to pr_debug

[Сергей Темерханов]

[-- Attachment #1: Type: Text/Plain, Size: 1030 bytes --]

On Tuesday 16 March 2010 17:46:10 Jeff Moyer wrote:
> Sergey Temerkhanov <temerkhanov@cifronik.ru> writes:
> > io_cancel() and aio_cancel_all() have inconsistent reference counting
> > (ki_users field of struct kiocb) which leads to unkillable processes upon
> > io_cancel() or io_destroy() syscalls. This patch fixes the undesired
> > behavior.
> 
> Hi, Sergey,
> 
> Thanks for the patch.  Would you mind resubmitting it to make it a bit
> easier to review?  The way you've done things, it is difficult to tell
> if you just moved the aio_cancel_all function or if you moved it and
> made changes.  Please have the patch that moves it separated out from
> other changes, or at least mention in the changelog that the function
> was unchanged.  Next, if you could tell what sorts of testing you've
> performed, that would be great.  Finally, it would be a good idea to CC
> linux-aio@kvack.org on aio patches.
> 
> Thanks!
> Jeff

And the last one changes dprintk statements to pr_debug

Regards, Sergey Temerkhanov, Cifronic ZAO

[-- Attachment #2: aio-debug.patch --]
[-- Type: text/x-patch, Size: 5050 bytes --]

diff -r b33005cb6b8d fs/aio.c
--- a/fs/aio.c	Tue Mar 16 19:08:49 2010 +0300
+++ b/fs/aio.c	Tue Mar 16 19:15:56 2010 +0300
@@ -36,12 +36,6 @@
 #include <asm/uaccess.h>
 #include <asm/mmu_context.h>
 
-#if DEBUG > 1
-#define dprintk		printk
-#else
-#define dprintk(x...)	do { ; } while (0)
-#endif
-
 /*------ sysctl variables----*/
 static DEFINE_SPINLOCK(aio_nr_lock);
 unsigned long aio_nr;		/* current system wide number of aio requests */
@@ -130,7 +124,7 @@
 	}
 
 	info->mmap_size = nr_pages * PAGE_SIZE;
-	dprintk("attempting mmap of %lu bytes\n", info->mmap_size);
+	pr_debug("attempting mmap of %lu bytes\n", info->mmap_size);
 	down_write(&ctx->mm->mmap_sem);
 	info->mmap_base = do_mmap(NULL, 0, info->mmap_size, 
 				  PROT_READ|PROT_WRITE, MAP_ANONYMOUS|MAP_PRIVATE,
@@ -142,7 +136,7 @@
 		return -EAGAIN;
 	}
 
-	dprintk("mmap address: 0x%08lx\n", info->mmap_base);
+	pr_debug("mmap address: 0x%08lx\n", info->mmap_base);
 	info->nr_pages = get_user_pages(current, ctx->mm,
 					info->mmap_base, nr_pages, 
 					1, 0, info->ring_pages, NULL);
@@ -300,7 +294,7 @@
 	hlist_add_head_rcu(&ctx->list, &mm->ioctx_list);
 	spin_unlock(&mm->ioctx_lock);
 
-	dprintk("aio: allocated ioctx %p[%ld]: mm=%p mask=0x%x\n",
+	pr_debug("aio: allocated ioctx %p[%ld]: mm=%p mask=0x%x\n",
 		ctx, ctx->user_id, current->mm, ctx->ring_info.nr);
 	return ctx;
 
@@ -313,7 +307,7 @@
 	kmem_cache_free(kioctx_cachep, ctx);
 	ctx = ERR_PTR(-ENOMEM);
 
-	dprintk("aio: error allocating ioctx %p\n", ctx);
+	pr_debug("aio: error allocating ioctx %p\n", ctx);
 	return ctx;
 }
 
@@ -468,6 +462,7 @@
 		kfree(req->ki_iovec);
 	kmem_cache_free(kiocb_cachep, req);
 	ctx->reqs_active--;
+	pr_debug("really_put_req: req->ki_users: %d\n", req->ki_users);
 
 	if (unlikely(!ctx->reqs_active && ctx->dead))
 		wake_up(&ctx->wait);
@@ -503,12 +498,14 @@
  */
 static int __aio_put_req(struct kioctx *ctx, struct kiocb *req)
 {
-	dprintk(KERN_DEBUG "aio_put(%p): f_count=%ld\n",
+	pr_debug("aio_put(%p): f_count=%ld\n",
 		req, atomic_long_read(&req->ki_filp->f_count));
 
 	assert_spin_locked(&ctx->ctx_lock);
 
 	req->ki_users--;
+	pr_debug("__aio_put_req: req: %p, req->ki_users: %d\n",
+		req, req->ki_users);
 	BUG_ON(req->ki_users < 0);
 	if (likely(req->ki_users))
 		return 0;
@@ -954,6 +951,7 @@
 	 * cancelled requests don't get events, userland was given one
 	 * when the event got cancelled.
 	 */
+	pr_debug("kiocbIsCancelled(iocb): %d\n", kiocbIsCancelled(iocb));
 	if (kiocbIsCancelled(iocb))
 		goto put_rq;
 
@@ -969,7 +967,7 @@
 	event->res = res;
 	event->res2 = res2;
 
-	dprintk("aio_complete: %p[%lu]: %p: %p %Lx %lx %lx\n",
+	pr_debug("aio_complete: %p[%lu]: %p: %p %Lx %lx %lx\n",
 		ctx, tail, iocb, iocb->ki_obj.user, iocb->ki_user_data,
 		res, res2);
 
@@ -1056,7 +1054,7 @@
 	int ret = 0;
 
 	ring = kmap_atomic(info->ring_pages[0], KM_USER0);
-	dprintk("in aio_read_evt h%lu t%lu m%lu\n",
+	pr_debug("in aio_read_evt h%lu t%lu m%lu\n",
 		 (unsigned long)ring->head, (unsigned long)ring->tail,
 		 (unsigned long)ring->nr);
 
@@ -1079,7 +1077,7 @@
 
 out:
 	kunmap_atomic(ring, KM_USER0);
-	dprintk("leaving aio_read_evt: %d  h%lu t%lu\n", ret,
+	pr_debug("leaving aio_read_evt: %d  h%lu t%lu\n", ret,
 		 (unsigned long)ring->head, (unsigned long)ring->tail);
 	return ret;
 }
@@ -1145,13 +1143,13 @@
 		if (unlikely(ret <= 0))
 			break;
 
-		dprintk("read event: %Lx %Lx %Lx %Lx\n",
+		pr_debug("read event: %Lx %Lx %Lx %Lx\n",
 			ent.data, ent.obj, ent.res, ent.res2);
 
 		/* Could we split the check in two? */
 		ret = -EFAULT;
 		if (unlikely(copy_to_user(event, &ent, sizeof(ent)))) {
-			dprintk("aio: lost an event due to EFAULT.\n");
+			pr_debug("aio: lost an event due to EFAULT.\n");
 			break;
 		}
 		ret = 0;
@@ -1186,6 +1184,7 @@
 	}
 
 	while (likely(i < nr)) {
+		pr_debug("aio: i: %d, nr: %ld\n", i, nr);
 		add_wait_queue_exclusive(&ctx->wait, &wait);
 		do {
 			set_task_state(tsk, TASK_INTERRUPTIBLE);
@@ -1198,6 +1197,7 @@
 				ret = -EINVAL;
 				break;
 			}
+			pr_debug("aio: to.timed_out: %d\n", to.timed_out);
 			if (to.timed_out)	/* Only check after read evt */
 				break;
 			/* Try to only show up in io wait if there are ops
@@ -1221,7 +1221,7 @@
 
 		ret = -EFAULT;
 		if (unlikely(copy_to_user(event, &ent, sizeof(ent)))) {
-			dprintk("aio: lost an event due to EFAULT.\n");
+			pr_debug("aio: lost an event due to EFAULT.\n");
 			break;
 		}
 
@@ -1252,7 +1252,7 @@
 	hlist_del_rcu(&ioctx->list);
 	spin_unlock(&mm->ioctx_lock);
 
-	dprintk("aio_release(%p)\n", ioctx);
+	pr_debug("aio_release(%p)\n", ioctx);
 	if (likely(!was_dead))
 		put_ioctx(ioctx);	/* twice for the list */
 
@@ -1541,7 +1541,7 @@
 			kiocb->ki_retry = aio_fsync;
 		break;
 	default:
-		dprintk("EINVAL: io_submit: no operation provided\n");
+		pr_debug("EINVAL: io_submit: no operation provided\n");
 		ret = -EINVAL;
 	}
 
@@ -1626,7 +1626,7 @@
 
 	ret = put_user(req->ki_key, &user_iocb->aio_key);
 	if (unlikely(ret)) {
-		dprintk("EFAULT: aio_key\n");
+		pr_debug("EFAULT: aio_key\n");
 		goto out_put_req;
 	}
 

Re: [PATCH resubmit 1/3] AIO: Move aio_cancel_all()

[Сергей Темерханов]

[-- Attachment #1: Type: text/plain, Size: 1051 bytes --]

On Tuesday 16 March 2010 17:46:10 Jeff Moyer wrote:
> Sergey Temerkhanov <temerkhanov@cifronik.ru> writes:
> > io_cancel() and aio_cancel_all() have inconsistent reference counting
> > (ki_users field of struct kiocb) which leads to unkillable processes upon
> > io_cancel() or io_destroy() syscalls. This patch fixes the undesired
> > behavior.
> 
> Hi, Sergey,
> 
> Thanks for the patch.  Would you mind resubmitting it to make it a bit
> easier to review?  The way you've done things, it is difficult to tell
> if you just moved the aio_cancel_all function or if you moved it and
> made changes.  Please have the patch that moves it separated out from
> other changes, or at least mention in the changelog that the function
> was unchanged.  Next, if you could tell what sorts of testing you've
> performed, that would be great.  Finally, it would be a good idea to CC
> linux-aio@kvack.org on aio patches.
> 
> Thanks!
> Jeff

Well, I've broken the 1st patch into 2. This one just moves aio_cancel_all()

Regards, Sergey Temerkhanov, Cifronic ZAO

[-- Attachment #2: aio-move-cancel-all.patch --]
[-- Type: text/x-patch, Size: 2294 bytes --]

diff -r b290b47c0880 fs/aio.c
--- a/fs/aio.c	Tue Mar 16 13:51:13 2010 +0300
+++ b/fs/aio.c	Tue Mar 16 18:55:59 2010 +0300
@@ -63,6 +63,8 @@
 static void aio_kick_handler(struct work_struct *);
 static void aio_queue_work(struct kioctx *);
 
+static void aio_cancel_all(struct kioctx *);
+
 /* aio_setup
  *	Creates the slab caches used by the aio routines, panic on
  *	failure as this is done early during the boot sequence.
@@ -315,33 +317,6 @@
 	return ctx;
 }
 
-/* aio_cancel_all
- *	Cancels all outstanding aio requests on an aio context.  Used 
- *	when the processes owning a context have all exited to encourage 
- *	the rapid destruction of the kioctx.
- */
-static void aio_cancel_all(struct kioctx *ctx)
-{
-	int (*cancel)(struct kiocb *, struct io_event *);
-	struct io_event res;
-	spin_lock_irq(&ctx->ctx_lock);
-	ctx->dead = 1;
-	while (!list_empty(&ctx->active_reqs)) {
-		struct list_head *pos = ctx->active_reqs.next;
-		struct kiocb *iocb = list_kiocb(pos);
-		list_del_init(&iocb->ki_list);
-		cancel = iocb->ki_cancel;
-		kiocbSetCancelled(iocb);
-		if (cancel) {
-			iocb->ki_users++;
-			spin_unlock_irq(&ctx->ctx_lock);
-			cancel(iocb, &res);
-			spin_lock_irq(&ctx->ctx_lock);
-		}
-	}
-	spin_unlock_irq(&ctx->ctx_lock);
-}
-
 static void wait_for_all_aios(struct kioctx *ctx)
 {
 	struct task_struct *tsk = current;
@@ -1038,6 +1013,33 @@
 	return ret;
 }
 
+/* aio_cancel_all
+ *	Cancels all outstanding aio requests on an aio context.  Used 
+ *	when the processes owning a context have all exited to encourage 
+ *	the rapid destruction of the kioctx.
+ */
+static void aio_cancel_all(struct kioctx *ctx)
+{
+	int (*cancel)(struct kiocb *, struct io_event *);
+	struct io_event res;
+	spin_lock_irq(&ctx->ctx_lock);
+	ctx->dead = 1;
+	while (!list_empty(&ctx->active_reqs)) {
+		struct list_head *pos = ctx->active_reqs.next;
+		struct kiocb *iocb = list_kiocb(pos);
+		list_del_init(&iocb->ki_list);
+		cancel = iocb->ki_cancel;
+		kiocbSetCancelled(iocb);
+		if (cancel) {
+			iocb->ki_users++;
+			spin_unlock_irq(&ctx->ctx_lock);
+			cancel(iocb, &res);
+			spin_lock_irq(&ctx->ctx_lock);
+		}
+	}
+	spin_unlock_irq(&ctx->ctx_lock);
+}
+
 /* aio_read_evt
  *	Pull an event off of the ioctx's event ring.  Returns the number of 
  *	events fetched (0 or 1 ;-)