[PATCH 0/7] aio: changes for 3.17

[PATCH 0/7] aio: changes for 3.17

[Benjamin LaHaise]

Hello Linus,

Please pull the following changes from the aio-next git tree at 
git://git.kvack.org/~bcrl/aio-next.git .  Most of the changes this time 
around are code cleanups.

		-ben

Benjamin LaHaise (1):
  aio: remove no longer needed preempt_disable()

Gu Zheng (4):
  aio: remove the needless registration of ring file's private_data
  aio: use the macro rather than the inline magic number
  aio: fix some comments
  aio: use iovec array rather than the single one

Oleg Nesterov (2):
  aio: change exit_aio() to load mm->ioctx_table once and avoid
    rcu_read_lock()
  aio: kill the misleading rcu read locks in ioctx_add_table() and
    kill_ioctx()


-- 
"Thought is the essence of where you are now."

[PATCH 1/7] aio: change exit_aio() to load mm->ioctx_table once and avoid rcu_read_lock()

[Oleg Nesterov]

On 04/30, Benjamin LaHaise wrote:
>
> > -		ctx->mmap_size = 0;
> > -
> > -		kill_ioctx(mm, ctx, NULL);
> > +		if (ctx) {
> > +			ctx->mmap_size = 0;
> > +			kill_ioctx(mm, ctx, NULL);
> > +		}
>
> Rather than indenting and moving the two lines changing mmap_size and the
> kill_ioctx() call, why not just do "if (!ctx) ... continue;"?  That reduces
> the number of lines changed and avoid excessive indentation.

OK. To me the code looks better/simpler with "if (ctx)", but this is subjective
of course, I won't argue.

The patch still removes the empty line between mmap_size = 0 and kill_ioctx(),
we reset mmap_size only for kill_ioctx(). But feel free to remove this change.

-------------------------------------------------------------------------------
Subject: [PATCH v3 1/2] aio: change exit_aio() to load mm->ioctx_table once and avoid rcu_read_lock()

1. We can read ->ioctx_table only once and we do not read rcu_read_lock()
   or even rcu_dereference().

   This mm has no users, nobody else can play with ->ioctx_table. Otherwise
   the code is buggy anyway, if we need rcu_read_lock() in a loop because
   ->ioctx_table can be updated then kfree(table) is obviously wrong.

2. Update the comment. "exit_mmap(mm) is coming" is the good reason to avoid
   munmap(), but another reason is that we simply can't do vm_munmap() unless
   current->mm == mm and this is not true in general, the caller is mmput().

3. We do not really need to nullify mm->ioctx_table before return, probably
   the current code does this to catch the potential problems. But in this
   case RCU_INIT_POINTER(NULL) looks better.

Signed-off-by: Oleg Nesterov <oleg@redhat.com>
Signed-off-by: Benjamin LaHaise <bcrl@kvack.org>
---
 fs/aio.c | 42 ++++++++++++++++--------------------------
 1 file changed, 16 insertions(+), 26 deletions(-)

