+ aio-completion-signal-notification.patch added to -mm tree

+ aio-completion-signal-notification.patch added to -mm tree

[akpm]

The patch titled
     aio: AIO completion signal notification
has been added to the -mm tree.  Its filename is
     aio-completion-signal-notification.patch

*** Remember to use Documentation/SubmitChecklist when testing your code ***

See http://www.zip.com.au/~akpm/linux/patches/stuff/added-to-mm.txt to find
out what to do about this

------------------------------------------------------
Subject: aio: AIO completion signal notification
From: Sbastien Dugu <sebastien.dugue@bull.net>

The current 2.6 kernel does not support notification of user space via an RT
signal upon an asynchronous IO completion.  The POSIX specification states
that when an AIO request completes, a signal can be delivered to the
application as notification.

This patch adds a struct sigevent *aio_sigeventp to the iocb.  The relevant
fields (pid, signal number and value) are stored in the kiocb for use when the
request completes.

That sigevent structure is filled by the application as part of the AIO
request preparation.  Upon request completion, the kernel notifies the
application using those sigevent parameters.  If SIGEV_NONE has been
specified, then the old behaviour is retained and the application must rely on
polling the completion queue using io_getevents().

A struct sigevent *aio_sigeventp field is added to struct iocb in
include/linux/aio_abi.h

A struct aio_notify containing the sigevent parameters is defined in aio.h:

  struct aio_notify {
	struct task_struct	*target;
	__u16			signo;
	__u16			notify;
	sigval_t		value;
  };

A struct aio_notify ki_notify is added to struct kiocb in include/linux/aio.h

In io_submit_one(), if the application provided a sigevent then
setup_sigevent() is called which does the following:

	- check access to the user sigevent and make a local copy

	- if the requested notification is SIGEV_NONE, then nothing to do

	- fill in the kiocb->ki_notify fields (notify, signo, value)

	- check sigevent consistency, get the signal target task and
	  save it in kiocb->ki_notify.target

	- preallocate a sigqueue for this event using sigqueue_alloc()

Upon request completion, in aio_complete(), if notification is needed for this
request (iocb->ki_notify.notify != SIGEV_NONE), then aio_send_signal() is
called to signal the target task as follows:

	- fill in the siginfo struct to be sent to the task

	- if notify is SIGEV_THREAD_ID then send signal to specific task
	  using send_sigqueue()

	- else send signal to task group using send_5group_sigqueue()

Notes concerning sigqueue preallocation:

To ensure reliable delivery of completion notification, the sigqueue is
preallocated in the submission path so that there is no chance it can fail in
the completion path.

Unlike the posix-timers case (currently the single other user of sigqueue
preallocation), where the sigqueue is allocated for the lifetime of the timer
and freed at timer destruction time, the aio case is a bit more tricky due to
the async nature of the whole thing.

In the aio case, the sigqueue exists for the lifetime of the request,
therefore it must be freed only once the signal for the request completion has
been delivered.  This involves changing __sigqueue_free() to free the sigqueue
when the signal is collected if si_code is SI_ASYNCIO even if it was
preallocated as well as explicitly calling sigqueue_free() in submission and
completion error paths.

Signed-off-by: Sébastien Dugué <sebastien.dugue@bull.net>
Signed-off-by: Laurent Vivier <laurent.vivier@bull.net>
Cc: Zach Brown <zach.brown@oracle.com>
Cc: Suparna Bhattacharya <suparna@in.ibm.com>
Cc: Benjamin LaHaise <bcrl@kvack.org>
Cc: Ulrich Drepper <drepper@redhat.com>
Cc: Ingo Molnar <mingo@elte.hu>
Cc: Thomas Gleixner <tglx@linutronix.de>
Signed-off-by: Andrew Morton <akpm@osdl.org>
---

 fs/aio.c                |  115 ++++++++++++++++++++++++++++++++++++--
 fs/compat.c             |   18 +++++
 include/linux/aio.h     |   12 +++
 include/linux/aio_abi.h |    3 
 kernel/signal.c         |    2 
 5 files changed, 144 insertions(+), 6 deletions(-)

diff -puN fs/aio.c~aio-completion-signal-notification fs/aio.c
--- a/fs/aio.c~aio-completion-signal-notification
+++ a/fs/aio.c
@@ -419,6 +419,7 @@ static struct kiocb fastcall *__aio_get_
 	req->ki_dtor = NULL;
 	req->private = NULL;
 	req->ki_iovec = NULL;
+	req->ki_notify.sigq = NULL;
 	INIT_LIST_HEAD(&req->ki_run_list);
 
 	/* Check if the completion queue has enough free space to
@@ -465,6 +466,12 @@ static inline void really_put_req(struct
 		req->ki_dtor(req);
 	if (req->ki_iovec != &req->ki_inline_vec)
 		kfree(req->ki_iovec);
+
+	/* Release task ref */
+	if (req->ki_notify.notify == SIGEV_THREAD_ID ||
+	    req->ki_notify.notify == SIGEV_SIGNAL)
+		put_task_struct(req->ki_notify.target);
+
 	kmem_cache_free(kiocb_cachep, req);
 	ctx->reqs_active--;
 
@@ -916,6 +923,79 @@ void fastcall kick_iocb(struct kiocb *io
 }
 EXPORT_SYMBOL(kick_iocb);
 
