[PATCH] tmpfs: support aio

[PATCH] tmpfs: support aio

[Hugh Dickins]

We have a request for tmpfs to support the AIO interface: easily done,
no more than replacing the old shmem_file_read by shmem_file_aio_read,
cribbed from generic_file_aio_read.  (In 2.6.25 its write side was
already changed to use generic_file_aio_write.)

Tests out fine with LTP's ltp-aiodio.sh, given hacks (not included)
to support O_DIRECT.  tmpfs cannot honestly support O_DIRECT: its
cache-avoiding-IO nature is at odds with direct IO-avoiding-cache.

Signed-off-by: Hugh Dickins <hugh@veritas.com>
Tested-by: Lawrence Greenfield <leg@google.com>
---

 mm/shmem.c |   57 +++++++++++++++++++++++++++++++++------------------
 1 file changed, 37 insertions(+), 20 deletions(-)

--- 2.6.26-rc4/mm/shmem.c	2008-05-03 21:55:12.000000000 +0100
+++ linux/mm/shmem.c	2008-05-28 23:51:24.000000000 +0100
@@ -1690,26 +1690,42 @@ static void do_shmem_file_read(struct fi
 	file_accessed(filp);
 }
 
-static ssize_t shmem_file_read(struct file *filp, char __user *buf, size_t count, loff_t *ppos)
+static ssize_t shmem_file_aio_read(struct kiocb *iocb,
+		const struct iovec *iov, unsigned long nr_segs, loff_t pos)
 {
-	read_descriptor_t desc;
-
-	if ((ssize_t) count < 0)
-		return -EINVAL;
-	if (!access_ok(VERIFY_WRITE, buf, count))
-		return -EFAULT;
-	if (!count)
-		return 0;
-
-	desc.written = 0;
-	desc.count = count;
-	desc.arg.buf = buf;
-	desc.error = 0;
-
-	do_shmem_file_read(filp, ppos, &desc, file_read_actor);
-	if (desc.written)
-		return desc.written;
-	return desc.error;
+	struct file *filp = iocb->ki_filp;
+	ssize_t retval;
+	unsigned long seg;
+	size_t count;
+	loff_t *ppos = &iocb->ki_pos;
+
+	count = 0;
+	retval = generic_segment_checks(iov, &nr_segs, &count, VERIFY_WRITE);
+	if (retval)
+		return retval;
+
+	retval = 0;
+	if (count) {
+		for (seg = 0; seg < nr_segs; seg++) {
+			read_descriptor_t desc;
+
+			desc.written = 0;
+			desc.arg.buf = iov[seg].iov_base;
+			desc.count = iov[seg].iov_len;
+			if (desc.count == 0)
+				continue;
+			desc.error = 0;
+			do_shmem_file_read(filp, ppos, &desc, file_read_actor);
+			retval += desc.written;
+			if (desc.error) {
+				retval = retval ?: desc.error;
+				break;
+			}
+			if (desc.count > 0)
+				break;
+		}
+	}
+	return retval;
 }
 
 static int shmem_statfs(struct dentry *dentry, struct kstatfs *buf)
@@ -2369,8 +2385,9 @@ static const struct file_operations shme
 	.mmap		= shmem_mmap,
 #ifdef CONFIG_TMPFS
 	.llseek		= generic_file_llseek,
-	.read		= shmem_file_read,
+	.read		= do_sync_read,
 	.write		= do_sync_write,
+	.aio_read	= shmem_file_aio_read,
 	.aio_write	= generic_file_aio_write,
 	.fsync		= simple_sync_file,
 	.splice_read	= generic_file_splice_read,

Re: [PATCH] tmpfs: support aio

[Andrew Morton]

On Thu, 29 May 2008 00:13:35 +0100 (BST)
Hugh Dickins <hugh@veritas.com> wrote:

> +	struct file *filp = iocb->ki_filp;
> +	ssize_t retval;
> +	unsigned long seg;
> +	size_t count;
> +	loff_t *ppos = &iocb->ki_pos;
> +
> +	count = 0;
> +	retval = generic_segment_checks(iov, &nr_segs, &count, VERIFY_WRITE);
> +	if (retval)
> +		return retval;
> +
> +	retval = 0;
> +	if (count) {
> +		for (seg = 0; seg < nr_segs; seg++) {
> +			read_descriptor_t desc;
> +
> +			desc.written = 0;
> +			desc.arg.buf = iov[seg].iov_base;
> +			desc.count = iov[seg].iov_len;
> +			if (desc.count == 0)
> +				continue;
> +			desc.error = 0;
> +			do_shmem_file_read(filp, ppos, &desc, file_read_actor);
> +			retval += desc.written;
> +			if (desc.error) {
> +				retval = retval ?: desc.error;
> +				break;
> +			}
> +			if (desc.count > 0)
> +				break;
> +		}
> +	}
> +	return retval;
>  }

hm.  This version:

