readv/writev rework
Subsystems (7)
PATCH 1/1 readv/writev rework
(Ripping out a few things for final review here....)
This is Janet Morgan's patch which converts the readv/writev code to submit all segments for IO before waiting on them, rather than submitting each segment separately.
This is a critical performance fix for O_DIRECT reads and writes. Prior to this change, O_DIRECT vectored IO was forced to wait for completion against each segment of the iovec rather than submitting all segments and waiting on the lot. ie: for ten segments, this code will be ten times faster.
There will also be moderate improvements for buffered IO - smaller code paths, plus writev() only takes i_sem once.
The patch ended up quite large unfortunately - turned out that the only sane way to implement this without duplicating significant amounts of code (the generic_file_write() bounds checking, all the O_DIRECT handling, etc) was to redo generic_file_read() and generic_file_write() to take an iovec/nr_segs pair rather than `buf, count'.
New exported functions generic_file_readv() and generic_file_writev() have been added:
ssize_t generic_file_readv(struct file *filp, const struct iovec *iov, unsigned long nr_segs, loff_t *ppos);ssize_t generic_file_writev(struct file *file, const struct iovec *iov, unsigned long nr_segs, loff_t * ppos);
If a driver does not use these in their file_operations then they will continue to use the old readv/writev code, which sits in a loop calling calls fops->read() or fops->write().
ext2, ext3, JFS and the blockdev driver are currently using this capability.
Some coding cleanups were made in fs/read_write.c. Mainly:
- pass "READ" or "WRITE" around to indicate the diretion of the operation, rather than the (confusing, inverted) VERIFY_READ/VERIFY_WRITE.
- Use the identifier nr_segs' everywhere to indicate the iovec length rather than count', which is often used to indicate the number of bytes in the syscall. It was confusing the heck out of me.
- Some cleanups to the raw driver.
- Some additional generality in fs/direct_io.c: the core `struct dio' used to be a "populate-and-go" thing. Janet has broken that up so you can initialise a struct dio once, then loop around feeding it more file segments, then wait on completion against everything.
- In a couple of places we needed to handle the situation where we knew, a-priori, that the user was going to get a short read or write. File size limit exceeded, read past i_size, etc. We handled that by shortening the iovec in-place with iov_shorten(). Which is not particularly pretty, but neither were the alternatives.
The performance benefits are much, much greater than I expected. Here is Janet's report:
I used a derivative of one of the LTP dio tests and the raw interface to get the following data points:
IO type: Vector io patch is:
Random writev 8.7 times faster
Sequential writev 6.6 times faster
Random readv not faster, but sys time improves 5x
Sequential readv not faster, but sys time improves 5x
Random mixed I/O 5 times faster
Sequential mixed I/O 6.6 times faster
Each raw readv or writev was for 32K (4k x 8). I used a disk attached to an aic controller.
Please let me know if you'd like a different set of data or whether I should repeat using O_DIRECT.
I've attached below the time and vmstat output for each test. P.S. Test times between say "Random writev" and "Random readv" should not be compared because I may have varied the number of times through the main testloop.
---------------------------------------------------------------------------
Random writev
before patch:
procs memory swap io system cpu r b w swpd free buff cache si so bi bo in cs us sy id
0 1 0 0 3772016 6564 61128 0 0 0 669 1176 346 0 0 100
real 0m14.254s user 0m0.000s sys 0m0.048s
after patch:
procs memory swap io system cpu r b w swpd free buff cache si so bi bo in cs us sy id
0 1 0 0 3771980 6568 61128 0 0 0 5920 1195 376 0 0 100
real 0m1.628s user 0m0.001s sys 0m0.009s
---------------------------------------------------------------------------
Sequential writev
before patch:
procs memory swap io system cpu r b w swpd free buff cache si so bi bo in cs us sy id
0 1 0 0 3771912 6620 61132 0 0 0 652 1170 330 0 0 100
real 0m14.853s user 0m0.000s sys 0m0.060s
after patch:
procs memory swap io system cpu r b w swpd free buff cache si so bi bo in cs us sy id
0 1 0 0 3771852 6636 61132 0 0 0 4352 1143 278 0 0 100
real 0m2.238s user 0m0.001s sys 0m0.011s
---------------------------------------------------------------------------
Random readv
before patch:
procs memory swap io system cpu r b w swpd free buff cache si so bi bo in cs us sy id
0 1 0 0 3771712 6732 61136 0 0 20012 0 6011 10012 0 4 96
real 0m4.760s user 0m0.002s sys 0m0.651s
after patch:
procs memory swap io system cpu r b w swpd free buff cache si so bi bo in cs us sy id
0 1 0 0 3769908 7924 61500 0 0 21440 0 1678 1346 0 3 97
real 0m4.510s user 0m0.002s sys 0m0.125s
---------------------------------------------------------------------------
Sequential readv
before patch:
procs memory swap io system cpu r b w swpd free buff cache si so bi bo in cs us sy id
0 1 0 0 3769340 7276 62832 0 0 20052 0 6020 10030 0 4 96
real 0m4.797s user 0m0.001s sys 0m0.684s
after patch:
procs memory swap io system cpu r b w swpd free buff cache si so bi bo in cs us sy id
0 1 0 0 3769320 7244 62832 0 0 21344 0 1674 1338 0 2 98
real 0m4.545s user 0m0.000s sys 0m0.119s
----------------------------------------------------------------------------
Random mixed I/O
before patch:
procs memory swap io system cpu r b w swpd free buff cache si so bi bo in cs us sy id
0 1 0 0 3770012 7884 61500 0 0 592 592 1303 596 0 2 98
real 0m16.237s user 0m0.000s sys 0m0.111s
after patch:
procs memory swap io system cpu r b w swpd free buff cache si so bi bo in cs us sy id
0 1 0 0 3770044 7840 61500 0 0 3104 3104 1201 392 0 0 100
real 0m3.193s user 0m0.001s sys 0m0.017s
----------------------------------------------------------------------------
Sequential mixed I/O
before patch:
procs memory swap io system cpu r b w swpd free buff cache si so bi bo in cs us sy id
0 1 0 0 3769232 7344 62832 0 0 640 648 1329 648 0 0 100
real 0m14.863s user 0m0.003s sys 0m0.106s
after patch:
procs memory swap io system cpu r b w swpd free buff cache si so bi bo in cs us sy id
0 1 0 0 3769192 7384 62832 0 0 4192 4197 1274 540 0 0 100
real 0m2.256s user 0m0.002s sys 0m0.023s
drivers/char/raw.c | 54 ++++++++++++++------- fs/block_dev.c | 52 ++++++++++++-------- fs/direct-io.c | 132 ++++++++++++++++++++++++++++++---------------------- fs/ext2/file.c | 2 fs/ext2/inode.c | 8 +-- fs/ext3/file.c | 22 ++++---- fs/ext3/inode.c | 10 ++- fs/jfs/file.c | 2 fs/jfs/inode.c | 8 +-- fs/read_write.c | 109 +++++++++++++++++++++++++++--------------- include/linux/fs.h | 19 ++++--- include/linux/uio.h | 15 +++++ kernel/ksyms.c | 4 + mm/filemap.c | 108 ++++++++++++++++++++++++++++++++++-------- 14 files changed, 363 insertions(+), 182 deletions(-)
drivers/char/raw.c renamed from drivers/char/raw.c~readv-writev +38 -16
@@ -201,25 +201,29 @@ out: }static ssize_t -rw_raw_dev (int rw,struct file *filp,char *buf,size_t size,loff_t *offp) +rw_raw_dev (int rw,struct file *filp,const struct iovec *iov,unsigned long nr_segs,loff_t *offp) {const int minor =minor (filp->f_dentry->d_inode->i_rdev);struct block_device *bdev = raw_devices[minor].binding;struct inode *inode = bdev->bd_inode; +size_t count =iov_length (iov, nr_segs);ssize_t ret =0 ; -if (size ==0 ) - goto out; - ret = -EINVAL ; -if (size <0 ) - goto out; - ret = -ENXIO ; -if (*offp >= inode->i_size) - goto out; - -if (size + *offp > inode->i_size) - size = inode->i_size - *offp; - ret =generic_file_direct_IO (rw, inode, buf, *offp, size); +if (count ==0 ) + goto out; + +if ((ssize_t )count <0 ) +return -EINVAL ; + +if (*offp >= inode->i_size) +return -ENXIO ; + +if (count + *offp > inode->i_size) { + count = inode->i_size - *offp; + nr_segs =iov_shorten ((struct iovec *)iov, nr_segs, count); + } + ret =generic_file_direct_IO (rw, inode, iov, *offp, nr_segs); +if (ret >0 ) *offp += ret; out:
@@ -227,15 +231,31 @@ out: }static ssize_t -raw_read (struct file *filp,char * buf,size_t size,loff_t *offp) +raw_read (struct file *filp,char *buf,size_t size,loff_t *offp) { -return rw_raw_dev (READ , filp, buf, size, offp); +struct iovec local_iov = { .iov_base = buf, .iov_len = size}; + +return rw_raw_dev (READ , filp, &local_iov,1 , offp); }static ssize_t raw_write (struct file *filp,const char *buf,size_t size,loff_t *offp) { -return rw_raw_dev (WRITE , filp, (char *)buf, size, offp); +struct iovec local_iov = { .iov_base = buf, .iov_len = size}; + +return rw_raw_dev (WRITE , filp, &local_iov,1 , offp); +} + +static ssize_t +raw_readv (struct file *filp,const struct iovec *iov,unsigned long nr_segs,loff_t *offp) +{ +return rw_raw_dev (READ , filp, iov, nr_segs, offp); +} + +static ssize_t +raw_writev (struct file *filp,const struct iovec *iov,unsigned long nr_segs,loff_t *offp) +{ +return rw_raw_dev (WRITE , filp, iov, nr_segs, offp); }static struct file_operations raw_fops = {
@@ -244,6 +264,8 @@ static struct file_operations raw_fops = .open = raw_open, .release= raw_release, .ioctl = raw_ioctl, + .readv = raw_readv, + .writev = raw_writev, .owner = THIS_MODULE, };
fs/block_dev.c renamed from fs/block_dev.c~readv-writev +31 -21
74 lines not on this page · raw message
fs/direct-io.c renamed from fs/direct-io.c~readv-writev +77 -55
197 lines not on this page · raw message
fs/ext2/file.c renamed from fs/ext2/file.c~readv-writev +2 -0
@@ -46,6 +46,8 @@ struct file_operations ext2_file_operati .open = generic_file_open, .release = ext2_release_file, .fsync = ext2_sync_file, + .readv = generic_file_readv, + .writev = generic_file_writev, .sendfile = generic_file_sendfile, };
fs/ext2/inode.c renamed from fs/ext2/inode.c~readv-writev +4 -4
@@ -619,11 +619,11 @@ ext2_get_blocks(struct inode *inode, sec }static int -ext2_direct_IO (int rw,struct inode *inode,char *buf, -loff_t offset,size_t count) +ext2_direct_IO (int rw,struct inode *inode,const struct iovec *iov, +loff_t offset,unsigned long nr_segs) { -return generic_direct_IO (rw, inode, buf, - offset, count, ext2_get_blocks); +return generic_direct_IO (rw, inode, iov, + offset, nr_segs, ext2_get_blocks); } static int
fs/jfs/file.c renamed from fs/jfs/file.c~readv-writev +2 -0
@@ -108,6 +108,8 @@ struct file_operations jfs_file_operatio .write = generic_file_write, .read = generic_file_read, .mmap = generic_file_mmap, + .readv = generic_file_readv, + .writev = generic_file_writev, .sendfile = generic_file_sendfile, .fsync = jfs_fsync, };
fs/jfs/inode.c renamed from fs/jfs/inode.c~readv-writev +4 -4
@@ -309,11 +309,11 @@ static int jfs_bmap(struct address_spacereturn generic_block_bmap (mapping, block, jfs_get_block); } -static int jfs_direct_IO (int rw,struct inode *inode,char *buf, -loff_t offset,size_t count) +static int jfs_direct_IO (int rw,struct inode *inode,const struct iovec *iov, +loff_t offset,unsigned long nr_segs) { -return generic_direct_IO (rw, inode, buf, - offset, count, jfs_get_blocks); +return generic_direct_IO (rw, inode, iov, + offset, nr_segs, jfs_get_blocks); }struct address_space_operations jfs_aops = {
fs/read_write.c renamed from fs/read_write.c~readv-writev +70 -39
201 lines not on this page · raw message
include/linux/fs.h renamed from include/linux/fs.h~readv-writev +11 -8
@@ -307,8 +307,7 @@ struct address_space_operations {int (*bmap)(struct address_space *,long );int (*invalidatepage) (struct page *,unsigned long );int (*releasepage) (struct page *,int ); -int (*direct_IO)(int ,struct inode *,char *buf, -loff_t offset,size_t count); +int (*direct_IO)(int ,struct inode *,const struct iovec *iov,loff_t offset,unsigned long nr_segs); };struct backing_dev_info ;
@@ -1245,14 +1244,18 @@ extern int generic_file_mmap(struct fileextern int file_read_actor (read_descriptor_t * desc,struct page *page,unsigned long offset,unsigned long size);extern ssize_t generic_file_read (struct file *,char *,size_t ,loff_t *);extern ssize_t generic_file_write (struct file *,const char *,size_t ,loff_t *); -extern ssize_t generic_file_write_nolock (struct file *,const char *,size_t ,loff_t *); +ssize_t generic_file_write_nolock (struct file *file,const struct iovec *iov, +unsigned long nr_segs,loff_t *ppos);extern ssize_t generic_file_sendfile (struct file *,struct file *,loff_t *,size_t );extern void do_generic_file_read (struct file *,loff_t *,read_descriptor_t *,read_actor_t ); -ssize_t generic_file_direct_IO (int rw,struct inode *inode,char *buf, -loff_t offset,size_t count); -int generic_direct_IO (int rw,struct inode *inode,char *buf, -loff_t offset,size_t count,get_blocks_t *get_blocks); - +extern ssize_t generic_file_direct_IO (int rw,struct inode *inode, +const struct iovec *iov,loff_t offset,unsigned long nr_segs); +extern int generic_direct_IO (int rw,struct inode *inode,const struct iovec + *iov,loff_t offset,unsigned long nr_segs,get_blocks_t *get_blocks); +extern ssize_t generic_file_readv (struct file *filp,const struct iovec *iov, +unsigned long nr_segs,loff_t *ppos); +ssize_t generic_file_writev (struct file *filp,const struct iovec *iov, +unsigned long nr_segs,loff_t *ppos);extern loff_t no_llseek (struct file *file,loff_t offset,int origin);extern loff_t generic_file_llseek (struct file *file,loff_t offset,int origin);extern loff_t remote_llseek (struct file *file,loff_t offset,int origin);
kernel/ksyms.c renamed from kernel/ksyms.c~readv-writev +4 -0
@@ -42,6 +42,7 @@#include <linux/highuid.h> #include <linux/brlock.h> #include <linux/fs.h> +#include <linux/uio.h> #include <linux/tty.h> #include <linux/in6.h> #include <linux/completion.h>
@@ -344,6 +345,9 @@ EXPORT_SYMBOL(register_disk);EXPORT_SYMBOL (read_dev_sector);EXPORT_SYMBOL (init_buffer);EXPORT_SYMBOL_GPL (generic_file_direct_IO); +EXPORT_SYMBOL (generic_file_readv); +EXPORT_SYMBOL (generic_file_writev); +EXPORT_SYMBOL (iov_shorten);/* tty routines */ EXPORT_SYMBOL (tty_hangup);
mm/filemap.c renamed from mm/filemap.c~readv-writev +87 -21
211 lines not on this page · raw message
fs/ext3/inode.c renamed from fs/ext3/inode.c~readv-writev +6 -4
@@ -1399,13 +1399,15 @@ static int ext3_releasepage(struct page* If the O_DIRECT write is intantiating holes inside i_size and the machine * crashes then stale disk data _may_ be exposed inside the file. */ -static int ext3_direct_IO (int rw,struct inode *inode,char *buf, -loff_t offset,size_t count) +static int ext3_direct_IO (int rw,struct inode *inode, +const struct iovec *iov,loff_t offset, +unsigned long nr_segs) {struct ext3_inode_info *ei =EXT3_I (inode);handle_t *handle =NULL ;int ret;int orphan =0 ; +size_t count =iov_length (iov, nr_segs);if (rw ==WRITE ) {loff_t final_size = offset + count;
@@ -1428,8 +1430,8 @@ static int ext3_direct_IO(int rw, struct } } - ret =generic_direct_IO (rw, inode, buf, offset, - count, ext3_direct_io_get_blocks); + ret =generic_direct_IO (rw, inode, iov, offset, + nr_segs, ext3_direct_io_get_blocks); out_stop:if (handle) {
include/linux/uio.h renamed from include/linux/uio.h~readv-writev +15 -0
@@ -34,4 +34,19 @@ struct iovec/* Beg pardon: BSD has 1024 --ANK */ #endif +/* +* Total number of bytes covered by an iovec +*/ +static inline size_t iov_length (const struct iovec *iov,unsigned long nr_segs) +{ +unsigned long seg; +size_t ret =0 ; + +for (seg =0 ; seg < nr_segs; seg++) + ret += iov[seg].iov_len; +return ret; +} + +unsigned long iov_shorten (struct iovec *iov,unsigned long nr_segs,size_t to); +#endif
fs/ext3/file.c renamed from fs/ext3/file.c~readv-writev +12 -10
@@ -76,19 +76,21 @@ ext3_file_write(struct file *file, const }struct file_operations ext3_file_operations = { - .llseek = generic_file_llseek,/* BKL held */ - .read = generic_file_read,/* BKL not held. Don't need */ - .write = ext3_file_write,/* BKL not held. Don't need */ - .ioctl = ext3_ioctl,/* BKL held */ + .llseek = generic_file_llseek, + .read = generic_file_read, + .write = ext3_file_write, + .readv = generic_file_readv, + .writev = generic_file_writev, + .ioctl = ext3_ioctl, .mmap = generic_file_mmap, - .open = ext3_open_file,/* BKL not held. Don't need */ - .release = ext3_release_file,/* BKL not held. Don't need */ - .fsync = ext3_sync_file,/* BKL held */ - .sendfile = generic_file_sendfile,/* BKL not held. Don't need */ + .open = ext3_open_file, + .release = ext3_release_file, + .fsync = ext3_sync_file, + .sendfile = generic_file_sendfile, };struct inode_operations ext3_file_inode_operations = { - .truncate = ext3_truncate,/* BKL held */ - .setattr = ext3_setattr,/* BKL held */ + .truncate = ext3_truncate, + .setattr = ext3_setattr, };
.
Hello,
Your readv/writev patch interested me and I checked it. I found we also have a chance to improve normal writev.
a_ops->prepare_write() and a_ops->commit_write will have a penalty when I/O size isn't PAGE_SIZE.
With following patch generic_file_write_nolock() will try to make each I/O size become PAGE_SIZE.
Could you apply it?
Quoted 9 lines
This is Janet Morgan's patch which converts the readv/writev code to submit all segments for IO before waiting on them, rather than submitting each segment separately.
This is a critical performance fix for
O_DIRECTreads and writes. Prior to this change,O_DIRECTvectored IO was forced to wait for completion against each segment of the iovec rather than submitting all segments and waiting on the lot. ie: for ten segments, this code will be ten times faster.
Thank you,
Hirokazu Takahashi.
mm/filemap.c renamed from mm/filemap.c.ORG +42 -9
@@ -2109,14 +2109,18 @@ generic_file_write_nolock(struct file *funsigned long index;unsigned long offset;long page_fault; +unsigned rest; +unsigned copy; +unsigned off; +struct iovec *work_iov; +char *work_buf; +unsigned work_iov_bytes; offset = (pos & (PAGE_CACHE_SIZE -1 ));/* Within page */ index = pos >>PAGE_CACHE_SHIFT ; bytes =PAGE_CACHE_SIZE - offset;if (bytes > count) bytes = count; -if (bytes + written > iov_bytes) - bytes = iov_bytes - written;/* * Bring in the user page that we will copy from _first_.
@@ -2144,7 +2148,29 @@ generic_file_write_nolock(struct file *fvmtruncate (inode, inode->i_size);break ; } - page_fault =filemap_copy_from_user (page, offset, buf, bytes); + + rest = bytes; + off =0 ; + work_iov = cur_iov; + work_buf = buf; + work_iov_bytes = iov_bytes; +while (rest) { + copy = rest; +if (written + off == work_iov_bytes) { + work_iov++; + work_iov_bytes += work_iov->iov_len; + work_buf = work_iov->iov_base; + } +if (copy + written + off > work_iov_bytes) + copy = work_iov_bytes - written - off; + + page_fault =filemap_copy_from_user (page, offset+off, work_buf, copy); + rest -= copy; + off += copy; + work_buf += copy; +if (unlikely (page_fault)) +break ; + } status = a_ops->commit_write (file, page, offset, offset+bytes);if (unlikely (page_fault)) { status = -EFAULT ;
@@ -2153,14 +2179,21 @@ generic_file_write_nolock(struct file *f status = bytes;if (status >=0 ) { - written += status; count -= status; pos += status; - buf += status; -if (written == iov_bytes && count) { - cur_iov++; - iov_bytes += cur_iov->iov_len; - buf = cur_iov->iov_base; + rest = status; +while (rest) { + copy = rest; +if (written == iov_bytes) { + cur_iov++; + iov_bytes += cur_iov->iov_len; + buf = cur_iov->iov_base; + } +if (copy + written > iov_bytes) + copy = iov_bytes - written; + rest -= copy; + written += copy; + buf += copy; } } }
Quoted 11 lines · Hirokazu Takahashi
Hello,
Your readv/writev patch interested me and I checked it. I found we also have a chance to improve normal writev.
a_ops->prepare_write()anda_ops->commit_writewill have a penalty when I/O size isn'tPAGE_SIZE.
With following patchgeneric_file_write_nolock()will try to make each I/O size becomePAGE_SIZE.
Certainly makes a lot of sense. If an application has a large number of small objects which are to be appended to a file, and they are not contiguous in user memory then this patch makes writev() a very attractive way of doing that. Tons faster.
However I'd be a little concerned over the increased work which a boring old write() has to do. Perhaps we could add a special code path for it:
struct iovstate {
unsigned rest;
unsigned copy;
unsigned off;
struct iovec *work_iov;
char *work_buf;
unsigned work_iov_bytes;
} iovstate;
if (nr_segs != 1) {
initialise iovstate;
}
...
if (nr_segs == 1)
page_fault = filemap_copy_from_user(page, offset, buf, bytes);
else
page_fault = iov_copy_from_user(&arg1, &arg2, &bytes, &iovstate);
status = a_ops->commit_write(file, page, offset, offset+bytes);then we can do all that fancy stuff in iov_copy_from_user(). That function should be inlined so we don't really pass all those arguments around. That's just a cleanliness thing, and branching around the complex code in the simple case. We should be able to get the impact on the common case down to a single taken branch per page.
Hello,
Quoted 9 lines
Quoted 8 lines
Your readv/writev patch interested me and I checked it. I found we also have a chance to improve normal writev.
a_ops->prepare_write()anda_ops->commit_writewill have a penalty when I/O size isn'tPAGE_SIZE.
With following patchgeneric_file_write_nolock()will try to make each I/O size becomePAGE_SIZE.
Quoted 4 lines
Certainly makes a lot of sense. If an application has a large number of small objects which are to be appended to a file, and they are not contiguous in user memory then this patch makes
writev()a very attractive way of doing that. Tons faster.
Yeah, I realized syslogd is using writev against logfiles which are opened with O_SYNC flag! I think heavy loaded mail-servers or web-servers may get good performance with the new writev as they are logging too much.
And I'd also like to make knfsd use it as a recevied nfs request may be splited into some IP fragments.
Quoted 3 lines
However I'd be a little concerned over the increased work which a boring old
write()has to do. Perhaps we could add a special code path for it:
It sounds nice.
I'll rewrite it soon.
Quoted 27 lines
struct iovstate { unsigned rest; unsigned copy; unsigned off; struct iovec *work_iov; char *work_buf; unsigned work_iov_bytes; } iovstate; if (nr_segs != 1) { initialise iovstate; } ... if (nr_segs == 1) page_fault = filemap_copy_from_user(page, offset, buf, bytes); else page_fault = iov_copy_from_user(&arg1, &arg2, &bytes, &iovstate); status = a_ops->commit_write(file, page, offset, offset+bytes);then we can do all that fancy stuff in
iov_copy_from_user(). That function should be inlined so we don't really pass all those arguments around. That's just a cleanliness thing, and branching around the complex code in the simple case. We should be able to get the impact on the common case down to a single taken branch per page.
Quoted 16 lines · Hirokazu Takahashi
Hello,
Quoted 13 lines
Quoted 8 lines
Your readv/writev patch interested me and I checked it. I found we also have a chance to improve normal writev.
a_ops->prepare_write()anda_ops->commit_writewill have a penalty when I/O size isn'tPAGE_SIZE.
With following patchgeneric_file_write_nolock()will try to make each I/O size becomePAGE_SIZE.Certainly makes a lot of sense. If an application has a large number of small objects which are to be appended to a file, and they are not contiguous in user memory then this patch makes
writev()a very attractive way of doing that. Tons faster.
I wrote a little app which simulates a text editor writing out its buffer. Just:
struct line {
char *data;
int length;
struct line *next;
};walk this linked list, writing the lines out. The input was `cat linux/kernel/*.c > inputfile' and the output was written 1000 times (300 megs). Benched four different ways of writing the output:
2.5.34 2.5.34-mm2 2.5.34-mm2-taka
write 54s 54s 55s fwrite 12.8s 12.8s 12.7s fwrite_unlocked 11.6s 11.6s 11.5s writev 39s 33.4s 15.8s
So Janet's patch made a 15% improvement with this test. Yours dropped it 50% again.
Quoted 4 lines
Yeah, I realized syslogd is using writev against logfiles which are opened with
O_SYNCflag! I think heavy loaded mail-servers or web-servers may get good performance with the new writev as they are logging too much.
O_SYNC writev? Ooh, oww, that hurts...
With 2.5.34, writing the 300k file once (1000x less data than above) with 1024-vector writev's, opened O_SYNC: 68 seconds.
With 2.5.34-mm2-taka the same write takes 0.23 seconds. (I had to write 100x as much data just to get a measurement).
A 300x speedup is nice, but based on these numbers syslogd should be using fwrite_unlocked() and fflush().
O_SYNC should be eradicated. It's basically always the wrong thing to do. Applications should write as much stuff as they can and then run fsync.
Quoted 4 lines
It sounds nice.
I'll rewrite it soon.
Great. The test app is at http://www.zip.com.au/~akpm/writev-speed.c
Quoted 9 lines · Andrew Morton
...
walk this linked list, writing the lines out. The input was `cat linux/kernel/*.c > inputfile' and the output was written 1000 times (300 megs). Benched four different ways of writing the output:2.5.34 2.5.34-mm2 2.5.34-mm2-taka
Quoted 8 lines
write 54s 54s 55s fwrite 12.8s 12.8s 12.7s fwrite_unlocked 11.6s 11.6s 11.5s writev 39s 33.4s 15.8sSo Janet's patch made a 15% improvement with this test. Yours dropped it 50% again.
I've retested with your latest patch.
2.5.34-mm4-taka2 write 55.543 fwrite 12.625 fwrite_unlocked 11.389
writev 9.219
So that's another 70% speedup on top of yesterday's 100%, and kernel beats glibc ;)
Hello
Brilliant!
It's great than I expected.
Quoted 6 lines
Quoted 3 lines
So Janet's patch made a 15% improvement with this test. Yours dropped it 50% again.
I've retested with your latest patch.
Quoted 8 lines
2.5.34-mm4-taka2 write 55.543 fwrite 12.625 fwrite_unlocked 11.389writev 9.219
So that's another 70% speedup on top of yesterday's 100%, and kernel beats glibc ;)
Thanks for your testing my patch and your detailed explanation.
Hirokazu Takahashi.
Hello,
I updated the writev patch which may be easy to understand. How about it?
But I have one question, Could let me know if you have any idea, why does filemap_copy_from_user() try to call kamp()+__copy_from_user() again after the first trial get fault.
Is there any meanings?
Quoted 6 lines
Quoted 5 lines
a_ops->prepare_write()anda_ops->commit_writewill have a penalty when I/O size isn'tPAGE_SIZE.
With following patchgeneric_file_write_nolock()will try to make each I/O size becomePAGE_SIZE.
Quoted 8 lines
Certainly makes a lot of sense. If an application has a large number of small objects which are to be appended to a file, and they are not contiguous in user memory then this patch makes
writev()a very attractive way of doing that. Tons faster.However I'd be a little concerned over the increased work which a boring old
write()has to do. Perhaps we could add a special code path for it:
Thank you,
Hirokazu Takahashi.
mm/filemap.c renamed from mm/filemap.c.ORG +53 -14
@@ -1940,6 +1940,48 @@ filemap_copy_from_user(struct page *pagereturn left; } +static inline int +filemap_copy_from_user_iovec (struct page *page,unsigned long offset, +const struct iovec *iov,size_t base,unsigned bytes) +{ +char *kaddr; +int left =0 ; + + kaddr =kmap_atomic (page,KM_USER0 ); +while (bytes) { +char *buf = iov->iov_base + base; +int copy =min (bytes, iov->iov_len - base); + base =0 ; +if ((left =__copy_from_user (kaddr + offset, buf, copy))) +break ; + bytes -= copy; + offset += copy; + iov++; + } +kunmap_atomic (kaddr,KM_USER0 ); +return left; +} + +static inline void +filemap_set_next_iovec (const struct iovec **iovp,size_t *basep,unsigned bytes) +{ +const struct iovec *iov = *iovp; +size_t base = *basep; + +while (bytes) { +int copy =min (bytes, iov->iov_len - base); + bytes -= copy; + base += copy; +if (iov->iov_len == base) { + iov++; + base =0 ; + } + } + *iovp = iov; + *basep = base; +} + +/* * Write to a file through the page cache. *
@@ -1968,9 +2010,8 @@ generic_file_write_nolock(struct file *funsigned bytes;time_t time_now;struct pagevec lru_pvec; -struct iovec *cur_iov; -unsigned iov_bytes;/* Cumulative count to the end of the -current iovec */ +const struct iovec *cur_iov = iov;/* current iovec */ +unsigned iov_base =0 ;/* offset in the current iovec */ unsigned long seg;char *buf;
@@ -2102,9 +2143,7 @@ generic_file_write_nolock(struct file *f goto out_status; } - cur_iov = (struct iovec *)iov; - iov_bytes = cur_iov->iov_len; - buf = cur_iov->iov_base; + buf = iov->iov_base; do { unsigned long index; unsigned long offset;
@@ -2115,8 +2154,6 @@ generic_file_write_nolock(struct file *f bytes =PAGE_CACHE_SIZE - offset;if (bytes > count) bytes = count; -if (bytes + written > iov_bytes) - bytes = iov_bytes - written;/* * Bring in the user page that we will copy from _first_.
@@ -2144,7 +2181,12 @@ generic_file_write_nolock(struct file *fvmtruncate (inode, inode->i_size);break ; } - page_fault =filemap_copy_from_user (page, offset, buf, bytes); + +if (nr_segs ==1 ) { + page_fault =filemap_copy_from_user (page, offset, buf, bytes); + }else { + page_fault =filemap_copy_from_user_iovec (page, offset, cur_iov, iov_base, bytes); + } status = a_ops->commit_write (file, page, offset, offset+bytes);if (unlikely (page_fault)) { status = -EFAULT ;
@@ -2157,11 +2199,8 @@ generic_file_write_nolock(struct file *f count -= status; pos += status; buf += status; -if (written == iov_bytes && count) { - cur_iov++; - iov_bytes += cur_iov->iov_len; - buf = cur_iov->iov_base; - } +if (nr_segs >1 ) +filemap_set_next_iovec (&cur_iov, &iov_base, status); } }if (!PageReferenced (page))
Quoted 5 lines · Hirokazu Takahashi
Hello,
I updated the writev patch which may be easy to understand. How about it?
Looks nice. And yes, you hung onto the atomic kmap across multiple
iov segments ;) That will save a tlb invalidate per segment.
Quoted 5 lines
But I have one question, Could let me know if you have any idea, why does
filemap_copy_from_user()try to call kamp()+__copy_from_user() again after the first trial get fault.Is there any meanings?
We're not allowed to schedule away inside atomic_kmap - must remain in the same task, on the same CPU etc. So the pagefault handler will return immediately if we take a pagefault while copying to/from userspace while holding an atomic kmap.
So the code first touches the userspace page (via __get_user) to fault it in. Now, there is a 99.999999% chance that the copy_*_user() will not fault - it will remain wholly atomic.
But there is the 0.0000001% chance that the VM will evict (or at least unmap) the page between the __get_user() and the completion of the copy_*_user(). In this case, copy_*_user() will fail and will return a short copy.
Now, we could just touch the page with another __get_user() and retry the atomic kmap approach. But I flipped a coin and decided to fall back to a regular sleeping kmap instead. With a sleeping kmap, in a non-atomic region the kernel will actually take the fault, fix it up and the copy_*_user() will work OK.
mm/filemap.c Fri Sep 13 16:08:51 2030
Quoted 3 lines
...
mm/filemap.c Fri Sep 13 16:08:51 2030 renamed from mm/filemap.c.ORG Wed Sep 11 19:48:00 2030 +0
-0
I shall retest...
Hello,
I appreciate for your kind answer.
I understand clearly.
Ok, this means new filemap_copy_from_user_iovec() also have to use regular kmap when page fault has happened.
I'll fix it soon.
Quoted 5 lines
Quoted 2 lines
I updated the writev patch which may be easy to understand. How about it?
Looks nice. And yes, you hung onto the atomic kmap across multiple
iov segments ;) That will save a tlb invalidate per segment.
Yes.
Quoted 10 lines
Quoted 5 lines
But I have one question, Could let me know if you have any idea, why does
filemap_copy_from_user()try to call kamp()+__copy_from_user() again after the first trial get fault.Is there any meanings?
We're not allowed to schedule away inside
atomic_kmap- must remain in the same task, on the same CPU etc. So the pagefault handler will return immediately if we take a pagefault while copying to/from userspace while holding an atomic kmap.
Hello,
I fixed the patch.
But I have one more question.
Please consider...
while a process sleep in copy_*_user() another one may call kmap_atomic and kunmap_atomic. And the process will restart and might access the wrong page as kunmap_atomic do nothing without CONFIG_DEBUG_HIGHMEM flag. I mean it wouldn't any faults as another page is still kmapped.
In my guess it would cause a woeful result.
Is it my misunderstanding ?
Quoted 14 lines
We're not allowed to schedule away inside
atomic_kmap- must remain in the same task, on the same CPU etc. So the pagefault handler will return immediately if we take a pagefault while copying to/from userspace while holding an atomic kmap.So the code first touches the userspace page (via
__get_user) to fault it in. Now, there is a 99.999999% chance that the copy_*_user() will not fault - it will remain wholly atomic.But there is the 0.0000001% chance that the VM will evict (or at least unmap) the page between the
__get_user()and the completion of the copy_*_user(). In this case, copy_*_user() will fail and will return a short copy.
Quoted 5 lines
Now, we could just touch the page with another
__get_user()and retry the atomic kmap approach. But I flipped a coin and decided to fall back to a regular sleeping kmap instead. With a sleeping kmap, in a non-atomic region the kernel will actually take the fault, fix it up and the copy_*_user() will work OK.
Thank you,
Hirokazu Takahashi.
mm/filemap.c renamed from mm/filemap.c.ORG +68 -14
@@ -1940,6 +1940,63 @@ filemap_copy_from_user(struct page *pagereturn left; } +static inline int +__filemap_copy_from_user_iovec (char *vaddr, +const struct iovec *iov,size_t base,unsigned bytes) +{ +int left =0 ; + +while (bytes) { +char *buf = iov->iov_base + base; +int copy =min (bytes, iov->iov_len - base); + base =0 ; +if ((left =__copy_from_user (vaddr, buf, copy))) +break ; + bytes -= copy; + vaddr += copy; + iov++; + } +return left; +} + +static inline int +filemap_copy_from_user_iovec (struct page *page,unsigned long offset, +const struct iovec *iov,size_t base,unsigned bytes) +{ +char *kaddr; +int left; + + kaddr =kmap_atomic (page,KM_USER0 ); + left =__filemap_copy_from_user_iovec (kaddr + offset, iov, base, bytes); +kunmap_atomic (kaddr,KM_USER0 ); +if (left !=0 ) { + kaddr =kmap (page); + left =__filemap_copy_from_user_iovec (kaddr + offset, iov, base, bytes); +kunmap (page); + } +return left; +} + +static inline void +filemap_set_next_iovec (const struct iovec **iovp,size_t *basep,unsigned bytes) +{ +const struct iovec *iov = *iovp; +size_t base = *basep; + +while (bytes) { +int copy =min (bytes, iov->iov_len - base); + bytes -= copy; + base += copy; +if (iov->iov_len == base) { + iov++; + base =0 ; + } + } + *iovp = iov; + *basep = base; +} + +/* * Write to a file through the page cache. *
@@ -1968,9 +2025,8 @@ generic_file_write_nolock(struct file *funsigned bytes;time_t time_now;struct pagevec lru_pvec; -struct iovec *cur_iov; -unsigned iov_bytes;/* Cumulative count to the end of the -current iovec */ +const struct iovec *cur_iov = iov;/* current iovec */ +unsigned iov_base =0 ;/* offset in the current iovec */ unsigned long seg;char *buf;
@@ -2102,9 +2158,7 @@ generic_file_write_nolock(struct file *f goto out_status; } - cur_iov = (struct iovec *)iov; - iov_bytes = cur_iov->iov_len; - buf = cur_iov->iov_base; + buf = iov->iov_base; do { unsigned long index; unsigned long offset;
@@ -2115,8 +2169,6 @@ generic_file_write_nolock(struct file *f bytes =PAGE_CACHE_SIZE - offset;if (bytes > count) bytes = count; -if (bytes + written > iov_bytes) - bytes = iov_bytes - written;/* * Bring in the user page that we will copy from _first_.
@@ -2144,7 +2196,12 @@ generic_file_write_nolock(struct file *fvmtruncate (inode, inode->i_size);break ; } - page_fault =filemap_copy_from_user (page, offset, buf, bytes); + +if (nr_segs ==1 ) { + page_fault =filemap_copy_from_user (page, offset, buf, bytes); + }else { + page_fault =filemap_copy_from_user_iovec (page, offset, cur_iov, iov_base, bytes); + } status = a_ops->commit_write (file, page, offset, offset+bytes);if (unlikely (page_fault)) { status = -EFAULT ;
@@ -2157,11 +2214,8 @@ generic_file_write_nolock(struct file *f count -= status; pos += status; buf += status; -if (written == iov_bytes && count) { - cur_iov++; - iov_bytes += cur_iov->iov_len; - buf = cur_iov->iov_base; - } +if (nr_segs >1 ) +filemap_set_next_iovec (&cur_iov, &iov_base, status); } }if (!PageReferenced (page))
Quoted 4 lines · Hirokazu Takahashi
Hello,
I fixed the patch.
Thanks again.
Quoted 7 lines
But I have one more question.
Please consider...
while a process sleep in copy_*_user() another one may callkmap_atomicandkunmap_atomic. And the process will restart and might access the wrong page askunmap_atomicdo nothing withoutCONFIG_DEBUG_HIGHMEMflag. I mean it wouldn't any faults as another page is still kmapped.
Yes; this is why it is illegal to sleep, or to switch CPUs by any means while holding an atomic kmap:
kmap_atomic(...); __copy_*_user(...); kunmap_atomic(...);
the kmap_atomic() will increment the preempt count (even on CONFIG_PREEMPT=n).
- The incremented preempt count pins this code path onto this CPU while the kmap is held. (This is only relevant to CONFIG_PREEMPT=y)
- The incremented preempt count tells do_page_fault() that we cannot handle a pagefault; if a fault is encountered during the copy_*_user(), do_page_fault() will arrange for the __copy_*_user() to return a short copy.
So. The code path is atomic, and is pinned to a single CPU. The atomic kmap pool uses a different batch of virtual addresses for each CPU (it's a per-CPU pool of addresses).