[PATCH] vfs: Return EINVAL for default SEEK_HOLE, SEEK_DATA implementation

[PATCH] vfs: Return EINVAL for default SEEK_HOLE, SEEK_DATA implementation

[Jan Kara]

Generic implementation of SEEK_HOLE & SEEK_DATA in
generic_file_llseek_size() and default_llseek() behaved as if everything
within i_size is data and everything beyond i_size is a hole. That makes
sense at the first sight (and definitely is a valid implementation of
the spec) but at the second sight it isn't very useful. If anyone
bothers with looking for holes / data, he should be better told we don't
really know so that he can fall back to his chosen backup strategy
(think of e.g. cp(1)).

This is a userspace API change however the kernel interface is there
only for two years so hopefully userspace is still prepared to handle
EINVAL return value.

Signed-off-by: Jan Kara <jack@suse.cz>
---
 fs/read_write.c | 38 ++------------------------------------
 1 file changed, 2 insertions(+), 36 deletions(-)

  So given our discussion in
http://www.spinics.net/lists/linux-fsdevel/msg71363.html
  I've decided to throw this patch in. Al, what do you think about it?

diff --git a/fs/read_write.c b/fs/read_write.c
index 58e440df1bc6..40a324f1d93b 100644
--- a/fs/read_write.c
+++ b/fs/read_write.c
@@ -111,22 +111,8 @@ generic_file_llseek_size(struct file *file, loff_t offset, int whence,
 		spin_unlock(&file->f_lock);
 		return offset;
 	case SEEK_DATA:
-		/*
-		 * In the generic case the entire file is data, so as long as
-		 * offset isn't at the end of the file then the offset is data.
-		 */
-		if (offset >= eof)
-			return -ENXIO;
-		break;
 	case SEEK_HOLE:
-		/*
-		 * There is a virtual hole at the end of the file, so as long as
-		 * offset isn't i_size or larger, return i_size.
-		 */
-		if (offset >= eof)
-			return -ENXIO;
-		offset = eof;
-		break;
+		return -EINVAL;
 	}
 
 	return vfs_setpos(file, offset, maxsize);
@@ -214,28 +200,8 @@ loff_t default_llseek(struct file *file, loff_t offset, int whence)
 			offset += file->f_pos;
 			break;
 		case SEEK_DATA:
-			/*
-			 * In the generic case the entire file is data, so as
-			 * long as offset isn't at the end of the file then the
-			 * offset is data.
-			 */
-			if (offset >= inode->i_size) {
-				retval = -ENXIO;
-				goto out;
-			}
-			break;
 		case SEEK_HOLE:
-			/*
-			 * There is a virtual hole at the end of the file, so
-			 * as long as offset isn't i_size or larger, return
-			 * i_size.
-			 */
-			if (offset >= inode->i_size) {
-				retval = -ENXIO;
-				goto out;
-			}
-			offset = inode->i_size;
-			break;
+			return -EINVAL;
 	}
 	retval = -EINVAL;
 	if (offset >= 0 || unsigned_offsets(file)) {
-- 
1.8.1.4

Re: [PATCH] vfs: Return EINVAL for default SEEK_HOLE, SEEK_DATA implementation

[Andreas Dilger]

Shouldn't this be -EOPNOTSUPP, since -EINVAL is for incorrect arguments, which isn't the case here. 

Cheers, Andreas

> On Jan 3, 2014, at 3:35, Jan Kara <jack@suse.cz> wrote:
> 
> Generic implementation of SEEK_HOLE & SEEK_DATA in
> generic_file_llseek_size() and default_llseek() behaved as if everything
> within i_size is data and everything beyond i_size is a hole. That makes
> sense at the first sight (and definitely is a valid implementation of
> the spec) but at the second sight it isn't very useful. If anyone
> bothers with looking for holes / data, he should be better told we don't
> really know so that he can fall back to his chosen backup strategy
> (think of e.g. cp(1)).
> 
> This is a userspace API change however the kernel interface is there
> only for two years so hopefully userspace is still prepared to handle
> EINVAL return value.
> 
> Signed-off-by: Jan Kara <jack@suse.cz>
> ---
> fs/read_write.c | 38 ++------------------------------------
> 1 file changed, 2 insertions(+), 36 deletions(-)
> 
>  So given our discussion in
> http://www.spinics.net/lists/linux-fsdevel/msg71363.html
>  I've decided to throw this patch in. Al, what do you think about it?
> 
> diff --git a/fs/read_write.c b/fs/read_write.c
> index 58e440df1bc6..40a324f1d93b 100644
> --- a/fs/read_write.c
> +++ b/fs/read_write.c
> @@ -111,22 +111,8 @@ generic_file_llseek_size(struct file *file, loff_t offset, int whence,
>        spin_unlock(&file->f_lock);
>        return offset;
>    case SEEK_DATA:
> -        /*
> -         * In the generic case the entire file is data, so as long as
> -         * offset isn't at the end of the file then the offset is data.
> -         */
> -        if (offset >= eof)
> -            return -ENXIO;
> -        break;
>    case SEEK_HOLE:
> -        /*
> -         * There is a virtual hole at the end of the file, so as long as
> -         * offset isn't i_size or larger, return i_size.
> -         */
> -        if (offset >= eof)
> -            return -ENXIO;
> -        offset = eof;
> -        break;
> +        return -EINVAL;
>    }
> 
>    return vfs_setpos(file, offset, maxsize);
> @@ -214,28 +200,8 @@ loff_t default_llseek(struct file *file, loff_t offset, int whence)
>            offset += file->f_pos;
>            break;
>        case SEEK_DATA:
> -            /*
> -             * In the generic case the entire file is data, so as
> -             * long as offset isn't at the end of the file then the
> -             * offset is data.
> -             */
> -            if (offset >= inode->i_size) {
> -                retval = -ENXIO;
> -                goto out;
> -            }
> -            break;
>        case SEEK_HOLE:
> -            /*
> -             * There is a virtual hole at the end of the file, so
> -             * as long as offset isn't i_size or larger, return
> -             * i_size.
> -             */
> -            if (offset >= inode->i_size) {
> -                retval = -ENXIO;
> -                goto out;
> -            }
> -            offset = inode->i_size;
> -            break;
> +            return -EINVAL;
>    }
>    retval = -EINVAL;
>    if (offset >= 0 || unsigned_offsets(file)) {
> -- 
> 1.8.1.4
> 
> --
> 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] vfs: Return EINVAL for default SEEK_HOLE, SEEK_DATA implementation

