sendfile removal
Subsystems (29)
PATCH 1/1 sendfile removal
Hi,
This patch removes the ->sendfile() hook from the file_operations structure, and replaces the sys_sendfile() mechanism to be based on ->splice_read() instead. There should be no functional changes.
Work to be done:
- The ext2 xip support needs a splice_read() implementation, currently I just if 0'ed out the send xip_file_sendfile(). CC'ed Carsten, who seems to be the author of this code.
- shmem needs a splice_read() implementation. Optimistically CC'ed Hugh.
- nfds: The ->rq_sendfile_ok optimization is gone for now. I can't determine the value of it, but I'm assuming it's there for a reason. Any chance this can be converted to splice, or use something else than ->sendfile()? CC'ed Neil.
- relay: needs a splice_read() implementation. I think Tom already has one, CC'ed him.
Apart from that, it was mostly straight forward. Almost everybody uses generic_file_sendfile(), which makes the conversion easy. I changed loop to use do_generic_file_read() instead of sendfile, it works for me...
Patch is against current git.
drivers/block/loop.c | 22 ++++++++++----- fs/adfs/file.c | 2 - fs/affs/file.c | 2 - fs/afs/file.c | 2 - fs/bad_inode.c | 7 ---- fs/bfs/file.c | 2 - fs/block_dev.c | 1 fs/cifs/cifsfs.c | 8 ++--- fs/coda/file.c | 11 ++++--- fs/ecryptfs/file.c | 15 +++++----- fs/ext2/file.c | 5 ++- fs/ext3/file.c | 1 fs/ext4/file.c | 1 fs/fat/file.c | 2 - fs/fuse/file.c | 4 +- fs/gfs2/ops_file.c | 1 fs/hfs/inode.c | 2 - fs/hfsplus/inode.c | 2 - fs/hostfs/hostfs_kern.c | 2 - fs/hpfs/file.c | 2 - fs/jffs2/file.c | 2 - fs/jfs/file.c | 1 fs/minix/file.c | 2 - fs/nfs/file.c | 15 ++++++---- fs/nfsd/vfs.c | 19 ++++++------ fs/ntfs/file.c | 2 - fs/ocfs2/file.c | 1 fs/qnx4/file.c | 2 - fs/ramfs/file-mmu.c | 2 - fs/ramfs/file-nommu.c | 2 - fs/read_write.c | 13 ++++++-- fs/reiserfs/file.c | 1 fs/smbfs/file.c | 9 +++--- fs/sysv/file.c | 2 - fs/udf/file.c | 2 - fs/ufs/file.c | 2 - fs/xfs/linux-2.6/xfs_file.c | 26 ----------------- fs/xfs/linux-2.6/xfs_linux.h | 1 fs/xfs/linux-2.6/xfs_lrw.c | 44 ------------------------------ fs/xfs/linux-2.6/xfs_lrw.h | 3 -- fs/xfs/linux-2.6/xfs_vnode.h | 6 ---- fs/xfs/xfs_vnodeops.c | 3 -- include/linux/fs.h | 5 --- include/linux/sunrpc/svc.h | 2 - kernel/relay.c | 16 ---------- mm/filemap.c | 20 ------------- mm/filemap_xip.c | 2 + mm/shmem.c | 32 ++++++++------------- net/sunrpc/auth_gss/svcauth_gss.c | 2 - net/sunrpc/svc.c | 2 - 50 files changed, 105 insertions(+), 230 deletions(-)
drivers/block/loop.c +15 -7
@@ -435,16 +435,24 @@ do_lo_receive(struct loop_device *lo, {struct lo_read_data cookie;struct file *file; -int retval; +read_descriptor_t desc; + + desc.written =0 ; + desc.count = bvec->bv_len; + desc.arg.data = &cookie; + desc.error =0 ; cookie.lo = lo; cookie.page = bvec->bv_page; cookie.offset = bvec->bv_offset; cookie.bsize = bsize; file = lo->lo_backing_file; - retval = file->f_op->sendfile (file, &pos, bvec->bv_len, - lo_read_actor, &cookie); -return (retval <0 )? retval:0 ; + +do_generic_file_read (file, &pos, &desc, lo_read_actor); +if (desc.written) +return 0 ; + +return desc.error; } static int
@@ -679,8 +687,8 @@ static int loop_change_fd(struct loop_device *lo, struct file *lo_file,if (!S_ISREG (inode->i_mode) && !S_ISBLK (inode->i_mode)) goto out_putf; -/* new backing store needs to support loop (eg sendfile) */ -if (!inode->i_fop->sendfile) +/* new backing store needs to support loop (eg readpage) */ +if (!file->f_mapping->a_ops->readpage) goto out_putf;/* size of the new backing store needs to be the same */
@@ -760,7 +768,7 @@ static int loop_set_fd(struct loop_device *lo, struct file *lo_file,* If we can't read - sorry. If we only can't write - well, * it's going to be read-only. */ -if (!file->f_op->sendfile) +if (!aops->readpage) goto out_putf;if (aops->prepare_write && aops->commit_write) lo_flags |=LO_FLAGS_USE_AOPS ;
fs/adfs/file.c +1 -1
@@ -33,7 +33,7 @@ const struct file_operations adfs_file_operations = { .fsync = file_fsync, .write = do_sync_write, .aio_write = generic_file_aio_write, - .sendfile = generic_file_sendfile, + .splice_read = generic_file_splice_read, };const struct inode_operations adfs_file_inode_operations = {
fs/affs/file.c +1 -1
@@ -35,7 +35,7 @@ const struct file_operations affs_file_operations = { .open = affs_file_open, .release = affs_file_release, .fsync = file_fsync, - .sendfile = generic_file_sendfile, + .splice_read = generic_file_splice_read, };const struct inode_operations affs_file_inode_operations = {
fs/afs/file.c +1 -1
@@ -32,7 +32,7 @@ const struct file_operations afs_file_operations = { .aio_read = generic_file_aio_read, .aio_write = afs_file_write, .mmap = generic_file_readonly_mmap, - .sendfile = generic_file_sendfile, + .splice_read = generic_file_splice_read, .fsync = afs_fsync, };
fs/bad_inode.c +0 -7
@@ -114,12 +114,6 @@ static int bad_file_lock(struct file *file, int cmd, struct file_lock *fl)return -EIO ; } -static ssize_t bad_file_sendfile (struct file *in_file,loff_t *ppos, -size_t count,read_actor_t actor,void *target) -{ -return -EIO ; -} -static ssize_t bad_file_sendpage (struct file *file,struct page *page,int off,size_t len,loff_t *pos,int more) {
@@ -182,7 +176,6 @@ static const struct file_operations bad_file_ops = .aio_fsync = bad_file_aio_fsync, .fasync = bad_file_fasync, .lock = bad_file_lock, - .sendfile = bad_file_sendfile, .sendpage = bad_file_sendpage, .get_unmapped_area = bad_file_get_unmapped_area, .check_flags = bad_file_check_flags,
fs/bfs/file.c +1 -1
@@ -24,7 +24,7 @@ const struct file_operations bfs_file_operations = { .write = do_sync_write, .aio_write = generic_file_aio_write, .mmap = generic_file_mmap, - .sendfile = generic_file_sendfile, + .splice_read = generic_file_splice_read, };static int bfs_move_block (unsigned long from,unsigned long to,struct super_block *sb)
fs/block_dev.c +0 -1
@@ -1346,7 +1346,6 @@ const struct file_operations def_blk_fops = {#ifdef CONFIG_COMPAT .compat_ioctl = compat_blkdev_ioctl, #endif - .sendfile = generic_file_sendfile, .splice_read = generic_file_splice_read, .splice_write = generic_file_splice_write, };
fs/cifs/cifsfs.c +4 -4
@@ -616,7 +616,7 @@ const struct file_operations cifs_file_ops = { .fsync = cifs_fsync, .flush = cifs_flush, .mmap = cifs_file_mmap, - .sendfile = generic_file_sendfile, + .splice_read = generic_file_splice_read, .llseek = cifs_llseek, #ifdef CONFIG_CIFS_POSIX .ioctl = cifs_ioctl,
@@ -637,7 +637,7 @@ const struct file_operations cifs_file_direct_ops = { .lock = cifs_lock, .fsync = cifs_fsync, .flush = cifs_flush, - .sendfile = generic_file_sendfile, /* BB removeme BB */ + .splice_read = generic_file_splice_read,#ifdef CONFIG_CIFS_POSIX .ioctl = cifs_ioctl,#endif /* CONFIG_CIFS_POSIX */
@@ -656,7 +656,7 @@ const struct file_operations cifs_file_nobrl_ops = { .fsync = cifs_fsync, .flush = cifs_flush, .mmap = cifs_file_mmap, - .sendfile = generic_file_sendfile, + .splice_read = generic_file_splice_read, .llseek = cifs_llseek, #ifdef CONFIG_CIFS_POSIX .ioctl = cifs_ioctl,
@@ -676,7 +676,7 @@ const struct file_operations cifs_file_direct_nobrl_ops = { .release = cifs_close, .fsync = cifs_fsync, .flush = cifs_flush, - .sendfile = generic_file_sendfile, /* BB removeme BB */ + .splice_read = generic_file_splice_read,#ifdef CONFIG_CIFS_POSIX .ioctl = cifs_ioctl,#endif /* CONFIG_CIFS_POSIX */
fs/coda/file.c +6 -5
@@ -47,8 +47,9 @@ coda_file_read(struct file *coda_file, char __user *buf, size_t count, loff_t *p }static ssize_t -coda_file_sendfile (struct file *coda_file,loff_t *ppos,size_t count, -read_actor_t actor,void *target) +coda_file_splice_read (struct file *coda_file,loff_t *ppos, +struct pipe_inode_info *pipe,size_t count, +unsigned int flags) {struct coda_file_info *cfi;struct file *host_file;
@@ -57,10 +58,10 @@ coda_file_sendfile(struct file *coda_file, loff_t *ppos, size_t count,BUG_ON (!cfi || cfi->cfi_magic !=CODA_MAGIC ); host_file = cfi->cfi_container; -if (!host_file->f_op || !host_file->f_op->sendfile) +if (!host_file->f_op || !host_file->f_op->splice_read)return -EINVAL ; -return host_file->f_op->sendfile (host_file, ppos, count, actor, target); +return host_file->f_op->splice_read (host_file, ppos, pipe, count,flags); } static ssize_t
@@ -295,6 +296,6 @@ const struct file_operations coda_file_operations = { .flush = coda_flush, .release = coda_release, .fsync = coda_fsync, - .sendfile = coda_file_sendfile, + .splice_read = coda_file_splice_read, };
fs/ecryptfs/file.c +8 -7
@@ -338,16 +338,17 @@ static int ecryptfs_fasync(int fd, struct file *file, int flag)return rc; } -static ssize_t ecryptfs_sendfile (struct file *file,loff_t * ppos, -size_t count,read_actor_t actor,void *target) +static ssize_t ecryptfs_splice_read (struct file *file,loff_t * ppos, +struct pipe_inode_info *pipe,size_t count, +unsigned int flags) {struct file *lower_file =NULL ;int rc = -EINVAL ; lower_file =ecryptfs_file_to_lower (file); -if (lower_file->f_op && lower_file->f_op->sendfile) - rc = lower_file->f_op->sendfile (lower_file, ppos, count, - actor, target); +if (lower_file->f_op && lower_file->f_op->splice_read) + rc = lower_file->f_op->splice_read (lower_file, ppos, pipe, + count, flags);return rc; }
@@ -364,7 +365,7 @@ const struct file_operations ecryptfs_dir_fops = { .release = ecryptfs_release, .fsync = ecryptfs_fsync, .fasync = ecryptfs_fasync, - .sendfile = ecryptfs_sendfile, + .splice_read = ecryptfs_splice_read, };const struct file_operations ecryptfs_main_fops = {
@@ -381,7 +382,7 @@ const struct file_operations ecryptfs_main_fops = { .release = ecryptfs_release, .fsync = ecryptfs_fsync, .fasync = ecryptfs_fasync, - .sendfile = ecryptfs_sendfile, + .splice_read = ecryptfs_splice_read, }; static int
fs/ext2/file.c +3 -2
@@ -53,7 +53,6 @@ const struct file_operations ext2_file_operations = { .open = generic_file_open, .release = ext2_release_file, .fsync = ext2_sync_file, - .sendfile = generic_file_sendfile, .splice_read = generic_file_splice_read, .splice_write = generic_file_splice_write, };
@@ -71,7 +70,9 @@ const struct file_operations ext2_xip_file_operations = { .open = generic_file_open, .release = ext2_release_file, .fsync = ext2_sync_file, - .sendfile = xip_file_sendfile, +#if 0 + .splice_read = xip_file_splice_read, +#endif };#endif
fs/ext3/file.c +0 -1
@@ -120,7 +120,6 @@ const struct file_operations ext3_file_operations = { .open = generic_file_open, .release = ext3_release_file, .fsync = ext3_sync_file, - .sendfile = generic_file_sendfile, .splice_read = generic_file_splice_read, .splice_write = generic_file_splice_write, };
fs/ext4/file.c +0 -1
@@ -120,7 +120,6 @@ const struct file_operations ext4_file_operations = { .open = generic_file_open, .release = ext4_release_file, .fsync = ext4_sync_file, - .sendfile = generic_file_sendfile, .splice_read = generic_file_splice_read, .splice_write = generic_file_splice_write, };
fs/fat/file.c +1 -1
@@ -134,7 +134,7 @@ const struct file_operations fat_file_operations = { .release = fat_file_release, .ioctl = fat_generic_ioctl, .fsync = file_fsync, - .sendfile = generic_file_sendfile, + .splice_read = generic_file_splice_read, };static int fat_cont_expand (struct inode *inode,loff_t size)
fs/fuse/file.c +2 -2
@@ -802,7 +802,7 @@ static const struct file_operations fuse_file_operations = { .release = fuse_release, .fsync = fuse_fsync, .lock = fuse_file_lock, - .sendfile = generic_file_sendfile, + .splice_read = generic_file_splice_read, };static const struct file_operations fuse_direct_io_file_operations = {
@@ -814,7 +814,7 @@ static const struct file_operations fuse_direct_io_file_operations = { .release = fuse_release, .fsync = fuse_fsync, .lock = fuse_file_lock, -/* no mmap and sendfile */ +/* no mmap and splice_read */ };static const struct address_space_operations fuse_file_aops = {
fs/gfs2/ops_file.c +0 -1
@@ -635,7 +635,6 @@ const struct file_operations gfs2_file_fops = { .release = gfs2_close, .fsync = gfs2_fsync, .lock = gfs2_lock, - .sendfile = generic_file_sendfile, .flock = gfs2_flock, .splice_read = generic_file_splice_read, .splice_write = generic_file_splice_write,
fs/hfs/inode.c +1 -1
@@ -607,7 +607,7 @@ static const struct file_operations hfs_file_operations = { .write = do_sync_write, .aio_write = generic_file_aio_write, .mmap = generic_file_mmap, - .sendfile = generic_file_sendfile, + .splice_read = generic_file_splice_read, .fsync = file_fsync, .open = hfs_file_open, .release = hfs_file_release,
fs/hfsplus/inode.c +1 -1
@@ -288,7 +288,7 @@ static const struct file_operations hfsplus_file_operations = { .write = do_sync_write, .aio_write = generic_file_aio_write, .mmap = generic_file_mmap, - .sendfile = generic_file_sendfile, + .splice_read = generic_file_splice_read, .fsync = file_fsync, .open = hfsplus_file_open, .release = hfsplus_file_release,
fs/hostfs/hostfs_kern.c +1 -1
@@ -390,7 +390,7 @@ int hostfs_fsync(struct file *file, struct dentry *dentry, int datasync)static const struct file_operations hostfs_file_fops = { .llseek = generic_file_llseek, .read = do_sync_read, - .sendfile = generic_file_sendfile, + .splice_read = generic_file_splice_read, .aio_read = generic_file_aio_read, .aio_write = generic_file_aio_write, .write = do_sync_write,
fs/hpfs/file.c +1 -1
@@ -129,7 +129,7 @@ const struct file_operations hpfs_file_ops = .mmap = generic_file_mmap, .release = hpfs_file_release, .fsync = hpfs_file_fsync, - .sendfile = generic_file_sendfile, + .splice_read = generic_file_splice_read, }; const struct inode_operations hpfs_file_iops =
fs/jffs2/file.c +1 -1
@@ -47,7 +47,7 @@ const struct file_operations jffs2_file_operations = .ioctl = jffs2_ioctl, .mmap = generic_file_readonly_mmap, .fsync = jffs2_fsync, - .sendfile = generic_file_sendfile + .splice_read = generic_file_splice_read, };/* jffs2_file_inode_operations */
fs/jfs/file.c +0 -1
@@ -108,7 +108,6 @@ const struct file_operations jfs_file_operations = { .aio_read = generic_file_aio_read, .aio_write = generic_file_aio_write, .mmap = generic_file_mmap, - .sendfile = generic_file_sendfile, .splice_read = generic_file_splice_read, .splice_write = generic_file_splice_write, .fsync = jfs_fsync,
fs/minix/file.c +1 -1
@@ -23,7 +23,7 @@ const struct file_operations minix_file_operations = { .aio_write = generic_file_aio_write, .mmap = generic_file_mmap, .fsync = minix_sync_file, - .sendfile = generic_file_sendfile, + .splice_read = generic_file_splice_read, };const struct inode_operations minix_file_inode_operations = {
fs/nfs/file.c +9 -6
@@ -41,7 +41,9 @@ static int nfs_file_open(struct inode *, struct file *);static int nfs_file_release (struct inode *,struct file *);static loff_t nfs_file_llseek (struct file *file,loff_t offset,int origin);static int nfs_file_mmap (struct file *,struct vm_area_struct *); -static ssize_t nfs_file_sendfile (struct file *,loff_t *,size_t ,read_actor_t ,void *); +static ssize_t nfs_file_splice_read (struct file *filp,loff_t *ppos, +struct pipe_inode_info *pipe, +size_t count,unsigned int flags);static ssize_t nfs_file_read (struct kiocb *,const struct iovec *iov,unsigned long nr_segs,loff_t pos); static ssize_t nfs_file_write(struct kiocb *, const struct iovec *iov,
@@ -65,7 +67,7 @@ const struct file_operations nfs_file_operations = { .fsync = nfs_fsync, .lock = nfs_lock, .flock = nfs_flock, - .sendfile = nfs_file_sendfile, + .splice_read = nfs_file_splice_read, .check_flags = nfs_check_flags, };
@@ -224,20 +226,21 @@ nfs_file_read(struct kiocb *iocb, const struct iovec *iov, }static ssize_t -nfs_file_sendfile (struct file *filp,loff_t *ppos,size_t count, -read_actor_t actor,void *target) +nfs_file_splice_read (struct file *filp,loff_t *ppos, +struct pipe_inode_info *pipe,size_t count, +unsigned int flags) {struct dentry *dentry = filp->f_path.dentry;struct inode *inode = dentry->d_inode;ssize_t res; -dfprintk (VFS ,"nfs: sendfile(%s/%s, %lu@%Lu)\n" , +dfprintk (VFS ,"nfs: splice_read(%s/%s, %lu@%Lu)\n" , dentry->d_parent->d_name.name, dentry->d_name.name, (unsigned long ) count, (unsigned long long ) *ppos); res =nfs_revalidate_mapping (inode, filp->f_mapping);if (!res) - res =generic_file_sendfile (filp, ppos, count, actor, target); + res =generic_file_splice_read (filp, ppos, pipe, count, flags);return res; }
fs/nfsd/vfs.c +9 -10
@@ -805,6 +805,7 @@ found:* so that they can be passed to the netowork sendmsg/sendpage routines * directrly. They will be released after the sending has completed. */ +#if 0 static int nfsd_read_actor (read_descriptor_t *desc,struct page *page,unsigned long offset ,unsigned long size) {
@@ -836,6 +837,7 @@ nfsd_read_actor(read_descriptor_t *desc, struct page *page, unsigned long offset desc->written += size;return size; } +#endif static __be32 nfsd_vfs_read(struct svc_rqst *rqstp, struct svc_fh *fhp, struct file *file,
@@ -861,16 +863,13 @@ nfsd_vfs_read(struct svc_rqst *rqstp, struct svc_fh *fhp, struct file *file,if (ra && ra->p_set) file->f_ra = ra->p_ra; -if (file->f_op->sendfile && rqstp->rq_sendfile_ok) { - rqstp->rq_resused =1 ; - host_err = file->f_op->sendfile (file, &offset, *count, - nfsd_read_actor, rqstp); - }else { - oldfs =get_fs (); -set_fs (KERNEL_DS ); - host_err =vfs_readv (file, (struct iovec __user *)vec, vlen, &offset); -set_fs (oldfs); - } +/* +* Can we use ->splice_read() for this? +*/ + oldfs =get_fs (); +set_fs (KERNEL_DS ); + host_err =vfs_readv (file, (struct iovec __user *)vec, vlen, &offset); +set_fs (oldfs);/* Write back readahead params */ if (ra) {
fs/ntfs/file.c +1 -1
@@ -2276,7 +2276,7 @@ const struct file_operations ntfs_file_ops = {mounted filesystem. */ .mmap = generic_file_mmap, /* Mmap file. */ .open = ntfs_file_open,/* Open file. */ - .sendfile = generic_file_sendfile, /* Zero-copy data send with + .splice_read = generic_file_splice_read /* Zero-copy data send withthe data source being on the ntfs partition. We do not need to care about the
fs/ocfs2/file.c +0 -1
@@ -1817,7 +1817,6 @@ const struct inode_operations ocfs2_special_file_iops = {const struct file_operations ocfs2_fops = { .read = do_sync_read, .write = do_sync_write, - .sendfile = generic_file_sendfile, .mmap = ocfs2_mmap, .fsync = ocfs2_sync_file, .release = ocfs2_file_release,
fs/qnx4/file.c +1 -1
@@ -25,7 +25,7 @@ const struct file_operations qnx4_file_operations = .read = do_sync_read, .aio_read = generic_file_aio_read, .mmap = generic_file_mmap, - .sendfile = generic_file_sendfile, + .splice_read = generic_file_splice_read, #ifdef CONFIG_QNX4FS_RW .write = do_sync_write, .aio_write = generic_file_aio_write,
fs/ramfs/file-mmu.c +1 -1
@@ -41,7 +41,7 @@ const struct file_operations ramfs_file_operations = { .aio_write = generic_file_aio_write, .mmap = generic_file_mmap, .fsync = simple_sync_file, - .sendfile = generic_file_sendfile, + .splice_read = generic_file_splice_read, .llseek = generic_file_llseek, };
fs/ramfs/file-nommu.c +1 -1
@@ -42,7 +42,7 @@ const struct file_operations ramfs_file_operations = { .write = do_sync_write, .aio_write = generic_file_aio_write, .fsync = simple_sync_file, - .sendfile = generic_file_sendfile, + .splice_read = generic_file_splice_read, .llseek = generic_file_llseek, };
fs/read_write.c +9 -4
@@ -15,6 +15,7 @@#include <linux/module.h> #include <linux/syscalls.h> #include <linux/pagemap.h> +#include <linux/pipe_fs_i.h> #include "read_write.h" #include <asm/uaccess.h>
@@ -25,7 +26,7 @@ const struct file_operations generic_ro_fops = { .read = do_sync_read, .aio_read = generic_file_aio_read, .mmap = generic_file_readonly_mmap, - .sendfile = generic_file_sendfile, + .splice_read = generic_file_splice_read, };EXPORT_SYMBOL (generic_ro_fops);
@@ -708,7 +709,7 @@ static ssize_t do_sendfile(int out_fd, int in_fd, loff_t *ppos,struct inode * in_inode, * out_inode;loff_t pos;ssize_t retval; -int fput_needed_in, fput_needed_out; +int fput_needed_in, fput_needed_out, fl;/* * Get input file, and verify that it is ok..
@@ -723,7 +724,7 @@ static ssize_t do_sendfile(int out_fd, int in_fd, loff_t *ppos, in_inode = in_file->f_path.dentry->d_inode;if (!in_inode) goto fput_in; -if (!in_file->f_op || !in_file->f_op->sendfile) +if (!in_file->f_op || !in_file->f_op->splice_read) goto fput_in; retval = -ESPIPE ;if (!ppos)
@@ -776,7 +777,11 @@ static ssize_t do_sendfile(int out_fd, int in_fd, loff_t *ppos, count = max - pos; } - retval = in_file->f_op->sendfile (in_file, ppos, count, file_send_actor, out_file); + fl =0 ; +if (in_file->f_flags &O_NONBLOCK ) + fl =SPLICE_F_NONBLOCK ; + + retval =do_splice_direct (in_file, ppos, out_file, count, fl);if (retval >0 ) {add_rchar (current, retval);
fs/reiserfs/file.c +0 -1
@@ -1531,7 +1531,6 @@ const struct file_operations reiserfs_file_operations = { .open = generic_file_open, .release = reiserfs_file_release, .fsync = reiserfs_sync_file, - .sendfile = generic_file_sendfile, .aio_read = generic_file_aio_read, .aio_write = generic_file_aio_write, .splice_read = generic_file_splice_read,
fs/smbfs/file.c +5 -4
@@ -262,8 +262,9 @@ out: }static ssize_t -smb_file_sendfile (struct file *file,loff_t *ppos, -size_t count,read_actor_t actor,void *target) +smb_file_splice_read (struct file *file,loff_t *ppos, +struct pipe_inode_info *pipe,size_t count, +unsigned int flags) {struct dentry *dentry = file->f_path.dentry;ssize_t status;
@@ -277,7 +278,7 @@ smb_file_sendfile(struct file *file, loff_t *ppos, DENTRY_PATH(dentry), status); goto out; } - status =generic_file_sendfile (file, ppos, count, actor, target); + status =generic_file_splice_read (file, ppos, pipe, count, flags); out:return status; }
@@ -416,7 +417,7 @@ const struct file_operations smb_file_operations = .open = smb_file_open, .release = smb_file_release, .fsync = smb_fsync, - .sendfile = smb_file_sendfile, + .splice_read = smb_file_splice_read, }; const struct inode_operations smb_file_inode_operations =
fs/sysv/file.c +1 -1
@@ -27,7 +27,7 @@ const struct file_operations sysv_file_operations = { .aio_write = generic_file_aio_write, .mmap = generic_file_mmap, .fsync = sysv_sync_file, - .sendfile = generic_file_sendfile, + .splice_read = generic_file_splice_read, };const struct inode_operations sysv_file_inode_operations = {
fs/udf/file.c +1 -1
@@ -261,7 +261,7 @@ const struct file_operations udf_file_operations = { .aio_write = udf_file_aio_write, .release = udf_release_file, .fsync = udf_fsync_file, - .sendfile = generic_file_sendfile, + .splice_read = generic_file_splice_read, };const struct inode_operations udf_file_inode_operations = {
fs/ufs/file.c +1 -1
@@ -60,5 +60,5 @@ const struct file_operations ufs_file_operations = { .mmap = generic_file_mmap, .open = generic_file_open, .fsync = ufs_sync_file, - .sendfile = generic_file_sendfile, + .splice_read = generic_file_splice_read, };
fs/xfs/linux-2.6/xfs_file.c +0 -26
@@ -124,30 +124,6 @@ xfs_file_aio_write_invis( } STATIC ssize_t -xfs_file_sendfile ( -struct file *filp, -loff_t *pos, -size_t count, -read_actor_t actor, -void *target) -{ -return bhv_vop_sendfile (vn_from_inode (filp->f_path.dentry->d_inode), - filp, pos,0 , count, actor, target,NULL ); -} - -STATIC ssize_t -xfs_file_sendfile_invis ( -struct file *filp, -loff_t *pos, -size_t count, -read_actor_t actor, -void *target) -{ -return bhv_vop_sendfile (vn_from_inode (filp->f_path.dentry->d_inode), - filp, pos,IO_INVIS , count, actor, target,NULL ); -} - -STATIC ssize_t xfs_file_splice_read( struct file *infilp, loff_t *ppos,
@@ -452,7 +428,6 @@ const struct file_operations xfs_file_operations = { .write = do_sync_write, .aio_read = xfs_file_aio_read, .aio_write = xfs_file_aio_write, - .sendfile = xfs_file_sendfile, .splice_read = xfs_file_splice_read, .splice_write = xfs_file_splice_write, .unlocked_ioctl = xfs_file_ioctl,
@@ -475,7 +450,6 @@ const struct file_operations xfs_invis_file_operations = { .write = do_sync_write, .aio_read = xfs_file_aio_read_invis, .aio_write = xfs_file_aio_write_invis, - .sendfile = xfs_file_sendfile_invis, .splice_read = xfs_file_splice_read_invis, .splice_write = xfs_file_splice_write_invis, .unlocked_ioctl = xfs_file_ioctl_invis,
fs/xfs/linux-2.6/xfs_linux.h +0 -1
@@ -101,7 +101,6 @@* Feature macros (disable/enable) */ #undef HAVE_REFCACHE/* reference cache not needed for NFS in 2.6 */ -#define HAVE_SENDFILE /* sendfile(2) exists in 2.6, but not in 2.4 */ #define HAVE_SPLICE /* a splice(2) exists in 2.6, but not in 2.4 */ #ifdef CONFIG_SMP #define HAVE_PERCPU_SB /* per cpu superblock counters are a 2.6 feature */
fs/xfs/linux-2.6/xfs_lrw.c +0 -44
50 lines not on this page · raw message
fs/xfs/linux-2.6/xfs_lrw.h +0 -3
@@ -90,9 +90,6 @@ extern ssize_t xfs_read(struct bhv_desc *, struct kiocb *,extern ssize_t xfs_write (struct bhv_desc *,struct kiocb *,const struct iovec *,unsigned int ,loff_t *,int ,struct cred *); -extern ssize_t xfs_sendfile (struct bhv_desc *,struct file *, -loff_t *,int ,size_t ,read_actor_t , -void *,struct cred *);extern ssize_t xfs_splice_read (struct bhv_desc *,struct file *,loff_t *,struct pipe_inode_info *,size_t ,int ,int ,struct cred *);
fs/xfs/linux-2.6/xfs_vnode.h +0 -6
@@ -139,9 +139,6 @@ typedef ssize_t (*vop_read_t)(bhv_desc_t *, struct kiocb *,typedef ssize_t (*vop_write_t )(bhv_desc_t *,struct kiocb *,const struct iovec *,unsigned int ,loff_t *,int ,struct cred *); -typedef ssize_t (*vop_sendfile_t )(bhv_desc_t *,struct file *, -loff_t *,int ,size_t ,read_actor_t , -void *,struct cred *);typedef ssize_t (*vop_splice_read_t )(bhv_desc_t *,struct file *,loff_t *,struct pipe_inode_info *,size_t ,int ,int ,struct cred *);
@@ -206,7 +203,6 @@ typedef struct bhv_vnodeops {vop_close_t vop_close;vop_read_t vop_read;vop_write_t vop_write; -vop_sendfile_t vop_sendfile;vop_splice_read_t vop_splice_read;vop_splice_write_t vop_splice_write;vop_ioctl_t vop_ioctl;
@@ -254,8 +250,6 @@ typedef struct bhv_vnodeops { VOP(vop_read, vp)(VNHEAD(vp),file,iov,segs,offset,ioflags,cr)#define bhv_vop_write (vp,file,iov,segs,offset,ioflags,cr) \ VOP(vop_write, vp)(VNHEAD(vp),file,iov,segs,offset,ioflags,cr) -#define bhv_vop_sendfile (vp,f,off,ioflags,cnt,act,targ,cr) \ - VOP(vop_sendfile, vp)(VNHEAD(vp),f,off,ioflags,cnt,act,targ,cr)#define bhv_vop_splice_read (vp,f,o,pipe,cnt,fl,iofl,cr) \ VOP(vop_splice_read, vp)(VNHEAD(vp),f,o,pipe,cnt,fl,iofl,cr)#define bhv_vop_splice_write (vp,f,o,pipe,cnt,fl,iofl,cr) \
fs/xfs/xfs_vnodeops.c +0 -3
@@ -4680,9 +4680,6 @@ bhv_vnodeops_t xfs_vnodeops = { .vop_open = xfs_open, .vop_close = xfs_close, .vop_read = xfs_read, -#ifdef HAVE_SENDFILE - .vop_sendfile = xfs_sendfile, -#endif #ifdef HAVE_SPLICE .vop_splice_read = xfs_splice_read, .vop_splice_write = xfs_splice_write,
include/linux/fs.h +0 -5
@@ -1104,7 +1104,6 @@ struct file_operations {int (*aio_fsync) (struct kiocb *,int datasync);int (*fasync) (int ,struct file *,int );int (*lock) (struct file *,int ,struct file_lock *); -ssize_t (*sendfile) (struct file *,loff_t *,size_t ,read_actor_t ,void *);ssize_t (*sendpage) (struct file *,struct page *,int ,size_t ,loff_t *,int );unsigned long (*get_unmapped_area)(struct file *,unsigned long ,unsigned long ,unsigned long ,unsigned long );int (*check_flags)(int );
@@ -1734,7 +1733,6 @@ extern ssize_t generic_file_buffered_write(struct kiocb *, const struct iovec *, unsigned long, loff_t, loff_t *, size_t, ssize_t);extern ssize_t do_sync_read (struct file *filp,char __user *buf,size_t len,loff_t *ppos);extern ssize_t do_sync_write (struct file *filp,const char __user *buf,size_t len,loff_t *ppos); -extern ssize_t generic_file_sendfile (struct file *,loff_t *,size_t ,read_actor_t ,void *);extern void do_generic_mapping_read (struct address_space *mapping,struct file_ra_state *,struct file *,loff_t *,read_descriptor_t *,read_actor_t );
@@ -1764,9 +1762,6 @@ extern int nonseekable_open(struct inode * inode, struct file * filp);#ifdef CONFIG_FS_XIP extern ssize_t xip_file_read (struct file *filp,char __user *buf,size_t len,loff_t *ppos); -extern ssize_t xip_file_sendfile (struct file *in_file,loff_t *ppos, -size_t count,read_actor_t actor, -void *target);extern int xip_file_mmap (struct file * file,struct vm_area_struct * vma);extern ssize_t xip_file_write (struct file *filp,const char __user *buf,size_t len,loff_t *ppos);
include/linux/sunrpc/svc.h +1 -1
@@ -253,7 +253,7 @@ struct svc_rqst {* determine what device number * to report (real or virtual) */ -int rq_sendfile_ok;/* turned off in gss privacy +int rq_splice_ok;/* turned off in gss privacy * to prevent encrypting page * cache pages */ wait_queue_head_t rq_wait;/* synchronization */
kernel/relay.c +0 -16
@@ -1060,21 +1060,6 @@ static ssize_t relay_file_read(struct file *filp, NULL, &desc); } -static ssize_t relay_file_sendfile (struct file *filp, -loff_t *ppos, -size_t count, -read_actor_t actor, -void *target) -{ -read_descriptor_t desc; - desc.written =0 ; - desc.count = count; - desc.arg.data = target; - desc.error =0 ; -return relay_file_read_subbufs (filp, ppos, subbuf_send_actor, - actor, &desc); -} -const struct file_operations relay_file_operations = { .open = relay_file_open, .poll = relay_file_poll,
@@ -1082,7 +1067,6 @@ const struct file_operations relay_file_operations = { .read = relay_file_read, .llseek = no_llseek, .release = relay_file_release, - .sendfile = relay_file_sendfile, };EXPORT_SYMBOL_GPL (relay_file_operations);
mm/filemap.c +0 -20
@@ -1245,26 +1245,6 @@ int file_send_actor(read_descriptor_t * desc, struct page *page, unsigned long oreturn written; } -ssize_t generic_file_sendfile (struct file *in_file,loff_t *ppos, -size_t count,read_actor_t actor,void *target) -{ -read_descriptor_t desc; - -if (!count) -return 0 ; - - desc.written =0 ; - desc.count = count; - desc.arg.data = target; - desc.error =0 ; - -do_generic_file_read (in_file, ppos, &desc, actor); -if (desc.written) -return desc.written; -return desc.error; -} -EXPORT_SYMBOL (generic_file_sendfile); -static ssize_t do_readahead (struct address_space *mapping,struct file *filp,unsigned long index,unsigned long nr)
mm/filemap_xip.c +2 -0
@@ -159,6 +159,7 @@ xip_file_read(struct file *filp, char __user *buf, size_t len, loff_t *ppos) }EXPORT_SYMBOL_GPL (xip_file_read); +#if 0 ssize_t xip_file_sendfile (struct file *in_file,loff_t *ppos,size_t count,read_actor_t actor,void *target)
@@ -180,6 +181,7 @@ xip_file_sendfile(struct file *in_file, loff_t *ppos, return desc.error; }EXPORT_SYMBOL_GPL (xip_file_sendfile); +#endif /* * __xip_unmap is invoked from xip_unmap and
mm/shmem.c +12 -20
@@ -1483,6 +1483,17 @@ static const struct inode_operations shmem_symlink_inode_operations;static const struct inode_operations shmem_symlink_inline_operations;/* +* FIXME: need a real implementation :-) +*/ +static ssize_t +shmem_file_splice_read (struct file *in,loff_t *ppos, +struct pipe_inode_info *pipe,size_t len, +unsigned int flags) +{ +return -EINVAL ; +} + +/* * Normally tmpfs makes no use of shmem_prepare_write, but it * lets a tmpfs file be used read-write below the loop driver. */
@@ -1709,25 +1720,6 @@ static ssize_t shmem_file_read(struct file *filp, char __user *buf, size_t countreturn desc.error; } -static ssize_t shmem_file_sendfile (struct file *in_file,loff_t *ppos, -size_t count,read_actor_t actor,void *target) -{ -read_descriptor_t desc; - -if (!count) -return 0 ; - - desc.written =0 ; - desc.count = count; - desc.arg.data = target; - desc.error =0 ; - -do_shmem_file_read (in_file, ppos, &desc, actor); -if (desc.written) -return desc.written; -return desc.error; -} -static int shmem_statfs (struct dentry *dentry,struct kstatfs *buf) {struct shmem_sb_info *sbinfo =SHMEM_SB (dentry->d_sb);
@@ -2397,7 +2389,7 @@ static const struct file_operations shmem_file_operations = { .read = shmem_file_read, .write = shmem_file_write, .fsync = simple_sync_file, - .sendfile = shmem_file_sendfile, + .splice_read = shmem_file_splice_read,#endif };
net/sunrpc/auth_gss/svcauth_gss.c +1 -1
@@ -853,7 +853,7 @@ unwrap_priv_data(struct svc_rqst *rqstp, struct xdr_buf *buf, u32 seq, struct gsu32 priv_len, maj_stat;int pad, saved_len, remaining_len, offset; - rqstp->rq_sendfile_ok =0 ; + rqstp->rq_splice_ok =0 ; priv_len =svc_getnl (&buf->head[0 ]);if (rqstp->rq_deferred) {
net/sunrpc/svc.c +1 -1
@@ -814,7 +814,7 @@ svc_process(struct svc_rqst *rqstp) rqstp->rq_res.tail[0 ].iov_base =NULL ; rqstp->rq_res.tail[0 ].iov_len =0 ;/* Will be turned off only in gss privacy case: */ - rqstp->rq_sendfile_ok =1 ; + rqstp->rq_splice_ok =1 ;/* tcp needs a space for the record length... */ if (rqstp->rq_prot ==IPPROTO_TCP )svc_putnl (resv,0 );
Signature · 1 line
Jens Axboe
fs/read_write.c static ssize_t do_sendfile(int out_fd, int in_fd, loff_t *ppos, · line 784
Quoted 13 lines · On Thu, 31 May 2007 12:33:16 +0200 Jens Axboe <[email protected]> wrote:
Hi,
This patch removes the ->sendfile() hook from the
file_operationsstructure, and replaces thesys_sendfile()mechanism to be based on ->splice_read() instead. There should be no functional changes.fs/read_write.c +5
-1- retval = in_file->f_op->sendfile (in_file, ppos, count, file_send_actor, out_file); + fl =0 ; +if (in_file->f_flags &O_NONBLOCK ) + fl =SPLICE_F_NONBLOCK ; + + retval =do_splice_direct (in_file, ppos, out_file, count, fl);
I like this, but are you sure it wont break user land ?
Some applications might react badly if sendfile() returns EAGAIN ?
Quoted 19 lines · Eric Dumazet
fs/read_write.c static ssize_t do_sendfile(int out_fd, int in_fd, loff_t *ppos, · line 784
Quoted 13 lines · On Thu, 31 May 2007 12:33:16 +0200 Jens Axboe <[email protected]> wrote:
Hi,
This patch removes the ->sendfile() hook from the
file_operationsstructure, and replaces thesys_sendfile()mechanism to be based on ->splice_read() instead. There should be no functional changes.fs/read_write.c +5
-1- retval = in_file->f_op->sendfile (in_file, ppos, count, file_send_actor, out_file); + fl =0 ; +if (in_file->f_flags &O_NONBLOCK ) + fl =SPLICE_F_NONBLOCK ; + + retval =do_splice_direct (in_file, ppos, out_file, count, fl);I like this, but are you sure it wont break user land ?
Quoted 1 line
Some applications might react badly if
sendfile()returns EAGAIN ?
Yeah, I didn't actually intend for that to sneak in. I'd think that userspace should handle it if they opened the file O_NONBLOCK (or used fcntl()), but it's a change in behaviour none the less and probably not a good idea.
Encourage those people to use splice() instead :-)
Signature · 1 line
Jens Axboe
Quoted 9 lines · Jens Axboe
Quoted 9 lines
fs/read_write.c static ssize_t do_sendfile(int out_fd, int in_fd, loff_t *ppos, · line 784
Quoted 7 lines
fs/read_write.c +5
-1- retval = in_file->f_op->sendfile (in_file, ppos, count, file_send_actor, out_file); + fl =0 ; +if (in_file->f_flags &O_NONBLOCK ) + fl =SPLICE_F_NONBLOCK ; + + retval =do_splice_direct (in_file, ppos, out_file, count, fl);I like this, but are you sure it wont break user land ?
Quoted 7 lines
Quoted 1 line
Some applications might react badly if
sendfile()returns EAGAIN ?Yeah, I didn't actually intend for that to sneak in. I'd think that userspace should handle it if they opened the file
O_NONBLOCK(or usedfcntl()), but it's a change in behaviour none the less and probably not a good idea.
I would personally argue that sendfile() blocking on an O_NONBLOCK desriptor, as opposed to returning EAGAIN, is a bug, and a fairly serious such.
-hpa
Quoted 18 lines · H. Peter Anvin
Quoted 9 lines · Jens Axboe
Quoted 9 lines
fs/read_write.c static ssize_t do_sendfile(int out_fd, int in_fd, loff_t *ppos, · line 784
Quoted 7 lines
fs/read_write.c +5
-1- retval = in_file->f_op->sendfile (in_file, ppos, count, file_send_actor, out_file); + fl =0 ; +if (in_file->f_flags &O_NONBLOCK ) + fl =SPLICE_F_NONBLOCK ; + + retval =do_splice_direct (in_file, ppos, out_file, count, fl);I like this, but are you sure it wont break user land ?
Quoted 7 lines
Quoted 1 line
Some applications might react badly if
sendfile()returns EAGAIN ?Yeah, I didn't actually intend for that to sneak in. I'd think that userspace should handle it if they opened the file
O_NONBLOCK(or usedfcntl()), but it's a change in behaviour none the less and probably not a good idea.
Quoted 3 lines
I would personally argue that
sendfile()blocking on anO_NONBLOCKdesriptor, as opposed to returning EAGAIN, is a bug, and a fairly serious such.
I agree, but it's still a change in behaviour. Even if we consider the app buggy (it is), can we potentially break it?
Signature · 1 line
Jens Axboe
Quoted 8 lines · Jens Axboe
Quoted 4 lines
Quoted 1 line
I would personally argue that
sendfile()blocking on anO_NONBLOCKdesriptor, as opposed to returning EAGAIN, is a bug, and a fairly serious such.I agree, but it's still a change in behaviour. Even if we consider the app buggy (it is), can we potentially break it?
It depends on which app it is, of course. However, I think we have to smoke that out the hard way. I don't think we should retain a bug in the kernel just because some unknown app might depend on that bug -- taking that to the extreme we could never fix bugs at all...
-hpa
Quoted 4 lines · H. Peter Anvin
Quoted 3 lines · Jens Axboe
Quoted 3 lines
I would personally argue that
sendfile()blocking on anO_NONBLOCKdesriptor, as opposed to returning EAGAIN, is a bug, and a fairly serious such.
Quoted 9 lines
Quoted 3 lines
I agree, but it's still a change in behaviour. Even if we consider the app buggy (it is), can we potentially break it?
It depends on which app it is, of course. However, I think we have to smoke that out the hard way. I don't think we should retain a bug in the kernel just because some unknown app might depend on that bug -- taking that to the extreme we could never fix bugs at all...
As I said, this new non blocking feature on the input side (disk), is nice and usefull. (For people scared by splice() syscall :) )
Just have to mention it is a change of behavior, and documentation probably needs to reflect this change. "Since linux 2.6.23, sendfile() repects O_NONBLOCK on in_fd as well"
My man page here says :
ERRORS
EAGAIN Non-blocking I/O has been selected using O_NONBLOCK and the
write would block.
EBADF The input file was not opened for reading or the output file
was not opened for writing.
EFAULT Bad address.
EINVAL Descriptor is not valid or locked, or an mmap()-like operation
is not available for in_fd.
EIO Unspecified error while reading from in_fd.
ENOMEM Insufficient memory to read from in_fd.
This implies O_NONBLOCK on the out filedesc, not the input one :)
Quoted 8 lines · Eric Dumazet
As I said, this new non blocking feature on the input side (disk), is nice and usefull. (For people scared by
splice()syscall :) )Just have to mention it is a change of behavior, and documentation probably needs to reflect this change. "Since linux 2.6.23,
sendfile()repectsO_NONBLOCKonin_fdas well"
Fair enough. Unix has traditionally not acknowledged the possibility of nonblocking I/O on conventional files, for some odd reason.
-hpa
Quoted 3 lines · H. Peter Anvin
Fair enough. Unix has traditionally not acknowledged the possibility of nonblocking I/O on conventional files, for some odd reason.
It's not odd at all.
If you return EAGAIN, you had better have a way to _wait_ for that EAGAIN to go away, otherwise the EAGAIN is just a total waste of time.
So the rule about EAGAIN is very simple:
(a) the file descriptor must be O_NONBLOCK
(b) the access must otherwise block
AND
(c) the condition must be something we can wait for with poll/select
I don't know why people continually ignore that (c) point, even though it's obvious and very very important!
If you cannot wait for it, tell me why the kernel should _ever_ return EAGAIN? The only option for the user is to just do the operation again immediately.
And the thing is, neither poll nor select work on regular files. And no, that is _not_ just an implementation issue. It's very fundamental: neither poll nor select get the file offset to wait for!
And that file offset is _critical_ for a regular file, in a way it obviously is _not_ for a socket, pipe, or other special file. Because without knowing the file offset, you cannot know which page you should be waiting for!
And no, the file offset is not "f_pos". sendfile(), along with pread/pwrite, uses a totally separate file offset, so if select/poll were to base their decision on f_pos, they'd be _wrong_.
This really is very fundamental.
Now, you can argue that you can always just return -EAGAIN anyway, but then the calling process will basically be busy-looping, calling sendfile() (or splice()) over and over again. That's _horrible_. It's much better to just not return EAGAIN, and sleep like a good process should!
So there's a few things to take away from this:
- regular file access MUST NOT return EAGAIN just because a page isn't in the cache. Doing so is simply a bug. No ifs, buts or maybe's about it!
Busy-looping is NOT ACCEPTABLE!
- you *could* make some alternative conventions:
(a) you could make O_NONBLOCK mean that you'll at least guarantee that you *start* the IO, and while you never return EAGAIN, you migth validly return a _partial_ result!
(b) variation on (a): it's ok to return EAGAIN if _you_ were the one who started the IO during this particular time aroudn the loop. But if you find a page that isn't up-to-date yet, and you didn't start the IO, you *must* wait for it, so that you end up returning EAGAIN atmost once! Exactly because busy-looping is simply not acceptable behaviour!
I have to admit that I didn't look at what raw splice() itself does these days. I would not be surprised if Jens also didn't realize this very fundamental issue. It seems too easy to miss, because people think that EAGAIN stands on its own, and don't realize that EAGAIN must be paired with select/poll to make sense.
Jens?
Linus
Quoted 29 lines · Linus Torvalds
Quoted 2 lines · H. Peter Anvin
Fair enough. Unix has traditionally not acknowledged the possibility of nonblocking I/O on conventional files, for some odd reason.
It's not odd at all.
If you return EAGAIN, you had better have a way to _wait_ for that EAGAIN to go away, otherwise the EAGAIN is just a total waste of time.
So the rule about EAGAIN is very simple:
(a) the file descriptor must beO_NONBLOCK
(b) the access must otherwise block
AND
(c) the condition must be something we can wait for with poll/selectI don't know why people continually ignore that (c) point, even though it's obvious and very very important!
If you cannot wait for it, tell me why the kernel should _ever_ return EAGAIN? The only option for the user is to just do the operation again immediately.
And the thing is, neither poll nor select work on regular files. And no, that is _not_ just an implementation issue. It's very fundamental: neither poll nor select get the file offset to wait for!
And that file offset is _critical_ for a regular file, in a way it obviously is _not_ for a socket, pipe, or other special file. Because without knowing the file offset, you cannot know which page you should be waiting for!
And no, the file offset is not "
f_pos".sendfile(), along with pread/pwrite, uses a totally separate file offset, so if select/poll were to base their decision onf_pos, they'd be _wrong_.This really is very fundamental.
Now, you can argue that you can always just return
-EAGAINanyway, but then the calling process will basically be busy-looping, callingsendfile()(orsplice()) over and over again. That's _horrible_. It's much better to just not return EAGAIN, and sleep like a good process should!
Quoted 7 lines
So there's a few things to take away from this:
- regular file access MUST NOT return EAGAIN just because a page isn't in the cache. Doing so is simply a bug. No ifs, buts or maybe's about it!
Busy-looping is NOT ACCEPTABLE!
yes, very true, but then some apps do this (and sometimes depends on yield())
Quoted 7 lines
- you *could* make some alternative conventions:
(a) you could make
O_NONBLOCKmean that you'll at least guarantee that you *start* the IO, and while you never return EAGAIN, you migth validly return a _partial_ result!
Quoted 9 lines
(b) variation on (a): it's ok to return EAGAIN if _you_ were the one who started the IO during this particular time aroudn the loop. But if you find a page that isn't up-to-date yet, and you didn't start the IO, you *must* wait for it, so that you end up returning EAGAIN atmost once! Exactly because busy-looping is simply not acceptable behaviour!
I have to admit that I didn't look at what raw
splice()itself does these days. I would not be surprised if Jens also didn't realize this very fundamental issue. It seems too easy to miss, because people think that EAGAIN stands on its own, and don't realize that EAGAIN must be paired with select/poll to make sense.
Right now, splice() has one SPLICE_F_NONBLOCK flag, and this flag is applied on both sides (in & out)
So either :
1) We separate the flag into two flags NONBLOCK_IN & NONBLOCK_OUT, so that the application is free to chose to busy-loop/yield if it wants.
2) We ignore NONBLOCK flag for regular files in splice() (and sendfile()), just following current facto
3) We consider select()/poll()/splice() can be extended to regular files on [f_pos] (select() and related functions have a meaning on non-seekable files, so consider it can be extended on files only on current file pos)
Quoted 4 lines · Linus Torvalds
And the thing is, neither poll nor select work on regular files. And no, that is _not_ just an implementation issue. It's very fundamental: neither poll nor select get the file offset to wait for!
Quoted 8 lines
And that file offset is _critical_ for a regular file, in a way it obviously is _not_ for a socket, pipe, or other special file. Because without knowing the file offset, you cannot know which page you should be waiting for!
And no, the file offset is not "
f_pos".sendfile(), along with pread/pwrite, uses a totally separate file offset, so if select/poll were to base their decision onf_pos, they'd be _wrong_.
This is obviously correct, although at the time those interfaces were designed, I don't believe either pread/pwrite nor sendfile() existed, and they still couldn't wait on real files. That there isn't a suitable way to wait for a file at an offset is probably a result of that past history.
Waiting at f_pos is still a possible interface, of course; it would mean that pread/pwrite/sendfile users would have to seek before waiting. However, implementing waiting on files in select/poll is prohibited by POSIX, so it would at least need some sort of Linux-specific flag anyway.
It seems that being able to do nonblocking I/O on files would be a useful thing. This really *does* require proper nonblocking I/O and not just the ability to wait, since you can never know when the kernel decides to recycle the page you are just about to want from the cache.
Quoted 14 lines
So there's a few things to take away from this:
- regular file access MUST NOT return EAGAIN just because a page isn't in the cache. Doing so is simply a bug. No ifs, buts or maybe's about it!
Busy-looping is NOT ACCEPTABLE!
- you *could* make some alternative conventions:
(a) you could make
O_NONBLOCKmean that you'll at least guarantee that you *start* the IO, and while you never return EAGAIN, you migth validly return a _partial_ result!
Quoted 6 lines
(b) variation on (a): it's ok to return EAGAIN if _you_ were the one who started the IO during this particular time aroudn the loop. But if you find a page that isn't up-to-date yet, and you didn't start the IO, you *must* wait for it, so that you end up returning EAGAIN atmost once! Exactly because busy-looping is simply not acceptable behaviour!
(b) seems really ugly. (a) is at least well-defined. Either seems wrong, though.
-hpa
Quoted 21 lines · H. Peter Anvin
Quoted 14 lines
So there's a few things to take away from this:
- regular file access MUST NOT return EAGAIN just because a page isn't in the cache. Doing so is simply a bug. No ifs, buts or maybe's about it!
Busy-looping is NOT ACCEPTABLE!
- you *could* make some alternative conventions:
(a) you could make
O_NONBLOCKmean that you'll at least guarantee that you *start* the IO, and while you never return EAGAIN, you migth validly return a _partial_ result!Quoted 6 lines
(b) variation on (a): it's ok to return EAGAIN if _you_ were the one who started the IO during this particular time aroudn the loop. But if you find a page that isn't up-to-date yet, and you didn't start the IO, you *must* wait for it, so that you end up returning EAGAIN atmost once! Exactly because busy-looping is simply not acceptable behaviour!
Quoted 2 lines
(b) seems really ugly. (a) is at least well-defined. Either seems wrong, though.
I totally agree, b) would get nasty. And while a) isn't perfect by any means, I do follow Linus' logic and agree it's probably the best (only?) way to handle it.
Signature · 1 line
Jens Axboe
Quoted 66 lines · Linus Torvalds
Quoted 3 lines · H. Peter Anvin
Fair enough. Unix has traditionally not acknowledged the possibility of nonblocking I/O on conventional files, for some odd reason.
It's not odd at all.
If you return EAGAIN, you had better have a way to _wait_ for that EAGAIN to go away, otherwise the EAGAIN is just a total waste of time.
So the rule about EAGAIN is very simple:
(a) the file descriptor must beO_NONBLOCK
(b) the access must otherwise block
AND
(c) the condition must be something we can wait for with poll/selectI don't know why people continually ignore that (c) point, even though it's obvious and very very important!
If you cannot wait for it, tell me why the kernel should _ever_ return EAGAIN? The only option for the user is to just do the operation again immediately.
And the thing is, neither poll nor select work on regular files. And no, that is _not_ just an implementation issue. It's very fundamental: neither poll nor select get the file offset to wait for!
And that file offset is _critical_ for a regular file, in a way it obviously is _not_ for a socket, pipe, or other special file. Because without knowing the file offset, you cannot know which page you should be waiting for!
And no, the file offset is not "
f_pos".sendfile(), along with pread/pwrite, uses a totally separate file offset, so if select/poll were to base their decision onf_pos, they'd be _wrong_.This really is very fundamental.
Now, you can argue that you can always just return
-EAGAINanyway, but then the calling process will basically be busy-looping, callingsendfile()(orsplice()) over and over again. That's _horrible_. It's much better to just not return EAGAIN, and sleep like a good process should!So there's a few things to take away from this:
- regular file access MUST NOT return EAGAIN just because a page isn't in the cache. Doing so is simply a bug. No ifs, buts or maybe's about it!
Busy-looping is NOT ACCEPTABLE!
- you *could* make some alternative conventions:
(a) you could make
O_NONBLOCKmean that you'll at least guarantee that you *start* the IO, and while you never return EAGAIN, you migth validly return a _partial_ result!(b) variation on (a): it's ok to return EAGAIN if _you_ were the one who started the IO during this particular time aroudn the loop. But if you find a page that isn't up-to-date yet, and you didn't start the IO, you *must* wait for it, so that you end up returning EAGAIN atmost once! Exactly because busy-looping is simply not acceptable behaviour!
Quoted 5 lines
I have to admit that I didn't look at what raw
splice()itself does these days. I would not be surprised if Jens also didn't realize this very fundamental issue. It seems too easy to miss, because people think that EAGAIN stands on its own, and don't realize that EAGAIN must be paired with select/poll to make sense.
splice() WILL return EAGAIN, but in that case it should have triggered the read-ahead and thus started some IO. At least that was/is the intention, and it worked like that originally. Since I'm deep on the splice updating for sendfile etc right now anyway, I'll doubly verify that it still works that way as expected!
For the from-file case, see __generic_file_splice_read(). splice does:
if (!PageUptodate(page)) {
/*
* If in nonblock mode then dont block on
* waiting
* for an in-flight io page
*/
if (flags & SPLICE_F_NONBLOCK) {
if (TestSetPageLocked(page))
break;
} else
lock_page(page);but it doesn't know whether this call into __generic_file_splice_read() initiated IO to that page or if someone else did. Naturally this will most often be the case that __generic_file_splice_read() was the one that initiated IO to that page, but it could be someone else of course.
The code above doesn't actually return EAGAIN, but generic_file_splice_read() turns a 0 return for SPLICE_F_NONBLOCK into EAGAIN.
So we need a bit of tweaking. I don't see how we can make b) work well enough without getting hackish and breaking into the page cache and read-ahead code, so I'd be inclined to suggest we make it conform to your a) case. Now I didn't test yet, but it seems that just doing:
if ((flags & SPLICE_F_NONBLOCK) && spd.nr_pages) {
if (TestSetPageLocked(page))
break;
} else
lock_page(page);should do that - always block for the first page and potentially return a partial results for the remaining pages that read-ahead kicked into gear.
Signature · 1 line
Jens Axboe
Quoted 3 lines · Jens Axboe
splice()WILL return EAGAIN, but in that case it should have triggered the read-ahead and thus started some IO.
That's not enough.
If the IO has already been started, splice needs to wait.
Quoted 13 lines
For the from-file case, see
__generic_file_splice_read(). splice does:if (!PageUptodate(page)) { /* * If in nonblock mode then dont block on * waiting * for an in-flight io page */ if (flags & SPLICE_F_NONBLOCK) { if (TestSetPageLocked(page)) break; } else lock_page(page);
Yeah, that's just wrong.
Your suggested:
Quoted 9 lines
if ((flags & SPLICE_F_NONBLOCK) && spd.nr_pages) { if (TestSetPageLocked(page)) break; } else lock_page(page);should do that - always block for the first page and potentially return a partial results for the remaining pages that read-ahead kicked into gear.
would work, but I suspect that for a server, returning EAGAIN once is actually the best option - if it has a select() loop, and something else is running, the "return EAGAIN once" actually makes tons of sense (it would basically boil down to the kernel effectively saying "ok, try anything else you might have pending in your queues first, if you get back to me, I'll block then").
Linus
Quoted 10 lines · Linus Torvalds
Quoted 3 lines · Jens Axboe
splice()WILL return EAGAIN, but in that case it should have triggered the read-ahead and thus started some IO.That's not enough.
If the IO has already been started, splice needs to wait.
But splice doesn't know, page_cache_readahead() may not have started anything.
Quoted 28 lines
Quoted 13 lines
For the from-file case, see
__generic_file_splice_read(). splice does:if (!PageUptodate(page)) { /* * If in nonblock mode then dont block on * waiting * for an in-flight io page */ if (flags & SPLICE_F_NONBLOCK) { if (TestSetPageLocked(page)) break; } else lock_page(page);Yeah, that's just wrong.
Your suggested:
Quoted 9 lines
if ((flags & SPLICE_F_NONBLOCK) && spd.nr_pages) { if (TestSetPageLocked(page)) break; } else lock_page(page);should do that - always block for the first page and potentially return a partial results for the remaining pages that read-ahead kicked into gear.
Quoted 6 lines
would work, but I suspect that for a server, returning EAGAIN once is actually the best option - if it has a
select()loop, and something else is running, the "return EAGAIN once" actually makes tons of sense (it would basically boil down to the kernel effectively saying "ok, try anything else you might have pending in your queues first, if you get back to me, I'll block then").
Well then the current code should work, _provided_ that we know read-ahead kicked off the IO. Well almost, still needs a bit of tweaking, with some knowledge of whether page_cache_readahead() actually called into read_pages() or not.
Signature · 1 line
Jens Axboe
Quoted 13 lines · Linus Torvalds
On Sat, 2 Jun 2007, Jens Axboe wrote:
Your suggested:Quoted 9 lines
if ((flags & SPLICE_F_NONBLOCK) && spd.nr_pages) { if (TestSetPageLocked(page)) break; } else lock_page(page);should do that - always block for the first page and potentially return a partial results for the remaining pages that read-ahead kicked into gear.
Quoted 6 lines
would work, but I suspect that for a server, returning EAGAIN once is actually the best option - if it has a
select()loop, and something else is running, the "return EAGAIN once" actually makes tons of sense (it would basically boil down to the kernel effectively saying "ok, try anything else you might have pending in your queues first, if you get back to me, I'll block then").
May be we can settle with "return EAGAIN once" for *all* possible waits, including I/O submitted by others:
if ((flags & SPLICE_F_NONBLOCK) &&
TestSetPageLocked(page) &&
in->f_ra.prev_index != index)
break;
else
lock_page(page);However, the upper layer generic_file_splice_read() may keep on calling __generic_file_splice_read(), so we need more code to stop it with flag SPLICE_INTERNAL_WILLBLOCK:
---
(not tested yet; still not sure if it's the best solution.)
In non-block splicing, return EAGAIN once for all possible I/O waits.
It works by checking (ra.prev_index != index) in __generic_file_splice_read(). Another reader on the same fd will at worst cause a few more splice syscalls.
If keep calling generic_file_splice_read() in non-block mode, it will return partial data before the first hole; then return EAGAIN at second call; at last it will block on the I/O page.
--- fs/splice.c | 38 ++++++++++++++++++++++-------------- include/linux/pipe_fs_i.h | 2 + 2 files changed, 26 insertions(+), 14 deletions(-)
fs/splice.c +24 -14
@@ -264,10 +264,11 @@ static ssize_t splice_to_pipe(struct pipstatic int __generic_file_splice_read (struct file *in,loff_t *ppos,struct pipe_inode_info *pipe,size_t len, -unsigned int flags) +unsigned int *flags) {struct address_space *mapping = in->f_mapping;unsigned int loff, nr_pages; +unsigned int first_hole;struct page *pages[PIPE_BUFFERS ];struct partial_page partial[PIPE_BUFFERS ];struct page *page;
@@ -278,7 +279,7 @@ __generic_file_splice_read(struct file *struct splice_pipe_desc spd = { .pages = pages, .partial = partial, - .flags = flags, + .flags = *flags, .ops = &page_cache_pipe_buf_ops, };
@@ -299,12 +300,17 @@ __generic_file_splice_read(struct file ** Lookup the (hopefully) full range of pages we need. */ spd.nr_pages =find_get_pages_contig (mapping, index, nr_pages, pages); + first_hole = spd.nr_pages; + index += spd.nr_pages;/* * If find_get_pages_contig() returned fewer pages than we needed, -* allocate the rest. +* readahead/allocate the rest. */ - index += spd.nr_pages; +if (spd.nr_pages < nr_pages) +page_cache_readahead_ondemand (mapping, &in->f_ra, in, +NULL , index, nr_pages - spd.nr_pages); +while (spd.nr_pages < nr_pages) {/* * Page could be there, find_get_pages_contig() breaks on
@@ -312,9 +318,6 @@ __generic_file_splice_read(struct file **/ page =find_get_page (mapping, index);if (!page) { -page_cache_readahead_ondemand (mapping, &in->f_ra, in, -NULL , index, nr_pages - spd.nr_pages); -/* * page didn't exist, allocate one. */
@@ -369,13 +372,13 @@ __generic_file_splice_read(struct file **/ if (!PageUptodate (page)) {/* -* If in nonblock mode then dont block on waiting -* for an in-flight io page +* In non-block mode, only block at the second try. */ -if (flags &SPLICE_F_NONBLOCK ) { -if (TestSetPageLocked (page)) -break ; - }else +if ((*flags &SPLICE_F_NONBLOCK ) && +TestSetPageLocked (page) && + in->f_ra.prev_index != index) +break ; +else lock_page (page);/*
@@ -454,6 +457,10 @@ fill_it:page_cache_release (pages[page_nr++]); in->f_ra.prev_index = index; +if ((*flags &SPLICE_F_NONBLOCK ) && (spd.nr_pages > first_hole)) { + *flags |=SPLICE_INTERNAL_WILLBLOCK ; + spd.nr_pages = first_hole; + }if (spd.nr_pages)return splice_to_pipe (pipe, &spd);
@@ -480,7 +487,7 @@ ssize_t generic_file_splice_read(struct spliced =0 ;while (len) { - ret =__generic_file_splice_read (in, ppos, pipe, len, flags); + ret =__generic_file_splice_read (in, ppos, pipe, len, &flags);if (ret <0 )break ;
@@ -496,6 +503,9 @@ ssize_t generic_file_splice_read(struct *ppos += ret; len -= ret; spliced += ret; + +if (flags &SPLICE_INTERNAL_WILLBLOCK ) +break ; }if (spliced)
include/linux/pipe_fs_i.h +2 -0
@@ -82,6 +82,8 @@ int generic_pipe_buf_steal(struct pipe_i#define SPLICE_F_MORE (0x04)/* expect more data */ #define SPLICE_F_GIFT (0x08)/* pages passed in are a gift */ +#define SPLICE_INTERNAL_WILLBLOCK (0x100)/* read on next page will block */ +/* * Passed to the actors */
A message that is not in the archive (sent off-list or not yet received): [email protected]
Quoted 9 lines · Fengguang Wu
In non-block splicing, return EAGAIN once for all possible I/O waits.
It works by checking (ra.prev_index != index) in
__generic_file_splice_read(). Another reader on the same fd will at worst cause a few more splice syscalls.If keep calling
generic_file_splice_read()in non-block mode, it will return partial data before the first hole; then return EAGAIN at second call; at last it will block on the I/O page.
Here is a better one, comments are welcome.
---
In non-block splicing read, return EAGAIN once for all possible I/O waits.
It avoids busy waiting by checking (ra.prev_index != index) in __generic_file_splice_read(). Possible other readers on the same fd may temporarily cheat the logic, but will at worst cause a few more splice syscalls.
If keep calling generic_file_splice_read() in non-blocking mode, it will return partial data before the first wait point; then return EAGAIN at second call; at last it will block on the I/O page. This behavior should be compatible with old apps that are unaware of EAGAIN.
--- fs/splice.c | 33 ++++++++++++++++++--------------- include/linux/pipe_fs_i.h | 2 ++ 2 files changed, 20 insertions(+), 15 deletions(-)
fs/splice.c +18 -15
@@ -264,7 +264,7 @@ static ssize_t splice_to_pipe(struct pipstatic int __generic_file_splice_read (struct file *in,loff_t *ppos,struct pipe_inode_info *pipe,size_t len, -unsigned int flags) +unsigned int *flags) {struct address_space *mapping = in->f_mapping;unsigned int loff, nr_pages;
@@ -278,7 +278,7 @@ __generic_file_splice_read(struct file *struct splice_pipe_desc spd = { .pages = pages, .partial = partial, - .flags = flags, + .flags = *flags, .ops = &page_cache_pipe_buf_ops, };
@@ -299,12 +299,16 @@ __generic_file_splice_read(struct file ** Lookup the (hopefully) full range of pages we need. */ spd.nr_pages =find_get_pages_contig (mapping, index, nr_pages, pages); + index += spd.nr_pages;/* * If find_get_pages_contig() returned fewer pages than we needed, -* allocate the rest. +* readahead/allocate the rest. */ - index += spd.nr_pages; +if (spd.nr_pages < nr_pages) +page_cache_readahead_ondemand (mapping, &in->f_ra, in, +NULL , index, nr_pages - spd.nr_pages); +while (spd.nr_pages < nr_pages) {/* * Page could be there, find_get_pages_contig() breaks on
@@ -312,9 +316,6 @@ __generic_file_splice_read(struct file **/ page =find_get_page (mapping, index);if (!page) { -page_cache_readahead_ondemand (mapping, &in->f_ra, in, -NULL , index, nr_pages - spd.nr_pages); -/* * page didn't exist, allocate one. */
@@ -326,8 +327,6 @@ __generic_file_splice_read(struct file * GFP_KERNEL);if (unlikely (error)) {page_cache_release (page); -if (error == -EEXIST ) -continue ;break ; }/*
@@ -369,12 +368,13 @@ __generic_file_splice_read(struct file **/ if (!PageUptodate (page)) {/* -* If in nonblock mode then dont block on waiting -* for an in-flight io page +* In non-block mode, only block at the second try. */ -if (flags &SPLICE_F_NONBLOCK ) { -if (TestSetPageLocked (page)) -break ; +if ((*flags &SPLICE_F_NONBLOCK ) && +TestSetPageLocked (page) && + in->f_ra.prev_index != index) { + *flags |=SPLICE_INTERNAL_WILLBLOCK ; +break ; }else lock_page (page);
@@ -480,7 +480,7 @@ ssize_t generic_file_splice_read(struct spliced =0 ;while (len) { - ret =__generic_file_splice_read (in, ppos, pipe, len, flags); + ret =__generic_file_splice_read (in, ppos, pipe, len, &flags);if (ret <0 )break ;
@@ -496,6 +496,9 @@ ssize_t generic_file_splice_read(struct *ppos += ret; len -= ret; spliced += ret; + +if (flags &SPLICE_INTERNAL_WILLBLOCK ) +break ; }if (spliced)
include/linux/pipe_fs_i.h +2 -0
@@ -82,6 +82,8 @@ int generic_pipe_buf_steal(struct pipe_i#define SPLICE_F_MORE (0x04)/* expect more data */ #define SPLICE_F_GIFT (0x08)/* pages passed in are a gift */ +#define SPLICE_INTERNAL_WILLBLOCK (0x100)/* read on next page will block */ +/* * Passed to the actors */
A message that is not in the archive (sent off-list or not yet received): [email protected]
Hi Jens,
This is another try, still not in a comfortable state though. //Busy waiting is possible for interleaved reads.
Fengguang
---
In non-block splicing read, return EAGAIN once for possible I/O waits.
It avoids busy waiting by checking (ra.prev_index != index) in __generic_file_splice_read(). If there are other readers on the same fd, busy waiting is still possible.
If keep calling generic_file_splice_read() in non-blocking mode, it will return partial data before the first wait point; then return EAGAIN at second call; at last it will block on the I/O page.
--- fs/splice.c | 41 +++++++++++++++++++++--------------- include/linux/pipe_fs_i.h | 2 + 2 files changed, 27 insertions(+), 16 deletions(-)
fs/splice.c +25 -16
@@ -264,7 +264,7 @@ static ssize_t splice_to_pipe(struct pipstatic int __generic_file_splice_read (struct file *in,loff_t *ppos,struct pipe_inode_info *pipe,size_t len, -unsigned int flags) +unsigned int *flags) {struct address_space *mapping = in->f_mapping;unsigned int loff, nr_pages;
@@ -278,7 +278,7 @@ __generic_file_splice_read(struct file *struct splice_pipe_desc spd = { .pages = pages, .partial = partial, - .flags = flags, + .flags = *flags, .ops = &page_cache_pipe_buf_ops, };
@@ -299,12 +299,16 @@ __generic_file_splice_read(struct file ** Lookup the (hopefully) full range of pages we need. */ spd.nr_pages =find_get_pages_contig (mapping, index, nr_pages, pages); + index += spd.nr_pages;/* * If find_get_pages_contig() returned fewer pages than we needed, -* allocate the rest. +* readahead/allocate the rest. */ - index += spd.nr_pages; +if (spd.nr_pages < nr_pages) +page_cache_readahead_ondemand (mapping, &in->f_ra, in, +NULL , index, nr_pages - spd.nr_pages); +while (spd.nr_pages < nr_pages) {/* * Page could be there, find_get_pages_contig() breaks on
@@ -312,9 +316,6 @@ __generic_file_splice_read(struct file **/ page =find_get_page (mapping, index);if (!page) { -page_cache_readahead_ondemand (mapping, &in->f_ra, in, -NULL , index, nr_pages - spd.nr_pages); -/* * page didn't exist, allocate one. */
@@ -326,8 +327,6 @@ __generic_file_splice_read(struct file * GFP_KERNEL);if (unlikely (error)) {page_cache_release (page); -if (error == -EEXIST ) -continue ;break ; }/*
@@ -369,12 +368,19 @@ __generic_file_splice_read(struct file **/ if (!PageUptodate (page)) {/* -* If in nonblock mode then dont block on waiting -* for an in-flight io page +* On retrying sequential reads in non-blocking mode: +* 1. (prev_index <= index - 2) => return partial data +* 2. (prev_index == index - 1) => return EAGAIN +* 3. (prev_index == index) => wait on I/O +* This works nicely for page aligned reads. */ -if (flags &SPLICE_F_NONBLOCK ) { -if (TestSetPageLocked (page)) -break ; +if ((*flags &SPLICE_F_NONBLOCK ) && +TestSetPageLocked (page) && + in->f_ra.prev_index != index) { +if (in->f_ra.prev_index != index -1 ) + index--; + *flags |=SPLICE_INTERNAL_WILLBLOCK ; +break ; }else lock_page (page);
@@ -452,7 +458,6 @@ fill_it:*/ while (page_nr < nr_pages)page_cache_release (pages[page_nr++]); - in->f_ra.prev_index = index;if (spd.nr_pages)return splice_to_pipe (pipe, &spd);
@@ -480,7 +485,7 @@ ssize_t generic_file_splice_read(struct spliced =0 ;while (len) { - ret =__generic_file_splice_read (in, ppos, pipe, len, flags); + ret =__generic_file_splice_read (in, ppos, pipe, len, &flags);if (ret <0 )break ;
@@ -496,8 +501,12 @@ ssize_t generic_file_splice_read(struct *ppos += ret; len -= ret; spliced += ret; + +if (flags &SPLICE_INTERNAL_WILLBLOCK ) +break ; } + in->f_ra.prev_index = (*ppos -1 ) >>PAGE_CACHE_SHIFT ;if (spliced)return spliced;
include/linux/pipe_fs_i.h +2 -0
@@ -82,6 +82,8 @@ int generic_pipe_buf_steal(struct pipe_i#define SPLICE_F_MORE (0x04)/* expect more data */ #define SPLICE_F_GIFT (0x08)/* pages passed in are a gift */ +#define SPLICE_INTERNAL_WILLBLOCK (0x100)/* read on next page will block */ +/* * Passed to the actors */
A message that is not in the archive (sent off-list or not yet received): [email protected]
Quoted 4 lines · Fengguang Wu
Hi Jens,
This is another try, still not in a comfortable state though. //Busy waiting is possible for interleaved reads.
A few random comments...
Adding an internal flag is fine, but please put it at the upper end of the spectrum. So, use (1 << 31) for that flag.
And please work on the #splice branch of the block repo, not -mm. There are quite a few things pending for inclusion in there, and I'm sure your patch as-is wont apply.
Signature · 1 line
Jens Axboe
Quoted 10 lines · Jens Axboe
Quoted 4 lines · Fengguang Wu
Hi Jens,
This is another try, still not in a comfortable state though. //Busy waiting is possible for interleaved reads.
A few random comments...
Adding an internal flag is fine, but please put it at the upper end of the spectrum. So, use (1 << 31) for that flag.
OK.
Quoted 3 lines
And please work on the #splice branch of the block repo, not -mm. There are quite a few things pending for inclusion in there, and I'm sure your patch as-is wont apply.
I'm afraid this patch cannot be moved over to your branch trivially.
The core of the algorithm reuses f_ra.prev_index to record the state. It is OK for the on-demand readahead in the -mm tree. But the current readahead code in 2.6.22-rc3 is sensible to the change. And it also does not reliably tell if readahead I/O has been submitted.
We can either try other ways of doing non-blocking I/O, or just wait until the merge of on-demand readahead?
The current patch should work perfect with single splice reader. In the case of multiple readers on the same fd, we might simply err on the side of I/O waiting, since busy EAGAIN looping is not acceptable.
Fengguang Wu
Quoted 11 lines · H. Peter Anvin
Quoted 7 lines · Eric Dumazet
As I said, this new non blocking feature on the input side (disk), is nice and usefull. (For people scared by
splice()syscall :) )Just have to mention it is a change of behavior, and documentation probably needs to reflect this change. "Since linux 2.6.23,
sendfile()repectsO_NONBLOCKonin_fdas well"Fair enough. Unix has traditionally not acknowledged the possibility of nonblocking I/O on conventional files, for some odd reason.
That reminds me of this patch:
http://lkml.org/lkml/2004/10/1/217
which went in for a while but was reverted:
http://lkml.org/lkml/2004/10/17/17
Pádraig
Quoted 5 lines · Jens Axboe
Hi,
This patch removes the ->sendfile() hook from the
file_operationsstructure, and replaces thesys_sendfile()mechanism to be based on ->splice_read() instead. There should be no functional changes.
Forgot to mention, that the modified kernel of course passes ltp sendfile tests. FWIW.
Signature · 1 line
Jens Axboe
Quoted 4 lines · Jens Axboe
- nfds: The ->rq_sendfile_ok optimization is gone for now. I can't determine the value of it, but I'm assuming it's there for a reason. Any chance this can be converted to splice, or use something else than ->sendfile()? CC'ed Neil.
sendfile useage in nfsd avoids a data copy and allows to use checksum offloading. it's quite important for nfs server workloads.
Quoted 20 lines
Apart from that, it was mostly straight forward. Almost everybody uses
generic_file_sendfile(), which makes the conversion easy. I changed loop to usedo_generic_file_read()instead of sendfile, it works for me...drivers/block/loop.c +6
-1@@ -435,16 +435,24 @@ do_lo_receive(struct loop_device *lo, {struct lo_read_data cookie;struct file *file; -int retval; +read_descriptor_t desc; + + desc.written =0 ; + desc.count = bvec->bv_len; + desc.arg.data = &cookie; + desc.error =0 ;
drivers/block/loop.c do_lo_receive(struct loop_device *lo, · line 451
Quoted 10 lines
drivers/block/loop.c +2
-3cookie.lo = lo; cookie.page = bvec->bv_page; cookie.offset = bvec->bv_offset; cookie.bsize = bsize; file = lo->lo_backing_file; - retval = file->f_op->sendfile (file, &pos, bvec->bv_len, - lo_read_actor, &cookie); -return (retval <0 )? retval:0 ; + +do_generic_file_read (file, &pos, &desc, lo_read_actor);
This change is wrong. loop or any existing user of ->sendfile absolutely needs to go through a file operations vector so that file-system specific actions such as locking are performed. This is required at least for the
clustered filesystems and XFS. The right way to implement this is
via do_splice_direct or something similar.
do_generic_file_read is only a library function for filesystem use and should never be called directly.
Quoted 8 lines · Christoph Hellwig
Quoted 4 lines · Jens Axboe
- nfds: The ->rq_sendfile_ok optimization is gone for now. I can't determine the value of it, but I'm assuming it's there for a reason. Any chance this can be converted to splice, or use something else than ->sendfile()? CC'ed Neil.
sendfile useage in nfsd avoids a data copy and allows to use checksum offloading. it's quite important for nfs server workloads.
OK, I hope Neil can provide some input on how to convert it. Of course I'm just fishing for Neil to actually do that work :-)
Quoted 37 lines
Quoted 20 lines
Apart from that, it was mostly straight forward. Almost everybody uses
generic_file_sendfile(), which makes the conversion easy. I changed loop to usedo_generic_file_read()instead of sendfile, it works for me...drivers/block/loop.c +6
-1@@ -435,16 +435,24 @@ do_lo_receive(struct loop_device *lo, {struct lo_read_data cookie;struct file *file; -int retval; +read_descriptor_t desc; + + desc.written =0 ; + desc.count = bvec->bv_len; + desc.arg.data = &cookie; + desc.error =0 ;drivers/block/loop.c do_lo_receive(struct loop_device *lo, · line 451
Quoted 10 lines
drivers/block/loop.c +2
-3cookie.lo = lo; cookie.page = bvec->bv_page; cookie.offset = bvec->bv_offset; cookie.bsize = bsize; file = lo->lo_backing_file; - retval = file->f_op->sendfile (file, &pos, bvec->bv_len, - lo_read_actor, &cookie); -return (retval <0 )? retval:0 ; + +do_generic_file_read (file, &pos, &desc, lo_read_actor);This change is wrong. loop or any existing user of ->sendfile absolutely needs to go through a file operations vector so that file-system specific actions such as locking are performed. This is required at least for the
clustered filesystems and XFS. The right way to implement this is
viado_splice_director something similar.
Quoted 2 lines
do_generic_file_readis only a library function for filesystem use and should never be called directly.
I'll convert it to do_splice_direct(), thanks.
Signature · 1 line
Jens Axboe
Quoted 7 lines · On Thursday May 31, [email protected] wrote:
Quoted 6 lines · Christoph Hellwig
Quoted 4 lines · Jens Axboe
- nfds: The ->rq_sendfile_ok optimization is gone for now. I can't determine the value of it, but I'm assuming it's there for a reason. Any chance this can be converted to splice, or use something else than ->sendfile()? CC'ed Neil.
Quoted 5 lines
Quoted 2 lines
sendfile useage in nfsd avoids a data copy and allows to use checksum offloading. it's quite important for nfs server workloads.
OK, I hope Neil can provide some input on how to convert it. Of course I'm just fishing for Neil to actually do that work :-)
Well, seeing you did that zero-length-barrier thing for me, how can I complain :-)
I'll have a look in the morning.
NeilBrown
Quoted 11 lines · Neil Brown
Quoted 10 lines · On Thursday May 31, [email protected] wrote:
Quoted 8 lines · Christoph Hellwig
Quoted 4 lines · Jens Axboe
- nfds: The ->rq_sendfile_ok optimization is gone for now. I can't determine the value of it, but I'm assuming it's there for a reason. Any chance this can be converted to splice, or use something else than ->sendfile()? CC'ed Neil.
sendfile useage in nfsd avoids a data copy and allows to use checksum offloading. it's quite important for nfs server workloads.
Quoted 7 lines
Quoted 2 lines
OK, I hope Neil can provide some input on how to convert it. Of course I'm just fishing for Neil to actually do that work :-)
Well, seeing you did that zero-length-barrier thing for me, how can I complain :-)
I'll have a look in the morning.
Woo, thanks Neil :-)
Signature · 1 line
Jens Axboe
Re: [PATCH] sendfile removal (nfsd update)
Ok, here is a patch that makes nfsd use splice instead of sendfile. It appears to both compile and work.
Some observations:
- __splice_from_pipe wants a "struct file*" and I wanted to pass a "struct svcrqst *". Maybe it should take a void * ?
- It also wants a *ppos which I had no use for.. It that really
need? Cannot &file->f_pos be used?
- I copied do_splice_to from splice.c as it wasn't exported, and then found I couldn't compile because rw_verify_area wasn't exported. As nfsd doesn't need that (we never export mandatory-locking files) I just remove it and some other cruft that I didn't need.... Not sure if that was the best approach.
- I needed to export alloc_pipe_info. Maybe there should be a get_current_pipe instead which does the alloc if needed.
- I would much rather have something like free_pipe_info exported than open code it in do_splice_read (which is based heavily on do_splice_direct).
NeilBrown
-------------------------------
Replace ->sendfile with ->splice_read
Apparently ->sendfile is going away, so change nfsd to use ->splice_read to get pages for a file.
### Diffstat output
./fs/nfsd/vfs.c | 125 ++++++++++++++++++++++++++++++++++++++++++++++++-------- ./fs/pipe.c | 1 2 files changed, 109 insertions(+), 17 deletions(-)
fs/nfsd/vfs.c +108 -17
@@ -23,7 +23,7 @@#include <linux/file.h> #include <linux/mount.h> #include <linux/major.h> -#include <linux/ext2_fs.h> +#include <linux/pipe_fs_i.h> #include <linux/proc_fs.h> #include <linux/stat.h> #include <linux/fcntl.h>
@@ -801,26 +801,32 @@ found: }/* -* Grab and keep cached pages assosiated with a file in the svc_rqst -* so that they can be passed to the netowork sendmsg/sendpage routines -* directrly. They will be released after the sending has completed. +* Grab and keep cached pages associated with a file in the svc_rqst +* so that they can be passed to the network sendmsg/sendpage routines +* directly. They will be released after the sending has completed. */ static int -nfsd_read_actor (read_descriptor_t *desc,struct page *page,unsigned long offset ,unsigned long size) +nfsd_splice_actor (struct pipe_inode_info *pipe,struct pipe_buffer *buf, +struct splice_desc *sd) { -unsigned long count = desc->count; -struct svc_rqst *rqstp = desc->arg.data; +struct svc_rqst *rqstp = (struct svc_rqst *)sd->file;struct page **pp = rqstp->rq_respages + rqstp->rq_resused; +struct page *page = buf->page; +size_t size; +int ret; + + ret = buf->ops->pin (pipe, buf); +if (unlikely (ret)) +return ret; -if (size > count) - size = count; + size = sd->len;if (rqstp->rq_res.page_len ==0 ) {get_page (page);put_page (*pp); *pp = page; rqstp->rq_resused++; - rqstp->rq_res.page_base = offset; + rqstp->rq_res.page_base = buf->offset; rqstp->rq_res.page_len = size; }else if (page != pp[-1 ]) {get_page (page);
@@ -832,11 +838,98 @@ nfsd_read_actor(read_descriptor_t *desc, }else rqstp->rq_res.page_len += size; - desc->count = count - size; - desc->written += size;return size; } +static long do_splice_to (struct file *in,loff_t *ppos, +struct pipe_inode_info *pipe,size_t len, +unsigned int flags) +{ +loff_t isize, left; + + isize =i_size_read (in->f_mapping->host); +if (unlikely (*ppos >= isize)) +return 0 ; + + left = isize - *ppos; +if (unlikely (left < len)) + len = left; + +return in->f_op->splice_read (in, ppos, pipe, len, flags); +} + +static int do_splice_read (struct file *in,loff_t *ppos,size_t count, +struct svc_rqst *rqstp) +{ +struct pipe_inode_info *pipe; +long ret, bytes; +int i; + + rqstp->rq_resused =1 ; + + pipe = current->splice_pipe; +if (unlikely (!pipe)) { + pipe =alloc_pipe_info (NULL ); +if (!pipe) +return -ENOMEM ; + + pipe->readers =1 ; + current->splice_pipe = pipe; + } + + ret =0 ; + bytes =0 ; + +while (count) { +loff_t unused =0 ; +size_t read_len, max_read_len; + + max_read_len =min (count, (size_t )(PIPE_BUFFERS *PAGE_SIZE )); + + ret =do_splice_to (in, ppos, pipe, max_read_len,0 ); +if (unlikely (ret <0 )) + goto out_release; + + read_len = ret; + + ret =__splice_from_pipe (pipe, (struct file *)rqstp, + &unused, read_len,0 , + nfsd_splice_actor); +if (unlikely (ret <0 )) + goto out_release; + + bytes += ret; + count -= ret; + } + + pipe->nrbufs = pipe->curbuf =0 ; + +return bytes; + + out_release: +/* +* If we did an incomplete transfer we must release +* the pipe buffers in question: +*/ +for (i =0 ; i <PIPE_BUFFERS ; i++) { +struct pipe_buffer *buf = pipe->bufs + i; + +if (buf->ops) { + buf->ops->release (pipe, buf); + buf->ops =NULL ; + } + } + pipe->nrbufs = pipe->curbuf =0 ; + +/* +* If we transferred some data, return the number of bytes: +*/ +if (bytes >0 ) +return bytes; + +return ret; +} +static __be32 nfsd_vfs_read (struct svc_rqst *rqstp,struct svc_fh *fhp,struct file *file,loff_t offset,struct kvec *vec,int vlen,unsigned long *count)
@@ -861,11 +954,9 @@ nfsd_vfs_read(struct svc_rqst *rqstp, stif (ra && ra->p_set) file->f_ra = ra->p_ra; -if (file->f_op->sendfile && rqstp->rq_sendfile_ok) { - rqstp->rq_resused =1 ; - host_err = file->f_op->sendfile (file, &offset, *count, - nfsd_read_actor, rqstp); - }else { +if (file->f_op->splice_read && rqstp->rq_sendfile_ok) + host_err =do_splice_read (file, &offset, *count, rqstp); +else { oldfs =get_fs ();set_fs (KERNEL_DS ); host_err =vfs_readv (file, (struct iovec __user *)vec, vlen, &offset);
fs/pipe.c +1 -0
@@ -865,6 +865,7 @@ struct pipe_inode_info * alloc_pipe_inforeturn pipe; } +EXPORT_SYMBOL (alloc_pipe_info);void __free_pipe_info (struct pipe_inode_info *pipe) {
Re: [PATCH] sendfile removal (nfsd update)
Quoted 9 lines · Neil Brown
Ok, here is a patch that makes nfsd use splice instead of sendfile. It appears to both compile and work.
Some observations:
-__splice_from_pipewants a "struct file*" and I wanted to pass a "struct svcrqst *". Maybe it should take a void * ?
- It also wants a *ppos which I had no use for.. It that reallyneed? Cannot &file->f_pos be used?
See:
I'll rebase the conversion on top of this one, loop will need the same change.
Quoted 10 lines
- I copied
do_splice_tofromsplice.cas it wasn't exported, and then found I couldn't compile becauserw_verify_areawasn't exported. As nfsd doesn't need that (we never export mandatory-locking files) I just remove it and some other cruft that I didn't need.... Not sure if that was the best approach.
- I needed to exportalloc_pipe_info. Maybe there should be aget_current_pipeinstead which does the alloc if needed.
- I would much rather have something likefree_pipe_infoexported than open code it indo_splice_read(which is based heavily ondo_splice_direct).
I need the same thing for loop. I think I'll add do_splice_foo() to fs/splice.c in a way that loop can use instead of do_splice_direct(), then nfsd should be able to do the same.
Thanks for this Neil!
Signature · 1 line
Jens Axboe
Re: [PATCH] sendfile removal (nfsd update)
Quoted 29 lines · Jens Axboe
Quoted 9 lines · Neil Brown
Ok, here is a patch that makes nfsd use splice instead of sendfile. It appears to both compile and work.
Some observations:
-__splice_from_pipewants a "struct file*" and I wanted to pass a "struct svcrqst *". Maybe it should take a void * ?
- It also wants a *ppos which I had no use for.. It that reallyneed? Cannot &file->f_pos be used?See:
I'll rebase the conversion on top of this one, loop will need the same change.
Quoted 10 lines
- I copied
do_splice_tofromsplice.cas it wasn't exported, and then found I couldn't compile becauserw_verify_areawasn't exported. As nfsd doesn't need that (we never export mandatory-locking files) I just remove it and some other cruft that I didn't need.... Not sure if that was the best approach.
- I needed to exportalloc_pipe_info. Maybe there should be aget_current_pipeinstead which does the alloc if needed.
- I would much rather have something likefree_pipe_infoexported than open code it indo_splice_read(which is based heavily ondo_splice_direct).
Quoted 3 lines
I need the same thing for loop. I think I'll add
do_splice_foo()tofs/splice.cin a way that loop can use instead ofdo_splice_direct(), then nfsd should be able to do the same.
I rebased and added a splice_direct_to_actor() helper that nfsd can then use. This makes the nfsd diff look like the below, which I think you'll agree is a lot more pleasant. You can view the tree here:
http://git.kernel.dk/?p=linux-2.6-block.git;a=shortlog;h=splice
The vmsplice patch probably needs to be split in two, for now it is the one introducing the framework that nfsd (and next, loop) can use.
fs/nfsd/vfs.c +31 -16
@@ -23,7 +23,7 @@#include <linux/file.h> #include <linux/mount.h> #include <linux/major.h> -#include <linux/ext2_fs.h> +#include <linux/pipe_fs_i.h> #include <linux/proc_fs.h> #include <linux/stat.h> #include <linux/fcntl.h>
@@ -801,26 +801,32 @@ found: }/* -* Grab and keep cached pages assosiated with a file in the svc_rqst -* so that they can be passed to the netowork sendmsg/sendpage routines -* directrly. They will be released after the sending has completed. +* Grab and keep cached pages associated with a file in the svc_rqst +* so that they can be passed to the network sendmsg/sendpage routines +* directly. They will be released after the sending has completed. */ static int -nfsd_read_actor (read_descriptor_t *desc,struct page *page,unsigned long offset ,unsigned long size) +nfsd_splice_actor (struct pipe_inode_info *pipe,struct pipe_buffer *buf, +struct splice_desc *sd) { -unsigned long count = desc->count; -struct svc_rqst *rqstp = desc->arg.data; +struct svc_rqst *rqstp = sd->data;struct page **pp = rqstp->rq_respages + rqstp->rq_resused; +struct page *page = buf->page; +size_t size; +int ret; + + ret = buf->ops->pin (pipe, buf); +if (unlikely (ret)) +return ret; -if (size > count) - size = count; + size = sd->len;if (rqstp->rq_res.page_len ==0 ) {get_page (page);put_page (*pp); *pp = page; rqstp->rq_resused++; - rqstp->rq_res.page_base = offset; + rqstp->rq_res.page_base = buf->offset; rqstp->rq_res.page_len = size; }else if (page != pp[-1 ]) {get_page (page);
@@ -832,11 +838,15 @@ nfsd_read_actor(read_descriptor_t *desc, struct page *page, unsigned long offset }else rqstp->rq_res.page_len += size; - desc->count = count - size; - desc->written += size;return size; } +static int nfsd_direct_splice_actor (struct pipe_inode_info *pipe, +struct splice_desc *sd) +{ +return __splice_from_pipe (pipe, sd, nfsd_splice_actor); +} +static __be32 nfsd_vfs_read (struct svc_rqst *rqstp,struct svc_fh *fhp,struct file *file,loff_t offset,struct kvec *vec,int vlen,unsigned long *count)
@@ -861,10 +871,15 @@ nfsd_vfs_read(struct svc_rqst *rqstp, struct svc_fh *fhp, struct file *file,if (ra && ra->p_set) file->f_ra = ra->p_ra; -if (file->f_op->sendfile && rqstp->rq_sendfile_ok) { - rqstp->rq_resused =1 ; - host_err = file->f_op->sendfile (file, &offset, *count, - nfsd_read_actor, rqstp); +if (file->f_op->splice_read && rqstp->rq_splice_ok) { +struct splice_desc sd = { + .len =0 , + .total_len = *count, + .pos = offset, + }; + + sd.data = rqstp; + host_err =splice_direct_to_actor (file, &sd, nfsd_direct_splice_actor); }else { oldfs =get_fs ();set_fs (KERNEL_DS );
Signature · 1 line
Jens Axboe
Quoted 10 lines · Jens Axboe
Quoted 8 lines
This change is wrong. loop or any existing user of ->sendfile absolutely needs to go through a file operations vector so that file-system specific actions such as locking are performed. This is required at least for the
clustered filesystems and XFS. The right way to implement this is
viado_splice_director something similar.
do_generic_file_readis only a library function for filesystem use and should never be called directly.I'll convert it to
do_splice_direct(), thanks.
So how does this look? Totally untested, will do that now. It only covers the read side, the write conversion to splice will happen later.
drivers/block/loop.c +38 -25
@@ -74,6 +74,7 @@#include <linux/highmem.h> #include <linux/gfp.h> #include <linux/kthread.h> +#include <linux/pipe_fs_i.h> #include <asm/uaccess.h>
@@ -401,58 +402,70 @@ struct lo_read_data { };static int -lo_read_actor (read_descriptor_t *desc,struct page *page, -unsigned long offset,unsigned long size) +lo_splice_actor (struct pipe_inode_info *pipe,struct pipe_buffer *buf, +struct splice_desc *sd) { -unsigned long count = desc->count; -struct lo_read_data *p = desc->arg.data; +struct lo_read_data *p = sd->data;struct loop_device *lo = p->lo; +struct page *page = buf->page;sector_t IV ; +size_t size; +int ret; -IV = ((sector_t ) page->index << (PAGE_CACHE_SHIFT -9 ))+(offset >>9 ); + ret = buf->ops->pin (pipe, buf); +if (unlikely (ret)) +return ret; -if (size > count) - size = count; +IV = ((sector_t ) page->index << (PAGE_CACHE_SHIFT -9 ))+(buf->offset >>9 ); + size = sd->len; -if (lo_do_transfer (lo,READ , page, offset, p->page, p->offset, size,IV )) { +if (lo_do_transfer (lo,READ , page, buf->offset, p->page, p->offset, size,IV )) { size =0 ;printk (KERN_ERR "loop: transfer error block %ld\n" , page->index); - desc->error = -EINVAL ; + size = -EINVAL ; }flush_dcache_page (p->page); - desc->count = count - size; - desc->written += size; - p->offset += size; +if (size >0 ) + p->offset += size;return size; }static int +lo_direct_splice_actor (struct pipe_inode_info *pipe,struct splice_desc *sd) +{ +return __splice_from_pipe (pipe, sd, lo_splice_actor); +} + +static int do_lo_receive (struct loop_device *lo,struct bio_vec *bvec,int bsize,loff_t pos) {struct lo_read_data cookie; +struct splice_desc sd;struct file *file; -read_descriptor_t desc; - - desc.written =0 ; - desc.count = bvec->bv_len; - desc.arg.data = &cookie; - desc.error =0 ; +int retval; cookie.lo = lo; cookie.page = bvec->bv_page; cookie.offset = bvec->bv_offset; cookie.bsize = bsize; + + sd.len =0 ; + sd.total_len = bsize; + sd.flags =0 ; + sd.pos = pos; + sd.data = &cookie; + file = lo->lo_backing_file; + retval =splice_direct_to_actor (file, &sd, lo_direct_splice_actor); -do_generic_file_read (file, &pos, &desc, lo_read_actor); -if (desc.written) -return 0 ; +if (retval <0 ) +return retval; -return desc.error; +return 0 ; } static int
@@ -687,8 +700,8 @@ static int loop_change_fd(struct loop_device *lo, struct file *lo_file,if (!S_ISREG (inode->i_mode) && !S_ISBLK (inode->i_mode)) goto out_putf; -/* new backing store needs to support loop (eg readpage) */ -if (!file->f_mapping->a_ops->readpage) +/* new backing store needs to support loop (eg splice_read) */ +if (!inode->i_fop->splice_read) goto out_putf;/* size of the new backing store needs to be the same */
@@ -768,7 +781,7 @@ static int loop_set_fd(struct loop_device *lo, struct file *lo_file,* If we can't read - sorry. If we only can't write - well, * it's going to be read-only. */ -if (!aops->readpage) +if (!file->f_op->splice_read) goto out_putf;if (aops->prepare_write && aops->commit_write) lo_flags |=LO_FLAGS_USE_AOPS ;
Signature · 1 line
Jens Axboe
Quoted 5 lines · Jens Axboe
Work to be done:
- The ext2 xip support needs a
splice_read()implementation, currently I just if 0'ed out the sendxip_file_sendfile(). CC'ed Carsten, who seems to be the author of this code.
Yup. Will do the splice_read implementation for xip. Do you want to see it in splice.c or should it go to filemap_xip?
Quoted 8 lines · Carsten Otte
Quoted 5 lines · Jens Axboe
Work to be done:
- The ext2 xip support needs a
splice_read()implementation, currently I just if 0'ed out the sendxip_file_sendfile(). CC'ed Carsten, who seems to be the author of this code.Yup. Will do the
splice_readimplementation for xip. Do you want to see it insplice.cor should it go tofilemap_xip?
Just put it in filemap_xip.c, I think that would be the best place. Thanks!
Quoted 1 line
Acked-by Carsten Otte <[email protected]>
Signature · 1 line
Jens Axboe
Quoted 14 lines · Jens Axboe
Hi,
This patch removes the ->sendfile() hook from the
file_operationsstructure, and replaces thesys_sendfile()mechanism to be based on ->splice_read() instead. There should be no functional changes.Work to be done:
- The ext2 xip support needs a
splice_read()implementation, currently I just if 0'ed out the sendxip_file_sendfile(). CC'ed Carsten, who seems to be the author of this code.- shmem needs a
splice_read()implementation. Optimistically CC'ed Hugh.
Quoted 8 lines
- nfds: The ->rq_sendfile_ok optimization is gone for now. I can't determine the value of it, but I'm assuming it's there for a reason. Any chance this can be converted to splice, or use something else than ->sendfile()? CC'ed Neil.
- relay: needs a
splice_read()implementation. I think Tom already has one, CC'ed him.
Hi Jens,
Yeah, I have it lying around somewhere - the patch I sent you awhile back of an initial version of splice_read() is the last work I did on this. Basically, it worked OK but IIRC there were a couple things in the code that needed fixing. I also modified blktrace to use splice so I could use it for testing; I'll send a patch for that too once I get it working again.
Anyway, that was ages ago (sometime last year ;-) so I'm going to have to dig up the code and probably rework it a bit.
Tom
Quoted 33 lines · Tom Zanussi
Quoted 14 lines · Jens Axboe
Hi,
This patch removes the ->sendfile() hook from the
file_operationsstructure, and replaces thesys_sendfile()mechanism to be based on ->splice_read() instead. There should be no functional changes.Work to be done:
- The ext2 xip support needs a
splice_read()implementation, currently I just if 0'ed out the sendxip_file_sendfile(). CC'ed Carsten, who seems to be the author of this code.- shmem needs a
splice_read()implementation. Optimistically CC'ed Hugh.Quoted 8 lines
- nfds: The ->rq_sendfile_ok optimization is gone for now. I can't determine the value of it, but I'm assuming it's there for a reason. Any chance this can be converted to splice, or use something else than ->sendfile()? CC'ed Neil.
- relay: needs a
splice_read()implementation. I think Tom already has one, CC'ed him.Hi Jens,
Yeah, I have it lying around somewhere - the patch I sent you awhile back of an initial version of
splice_read()is the last work I did on this. Basically, it worked OK but IIRC there were a couple things in the code that needed fixing. I also modified blktrace to use splice so I could use it for testing; I'll send a patch for that too once I get it working again.
Quoted 2 lines
Anyway, that was ages ago (sometime last year ;-) so I'm going to have to dig up the code and probably rework it a bit.
It'd be great if you can polish it a bit (basically just make it apply to the current kernel). I don't think it's hard, aside from moving the code from fs/relayfs/ to kernel/relay.c, there hasn't been many changes.
Signature · 1 line
Jens Axboe
Quoted 5 lines · Jens Axboe
This patch removes the ->sendfile() hook from the
file_operationsstructure, and replaces thesys_sendfile()mechanism to be based on ->splice_read() instead. There should be no functional changes.
Quoted 7 lines
Work to be done:
- The ext2 xip support needs a
splice_read()implementation, currently I just if 0'ed out the sendxip_file_sendfile(). CC'ed Carsten, who seems to be the author of this code.- shmem needs a
splice_read()implementation. Optimistically CC'ed Hugh.
I'll take that "Optimistically" as a special compliment, rather than as a particular insult ;)
Yes, thanks, please leave shmem_file_splice_read() to me, I'll give it priority now. Not deep enough in yet, but I'll probably aim for something simple-minded (correct but slow once it hits swap).
Quoted 6 lines
- nfds: The ->rq_sendfile_ok optimization is gone for now. I can't determine the value of it, but I'm assuming it's there for a reason. Any chance this can be converted to splice, or use something else than ->sendfile()? CC'ed Neil.
Quoted 6 lines
- relay: needs a
splice_read()implementation. I think Tom already has one, CC'ed him.Apart from that, it was mostly straight forward. Almost everybody uses
generic_file_sendfile(), which makes the conversion easy. I changed loop to usedo_generic_file_read()instead of sendfile, it works for me...
Christoph already picked up on that, and it's of interest to shmem too:
loop over tmpfs in 2.6 was relying on shmem_file_sendfile, for which the generic route is not good enough.
If we're giving a .splice_read to everything which used to have a .sendfile, then I think you just need to make do_lo_read() use ->splice_read now?
Hugh
Quoted 7 lines · Hugh Dickins
Christoph already picked up on that, and it's of interest to shmem too:
loop over tmpfs in 2.6 was relying onshmem_file_sendfile, for which the generic route is not good enough.If we're giving a .splice_read to everything which used to have a .sendfile, then I think you just need to make
do_lo_read()use ->splice_read now?
I'd be perfectly happy with killing support for using ->read. Any simple filesystem can use the generic methods, and any more complex filesystem should better have splice aswell. Btw, someone should look into using splice for the loop write path aswell, the current useage of ->prepare_write and ->commit_write has similar locking problems aswell.
Quoted 8 lines · Hugh Dickins
Quoted 7 lines · Jens Axboe
This patch removes the ->sendfile() hook from the
file_operationsstructure, and replaces thesys_sendfile()mechanism to be based on ->splice_read() instead. There should be no functional changes.Work to be done:
Quoted 8 lines
Quoted 5 lines
- The ext2 xip support needs a
splice_read()implementation, currently I just if 0'ed out the sendxip_file_sendfile(). CC'ed Carsten, who seems to be the author of this code.- shmem needs a
splice_read()implementation. Optimistically CC'ed Hugh.I'll take that "Optimistically" as a special compliment, rather than as a particular insult ;)
It's a compliment for sure, the "optimistically" was just mentioned because there are lots of people mentioned in that file, but I mostly remember you doing some heavy lifting there :-)
Quoted 3 lines
Yes, thanks, please leave
shmem_file_splice_read()to me, I'll give it priority now. Not deep enough in yet, but I'll probably aim for something simple-minded (correct but slow once it hits swap).
Super, thanks!!
Quoted 12 lines
Quoted 11 lines
- nfds: The ->rq_sendfile_ok optimization is gone for now. I can't determine the value of it, but I'm assuming it's there for a reason. Any chance this can be converted to splice, or use something else than ->sendfile()? CC'ed Neil.
- relay: needs a
splice_read()implementation. I think Tom already has one, CC'ed him.Apart from that, it was mostly straight forward. Almost everybody uses
generic_file_sendfile(), which makes the conversion easy. I changed loop to usedo_generic_file_read()instead of sendfile, it works for me...
Quoted 7 lines
Christoph already picked up on that, and it's of interest to shmem too:
loop over tmpfs in 2.6 was relying onshmem_file_sendfile, for which the generic route is not good enough.If we're giving a .splice_read to everything which used to have a .sendfile, then I think you just need to make
do_lo_read()use ->splice_read now?
Yes, I will make that change myself. I can do that, as I seem to have sucessfully pushed out the remaining work to others :-)
Signature · 1 line
Jens Axboe