diff --git a/fs/aio.c b/fs/aio.c
index 955947e..b669646 100644
--- a/fs/aio.c
+++ b/fs/aio.c
@@ -791,40 +791,30 @@ EXPORT_SYMBOL(wait_on_sync_kiocb);
  */
 void exit_aio(struct mm_struct *mm)
 {
-	struct kioctx_table *table;
-	struct kioctx *ctx;
-	unsigned i = 0;
-
-	while (1) {
-		rcu_read_lock();
-		table = rcu_dereference(mm->ioctx_table);
-
-		do {
-			if (!table || i >= table->nr) {
-				rcu_read_unlock();
-				rcu_assign_pointer(mm->ioctx_table, NULL);
-				if (table)
-					kfree(table);
-				return;
-			}
+	struct kioctx_table *table = rcu_dereference_raw(mm->ioctx_table);
+	int i;
 
-			ctx = table->table[i++];
-		} while (!ctx);
+	if (!table)
+		return;
 
-		rcu_read_unlock();
+	for (i = 0; i < table->nr; ++i) {
+		struct kioctx *ctx = table->table[i];
 
+		if (!ctx)
+			continue;
 		/*
-		 * We don't need to bother with munmap() here -
-		 * exit_mmap(mm) is coming and it'll unmap everything.
-		 * Since aio_free_ring() uses non-zero ->mmap_size
-		 * as indicator that it needs to unmap the area,
-		 * just set it to 0; aio_free_ring() is the only
-		 * place that uses ->mmap_size, so it's safe.
+		 * We don't need to bother with munmap() here - exit_mmap(mm)
+		 * is coming and it'll unmap everything. And we simply can't,
+		 * this is not necessarily our ->mm.
+		 * Since kill_ioctx() uses non-zero ->mmap_size as indicator
+		 * that it needs to unmap the area, just set it to 0.
 		 */
 		ctx->mmap_size = 0;
-
 		kill_ioctx(mm, ctx, NULL);
 	}
+
+	RCU_INIT_POINTER(mm->ioctx_table, NULL);
+	kfree(table);
 }
 
 static void put_reqs_available(struct kioctx *ctx, unsigned nr)
-- 
1.8.2.1


-- 
"Thought is the essence of where you are now."

[PATCH 2/7] aio: kill the misleading rcu read locks in ioctx_add_table() and kill_ioctx()

[Oleg Nesterov]

ioctx_add_table() is the writer, it does not need rcu_read_lock() to
protect ->ioctx_table. It relies on mm->ioctx_lock and rcu locks just
add the confusion.

And it doesn't need rcu_dereference() by the same reason, it must see
any updates previously done under the same ->ioctx_lock. We could use
rcu_dereference_protected() but the patch uses rcu_dereference_raw(),
the function is simple enough.

The same for kill_ioctx(), although it does not update the pointer.

Signed-off-by: Oleg Nesterov <oleg@redhat.com>
Signed-off-by: Benjamin LaHaise <bcrl@kvack.org>
---
 fs/aio.c | 14 +++-----------
 1 file changed, 3 insertions(+), 11 deletions(-)

diff --git a/fs/aio.c b/fs/aio.c
index b669646..c1d8c48 100644
--- a/fs/aio.c
+++ b/fs/aio.c
@@ -554,8 +554,7 @@ static int ioctx_add_table(struct kioctx *ctx, struct mm_struct *mm)
 	struct aio_ring *ring;
 
 	spin_lock(&mm->ioctx_lock);
-	rcu_read_lock();
-	table = rcu_dereference(mm->ioctx_table);
+	table = rcu_dereference_raw(mm->ioctx_table);
 
 	while (1) {
 		if (table)
@@ -563,7 +562,6 @@ static int ioctx_add_table(struct kioctx *ctx, struct mm_struct *mm)
 				if (!table->table[i]) {
 					ctx->id = i;
 					table->table[i] = ctx;
-					rcu_read_unlock();
 					spin_unlock(&mm->ioctx_lock);
 
 					/* While kioctx setup is in progress,
@@ -577,8 +575,6 @@ static int ioctx_add_table(struct kioctx *ctx, struct mm_struct *mm)
 				}
 
 		new_nr = (table ? table->nr : 1) * 4;
-
-		rcu_read_unlock();
 		spin_unlock(&mm->ioctx_lock);
 
 		table = kzalloc(sizeof(*table) + sizeof(struct kioctx *) *
@@ -589,8 +585,7 @@ static int ioctx_add_table(struct kioctx *ctx, struct mm_struct *mm)
 		table->nr = new_nr;
 
 		spin_lock(&mm->ioctx_lock);
-		rcu_read_lock();
-		old = rcu_dereference(mm->ioctx_table);
+		old = rcu_dereference_raw(mm->ioctx_table);
 
 		if (!old) {
 			rcu_assign_pointer(mm->ioctx_table, table);
@@ -737,12 +732,9 @@ static int kill_ioctx(struct mm_struct *mm, struct kioctx *ctx,
 
 
 	spin_lock(&mm->ioctx_lock);
-	rcu_read_lock();
-	table = rcu_dereference(mm->ioctx_table);
-
+	table = rcu_dereference_raw(mm->ioctx_table);
 	WARN_ON(ctx != table->table[ctx->id]);
 	table->table[ctx->id] = NULL;
-	rcu_read_unlock();
 	spin_unlock(&mm->ioctx_lock);
 
 	/* percpu_ref_kill() will do the necessary call_rcu() */
-- 
1.8.2.1


-- 
"Thought is the essence of where you are now."

[PATCH 3/7] aio: remove no longer needed preempt_disable()

[Benjamin LaHaise]

Based on feedback from Jens Axboe on 263782c1c95bbddbb022dc092fd89a36bb8d5577,
clean up get/put_reqs_available() to remove the no longer needed preempt_disable()
and preempt_enable() pair.

Signed-off-by: Benjamin LaHaise <bcrl@kvack.org>
Cc: Jens Axboe <axboe@kernel.dk>
---
 fs/aio.c | 10 ++--------
 1 file changed, 2 insertions(+), 8 deletions(-)

diff --git a/fs/aio.c b/fs/aio.c
index 8216aa0..9ce9e8e 100644
--- a/fs/aio.c
+++ b/fs/aio.c
@@ -814,10 +814,8 @@ static void put_reqs_available(struct kioctx *ctx, unsigned nr)
 	struct kioctx_cpu *kcpu;
 	unsigned long flags;
 
-	preempt_disable();
-	kcpu = this_cpu_ptr(ctx->cpu);
-
 	local_irq_save(flags);
+	kcpu = this_cpu_ptr(ctx->cpu);
 	kcpu->reqs_available += nr;
 
 	while (kcpu->reqs_available >= ctx->req_batch * 2) {
@@ -826,7 +824,6 @@ static void put_reqs_available(struct kioctx *ctx, unsigned nr)
 	}
 
 	local_irq_restore(flags);
-	preempt_enable();
 }
 
 static bool get_reqs_available(struct kioctx *ctx)
@@ -835,10 +832,8 @@ static bool get_reqs_available(struct kioctx *ctx)
 	bool ret = false;
 	unsigned long flags;
 
-	preempt_disable();
-	kcpu = this_cpu_ptr(ctx->cpu);
-
 	local_irq_save(flags);
+	kcpu = this_cpu_ptr(ctx->cpu);
 	if (!kcpu->reqs_available) {
 		int old, avail = atomic_read(&ctx->reqs_available);
 
@@ -858,7 +853,6 @@ static bool get_reqs_available(struct kioctx *ctx)
 	kcpu->reqs_available--;
 out:
 	local_irq_restore(flags);
-	preempt_enable();
 	return ret;
 }
 
-- 
1.8.2.1


-- 
"Thought is the essence of where you are now."

[PATCH 4/7] aio: remove the needless registration of ring file's private_data

[Gu Zheng]

Remove the registration of ring file's private_data, we do not use
it.

Reviewed-by: Jeff Moyer <jmoyer@redhat.com>
Signed-off-by: Gu Zheng <guz.fnst@cn.fujitsu.com>
Signed-off-by: Benjamin LaHaise <bcrl@kvack.org>
---
 fs/aio.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/fs/aio.c b/fs/aio.c
index 9ce9e8e..55e03a8 100644
--- a/fs/aio.c
+++ b/fs/aio.c
@@ -192,7 +192,6 @@ static struct file *aio_private_file(struct kioctx *ctx, loff_t nr_pages)
 	}
 
 	file->f_flags = O_RDWR;
-	file->private_data = ctx;
 	return file;
 }
 
