Re: [PATCH] splice: fix wrong __splice_from_pipe() usage
On Mon, Jul 16 2007, OGAWA Hirofumi wrote: > Jens Axboe <[EMAIL PROTECTED]> writes: > > >> nfsd_vfs_read() path. > >> > >> nfsd_vfs_read() > >> splice_direct_to_actor() > >> while(len) { > >> do_splice_to() [update sd->pos] > >> -> generic_file_splice_read() [read from sd->pos] > >> nfsd_direct_splice_actor() > >> -> __splice_from_pipe()[update sd->pos] > >> } > >> > >> do_splice_to() updates sd->pos for read, and pass updated sd to > >> __splice_from_pipe(), and __splice_from_pipe() updates sd->pos for > >> write. So, next do_splice_to() read from wrong position. > > > > Ah, I see. Copying the structure over would work, but it seems like a > > big work-around for something that is essentially bad behaviour in the > > splice core for direct splicing. > > > > Can you check if this works for you? > > Simple test was done. The patch works for me. Great, thanks a lot for retesting. I'll get the patch upstream asap. -- Jens Axboe - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] splice: fix wrong __splice_from_pipe() usage
Jens Axboe <[EMAIL PROTECTED]> writes: >> nfsd_vfs_read() path. >> >> nfsd_vfs_read() >> splice_direct_to_actor() >> while(len) { >> do_splice_to() [update sd->pos] >> -> generic_file_splice_read() [read from sd->pos] >> nfsd_direct_splice_actor() >> -> __splice_from_pipe()[update sd->pos] >> } >> >> do_splice_to() updates sd->pos for read, and pass updated sd to >> __splice_from_pipe(), and __splice_from_pipe() updates sd->pos for >> write. So, next do_splice_to() read from wrong position. > > Ah, I see. Copying the structure over would work, but it seems like a > big work-around for something that is essentially bad behaviour in the > splice core for direct splicing. > > Can you check if this works for you? Simple test was done. The patch works for me. Thanks. -- OGAWA Hirofumi <[EMAIL PROTECTED]> - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] splice: fix wrong __splice_from_pipe() usage
On Mon, Jul 16 2007, OGAWA Hirofumi wrote: > Jens Axboe <[EMAIL PROTECTED]> writes: > > > On Mon, Jul 16 2007, OGAWA Hirofumi wrote: > >> Hi, > >> > >> I've noticed the nfsd read corruption by recent change. And this patch > >> fixes the problem for me, is this right fix? > >> -- > >> OGAWA Hirofumi <[EMAIL PROTECTED]> > >> > >> > >> __splice_from_pipe() is updating the sd->pos for the actor, but those > >> functions are passing the sd of reader side directory. So, splice > >> updates sd->pos twice. > >> > >> This fixes usage of __splice_from_pipe(). > > > > For sendfile() usage, or the nfsd path that uses splice to send? > > nfsd_vfs_read() path. > > nfsd_vfs_read() > splice_direct_to_actor() > while(len) { > do_splice_to() [update sd->pos] > -> generic_file_splice_read() [read from sd->pos] > nfsd_direct_splice_actor() > -> __splice_from_pipe()[update sd->pos] > } > > do_splice_to() updates sd->pos for read, and pass updated sd to > __splice_from_pipe(), and __splice_from_pipe() updates sd->pos for > write. So, next do_splice_to() read from wrong position. Ah, I see. Copying the structure over would work, but it seems like a big work-around for something that is essentially bad behaviour in the splice core for direct splicing. Can you check if this works for you? diff --git a/fs/splice.c b/fs/splice.c index 6c98286..5e5071a 100644 --- a/fs/splice.c +++ b/fs/splice.c @@ -1061,8 +1061,9 @@ ssize_t splice_direct_to_actor(struct file *in, struct splice_desc *sd, while (len) { size_t read_len; + loff_t pos = sd->pos; - ret = do_splice_to(in, &sd->pos, pipe, len, flags); + ret = do_splice_to(in, &pos, pipe, len, flags); if (unlikely(ret <= 0)) goto out_release; @@ -1080,6 +1081,7 @@ ssize_t splice_direct_to_actor(struct file *in, struct splice_desc *sd, bytes += ret; len -= ret; + sd->pos = pos; if (ret < read_len) goto out_release; -- Jens Axboe - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] splice: fix wrong __splice_from_pipe() usage
Jens Axboe <[EMAIL PROTECTED]> writes: > On Mon, Jul 16 2007, OGAWA Hirofumi wrote: >> Hi, >> >> I've noticed the nfsd read corruption by recent change. And this patch >> fixes the problem for me, is this right fix? >> -- >> OGAWA Hirofumi <[EMAIL PROTECTED]> >> >> >> __splice_from_pipe() is updating the sd->pos for the actor, but those >> functions are passing the sd of reader side directory. So, splice >> updates sd->pos twice. >> >> This fixes usage of __splice_from_pipe(). > > For sendfile() usage, or the nfsd path that uses splice to send? nfsd_vfs_read() path. nfsd_vfs_read() splice_direct_to_actor() while(len) { do_splice_to() [update sd->pos] -> generic_file_splice_read() [read from sd->pos] nfsd_direct_splice_actor() -> __splice_from_pipe()[update sd->pos] } do_splice_to() updates sd->pos for read, and pass updated sd to __splice_from_pipe(), and __splice_from_pipe() updates sd->pos for write. So, next do_splice_to() read from wrong position. -- OGAWA Hirofumi <[EMAIL PROTECTED]> - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] splice: fix wrong __splice_from_pipe() usage
On Mon, Jul 16 2007, OGAWA Hirofumi wrote: > Hi, > > I've noticed the nfsd read corruption by recent change. And this patch > fixes the problem for me, is this right fix? > -- > OGAWA Hirofumi <[EMAIL PROTECTED]> > > > __splice_from_pipe() is updating the sd->pos for the actor, but those > functions are passing the sd of reader side directory. So, splice > updates sd->pos twice. > > This fixes usage of __splice_from_pipe(). For sendfile() usage, or the nfsd path that uses splice to send? -- Jens Axboe - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
[PATCH] splice: fix wrong __splice_from_pipe() usage
Hi, I've noticed the nfsd read corruption by recent change. And this patch fixes the problem for me, is this right fix? -- OGAWA Hirofumi <[EMAIL PROTECTED]> __splice_from_pipe() is updating the sd->pos for the actor, but those functions are passing the sd of reader side directory. So, splice updates sd->pos twice. This fixes usage of __splice_from_pipe(). Signed-off-by: OGAWA Hirofumi <[EMAIL PROTECTED]> --- drivers/block/loop.c |5 +++-- fs/nfsd/vfs.c|5 +++-- 2 files changed, 6 insertions(+), 4 deletions(-) diff -puN fs/nfsd/vfs.c~splice-fix-__splice_from_pipe-usage fs/nfsd/vfs.c --- linux-2.6/fs/nfsd/vfs.c~splice-fix-__splice_from_pipe-usage 2007-07-16 09:30:10.0 +0900 +++ linux-2.6-hirofumi/fs/nfsd/vfs.c2007-07-16 09:30:10.0 +0900 @@ -842,9 +842,10 @@ nfsd_splice_actor(struct pipe_inode_info } static int nfsd_direct_splice_actor(struct pipe_inode_info *pipe, - struct splice_desc *sd) + struct splice_desc *__sd) { - return __splice_from_pipe(pipe, sd, nfsd_splice_actor); + struct splice_desc sd = *__sd; + return __splice_from_pipe(pipe, &sd, nfsd_splice_actor); } static __be32 diff -puN drivers/block/loop.c~splice-fix-__splice_from_pipe-usage drivers/block/loop.c --- linux-2.6/drivers/block/loop.c~splice-fix-__splice_from_pipe-usage 2007-07-16 09:30:10.0 +0900 +++ linux-2.6-hirofumi/drivers/block/loop.c 2007-07-16 09:30:10.0 +0900 @@ -437,9 +437,10 @@ lo_splice_actor(struct pipe_inode_info * } static int -lo_direct_splice_actor(struct pipe_inode_info *pipe, struct splice_desc *sd) +lo_direct_splice_actor(struct pipe_inode_info *pipe, struct splice_desc *__sd) { - return __splice_from_pipe(pipe, sd, lo_splice_actor); + struct splice_desc sd = *__sd; + return __splice_from_pipe(pipe, &sd, lo_splice_actor); } static int _ - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/