[PATCH 2/3] splice: allow non-block splice only when in file is seekable

[PATCH 2/3] splice: allow non-block splice only when in file is seekable

[Changli Gao]

allow non-block splice only when in file is seekable.

do_splice_to() is split to two parts: read data from in file to spd, and move
data from spd to pipe. If there isn't much space in pipe for the data in spd,
and the splice is called with non-block flag, the data dropped due to non
enough space in pipe will be lost forever when in file isn't seekable.

Signed-off-by: Changli Gao <xiaosuo@gmail.com>
----
 fs/splice.c |    6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)
diff --git a/fs/splice.c b/fs/splice.c
index 9e52de5..8137e23 100644
--- a/fs/splice.c
+++ b/fs/splice.c
@@ -1346,8 +1346,12 @@ static long do_splice(struct file *in, loff_t __user *off_in,
 			if (copy_from_user(&offset, off_in, sizeof(loff_t)))
 				return -EFAULT;
 			off = &offset;
-		} else
+		} else {
+			if ((flags & SPLICE_F_NONBLOCK) &&
+			    !(in->f_mode & FMODE_PREAD))
+				return -EINVAL;
 			off = &in->f_pos;
+		}
 
 		ret = do_splice_to(in, off, opipe, len, flags);
 

Re: [PATCH 2/3] splice: allow non-block splice only when in file is seekable

[Miklos Szeredi]

On Wed, 26 May 2010, Changli Gao wrote:
> allow non-block splice only when in file is seekable.
> 
> do_splice_to() is split to two parts: read data from in file to spd, and move
> data from spd to pipe. If there isn't much space in pipe for the data in spd,
> and the splice is called with non-block flag, the data dropped due to non
> enough space in pipe will be lost forever when in file isn't seekable.

This is a gratuitous ABI change.  I don't think we should hold the
hand of application writers, maybe they _know_ the splice will not
block.

NACK from me.

Miklos

> 
> Signed-off-by: Changli Gao <xiaosuo@gmail.com>
> ----
>  fs/splice.c |    6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)
> diff --git a/fs/splice.c b/fs/splice.c
> index 9e52de5..8137e23 100644
> --- a/fs/splice.c
> +++ b/fs/splice.c
> @@ -1346,8 +1346,12 @@ static long do_splice(struct file *in, loff_t __user *off_in,
>  			if (copy_from_user(&offset, off_in, sizeof(loff_t)))
>  				return -EFAULT;
>  			off = &offset;
> -		} else
> +		} else {
> +			if ((flags & SPLICE_F_NONBLOCK) &&
> +			    !(in->f_mode & FMODE_PREAD))
> +				return -EINVAL;
>  			off = &in->f_pos;
> +		}
>  
>  		ret = do_splice_to(in, off, opipe, len, flags);
>  
> --
> To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

Re: [PATCH 2/3] splice: allow non-block splice only when in file is seekable

[Changli Gao]

On Wed, May 26, 2010 at 6:29 PM, Miklos Szeredi <miklos@szeredi.hu> wrote:
> On Wed, 26 May 2010, Changli Gao wrote:
>> allow non-block splice only when in file is seekable.
>>
>> do_splice_to() is split to two parts: read data from in file to spd, and move
>> data from spd to pipe. If there isn't much space in pipe for the data in spd,
>> and the splice is called with non-block flag, the data dropped due to non
>> enough space in pipe will be lost forever when in file isn't seekable.
>
> This is a gratuitous ABI change.  I don't think we should hold the
> hand of application writers,

Yea, it is an ABI change. But without that, we'll fail user silently
currently. Some fundamental change is needed to fix this issue fully,
if you think it is a bug.

> maybe they _know_ the splice will not
> block.
>

If they know the splice won't block, why he specifies a non-block flag?

-- 
Regards，
Changli Gao(xiaosuo@gmail.com)

Re: [PATCH 2/3] splice: allow non-block splice only when in file is seekable

[Miklos Szeredi]

On Wed, 26 May 2010, Changli Gao wrote:
> On Wed, May 26, 2010 at 6:29 PM, Miklos Szeredi <miklos@szeredi.hu> wrote:
> > On Wed, 26 May 2010, Changli Gao wrote:
> >> allow non-block splice only when in file is seekable.
> >>
> >> do_splice_to() is split to two parts: read data from in file to spd, and move
> >> data from spd to pipe. If there isn't much space in pipe for the data in spd,
> >> and the splice is called with non-block flag, the data dropped due to non
> >> enough space in pipe will be lost forever when in file isn't seekable.
> >
> > This is a gratuitous ABI change.  I don't think we should hold the
> > hand of application writers,
> 
> Yea, it is an ABI change. But without that, we'll fail user silently
> currently. Some fundamental change is needed to fix this issue fully,
> if you think it is a bug.

I don't think it's a bug.  The same thing will happen without
SPLICE_F_NONBLOCK if splice_to_pipe() is interrupted by a signal while
it blocks.

To prevent data loss the application needs to be aware of the fact
that doing a splice from a non-seekable fd needs to be done carefully,
making sure that it doesn't block on the pipe.

Miklos
--
To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html