-- 
1.8.2.1


-- 
"Thought is the essence of where you are now."

[PATCH 5/7] aio: use the macro rather than the inline magic number

[Gu Zheng]

Replace the inline magic number with the ready-made macro(AIO_RING_MAGIC),
just clean up.

Reviewed-by: Jeff Moyer <jmoyer@redhat.com>
Signed-off-by: Gu Zheng <guz.fnst@cn.fujitsu.com>
Signed-off-by: Benjamin LaHaise <bcrl@kvack.org>
---
 fs/aio.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/fs/aio.c b/fs/aio.c
index 55e03a8..6fc6b98 100644
--- a/fs/aio.c
+++ b/fs/aio.c
@@ -201,7 +201,7 @@ static struct dentry *aio_mount(struct file_system_type *fs_type,
 	static const struct dentry_operations ops = {
 		.d_dname	= simple_dname,
 	};
-	return mount_pseudo(fs_type, "aio:", NULL, &ops, 0xa10a10a1);
+	return mount_pseudo(fs_type, "aio:", NULL, &ops, AIO_RING_MAGIC);
 }
 
 /* aio_setup
-- 
1.8.2.1


-- 
"Thought is the essence of where you are now."

[PATCH 6/7] aio: fix some comments

[Gu Zheng]

The function comments of aio_run_iocb and aio_read_events are out of date, so
fix them here.

Reviewed-by: Jeff Moyer <jmoyer@redhat.com>
Signed-off-by: Gu Zheng <guz.fnst@cn.fujitsu.com>
Signed-off-by: Benjamin LaHaise <bcrl@kvack.org>
---
 fs/aio.c | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

diff --git a/fs/aio.c b/fs/aio.c
index 6fc6b98..d6d9520 100644
--- a/fs/aio.c
+++ b/fs/aio.c
@@ -1020,7 +1020,7 @@ void aio_complete(struct kiocb *iocb, long res, long res2)
 }
 EXPORT_SYMBOL(aio_complete);
 
-/* aio_read_events
+/* aio_read_events_ring
  *	Pull an event off of the ioctx's event ring.  Returns the number of
  *	events fetched
  */