static ssize_t shmem_file_aio_read(struct kiocb *iocb,
		const struct iovec *iov, unsigned long nr_segs, loff_t pos)
{
	struct file *filp = iocb->ki_filp;
	ssize_t retval;
	unsigned long seg;
	size_t count;
	loff_t *ppos = &iocb->ki_pos;

	count = 0;
	retval = generic_segment_checks(iov, &nr_segs, &count, VERIFY_WRITE);
	if (retval)
		return retval;
	if (count == 0)
		return 0;

	retval = 0;
	for (seg = 0; seg < nr_segs; seg++) {
		if (iov[seg].iov_len) {
			read_descriptor_t desc = {
				.arg.buf = iov[seg].iov_base,
				.count = iov[seg].iov_len;
			};

			do_shmem_file_read(filp, ppos, &desc, file_read_actor);
			retval += desc.written;
			if (desc.error) {
				retval = retval ?: desc.error;
				break;
			}
			if (desc.count > 0)
				break;
		}
	}
	return retval;
}

is neater but generates 21 bytes more code.  Stupid gcc.

I don't believe we needed to check for count == 0?  We'd just loop around
N times doing nothing.

Re: [PATCH] tmpfs: support aio

[Harvey Harrison]

On Wed, 2008-05-28 at 16:58 -0700, Andrew Morton wrote:
> On Thu, 29 May 2008 00:13:35 +0100 (BST)
> Hugh Dickins <hugh@veritas.com> wrote:
> hm.  This version:
> 
> static ssize_t shmem_file_aio_read(struct kiocb *iocb,
> 		const struct iovec *iov, unsigned long nr_segs, loff_t pos)
> {
> 	struct file *filp = iocb->ki_filp;
> 	ssize_t retval;
> 	unsigned long seg;
> 	size_t count;
> 	loff_t *ppos = &iocb->ki_pos;
> 
> 	count = 0;
> 	retval = generic_segment_checks(iov, &nr_segs, &count, VERIFY_WRITE);
> 	if (retval)
> 		return retval;
> 	if (count == 0)
> 		return 0;
> 
> 	retval = 0;

retval has to be 0 here, no need to assign it again?

Harvey

Re: [PATCH] tmpfs: support aio

[Hugh Dickins]

On Wed, 28 May 2008, Harvey Harrison wrote:
> On Wed, 2008-05-28 at 16:58 -0700, Andrew Morton wrote:
> > 
> > 	count = 0;
> > 	retval = generic_segment_checks(iov, &nr_segs, &count, VERIFY_WRITE);
> > 	if (retval)
> > 		return retval;
> > 	if (count == 0)
> > 		return 0;
> > 
> > 	retval = 0;
> 
> retval has to be 0 here, no need to assign it again?

You're right, thanks: it was even more obviously stupid in my patch.
That came from copying generic_file_aio_read, then removing its O_DIRECT
block: though now I look closer, it's actually just as redundant in the
original.  I'll fix that up in the shmem.c and filemap.c patches to follow.

Hugh

Re: [PATCH] tmpfs: support aio

[Hugh Dickins]

On Wed, 28 May 2008, Andrew Morton wrote:
> 
> hm.  This version:
> is neater but generates 21 bytes more code.  Stupid gcc.
> 
> I don't believe we needed to check for count == 0?  We'd just loop around
> N times doing nothing.

You're right, thanks, and that's hardly a path we need to optimize.
I was just blindly copying generic_file_aio_read, but they would
both benefit from your improvement.  If you don't mind, I'll send
you a replacement patch for shmem.c, and a patch for filemap.c.

Hugh

[PATCH] generic_file_aio_read cleanups

[Hugh Dickins]

As akpm points out, there's really no need for generic_file_aio_read to
make a special case of count 0: just loop through nr_segs doing nothing.
And as Harvey Harrison points out, there's no need to reset retval to 0
where it's already 0.

Signed-off-by: Hugh Dickins <hugh@veritas.com>
---
Setting count (or ocount) to 0 before calling generic_segment_checks is
unnecessary too; but reluctantly I'll leave that removal to someone with
a wider range of gcc versions to hand - 4.1.2 and 4.2.1 don't warn about
it, but perhaps others do - I forget which are the warniest versions.

 mm/filemap.c |   44 ++++++++++++++++++++------------------------
 1 file changed, 20 insertions(+), 24 deletions(-)

--- 2.6.26-rc4/mm/filemap.c	2008-05-19 11:19:03.000000000 +0100
+++ linux/mm/filemap.c	2008-05-29 21:31:55.000000000 +0100
@@ -1199,7 +1199,6 @@ generic_file_aio_read(struct kiocb *iocb
 
 		mapping = filp->f_mapping;
 		inode = mapping->host;
-		retval = 0;
 		if (!count)
 			goto out; /* skip atime */
 		size = i_size_read(inode);
@@ -1208,33 +1207,30 @@ generic_file_aio_read(struct kiocb *iocb
 						iov, pos, nr_segs);
 			if (retval > 0)
 				*ppos = pos + retval;
-		}
-		if (likely(retval != 0)) {
-			file_accessed(filp);
-			goto out;
+			if (retval) {
+				file_accessed(filp);
+				goto out;
+			}
 		}
 	}
 