+static int aio_send_signal(struct aio_notify *notify)
+{
+	struct sigqueue *sigq = notify->sigq;
+	struct siginfo *info = &sigq->info;
+	int ret;
+
+	memset(info, 0, sizeof(struct siginfo));
+
+	info->si_signo = notify->signo;
+	info->si_errno = 0;
+	info->si_code = SI_ASYNCIO;
+	info->si_pid = 0;
+	info->si_uid = 0;
+	info->si_value = notify->value;
+
+	if (notify->notify & SIGEV_THREAD_ID)
+		ret = send_sigqueue(notify->signo, sigq, notify->target);
+	else
+		ret = send_group_sigqueue(notify->signo, sigq, notify->target);
+
+	return ret;
+}
+
+static long aio_setup_sigevent(struct aio_notify *notify,
+			       struct sigevent __user *user_event)
+{
+	sigevent_t event;
+	struct task_struct *target;
+
+	if (copy_from_user(&event, user_event, sizeof(event)))
+		return -EFAULT;
+
+	if (event.sigev_notify == SIGEV_NONE)
+		return 0;
+
+	notify->notify = event.sigev_notify;
+	notify->signo = event.sigev_signo;
+	notify->value = event.sigev_value;
+
+	read_lock(&tasklist_lock);
+	target = good_sigevent(&event);
+
+	if (unlikely(!target || (target->flags & PF_EXITING)))
+		goto out_unlock;
+
+	/*
+	 * At this point, we know that notify is either SIGEV_SIGNAL or
+	 * SIGEV_THREAD_ID and the target task is valid. So get a reference
+	 * on the task, it will be dropped in really_put_req() when
+	 * we're done with the request.
+	 */
+	get_task_struct(target);
+	notify->target = target;
+	read_unlock(&tasklist_lock);
+
+	/*
+	 * NOTE: we cannot free the sigqueue in the completion path as
+	 * the signal may not have been delivered to the target task.
+	 * Therefore it has to be freed in __sigqueue_free() when the
+	 * signal is collected if si_code is SI_ASYNCIO.
+	 */
+	notify->sigq = sigqueue_alloc();
+
+	if (unlikely(!notify->sigq))
+		return -EAGAIN;
+
+	return 0;
+
+out_unlock:
+	read_unlock(&tasklist_lock);
+	return -EINVAL;
+}
+
 /* aio_complete
  *	Called when the io request on the given iocb is complete.
  *	Returns true if this is the last user of the request.  The 
@@ -963,8 +1043,11 @@ int fastcall aio_complete(struct kiocb *
 	 * cancelled requests don't get events, userland was given one
 	 * when the event got cancelled.
 	 */
-	if (kiocbIsCancelled(iocb))
+	if (kiocbIsCancelled(iocb)) {
+		if (iocb->ki_notify.sigq)
+			sigqueue_free(iocb->ki_notify.sigq);
 		goto put_rq;
+	}
 
 	ring = kmap_atomic(info->ring_pages[0], KM_IRQ1);
 