@@ -1272,9 +1272,8 @@ static ssize_t aio_setup_single_vector(struct kiocb *kiocb,
 }
 
 /*
- * aio_setup_iocb:
- *	Performs the initial checks and aio retry method
- *	setup for the kiocb at the time of io submission.
+ * aio_run_iocb:
+ *	Performs the initial checks and io submission.
  */
 static ssize_t aio_run_iocb(struct kiocb *req, unsigned opcode,
 			    char __user *buf, bool compat)
-- 
1.8.2.1


-- 
"Thought is the essence of where you are now."

[PATCH 7/7] aio: use iovec array rather than the single one

[Gu Zheng]

Previously, we only offer a single iovec to handle all the read/write cases, so
the PREADV/PWRITEV request always need to alloc more iovec buffer when copying
user vectors.
If we use a tmp iovec array rather than the single one, some small PREADV/PWRITEV
workloads(vector size small than the tmp buffer) will not need to alloc more
iovec buffer when copying user vectors.

Reviewed-by: Jeff Moyer <jmoyer@redhat.com>
Signed-off-by: Gu Zheng <guz.fnst@cn.fujitsu.com>
Signed-off-by: Benjamin LaHaise <bcrl@kvack.org>
---
 fs/aio.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/fs/aio.c b/fs/aio.c
index d6d9520..0fd9181 100644
--- a/fs/aio.c
+++ b/fs/aio.c
@@ -1243,12 +1243,12 @@ static ssize_t aio_setup_vectored_rw(struct kiocb *kiocb,
 	if (compat)
 		ret = compat_rw_copy_check_uvector(rw,
 				(struct compat_iovec __user *)buf,
-				*nr_segs, 1, *iovec, iovec);
+				*nr_segs, UIO_FASTIOV, *iovec, iovec);
 	else
 #endif
 		ret = rw_copy_check_uvector(rw,
 				(struct iovec __user *)buf,
-				*nr_segs, 1, *iovec, iovec);
+				*nr_segs, UIO_FASTIOV, *iovec, iovec);
 	if (ret < 0)
 		return ret;
 