-	retval = 0;
-	if (count) {
-		for (seg = 0; seg < nr_segs; seg++) {
-			read_descriptor_t desc;
-
-			desc.written = 0;
-			desc.arg.buf = iov[seg].iov_base;
-			desc.count = iov[seg].iov_len;
-			if (desc.count == 0)
-				continue;
-			desc.error = 0;
-			do_generic_file_read(filp,ppos,&desc,file_read_actor);
-			retval += desc.written;
-			if (desc.error) {
-				retval = retval ?: desc.error;
-				break;
-			}
-			if (desc.count > 0)
-				break;
+	for (seg = 0; seg < nr_segs; seg++) {
+		read_descriptor_t desc;
+
+		desc.written = 0;
+		desc.arg.buf = iov[seg].iov_base;
+		desc.count = iov[seg].iov_len;
+		if (desc.count == 0)
+			continue;
+		desc.error = 0;
+		do_generic_file_read(filp, ppos, &desc, file_read_actor);
+		retval += desc.written;
+		if (desc.error) {
+			retval = retval ?: desc.error;
+			break;
 		}
+		if (desc.count > 0)
+			break;
 	}
 out:
 	return retval;

[PATCH v2] tmpfs: support aio

[Hugh Dickins]

We have a request for tmpfs to support the AIO interface: easily done,
no more than replacing the old shmem_file_read by shmem_file_aio_read,
cribbed from generic_file_aio_read.  (In 2.6.25 its write side was
already changed to use generic_file_aio_write.)

Incorporate cleanups from Andrew Morton and Harvey Harrison.

Tests out fine with LTP's ltp-aiodio.sh, given hacks (not included)
to support O_DIRECT.  tmpfs cannot honestly support O_DIRECT: its
cache-avoiding-IO nature is at odds with direct IO-avoiding-cache.

Signed-off-by: Hugh Dickins <hugh@veritas.com>
Tested-by: Lawrence Greenfield <leg@google.com>
---

 mm/shmem.c |   53 +++++++++++++++++++++++++++++++--------------------
 1 file changed, 33 insertions(+), 20 deletions(-)

--- 2.6.26-rc4/mm/shmem.c	2008-05-03 21:55:12.000000000 +0100
+++ linux/mm/shmem.c	2008-05-29 18:47:20.000000000 +0100
@@ -1690,26 +1690,38 @@ static void do_shmem_file_read(struct fi
 	file_accessed(filp);
 }
 
-static ssize_t shmem_file_read(struct file *filp, char __user *buf, size_t count, loff_t *ppos)
+static ssize_t shmem_file_aio_read(struct kiocb *iocb,
+		const struct iovec *iov, unsigned long nr_segs, loff_t pos)
 {
-	read_descriptor_t desc;
-
-	if ((ssize_t) count < 0)
-		return -EINVAL;
-	if (!access_ok(VERIFY_WRITE, buf, count))
-		return -EFAULT;
-	if (!count)
-		return 0;
-
-	desc.written = 0;
-	desc.count = count;
-	desc.arg.buf = buf;
-	desc.error = 0;
-
-	do_shmem_file_read(filp, ppos, &desc, file_read_actor);
-	if (desc.written)
-		return desc.written;
-	return desc.error;
+	struct file *filp = iocb->ki_filp;
+	ssize_t retval;
+	unsigned long seg;
+	size_t count;
+	loff_t *ppos = &iocb->ki_pos;
+
+	retval = generic_segment_checks(iov, &nr_segs, &count, VERIFY_WRITE);
+	if (retval)
+		return retval;
+
+	for (seg = 0; seg < nr_segs; seg++) {
+		read_descriptor_t desc;
+
+		desc.written = 0;
+		desc.arg.buf = iov[seg].iov_base;
+		desc.count = iov[seg].iov_len;
+		if (desc.count == 0)
+			continue;
+		desc.error = 0;
+		do_shmem_file_read(filp, ppos, &desc, file_read_actor);
+		retval += desc.written;
+		if (desc.error) {
+			retval = retval ?: desc.error;
+			break;
+		}
+		if (desc.count > 0)
+			break;
+	}
+	return retval;
 }
 
 static int shmem_statfs(struct dentry *dentry, struct kstatfs *buf)
@@ -2369,8 +2381,9 @@ static const struct file_operations shme
 	.mmap		= shmem_mmap,
 #ifdef CONFIG_TMPFS
 	.llseek		= generic_file_llseek,
-	.read		= shmem_file_read,
+	.read		= do_sync_read,
 	.write		= do_sync_write,
+	.aio_read	= shmem_file_aio_read,
 	.aio_write	= generic_file_aio_write,
 	.fsync		= simple_sync_file,
 	.splice_read	= generic_file_splice_read,