I have a few comments. Some of them are inline with the code.

The one generic thing is that the Sun documentation specifically
says to check pathconf(_PC_MIN_HOLE_SIZE) on the filesystem to see
if it supports this request. Have you looked into adding this to
the Linux version of pathconf? I haven't tried to look at the latest
glibc, but 2.3.2 doesn't appear to have it. If we do have it, this
request should really go to the kernel to ask the individual
filesystem.  However, it looks like pathconf is entirely implemented
in glibc.  Should we add a system call or some other way to pass
pathconf requests into the kernel? I know pathconf currently returns
some inaccurate results in some cases due to this limitation. There
are some existing ones like _PC_LINK_MAX that glibc tries to guess
the correct result based on statfs and can get wrong in any
non-standard setup.

	Brad Boyer
	flar@allandria.com

On Wed, Nov 28, 2007 at 03:02:07PM -0500, Josef Bacik wrote:
quoted text
> diff --git a/fs/read_write.c b/fs/read_write.c
> index ea1f94c..cf61e1e 100644
> --- a/fs/read_write.c
> +++ b/fs/read_write.c
> @@ -31,10 +31,32 @@ const struct file_operations generic_ro_fops = {
>  
>  EXPORT_SYMBOL(generic_ro_fops);
>  
> +static loff_t generic_seek_hole_data(struct file *file, loff_t offset,
> +				     int origin)
> +{
> +	loff_t retval = -ENXIO;
> +	struct inode *inode = file->f_mapping->host;
> +	sector_t block, found_block;
> +	sector_t last_block = (i_size_read(inode) - 1) >> inode->i_blkbits;
> +	int want = (origin == SEEK_HOLE) ? 0 : 1;
> +
> +	for (block = offset >> inode->i_blkbits; block <= last_block; block++) {
> +		found_block = bmap(inode, block);
> +
> +		if (!!found_block == want) {
> +			retval = block << inode->i_blkbits;
> +			break;
> +		}
> +	}
> +
> +	return retval;
> +}
> +

It looks like this function can return an offset that is before the
one passed in as an argument due to losing the lower bits. I think
it needs to do somthing more like this in the loop initializer:

    block = (offset + (1 << inode->i_blkbits) - 1) >> inode->i_blkbits;

By adding 1 block if it wasn't already on an even block, this assures
us that it won't go backwards from the original offset while allowing
someone to get a block that really does start exactly on the offset.

quoted text
>  loff_t generic_file_llseek(struct file *file, loff_t offset, int origin)
>  {
>  	long long retval;
>  	struct inode *inode = file->f_mapping->host;
> +	loff_t (*fn)(struct file *, loff_t, int);
>  
>  	mutex_lock(&inode->i_mutex);
>  	switch (origin) {
> @@ -43,15 +65,24 @@ loff_t generic_file_llseek(struct file *file, loff_t offset, int origin)
>  			break;
>  		case SEEK_CUR:
>  			offset += file->f_pos;
> +			break;
> +		case SEEK_HOLE:
> +		case SEEK_DATA:
> +			fn = generic_seek_hole_data;
> +			if (file->f_op->seek_hole_data)
> +				fn = file->f_op->seek_hole_data;
> +			offset = fn(file, offset, origin);
>  	}
>  	retval = -EINVAL;
>  	if (offset>=0 && offset<=inode->i_sb->s_maxbytes) {
> -		if (offset != file->f_pos) {
> +		if (offset != file->f_pos && origin != SEEK_HOLE) {

Why is SEEK_HOLE special in this case? The description of SEEK_HOLE
and SEEK_DATA in the Sun document would imply to me that they should
be handled the same.

quoted text
>  			file->f_pos = offset;
>  			file->f_version = 0;
>  		}
>  		retval = offset;
> -	}
> +	} else if (offset < 0)
> +		retval = offset;
> +
>  	mutex_unlock(&inode->i_mutex);
>  	return retval;
>  }
-
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