@@ -994,6 +1077,15 @@ int fastcall aio_complete(struct kiocb *
 	kunmap_atomic(ring, KM_IRQ1);
 
 	pr_debug("added to ring %p at [%lu]\n", iocb, tail);
+
+	if (iocb->ki_notify.notify != SIGEV_NONE) {
+		ret = aio_send_signal(&iocb->ki_notify);
+
+		/* If signal generation failed, release the sigqueue */
+		if (ret)
+			sigqueue_free(iocb->ki_notify.sigq);
+	}
+
 put_rq:
 	/* everything turned out well, dispose of the aiocb. */
 	ret = __aio_put_req(ctx, iocb);
@@ -1545,8 +1637,7 @@ int fastcall io_submit_one(struct kioctx
 	ssize_t ret;
 
 	/* enforce forwards compatibility on users */
-	if (unlikely(iocb->aio_reserved1 || iocb->aio_reserved2 ||
-		     iocb->aio_reserved3)) {
+	if (unlikely(iocb->aio_reserved1 || iocb->aio_reserved3)) {
 		pr_debug("EINVAL: io_submit: reserve field set\n");
 		return -EINVAL;
 	}
@@ -1555,6 +1646,7 @@ int fastcall io_submit_one(struct kioctx
 	if (unlikely(
 	    (iocb->aio_buf != (unsigned long)iocb->aio_buf) ||
 	    (iocb->aio_nbytes != (size_t)iocb->aio_nbytes) ||
+	    (iocb->aio_sigeventp != (unsigned long)iocb->aio_sigeventp) ||
 	    ((ssize_t)iocb->aio_nbytes < 0)
 	   )) {
 		pr_debug("EINVAL: io_submit: overflow check\n");
@@ -1588,11 +1680,21 @@ int fastcall io_submit_one(struct kioctx
 	init_waitqueue_func_entry(&req->ki_wait.wait, aio_wake_function);
 	INIT_LIST_HEAD(&req->ki_wait.wait.task_list);
  	req->ki_run_list.next = req->ki_run_list.prev = NULL;
+	/* handle setting up the sigevent for POSIX AIO signals */
+	req->ki_notify.notify = SIGEV_NONE;
+
+	if (iocb->aio_sigeventp) {
+		ret = aio_setup_sigevent(&req->ki_notify,
+					 (struct sigevent __user *)(unsigned long)
+					 iocb->aio_sigeventp);
+		if (ret)
+			goto out_put_req;
+	}
 
 	ret = aio_setup_iocb(req);
 
 	if (ret)
-		goto out_put_req;
+		goto out_sigqfree;
 
 	spin_lock_irq(&ctx->ctx_lock);
 	aio_run_iocb(req);
@@ -1605,6 +1707,11 @@ int fastcall io_submit_one(struct kioctx
 	aio_put_req(req);	/* drop extra ref to req */
 	return 0;
 
+out_sigqfree:
+	/* Undo the sigqueue alloc if someting went bad */
+	if (req->ki_notify.sigq)
+		sigqueue_free(req->ki_notify.sigq);
+
 out_put_req:
 	aio_put_req(req);	/* drop extra ref to req */
 	aio_put_req(req);	/* drop i/o ref to req */
diff -puN fs/compat.c~aio-completion-signal-notification fs/compat.c
--- a/fs/compat.c~aio-completion-signal-notification
+++ a/fs/compat.c
@@ -665,6 +665,7 @@ compat_sys_io_submit(aio_context_t ctx_i
 		compat_uptr_t uptr;
 		struct iocb __user *user_iocb;
 		struct iocb tmp;
+		struct compat_sigevent __user *uevent;
 
 		if (unlikely(get_user(uptr, iocb + i))) {
 			ret = -EFAULT;
@@ -678,6 +679,23 @@ compat_sys_io_submit(aio_context_t ctx_i
 			break;
 		}
 
+		uevent = (struct compat_sigevent __user *)tmp.aio_sigeventp;
+
+		if (uevent) {
+			struct sigevent __user *event = NULL;
+			struct sigevent kevent;
+
+			event = compat_alloc_user_space(sizeof(*event));
+
+			if (get_compat_sigevent(&kevent, uevent) ||
+			    copy_to_user(event, &kevent, sizeof(*event))) {
+				ret = -EFAULT;
+				break;
+			}
+
+			tmp.aio_sigeventp = (__u64)event;
+		}
+
 		ret = io_submit_one(ctx, user_iocb, &tmp);
 		if (ret)
 			break;
diff -puN include/linux/aio.h~aio-completion-signal-notification include/linux/aio.h
--- a/include/linux/aio.h~aio-completion-signal-notification
+++ a/include/linux/aio.h
@@ -7,6 +7,7 @@
 #include <linux/uio.h>
 
 #include <asm/atomic.h>
+#include <asm/siginfo.h>
 
 #define AIO_MAXSEGS		4
 #define AIO_KIOGRP_NR_ATOMIC	8
@@ -54,6 +55,14 @@ struct kioctx;
 #define kiocbIsCancelled(iocb)	test_bit(KIF_CANCELLED, &(iocb)->ki_flags)
 #define kiocbIsRestarted(iocb)	test_bit(KIF_RESTARTED, &(iocb)->ki_flags)
 
+struct aio_notify {
+	struct task_struct	*target;
+	__u16			signo;
+	__u16			notify;
+	sigval_t		value;
+	struct sigqueue		*sigq;
+};
+
 /* is there a better place to document function pointer methods? */
 /**
  * ki_retry	-	iocb forward progress callback
@@ -123,6 +132,9 @@ struct kiocb {
 
 	struct list_head	ki_list;	/* the aio core uses this
 						 * for cancellation */
+
+	/* to notify a process on I/O event */
+	struct aio_notify	ki_notify;
 };
 
 #define is_sync_kiocb(iocb)	((iocb)->ki_key == KIOCB_SYNC_KEY)
diff -puN include/linux/aio_abi.h~aio-completion-signal-notification include/linux/aio_abi.h
--- a/include/linux/aio_abi.h~aio-completion-signal-notification
+++ a/include/linux/aio_abi.h
@@ -82,8 +82,9 @@ struct iocb {
 	__u64	aio_nbytes;
 	__s64	aio_offset;
 
+	__u64	aio_sigeventp;	/* pointer to struct sigevent */
+
 	/* extra parameters */
-	__u64	aio_reserved2;	/* TODO: use this for a (struct sigevent *) */
 	__u64	aio_reserved3;
 }; /* 64 bytes */
 
diff -puN kernel/signal.c~aio-completion-signal-notification kernel/signal.c
--- a/kernel/signal.c~aio-completion-signal-notification
+++ a/kernel/signal.c
@@ -296,7 +296,7 @@ static struct sigqueue *__sigqueue_alloc
 
 static void __sigqueue_free(struct sigqueue *q)
 {
-	if (q->flags & SIGQUEUE_PREALLOC)
+	if (q->flags & SIGQUEUE_PREALLOC && q->info.si_code != SI_ASYNCIO)
 		return;
 	atomic_dec(&q->user->sigpending);
 	free_uid(q->user);
_

Patches currently in -mm which might be from sebastien.dugue@bull.net are

rework-compat_sys_io_submit.patch
fix-aioh-includes.patch
make-good_sigevent-non-static.patch
aio-completion-signal-notification.patch
add-listio-syscall-support.patch

Re: + aio-completion-signal-notification.patch added to -mm tree

[Oleg Nesterov]

Sebastien Dugue wrote:
>
> +static long aio_setup_sigevent(struct aio_notify *notify,
> +			       struct sigevent __user *user_event)
> +{
> +	sigevent_t event;
> +	struct task_struct *target;
> +
> +	if (copy_from_user(&event, user_event, sizeof(event)))
> +		return -EFAULT;
> +
> +	if (event.sigev_notify == SIGEV_NONE)
> +		return 0;
> +
> +	notify->notify = event.sigev_notify;
> +	notify->signo = event.sigev_signo;
> +	notify->value = event.sigev_value;
> +
> +	read_lock(&tasklist_lock);

We don't need tasklist_lock, we can use rcu_read_lock() instead.

> +	target = good_sigevent(&event);
> +
> +	if (unlikely(!target || (target->flags & PF_EXITING)))
> +		goto out_unlock;

PF_EXITING check is racy and unneded. In fact, it is wrong. If the main
thread is already died, we can only use SIGEV_THREAD_ID signals, because
otherwise good_sigevent() returns ->group_leader.

> @@ -994,6 +1077,15 @@ int fastcall aio_complete(struct kiocb *
>  	kunmap_atomic(ring, KM_IRQ1);
>
>  	pr_debug("added to ring %p at [%lu]\n", iocb, tail);
> +
> +	if (iocb->ki_notify.notify != SIGEV_NONE) {
> +		ret = aio_send_signal(&iocb->ki_notify);
> +
> +		/* If signal generation failed, release the sigqueue */
> +		if (ret)
> +			sigqueue_free(iocb->ki_notify.sigq);

We should not use sigqueue_free() here. It takes current->sighand->siglock
to remove sigqueue from "struct sigpending". But current is just a "random"
process here.

Yes, if I understand this patch correctly, it is not possible that this
sigqueue is pending, but still this is bad imho.

>  static void __sigqueue_free(struct sigqueue *q)
>  {
> -	if (q->flags & SIGQUEUE_PREALLOC)
> +	if (q->flags & SIGQUEUE_PREALLOC && q->info.si_code != SI_ASYNCIO)
>  		return;

Oh, this is not nice. Could we change send_sigqueue/send_group_sigqueue
instead ?

-	BUG_ON(!(q->flags & SIGQUEUE_PREALLOC));
+	BUG_ON(!(q->flags & SIGQUEUE_PREALLOC) && q->info.si_code != SI_ASYNCIO);

This way aio can use __sigqueue_alloc/__sigqueue_free directly and forget
about SIGQUEUE_PREALLOC.

On the other hand, imho this patch takes a wrong direction.

The purpose of SIGQUEUE_PREALLOC + send_sigqueue() is to re-use the same
sigqueue while sending a stream of signals. But in aio case we allocate
sigqueue to send only 1 signal, then it freed after the delivery like
the regular sigqueue. So what is the point?

I'd suggest to not use this interface. Just use group_send_sig_info() or
specific_send_sig_info(). Yes, this way we will do GFP_ATOMIC allocation
of sigqueue in interrupt context, but is this so bad in this case?

Oleg.

Re: + aio-completion-signal-notification.patch added to -mm tree

[Oleg Nesterov]

On 01/25, Oleg Nesterov wrote:
>
> Sebastien Dugue wrote:
> >
> > +	if (iocb->ki_notify.notify != SIGEV_NONE) {
> > +		ret = aio_send_signal(&iocb->ki_notify);
> > +
> > +		/* If signal generation failed, release the sigqueue */
> > +		if (ret)
> > +			sigqueue_free(iocb->ki_notify.sigq);
> 
> We should not use sigqueue_free() here. It takes current->sighand->siglock
> to remove sigqueue from "struct sigpending". But current is just a "random"
> process here.

... and it is possible that current->sighand == NULL.

Oleg.

Re: + aio-completion-signal-notification.patch added to -mm tree

[Sébastien Dugué]

On Thu, 25 Jan 2007 19:21:41 +0300 Oleg Nesterov <oleg@tv-sign.ru> wrote:

> Sebastien Dugue wrote:
> >
> > +static long aio_setup_sigevent(struct aio_notify *notify,
> > +			       struct sigevent __user *user_event)
> > +{
> > +	sigevent_t event;
> > +	struct task_struct *target;
> > +
> > +	if (copy_from_user(&event, user_event, sizeof(event)))
> > +		return -EFAULT;
> > +
> > +	if (event.sigev_notify == SIGEV_NONE)
> > +		return 0;
> > +
> > +	notify->notify = event.sigev_notify;
> > +	notify->signo = event.sigev_signo;
> > +	notify->value = event.sigev_value;
> > +
> > +	read_lock(&tasklist_lock);
> 
> We don't need tasklist_lock, we can use rcu_read_lock() instead.

  Right that will be beneficial, will change.

> 
> > +	target = good_sigevent(&event);
> > +
> > +	if (unlikely(!target || (target->flags & PF_EXITING)))
> > +		goto out_unlock;
> 
> PF_EXITING check is racy and unneded. In fact, it is wrong. If the main
> thread is already died, we can only use SIGEV_THREAD_ID signals, because
> otherwise good_sigevent() returns ->group_leader.

  Care to explain here please, I'm not following you.

> 
> > @@ -994,6 +1077,15 @@ int fastcall aio_complete(struct kiocb *
> >  	kunmap_atomic(ring, KM_IRQ1);
> >
> >  	pr_debug("added to ring %p at [%lu]\n", iocb, tail);
> > +
> > +	if (iocb->ki_notify.notify != SIGEV_NONE) {
> > +		ret = aio_send_signal(&iocb->ki_notify);
> > +
> > +		/* If signal generation failed, release the sigqueue */
> > +		if (ret)
> > +			sigqueue_free(iocb->ki_notify.sigq);
> 
> We should not use sigqueue_free() here. It takes current->sighand->siglock
> to remove sigqueue from "struct sigpending". But current is just a "random"
> process here.
> 
> Yes, if I understand this patch correctly, it is not possible that this
> sigqueue is pending, but still this is bad imho.

 Yes, in fact the sigqueue is used for a single signal delivery and then
free. In fact I could have used directly __sigqueue_free() instead here
except for the fact that it's private to signal.c and I'm reluctant
to export it to other subsystems.

> 
> >  static void __sigqueue_free(struct sigqueue *q)
> >  {
> > -	if (q->flags & SIGQUEUE_PREALLOC)
> > +	if (q->flags & SIGQUEUE_PREALLOC && q->info.si_code != SI_ASYNCIO)
> >  		return;
> 
> Oh, this is not nice. Could we change send_sigqueue/send_group_sigqueue
> instead ?

  Yep, that's the other solution.

> 
> -	BUG_ON(!(q->flags & SIGQUEUE_PREALLOC));
> +	BUG_ON(!(q->flags & SIGQUEUE_PREALLOC) && q->info.si_code != SI_ASYNCIO);
> 
> This way aio can use __sigqueue_alloc/__sigqueue_free directly and forget
> about SIGQUEUE_PREALLOC.

  Well, I don't think it's cleaner. The aio error path calls sigqueue_free()
directly whereas in case of success sigqueue_free() is called from the signal
delivery path.

> 
> On the other hand, imho this patch takes a wrong direction.
> 
> The purpose of SIGQUEUE_PREALLOC + send_sigqueue() is to re-use the same
> sigqueue while sending a stream of signals. But in aio case we allocate
> sigqueue to send only 1 signal, then it freed after the delivery like
> the regular sigqueue. So what is the point?
> 
> I'd suggest to not use this interface. Just use group_send_sig_info() or
> specific_send_sig_info(). Yes, this way we will do GFP_ATOMIC allocation
> of sigqueue in interrupt context, but is this so bad in this case?

  Well, the thihere is that in the past we used group_send_sig_info()
and specific_send_sig_info() for notification but Zach Brown raised
the question about reliable signal delivery. IOW an aio submission
should not succeed if signal delivery is going to fail. Hence the
use of the preallocated sigqueue.

  Thanks,

  Sébastien.

Re: + aio-completion-signal-notification.patch added to -mm tree

[Oleg Nesterov]

On 01/26, S?bastien Dugu? wrote:
>
> On Thu, 25 Jan 2007 19:21:41 +0300 Oleg Nesterov <oleg@tv-sign.ru> wrote:
> 
> > > +	target = good_sigevent(&event);
> > > +
> > > +	if (unlikely(!target || (target->flags & PF_EXITING)))
> > > +		goto out_unlock;
> > 
> > PF_EXITING check is racy and unneded. In fact, it is wrong. If the main
> > thread is already died, we can only use SIGEV_THREAD_ID signals, because
> > otherwise good_sigevent() returns ->group_leader.
> 
>   Care to explain here please, I'm not following you.

My apologies, I was unclear.

This check is racy, the condition could be changed right after the check.

It is unneeded, it is ok to do send_sigqueue(tsk) if if that task is already
dead. (we hold the reference to task_struct).

Now suppose that the main thread (->group_leader) already exited. This is
normal, the thread group is still alive, it should be ok to send a signal to
it via send_group_sigqueue(). But we can't: without SIGEV_THREAD_ID in
->sigev_notify good_event() returns ->group_leader, and it has PF_EXITING.

Yes, kernel/posix-timers.c needs a cleanup too. But please note that it does
this check for another reason (according to the comment). This reason is not
valid now, the callsite for exit_itimers() was moved from __exit_signal() to
do_exit().

> > > +	if (iocb->ki_notify.notify != SIGEV_NONE) {
> > > +		ret = aio_send_signal(&iocb->ki_notify);
> > > +
> > > +		/* If signal generation failed, release the sigqueue */
> > > +		if (ret)
> > > +			sigqueue_free(iocb->ki_notify.sigq);
> > 
> > We should not use sigqueue_free() here. It takes current->sighand->siglock
> > to remove sigqueue from "struct sigpending". But current is just a "random"
> > process here.
> > 
> > Yes, if I understand this patch correctly, it is not possible that this
> > sigqueue is pending, but still this is bad imho.
> 
>  Yes, in fact the sigqueue is used for a single signal delivery and then
> free. In fact I could have used directly __sigqueue_free() instead here
> except for the fact that it's private to signal.c and I'm reluctant
> to export it to other subsystems.

I personally think it is better to export __sigqueue_free() even if sigqueue_free()
happens to work. It is to fragile imho to reference current->sighand. At least
we need a fat comment.

> > >  static void __sigqueue_free(struct sigqueue *q)
> > >  {
> > > -	if (q->flags & SIGQUEUE_PREALLOC)
> > > +	if (q->flags & SIGQUEUE_PREALLOC && q->info.si_code != SI_ASYNCIO)
> > >  		return;
> > 
> > Oh, this is not nice. Could we change send_sigqueue/send_group_sigqueue
> > instead ?
> 
>   Yep, that's the other solution.
> 
> > 
> > -	BUG_ON(!(q->flags & SIGQUEUE_PREALLOC));
> > +	BUG_ON(!(q->flags & SIGQUEUE_PREALLOC) && q->info.si_code != SI_ASYNCIO);
> > 
> > This way aio can use __sigqueue_alloc/__sigqueue_free directly and forget
> > about SIGQUEUE_PREALLOC.
> 
>   Well, I don't think it's cleaner. The aio error path calls sigqueue_free()
> directly whereas in case of success sigqueue_free() is called from the signal
> delivery path.

Hmm... now I don't understand you. Of course, the aio error path should use
__sigqueue_free() if we don't use SIGQUEUE_PREALLOC (and imho we should not).

And the signal delivery path uses __sigqueue_free() too.

?

> > I'd suggest to not use this interface. Just use group_send_sig_info() or
> > specific_send_sig_info(). Yes, this way we will do GFP_ATOMIC allocation
> > of sigqueue in interrupt context, but is this so bad in this case?
> 
>   Well, the thihere is that in the past we used group_send_sig_info()
> and specific_send_sig_info() for notification but Zach Brown raised
> the question about reliable signal delivery. IOW an aio submission
> should not succeed if signal delivery is going to fail. Hence the
> use of the preallocated sigqueue.

Ok, I see, thanks.

Oleg.

Re: + aio-completion-signal-notification.patch added to -mm tree

[Sébastien Dugué]

On Fri, 26 Jan 2007 14:52:33 +0300 Oleg Nesterov <oleg@tv-sign.ru> wrote:

> On 01/26, S?bastien Dugu? wrote:
> >
> > On Thu, 25 Jan 2007 19:21:41 +0300 Oleg Nesterov <oleg@tv-sign.ru> wrote:
> > 
> > > > +	target = good_sigevent(&event);
> > > > +
> > > > +	if (unlikely(!target || (target->flags & PF_EXITING)))
> > > > +		goto out_unlock;
> > > 
> > > PF_EXITING check is racy and unneded. In fact, it is wrong. If the main
> > > thread is already died, we can only use SIGEV_THREAD_ID signals, because
> > > otherwise good_sigevent() returns ->group_leader.
> > 
> >   Care to explain here please, I'm not following you.
> 
> My apologies, I was unclear.
> 
> This check is racy, the condition could be changed right after the check.
> 
> It is unneeded, it is ok to do send_sigqueue(tsk) if if that task is already
> dead. (we hold the reference to task_struct).
> 
> Now suppose that the main thread (->group_leader) already exited. This is
> normal, the thread group is still alive, it should be ok to send a signal to
> it via send_group_sigqueue(). But we can't: without SIGEV_THREAD_ID in
> ->sigev_notify good_event() returns ->group_leader, and it has PF_EXITING.

  Thanks, I understand the problem now. I will fix this.

> 
> Yes, kernel/posix-timers.c needs a cleanup too. But please note that it does
> this check for another reason (according to the comment). This reason is not
> valid now, the callsite for exit_itimers() was moved from __exit_signal() to
> do_exit().
> 
> > > > +	if (iocb->ki_notify.notify != SIGEV_NONE) {
> > > > +		ret = aio_send_signal(&iocb->ki_notify);
> > > > +
> > > > +		/* If signal generation failed, release the sigqueue */
> > > > +		if (ret)
> > > > +			sigqueue_free(iocb->ki_notify.sigq);
> > > 
> > > We should not use sigqueue_free() here. It takes current->sighand->siglock
> > > to remove sigqueue from "struct sigpending". But current is just a "random"
> > > process here.
> > > 
> > > Yes, if I understand this patch correctly, it is not possible that this
> > > sigqueue is pending, but still this is bad imho.
> > 
> >  Yes, in fact the sigqueue is used for a single signal delivery and then
> > free. In fact I could have used directly __sigqueue_free() instead here
> > except for the fact that it's private to signal.c and I'm reluctant
> > to export it to other subsystems.
> 
> I personally think it is better to export __sigqueue_free() even if sigqueue_free()
> happens to work. It is to fragile imho to reference current->sighand. At least
> we need a fat comment.

  OK.

> 
> > > >  static void __sigqueue_free(struct sigqueue *q)
> > > >  {
> > > > -	if (q->flags & SIGQUEUE_PREALLOC)
> > > > +	if (q->flags & SIGQUEUE_PREALLOC && q->info.si_code != SI_ASYNCIO)
> > > >  		return;
> > > 
> > > Oh, this is not nice. Could we change send_sigqueue/send_group_sigqueue
> > > instead ?
> > 
> >   Yep, that's the other solution.
> > 
> > > 
> > > -	BUG_ON(!(q->flags & SIGQUEUE_PREALLOC));
> > > +	BUG_ON(!(q->flags & SIGQUEUE_PREALLOC) && q->info.si_code != SI_ASYNCIO);
> > > 
> > > This way aio can use __sigqueue_alloc/__sigqueue_free directly and forget
> > > about SIGQUEUE_PREALLOC.
> > 
> >   Well, I don't think it's cleaner. The aio error path calls sigqueue_free()
> > directly whereas in case of success sigqueue_free() is called from the signal
> > delivery path.
> 
> Hmm... now I don't understand you. Of course, the aio error path should use
> __sigqueue_free() if we don't use SIGQUEUE_PREALLOC (and imho we should not).
> 
> And the signal delivery path uses __sigqueue_free() too.
> 
> ?
> 
> > > I'd suggest to not use this interface. Just use group_send_sig_info() or
> > > specific_send_sig_info(). Yes, this way we will do GFP_ATOMIC allocation
> > > of sigqueue in interrupt context, but is this so bad in this case?
> > 
> >   Well, the thihere is that in the past we used group_send_sig_info()
> > and specific_send_sig_info() for notification but Zach Brown raised
> > the question about reliable signal delivery. IOW an aio submission
> > should not succeed if signal delivery is going to fail. Hence the
> > use of the preallocated sigqueue.
> 
> Ok, I see, thanks.
> 
> Oleg.
> 

+ aio-completion-signal-notification.patch added to -mm tree

[akpm]

The patch titled
     AIO completion signal notification
has been added to the -mm tree.  Its filename is
     aio-completion-signal-notification.patch

*** Remember to use Documentation/SubmitChecklist when testing your code ***

See http://www.zip.com.au/~akpm/linux/patches/stuff/added-to-mm.txt to find
out what to do about this

------------------------------------------------------
Subject: AIO completion signal notification
From: Sébastien Dugué <sebastien.dugue@bull.net>

The current 2.6 kernel does not support notification of user space via an RT
signal upon an asynchronous IO completion.  The POSIX specification states
that when an AIO request completes, a signal can be delivered to the
application as notification.

This patch adds a struct sigevent *aio_sigeventp to the iocb.  The relevant
fields (pid, signal number and value) are stored in the kiocb for use when the
request completes.

That sigevent structure is filled by the application as part of the AIO
request preparation.  Upon request completion, the kernel notifies the
application using those sigevent parameters.  If SIGEV_NONE has been
specified, then the old behaviour is retained and the application must rely on
polling the completion queue using io_getevents().

A struct sigevent *aio_sigeventp field is added to struct iocb in
include/linux/aio_abi.h

A struct aio_notify containing the sigevent parameters is defined in aio.h:

  struct aio_notify {
	struct task_struct	*target;
	__u16			signo;
	__u16			notify;
	sigval_t		value;
  };

A struct aio_notify ki_notify is added to struct kiocb in include/linux/aio.h

In io_submit_one(), if the application provided a sigevent then
aio_setup_sigevent() is called which does the following:

	- check access to the user sigevent and make a local copy

	- if the requested notification is SIGEV_NONE, then nothing to do

	- fill in the kiocb->ki_notify fields (notify, signo, value)

	- check sigevent consistency, get the signal target task and
	  save it in kiocb->ki_notify.target

	- preallocate a sigqueue for this event using __sigqueue_alloc()

Upon request completion, in aio_complete(), if notification is needed for this
request (iocb->ki_notify.notify != SIGEV_NONE), then aio_send_signal() is
called to signal the target task as follows:

	- fill in the siginfo struct to be sent to the task

	- if notify is SIGEV_THREAD_ID then send signal to specific task
	  using send_sigqueue()

	- else send signal to task group using send_5group_sigqueue()

Notes concerning sigqueue preallocation:

To ensure reliable delivery of completion notification, the sigqueue is
preallocated in the submission path so that there is no chance it can fail in
the completion path.

Unlike the posix-timers case (currently the single other user of sigqueue
preallocation), where the sigqueue is allocated for the lifetime of the timer
and freed at timer destruction time, the aio case is a bit more tricky due to
the async nature of the whole thing.

In the aio case, the sigqueue exists for the lifetime of the request,
therefore it must be freed only once the signal for the request completion has
been delivered.  The sigqueue is therefore freed by the signal delivery
mechanism when the signal is collected.  The aio submission and completion
error paths do an explicit __sigqueue_free() to free the sigqueue.

Signed-off-by: Sébastien Dugué <sebastien.dugue@bull.net>
Signed-off-by: Laurent Vivier <laurent.vivier@bull.net>
Cc: Bharata B Rao <bharata@in.ibm.com>
Cc: Christoph Hellwig <hch@infradead.org>
Cc: Suparna Bhattacharya <suparna@in.ibm.com>
Cc: Zach Brown <zach.brown@oracle.com>
Cc: Oleg Nesterov <oleg@tv-sign.ru>
Cc: Badari Pulavarty <pbadari@us.ibm.com>
Cc: Benjamin LaHaise <bcrl@linux.intel.com>
Cc: Jean Pierre Dion <jean-pierre.dion@bull.net>
Signed-off-by: Andrew Morton <akpm@osdl.org>
---

 fs/aio.c                |  114 ++++++++++++++++++++++++++++++++++++--
 fs/compat.c             |   18 ++++++
 include/linux/aio.h     |   12 ++++
 include/linux/aio_abi.h |    3 -
 kernel/signal.c         |    4 -
 5 files changed, 144 insertions(+), 7 deletions(-)

diff -puN fs/aio.c~aio-completion-signal-notification fs/aio.c
--- a/fs/aio.c~aio-completion-signal-notification
+++ a/fs/aio.c
@@ -419,6 +419,7 @@ static struct kiocb fastcall *__aio_get_
 	req->ki_dtor = NULL;
 	req->private = NULL;
 	req->ki_iovec = NULL;
+	req->ki_notify.sigq = NULL;
 	INIT_LIST_HEAD(&req->ki_run_list);
 
 	/* Check if the completion queue has enough free space to
@@ -465,6 +466,12 @@ static inline void really_put_req(struct
 		req->ki_dtor(req);
 	if (req->ki_iovec != &req->ki_inline_vec)
 		kfree(req->ki_iovec);
+
+	/* Release task ref */
+	if (req->ki_notify.notify == SIGEV_THREAD_ID ||
+	    req->ki_notify.notify == SIGEV_SIGNAL)
+		put_task_struct(req->ki_notify.target);
+
 	kmem_cache_free(kiocb_cachep, req);
 	ctx->reqs_active--;
 
@@ -916,6 +923,78 @@ void fastcall kick_iocb(struct kiocb *io
 }
 EXPORT_SYMBOL(kick_iocb);
 
+static int aio_send_signal(struct aio_notify *notify)
+{
+	struct sigqueue *sigq = notify->sigq;
+	struct siginfo *info = &sigq->info;
+	int ret;
+
+	memset(info, 0, sizeof(struct siginfo));
+
+	info->si_signo = notify->signo;
+	info->si_errno = 0;
+	info->si_code = SI_ASYNCIO;
+	info->si_pid = 0;
+	info->si_uid = 0;
+	info->si_value = notify->value;
+
+	if (notify->notify & SIGEV_THREAD_ID)
+		ret = send_sigqueue(notify->signo, sigq, notify->target);
+	else
+		ret = send_group_sigqueue(notify->signo, sigq, notify->target);
+
+	return ret;
+}
+
+static long aio_setup_sigevent(struct aio_notify *notify,
+			       struct sigevent __user *user_event)
+{
+	sigevent_t event;
+	struct task_struct *target;
+
+	if (copy_from_user(&event, user_event, sizeof (event)))
+		return -EFAULT;
+
+	if (event.sigev_notify == SIGEV_NONE)
+		return 0;
+
+	notify->notify = event.sigev_notify;
+	notify->signo = event.sigev_signo;
+	notify->value = event.sigev_value;
+
+	rcu_read_lock();
+	target = sigevent_find_task(&event);
+
+	if (unlikely(!target))
+		goto out_unlock;
+
+	/*
+	 * At this point, we know that notify is either SIGEV_SIGNAL or
+	 * SIGEV_THREAD_ID and the target task is valid. So get a reference
+	 * on the task, it will be dropped in really_put_req() when
+	 * we're done with the request.
+	 */
+	get_task_struct(target);
+	notify->target = target;
+	rcu_read_unlock();
+
+	notify->sigq = __sigqueue_alloc(current, GFP_KERNEL, 0);
+
+	/*
+	 * The task ref will be released in really_put_req()
+	 * when we dispose of the iocb later on in the submission
+	 * path.
+	 */
+	if (unlikely(!notify->sigq))
+		return -EAGAIN;
+
+	return 0;
+
+out_unlock:
+	read_unlock(&tasklist_lock);
+	return -EINVAL;
+}
+
 /* aio_complete
  *	Called when the io request on the given iocb is complete.
  *	Returns true if this is the last user of the request.  The 
@@ -963,8 +1042,11 @@ int fastcall aio_complete(struct kiocb *
 	 * cancelled requests don't get events, userland was given one
 	 * when the event got cancelled.
 	 */
-	if (kiocbIsCancelled(iocb))
+	if (kiocbIsCancelled(iocb)) {
+		if (iocb->ki_notify.sigq)
+			__sigqueue_free(iocb->ki_notify.sigq);
 		goto put_rq;
+	}
 
 	ring = kmap_atomic(info->ring_pages[0], KM_IRQ1);
 
@@ -994,6 +1076,15 @@ int fastcall aio_complete(struct kiocb *
 	kunmap_atomic(ring, KM_IRQ1);
 
 	pr_debug("added to ring %p at [%lu]\n", iocb, tail);
+
+	if (iocb->ki_notify.notify != SIGEV_NONE) {
+		ret = aio_send_signal(&iocb->ki_notify);
+
+		/* If signal generation failed, release the sigqueue */
+		if (ret)
+			__sigqueue_free(iocb->ki_notify.sigq);
+	}
+
 put_rq:
 	/* everything turned out well, dispose of the aiocb. */
 	ret = __aio_put_req(ctx, iocb);
@@ -1545,8 +1636,7 @@ int fastcall io_submit_one(struct kioctx
 	ssize_t ret;
 
 	/* enforce forwards compatibility on users */
-	if (unlikely(iocb->aio_reserved1 || iocb->aio_reserved2 ||
-		     iocb->aio_reserved3)) {
+	if (unlikely(iocb->aio_reserved1 || iocb->aio_reserved3)) {
 		pr_debug("EINVAL: io_submit: reserve field set\n");
 		return -EINVAL;
 	}
@@ -1555,6 +1645,7 @@ int fastcall io_submit_one(struct kioctx
 	if (unlikely(
 	    (iocb->aio_buf != (unsigned long)iocb->aio_buf) ||
 	    (iocb->aio_nbytes != (size_t)iocb->aio_nbytes) ||
+	    (iocb->aio_sigeventp != (unsigned long)iocb->aio_sigeventp) ||
 	    ((ssize_t)iocb->aio_nbytes < 0)
 	   )) {
 		pr_debug("EINVAL: io_submit: overflow check\n");
@@ -1588,11 +1679,21 @@ int fastcall io_submit_one(struct kioctx
 	init_waitqueue_func_entry(&req->ki_wait.wait, aio_wake_function);
 	INIT_LIST_HEAD(&req->ki_wait.wait.task_list);
  	req->ki_run_list.next = req->ki_run_list.prev = NULL;
+	/* handle setting up the sigevent for POSIX AIO signals */
+	req->ki_notify.notify = SIGEV_NONE;
+
+	if (iocb->aio_sigeventp) {
+		ret = aio_setup_sigevent(&req->ki_notify,
+					 (struct sigevent __user *)(unsigned long)
+					 iocb->aio_sigeventp);
+		if (ret)
+			goto out_put_req;
+	}
 
 	ret = aio_setup_iocb(req);
 
 	if (ret)
-		goto out_put_req;
+		goto out_sigqfree;
 
 	spin_lock_irq(&ctx->ctx_lock);
 	aio_run_iocb(req);
@@ -1605,6 +1706,11 @@ int fastcall io_submit_one(struct kioctx
 	aio_put_req(req);	/* drop extra ref to req */
 	return 0;
 
+out_sigqfree:
+	/* Undo the sigqueue alloc if someting went bad */
+	if (req->ki_notify.sigq)
+		__sigqueue_free(req->ki_notify.sigq);
+
 out_put_req:
 	aio_put_req(req);	/* drop extra ref to req */
 	aio_put_req(req);	/* drop i/o ref to req */
diff -puN fs/compat.c~aio-completion-signal-notification fs/compat.c
--- a/fs/compat.c~aio-completion-signal-notification
+++ a/fs/compat.c
@@ -665,6 +665,7 @@ compat_sys_io_submit(aio_context_t ctx_i
 		compat_uptr_t uptr;
 		struct iocb __user *user_iocb;
 		struct iocb tmp;
+		struct compat_sigevent __user *uevent;
 
 		if (unlikely(get_user(uptr, iocb + i))) {
 			ret = -EFAULT;
@@ -678,6 +679,23 @@ compat_sys_io_submit(aio_context_t ctx_i
 			break;
 		}
 
+		uevent = (struct compat_sigevent __user *)tmp.aio_sigeventp;
+
+		if (uevent) {
+			struct sigevent __user *event = NULL;
+			struct sigevent kevent;
+
+			event = compat_alloc_user_space(sizeof(*event));
+
+			if (get_compat_sigevent(&kevent, uevent) ||
+			    copy_to_user(event, &kevent, sizeof(*event))) {
+				ret = -EFAULT;
+				break;
+			}
+
+			tmp.aio_sigeventp = (__u64)event;
+		}
+
 		ret = io_submit_one(ctx, user_iocb, &tmp);
 		if (ret)
 			break;
diff -puN include/linux/aio.h~aio-completion-signal-notification include/linux/aio.h
--- a/include/linux/aio.h~aio-completion-signal-notification
+++ a/include/linux/aio.h
@@ -7,6 +7,7 @@
 #include <linux/uio.h>
 
 #include <asm/atomic.h>
+#include <asm/siginfo.h>
 
 #define AIO_MAXSEGS		4
 #define AIO_KIOGRP_NR_ATOMIC	8
@@ -54,6 +55,14 @@ struct kioctx;
 #define kiocbIsCancelled(iocb)	test_bit(KIF_CANCELLED, &(iocb)->ki_flags)
 #define kiocbIsRestarted(iocb)	test_bit(KIF_RESTARTED, &(iocb)->ki_flags)
 
+struct aio_notify {
+	struct task_struct	*target;
+	__u16			signo;
+	__u16			notify;
+	sigval_t		value;
+	struct sigqueue		*sigq;
+};
+
 /* is there a better place to document function pointer methods? */
 /**
  * ki_retry	-	iocb forward progress callback
@@ -123,6 +132,9 @@ struct kiocb {
 
 	struct list_head	ki_list;	/* the aio core uses this
 						 * for cancellation */
+
+	/* to notify a process on I/O event */
+	struct aio_notify	ki_notify;
 };
 
 #define is_sync_kiocb(iocb)	((iocb)->ki_key == KIOCB_SYNC_KEY)
diff -puN include/linux/aio_abi.h~aio-completion-signal-notification include/linux/aio_abi.h
--- a/include/linux/aio_abi.h~aio-completion-signal-notification
+++ a/include/linux/aio_abi.h
@@ -82,8 +82,9 @@ struct iocb {
 	__u64	aio_nbytes;
 	__s64	aio_offset;
 
+	__u64	aio_sigeventp;	/* pointer to struct sigevent */
+
 	/* extra parameters */
-	__u64	aio_reserved2;	/* TODO: use this for a (struct sigevent *) */
 	__u64	aio_reserved3;
 }; /* 64 bytes */
 
diff -puN kernel/signal.c~aio-completion-signal-notification kernel/signal.c
--- a/kernel/signal.c~aio-completion-signal-notification
+++ a/kernel/signal.c
@@ -1393,7 +1393,7 @@ int send_sigqueue(int sig, struct sigque
 	unsigned long flags;
 	int ret = 0;
 
-	BUG_ON(!(q->flags & SIGQUEUE_PREALLOC));
+	BUG_ON(!(q->flags & SIGQUEUE_PREALLOC) && q->info.si_code != SI_ASYNCIO);
 
 	/*
 	 * The rcu based delayed sighand destroy makes it possible to
@@ -1444,7 +1444,7 @@ send_group_sigqueue(int sig, struct sigq
 	unsigned long flags;
 	int ret = 0;
 
-	BUG_ON(!(q->flags & SIGQUEUE_PREALLOC));
+	BUG_ON(!(q->flags & SIGQUEUE_PREALLOC) && q->info.si_code != SI_ASYNCIO);
 
 	read_lock(&tasklist_lock);
 	/* Since it_lock is held, p->sighand cannot be NULL. */
_

Patches currently in -mm which might be from sebastien.dugue@bull.net are

rework-compat_sys_io_submit.patch
fix-aioh-includes.patch
fix-access_ok-checks.patch
make-good_sigevent-non-static.patch
make-__sigqueue_free-and.patch
aio-completion-signal-notification.patch
add-listio-syscall-support.patch