@@ -1285,7 +1285,7 @@ static ssize_t aio_run_iocb(struct kiocb *req, unsigned opcode,
 	fmode_t mode;
 	aio_rw_op *rw_op;
 	rw_iter_op *iter_op;
-	struct iovec inline_vec, *iovec = &inline_vec;
+	struct iovec inline_vecs[UIO_FASTIOV], *iovec = inline_vecs;
 	struct iov_iter iter;
 
 	switch (opcode) {
@@ -1320,7 +1320,7 @@ rw_common:
 		if (!ret)
 			ret = rw_verify_area(rw, file, &req->ki_pos, req->ki_nbytes);
 		if (ret < 0) {
-			if (iovec != &inline_vec)
+			if (iovec != inline_vecs)
 				kfree(iovec);
 			return ret;
 		}
@@ -1367,7 +1367,7 @@ rw_common:
 		return -EINVAL;
 	}
 
-	if (iovec != &inline_vec)
+	if (iovec != inline_vecs)
 		kfree(iovec);
 
 	if (ret != -EIOCBQUEUED) {
-- 
1.8.2.1


-- 
"Thought is the essence of where you are now."

Re: [PATCH 7/7] aio: use iovec array rather than the single one

[Linus Torvalds]

Ben,

*please* stop doing this kind of crap.

You're creating fake emails with invalid senders. That's simply not acceptable.

The way to specify the author of a commit is *not* to fake the email
sender information, it's to have

   From: Real A U Thor <real.thor@fake.com>

at the top of the message body. NOT by sending fake email from that name.

Seriously, faking email authorship is too distasteful for words. What
the F*CK were you thinking? Fix your scripts immediately. I refuse to
have anything to do with people who send fake emails.

Not acceptable. Not acceptable AT ALL.

                  Linus

On Fri, Aug 15, 2014 at 11:25 AM, Gu Zheng <guz.fnst@cn.fujitsu.com> wrote:
> Previously, we only offer a single iovec to handle all the read/write cases, so
> the PREADV/PWRITEV request always need to alloc more iovec buffer when copying
> user vectors.
> If we use a tmp iovec array rather than the single one, some small PREADV/PWRITEV
> workloads(vector size small than the tmp buffer) will not need to alloc more
> iovec buffer when copying user vectors.
>
> Reviewed-by: Jeff Moyer <jmoyer@redhat.com>
> Signed-off-by: Gu Zheng <guz.fnst@cn.fujitsu.com>
> Signed-off-by: Benjamin LaHaise <bcrl@kvack.org>
> ---
>  fs/aio.c | 10 +++++-----
>  1 file changed, 5 insertions(+), 5 deletions(-)
>
> diff --git a/fs/aio.c b/fs/aio.c
> index d6d9520..0fd9181 100644
> --- a/fs/aio.c
> +++ b/fs/aio.c
> @@ -1243,12 +1243,12 @@ static ssize_t aio_setup_vectored_rw(struct kiocb *kiocb,
>         if (compat)
>                 ret = compat_rw_copy_check_uvector(rw,
>                                 (struct compat_iovec __user *)buf,
> -                               *nr_segs, 1, *iovec, iovec);
> +                               *nr_segs, UIO_FASTIOV, *iovec, iovec);
>         else
>  #endif
>                 ret = rw_copy_check_uvector(rw,
>                                 (struct iovec __user *)buf,
> -                               *nr_segs, 1, *iovec, iovec);
> +                               *nr_segs, UIO_FASTIOV, *iovec, iovec);
>         if (ret < 0)
>                 return ret;
>
> @@ -1285,7 +1285,7 @@ static ssize_t aio_run_iocb(struct kiocb *req, unsigned opcode,
>         fmode_t mode;
>         aio_rw_op *rw_op;
>         rw_iter_op *iter_op;
> -       struct iovec inline_vec, *iovec = &inline_vec;
> +       struct iovec inline_vecs[UIO_FASTIOV], *iovec = inline_vecs;
>         struct iov_iter iter;
>
>         switch (opcode) {
> @@ -1320,7 +1320,7 @@ rw_common:
>                 if (!ret)
>                         ret = rw_verify_area(rw, file, &req->ki_pos, req->ki_nbytes);
>                 if (ret < 0) {
> -                       if (iovec != &inline_vec)
> +                       if (iovec != inline_vecs)
>                                 kfree(iovec);
>                         return ret;
>                 }
> @@ -1367,7 +1367,7 @@ rw_common:
>                 return -EINVAL;
>         }
>
> -       if (iovec != &inline_vec)
> +       if (iovec != inline_vecs)
>                 kfree(iovec);
>
>         if (ret != -EIOCBQUEUED) {
> --
> 1.8.2.1
>
>
> --
> "Thought is the essence of where you are now."

Re: [PATCH 7/7] aio: use iovec array rather than the single one

[Benjamin LaHaise]

On Fri, Aug 15, 2014 at 06:35:25PM -0600, Linus Torvalds wrote:
> Ben,
> 
> *please* stop doing this kind of crap.
> 
> You're creating fake emails with invalid senders. That's simply not acceptable.
...

I'll admit this was my mistake and I should have caught it while reviewing 
the emails.  At least in the past my mail client has generated the correct 
From: headers, but I made a mistake somewhere in the process today.  Evidently 
passing in the --from= parameter to git-format-patch has been missing from 
my scripts forever.

That said: git should *not* default to generating fake From: headers in 
generated emails!  Having the git default being completely unreasonable in 
this case makes absolutely no sense to me, especially when git already 
knows what my bloody email address is!  It's wrong, it's clear that it's 
wrong, so why is the default that way?

Damned rope shooting my foot off.  Please excuse me while I go find a medic 
to stop the bleeding...

		-ben
-- 
"Thought is the essence of where you are now."