s390 hypfs fixes for 2.6.18-rc1-mm1

PATCH In review Badari Pulavarty 4 participants 10 messages Jul 13, 2006 – Jul 14, 2006 s390

lore

Share & tools

lore

Badari Pulavarty

author

2006-07-13 04:10 UTC #0 lore raw

PATCH 1/1 s390 hypfs fixes for 2.6.18-rc1-mm1

Hi Micheal,

I made fixes to hypfs to fit new vfs ops interfaces. I am not sure if we really need to vectorize aio interfaces, can you check and see if this is okay ?

And also, I am not sure what hypfs_aio_write() is actually doing. It doesn't seem to be doing with "buf" ?

(BTW - I have no way to verify these change. Can you give them a spin ?)

Thanks,
Badari

Vectorize aio interfaces to hypfs to fit new vfs ops interfaces.

Signed-off-by Badari Pulavarty <[email protected]>

arch/s390/hypfs/inode.c +13 -4

Index: linux-2.6.18-rc1/arch/s390/hypfs/inode.c
===================================================================
@@ -134,12 +134,20 @@ static int hypfs_open(struct inode *inod
 	return 0;
 }
 
-static ssize_t hypfs_aio_read(struct kiocb *iocb, __user char *buf,
-			      size_t count, loff_t offset)
+static ssize_t hypfs_aio_read(struct kiocb *iocb, const struct iovec *iov,
+			      unsigned long nr_segs, loff_t offset)
 {
 	char *data;
 	size_t len;
 	struct file *filp = iocb->ki_filp;
+	/* XXX: temporary */
+	const struct __user *buf = iov[0].iov_base;
+	size_t count = iov[0].iov_len;
+
+	if (nr_segs != 1) {
+		count = -EINVAL;
+		goto out;
+	}
 
 	data = filp->private_data;
 	len = strlen(data);

@@ -158,12 +166,13 @@ static ssize_t hypfs_aio_read(struct kio
 out:
 	return count;
 }
-static ssize_t hypfs_aio_write(struct kiocb *iocb, const char __user *buf,
-			       size_t count, loff_t pos)
+static ssize_t hypfs_aio_write(struct kiocb *iocb, const struct iovec *iov,
+			      unsigned long nr_segs, loff_t offset)
 {
 	int rc;
 	struct super_block *sb;
 	struct hypfs_sb_info *fs_info;
+	size_t count = iov_length(iov, nr_segs);
 
 	sb = iocb->ki_filp->f_dentry->d_inode->i_sb;
 	fs_info = sb->s_fs_info;

Badari Pulavarty

author

2006-07-13 04:17 UTC #1 lore raw

Quoted 5 lines · Badari Pulavarty

Hi Micheal,
I made fixes to hypfs to fit new vfs ops interfaces. I am not sure if we really need to vectorize aio interfaces, can you check and see if this is okay ?

Quoted 7 lines

And also, I am not sure what hypfs_aio_write() is actually doing. It doesn't seem to be doing with "buf" ?
(BTW - I have no way to verify these change. Can you give them a spin ?)
Thanks,
Badari

Here is the updated version ..

Vectorize aio interfaces to hypfs to fit new vfs ops interfaces.

Signed-off-by Badari Pulavarty <[email protected]>

arch/s390/hypfs/inode.c +13 -4

Index: linux-2.6.18-rc1/arch/s390/hypfs/inode.c
===================================================================
@@ -134,12 +134,20 @@ static int hypfs_open(struct inode *inod
 	return 0;
 }
 
-static ssize_t hypfs_aio_read(struct kiocb *iocb, __user char *buf,
-			      size_t count, loff_t offset)
+static ssize_t hypfs_aio_read(struct kiocb *iocb, const struct iovec *iov,
+			      unsigned long nr_segs, loff_t offset)
 {
 	char *data;
 	size_t len;
 	struct file *filp = iocb->ki_filp;
+	/* XXX: temporary */
+	char __user *buf = iov[0].iov_base;
+	size_t count = iov[0].iov_len;
+
+	if (nr_segs != 1) {
+		count = -EINVAL;
+		goto out;
+	}
 
 	data = filp->private_data;
 	len = strlen(data);

@@ -158,12 +166,13 @@ static ssize_t hypfs_aio_read(struct kio
 out:
 	return count;
 }
-static ssize_t hypfs_aio_write(struct kiocb *iocb, const char __user *buf,
-			       size_t count, loff_t pos)
+static ssize_t hypfs_aio_write(struct kiocb *iocb, const struct iovec *iov,
+			      unsigned long nr_segs, loff_t offset)
 {
 	int rc;
 	struct super_block *sb;
 	struct hypfs_sb_info *fs_info;
+	size_t count = iov_length(iov, nr_segs);
 
 	sb = iocb->ki_filp->f_dentry->d_inode->i_sb;
 	fs_info = sb->s_fs_info;

Cedric Le Goater

2006-07-13 09:04 UTC #2 lore raw

Quoted 10 lines · Badari Pulavarty

Quoted 9 lines · Badari Pulavarty
Hi Micheal,
I made fixes to hypfs to fit new vfs ops interfaces. I am not sure if we really
need to vectorize aio interfaces, can you check and see if this is okay ?
And also, I am not sure what hypfs_aio_write() is actually doing. It doesn't seem to be doing with "buf" ?

Quoted 6 lines

Quoted 4 lines
(BTW - I have no way to verify these change. Can you give them a spin ?)
Thanks,
Badari
Here is the updated version ..

indeed it compiles better :)

it boots fine. what kind of tests do you run ?

thanks,

Andrew Morton

2006-07-13 09:23 UTC #4 lore raw

In reply to Badari Pulavarty

Quoted 4 lines · On Wed, 12 Jul 2006 21:17:53 -0700 Badari Pulavarty <[email protected]> wrote:

arch/s390/hypfs/inode.c +2 -0

+static ssize_t hypfs_aio_read(struct kiocb *iocb, const struct iovec *iov,
+			      unsigned long nr_segs, loff_t offset)
 {
 	char *data;

arch/s390/hypfs/inode.c static int hypfs_open(struct inode *inod · line 150

Quoted 10 lines

arch/s390/hypfs/inode.c +8 -0

 	size_t len;
 	struct file *filp = iocb->ki_filp;
+	/* XXX: temporary */
+	char __user *buf = iov[0].iov_base;
+	size_t count = iov[0].iov_len;
+
+	if (nr_segs != 1) {
+		count = -EINVAL;
+		goto out;
+	}

err, "temporary" things tend to become permanent. What's the real fix?

Michael Holzheu

2006-07-13 13:44 UTC #9 lore raw

In reply to Badari Pulavarty

Hi Badari

Your new patch compiles and seems to work.

Badari Pulavarty <[email protected]> wrote on 07/13/2006 06:17:53 AM:

Quoted 6 lines

Quoted 5 lines · Badari Pulavarty
Hi Micheal,
I made fixes to hypfs to fit new vfs ops interfaces. I am not sure if we really
need to vectorize aio interfaces, can you check and see if this is okay

Quoted 2 lines

Quoted 2 lines
And also, I am not sure what hypfs_aio_write() is actually doing. It doesn't seem to be doing with "buf" ?

The hypfs write function currently is only used to trigger the update process of hypfs. We just ignore the buffer.

Thanks

Michael

Badari Pulavarty

author

2006-07-13 14:31 UTC #5 lore raw

In reply to Andrew Morton

Quoted 13 lines · Andrew Morton

Quoted 9 lines · On Wed, 12 Jul 2006 21:17:53 -0700 Badari Pulavarty <[email protected]> wrote:

arch/s390/hypfs/inode.c +5 -0

+static ssize_t hypfs_aio_read(struct kiocb *iocb, const struct iovec *iov,
+			      unsigned long nr_segs, loff_t offset)
 {
 	char *data;
 	size_t len;
 	struct file *filp = iocb->ki_filp;
+	/* XXX: temporary */
+	char __user *buf = iov[0].iov_base;
+	size_t count = iov[0].iov_len;

Quoted 9 lines

arch/s390/hypfs/inode.c static int hypfs_open(struct inode *inod · line 150
Quoted 6 lines
arch/s390/hypfs/inode.c +5 -0
+
+	if (nr_segs != 1) {
+		count = -EINVAL;
+		goto out;
+	}
err, "temporary" things tend to become permanent. What's the real fix?

I am not sure, if we really need to vectorize this method or not - meaning will this be ever called with more than one items in the vector.

Micheal, is it possible ? Can some one directly use AIO interface on hypfs ? If not, we can always look at only first element and ignore rest of them. Otherwise, we need to iterate on all the elements of the vector.

Thanks,
Badari

Michael Holzheu

2006-07-13 15:41 UTC #6 lore raw

Badari Pulavarty <[email protected]> wrote on 07/13/2006 04:31:00 PM:

Quoted 4 lines

Quoted 3 lines · Andrew Morton
On Wed, 12 Jul 2006 21:17:53 -0700
Badari Pulavarty <[email protected]> wrote:

[snip]

Quoted 7 lines

Quoted 3 lines
err, "temporary" things tend to become permanent. What's the real fix?
I am not sure, if we really need to vectorize this method or not - meaning will this be ever called
with more than one items in the vector.

Quoted 5 lines

Micheal, is it possible ? Can some one directly use AIO interface on hypfs ? If not, we can always
look at only first element and ignore rest of them. Otherwise, we need to iterate on all the elements
of the vector.

Of course it is possible that someone uses AIO on hypfs files, but normally the synchronous IO functions are used. I used the AIO implementation together with do_sync_read/write() only because it was not more effort regarding the implementation and we got the AIO interface for free.

Nevertheless we probably should implement the complete function.

Michael

Badari Pulavarty

author

2006-07-13 16:46 UTC #3 lore raw

In reply to Cedric Le Goater

Quoted 13 lines · Cedric Le Goater

Quoted 12 lines · Badari Pulavarty
Quoted 11 lines · Badari Pulavarty
Hi Micheal,
I made fixes to hypfs to fit new vfs ops interfaces. I am not sure if we really
need to vectorize aio interfaces, can you check and see if this is okay ?
And also, I am not sure what hypfs_aio_write() is actually doing. It doesn't seem to be doing with "buf" ?
(BTW - I have no way to verify these change. Can you give them a spin ?)

Quoted 8 lines

Quoted 4 lines
Quoted 2 lines
Thanks,
Badari
Here is the updated version ..
indeed it compiles better :)
it boots fine. what kind of tests do you run ?

I have no way of testing hypfs, since I don't even know what it does :( Is it possible to test it for me ?

Thanks,
Badari

Badari Pulavarty

author

2006-07-13 17:36 UTC #7 lore raw

In reply to Michael Holzheu

Quoted 21 lines · Michael Holzheu

Badari Pulavarty <[email protected]> wrote on 07/13/2006 04:31:00 PM:
Quoted 4 lines
Quoted 3 lines · Andrew Morton
On Wed, 12 Jul 2006 21:17:53 -0700
Badari Pulavarty <[email protected]> wrote:
[snip]
Quoted 7 lines
Quoted 3 lines
err, "temporary" things tend to become permanent. What's the real fix?
I am not sure, if we really need to vectorize this method or not - meaning will this be ever called
with more than one items in the vector.
Quoted 5 lines
Micheal, is it possible ? Can some one directly use AIO interface on hypfs ? If not, we can always
look at only first element and ignore rest of them. Otherwise, we need to iterate on all the elements
of the vector.

Quoted 8 lines

Of course it is possible that someone uses AIO on hypfs files, but normally the synchronous IO functions are used. I used the AIO implementation together with do_sync_read/write() only because it was not more effort regarding the implementation and we got the AIO interface for free.
Nevertheless we probably should implement the complete function.

Well, if you say so.. I have no idea what hypfs provides, if you think that some one can do vector and/or async operations on these files, its worth adding the complexity.

I was inclined towards just dealing with only single entry in the vector and fail if some one called with multiple vectors. But, its your call. Here is the complete code - completely untested :( Could you please compile and test it ?

Thanks,
Badari

Vectorize aio interfaces to hypfs to fit new vfs ops interfaces.

Signed-off-by Badari Pulavarty <[email protected]>

arch/s390/hypfs/inode.c +28 ~~-14~~

Index: linux-2.6.18-rc1/arch/s390/hypfs/inode.c
===================================================================
@@ -134,36 +134,50 @@ static int hypfs_open(struct inode *inod
 	return 0;
 }
 
-static ssize_t hypfs_aio_read(struct kiocb *iocb, __user char *buf,
-			      size_t count, loff_t offset)
+static ssize_t hypfs_aio_read(struct kiocb *iocb, const struct iovec *iov,
+			      unsigned long nr_segs, loff_t offset)
 {
 	char *data;
 	size_t len;
 	struct file *filp = iocb->ki_filp;
+	int i, ret = 0;
 
 	data = filp->private_data;
 	len = strlen(data);
-	if (offset > len) {
-		count = 0;
-		goto out;
-	}
-	if (count > len - offset)
-		count = len - offset;
-	if (copy_to_user(buf, data + offset, count)) {
-		count = -EFAULT;
+	if (offset < len)
+		len -= offset;
+	else
 		goto out;
+
+	for (i=0; i < nr_segs; i++) {
+		char __user *buf = iov[i].iov_base;
+		size_t count = iov[i].iov_len;
+
+		if (count > len)
+			count = len;
+		if (copy_to_user(buf, data + offset, count)) {
+			if (!ret)
+				ret = -EFAULT;
+			break;
+		}
+		offset += count;
+		len -= count;
+		ret += count;
+		if (len == 0)
+			break;
 	}
-	iocb->ki_pos += count;
+	iocb->ki_pos = offset;
 	file_accessed(filp);
 out:
-	return count;
+	return ret;
 }
-static ssize_t hypfs_aio_write(struct kiocb *iocb, const char __user *buf,
-			       size_t count, loff_t pos)
+static ssize_t hypfs_aio_write(struct kiocb *iocb, const struct iovec *iov,
+			      unsigned long nr_segs, loff_t offset)
 {
 	int rc;
 	struct super_block *sb;
 	struct hypfs_sb_info *fs_info;
+	size_t count = iov_length(iov, nr_segs);
 
 	sb = iocb->ki_filp->f_dentry->d_inode->i_sb;
 	fs_info = sb->s_fs_info;

Michael Holzheu

2006-07-14 10:23 UTC #8 lore raw

Hi Badari,

Badari Pulavarty <[email protected]> wrote on 07/13/2006 07:39:04 PM:

Quoted 4 lines

Well, if you say so.. I have no idea what hypfs provides, if you think that some one can do vector and/or async operations on these files, its worth adding the complexity.

Quoted 8 lines

I was inclined towards just dealing with only single entry in the vector and fail if some one called with multiple vectors. But, its your call. Here is the complete code - completely untested :( Could you please compile and test it ?
Thanks,
Badari

I tested your patch. It works fine! You can test hypfs only on zSeries machines in LPAR mode. It provides acounting information for the zSeries Hypervisor LPAR environment.

Thanks!

Michael

Quoted 11 lines

Vectorize aio interfaces to hypfs to fit new vfs ops interfaces.
Signed-off-by Badari Pulavarty <[email protected]>
Index: linux-2.6.18-rc1/arch/s390/hypfs/inode.c
===================================================================
--- linux-2.6.18-rc1.orig/arch/s390/hypfs/inode.c 2006-07-11 21:
28:07.000000000 -0700
+++ linux-2.6.18-rc1/arch/s390/hypfs/inode.c 2006-07-13 10:47:07.
000000000 -0700

arch/s390/hypfs/inode.c static int hypfs_open(struct inode *inod · line 137

Quoted 6 lines

arch/s390/hypfs/inode.c +1 -2

@@ -134,36 +134,50 @@ static int hypfs_open(struct inode *inod
    return 0;
 }
 
-static ssize_t hypfs_aio_read(struct kiocb *iocb, __user char *buf,
-               size_t count, loff_t offset)
+static ssize_t hypfs_aio_read(struct kiocb *iocb, const struct iovec

*iov,

Quoted 37 lines

arch/s390/hypfs/inode.c +21 -8

+               unsigned long nr_segs, loff_t offset)
 {
    char *data;
    size_t len;
    struct file *filp = iocb->ki_filp;
+   int i, ret = 0;
 
    data = filp->private_data;
    len = strlen(data);
-   if (offset > len) {
-      count = 0;
-      goto out;
-   }
-   if (count > len - offset)
-      count = len - offset;
-   if (copy_to_user(buf, data + offset, count)) {
-      count = -EFAULT;
+   if (offset < len)
+      len -= offset;
+   else
       goto out;
+
+   for (i=0; i < nr_segs; i++) {
+      char __user *buf = iov[i].iov_base;
+      size_t count = iov[i].iov_len;
+
+      if (count > len)
+         count = len;
+      if (copy_to_user(buf, data + offset, count)) {
+         if (!ret)
+            ret = -EFAULT;
+         break;
+      }
+      offset += count;
+      len -= count;
+      ret += count;
+      if (len == 0)

arch/s390/hypfs/inode.c static int hypfs_open(struct inode *inod

Quoted 10 lines

arch/s390/hypfs/inode.c +3 -3

+         break;
    }
-   iocb->ki_pos += count;
+   iocb->ki_pos = offset;
    file_accessed(filp);
 out:
-   return count;
+   return ret;
 }
-static ssize_t hypfs_aio_write(struct kiocb *iocb, const char __user

*buf,

Review static int hypfs_open(struct inode *inod · line 174

Quoted 2 lines

(quoted diff) +1 -1

-                size_t count, loff_t pos)
+static ssize_t hypfs_aio_write(struct kiocb *iocb, const struct iovec

*iov,

Quoted 12 lines

arch/s390/hypfs/inode.c +2 -0

+               unsigned long nr_segs, loff_t offset)
 {
    int rc;
    struct super_block *sb;
    struct hypfs_sb_info *fs_info;
+   size_t count = iov_length(iov, nr_segs);
 
    sb = iocb->ki_filp->f_dentry->d_inode->i_sb;
    fs_info = sb->s_fs_info;