[Josef Bacik]

On 01/03/2014 05:17 PM, Andreas Dilger wrote:
> Shouldn't this be -EOPNOTSUPP, since -EINVAL is for incorrect arguments, which isn't the case here.
>
I think we want to be consistent with previous kernels which would 
return -EINVAL if whence wasn't set properly, hopefully breaking as few 
userspace stuff as possible.  Thanks,

Josef

Re: [PATCH] vfs: Return EINVAL for default SEEK_HOLE, SEEK_DATA implementation

[Jeff Liu]

On 01/05 2014 08:36 AM, Josef Bacik wrote:
> 
> On 01/03/2014 05:17 PM, Andreas Dilger wrote:
>> Shouldn't this be -EOPNOTSUPP, since -EINVAL is for incorrect
>> arguments, which isn't the case here.
>>
> I think we want to be consistent with previous kernels which would
> return -EINVAL if whence wasn't set properly, hopefully breaking as few
> userspace stuff as possible.  Thanks,

It seems like that this fix would have a bit influence to man page of lseek(2)
as it suppose to:

       EINVAL whence is not valid.  Or: the resulting  file  offset  would  be
              negative, or beyond the end of a seekable device.

Where probably need an update to reflect this change regardless of whether
return -EINVAL or -EOPNOTSUPP IMHO.

Moreover, Solaris/FreeBSD use {f}pathconf(2) syscalls to detect if a file
system is support SEEK_HOLE function or not, maybe Glibc/GNULib folks would
like to couple lseek(..., SEEK_HOLE) to {f}pathconf(3) in the future, which
could make the userspace stuff a bit more flexible across different OS.

So, I gone through their man pages and in this case, seems both of them will
return -EINVAL if call to {f}pathconf(2) with _PC_MIN_HOLE_SIZE is failed:

Solaris: http://docs.oracle.com/cd/E23824_01/html/821-1463/pathconf-2.html
FreeBSD: http://www.freebsd.org/cgi/man.cgi?query=pathconf&sektion=2


Thanks,
-Jeff

Re: [PATCH] vfs: Return EINVAL for default SEEK_HOLE, SEEK_DATA implementation

[Jan Kara]

On Sat 04-01-14 19:36:12, Josef Bacik wrote:
> 
> On 01/03/2014 05:17 PM, Andreas Dilger wrote:
> >Shouldn't this be -EOPNOTSUPP, since -EINVAL is for incorrect arguments, which isn't the case here.
> >
> I think we want to be consistent with previous kernels which would
> return -EINVAL if whence wasn't set properly, hopefully breaking as
> few userspace stuff as possible.  Thanks,
  Yeah, that's exactly why I chose -EINVAL instead of a bit more logical
-EOPNOTSUPP.

								Honza
-- 
Jan Kara <jack@suse.cz>
SUSE Labs, CR