Patchwork [1/3] fs: add SEEK_HOLE and SEEK_DATA flags V4

login
register
mail settings
Submitter Josef Bacik
Date May 23, 2011, 9:43 p.m.
Message ID <1306186991-1905-1-git-send-email-josef@redhat.com>
Download mbox | patch
Permalink /patch/97086/
State New
Headers show

Comments

Josef Bacik - May 23, 2011, 9:43 p.m.
This just gets us ready to support the SEEK_HOLE and SEEK_DATA flags.  Turns out
using fiemap in things like cp cause more problems than it solves, so lets try
and give userspace an interface that doesn't suck.  We need to match solaris
here, and the definitions are

*o* If /whence/ is SEEK_HOLE, the offset of the start of the
next hole greater than or equal to the supplied offset
is returned. The definition of a hole is provided near
the end of the DESCRIPTION.

*o* If /whence/ is SEEK_DATA, the file pointer is set to the
start of the next non-hole file region greater than or
equal to the supplied offset.

So in the generic case the entire file is data and there is a virtual hole at
the end.  That means we will just return i_size for SEEK_HOLE and will return
the same offset for SEEK_DATA.  This is how Solaris does it so we have to do it
the same way.

Thanks,

Signed-off-by: Josef Bacik <josef@redhat.com>
---
V3->V4:
-Fix the SEEK_HOLE/SEEK_DATA values to match solaris
-Fix the generic case to work the same way solaris works as best as possible

 fs/read_write.c    |   17 +++++++++++++++++
 include/linux/fs.h |    4 +++-
 2 files changed, 20 insertions(+), 1 deletions(-)
Andreas Dilger - May 25, 2011, 7:45 p.m.
On May 23, 2011, at 15:43, Josef Bacik wrote:
> This just gets us ready to support the SEEK_HOLE and SEEK_DATA flags.  Turns out
> using fiemap in things like cp cause more problems than it solves, so lets try
> and give userspace an interface that doesn't suck.  We need to match solaris
> here, and the definitions are
> 
> diff --git a/fs/read_write.c b/fs/read_write.c
> index 5520f8a..9c3b453 100644
> --- a/fs/read_write.c
> +++ b/fs/read_write.c
> @@ -64,6 +64,23 @@ generic_file_llseek_unlocked(struct file *file, loff_t offset, int origin)
> 			return file->f_pos;
> 		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)
> +			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 >= inode->i_size)
> +			return -ENXIO;
> +		offset = inode->i_size;
> +		break;
> 	}

What about all of the existing filesystems that currently just ignore
values of "origin" that they don't understand?  Looking through those
it appears that most of them will return "offset" for unknown values
of "origin", which I guess is OK for SEEK_DATA, but is confusing for
SEEK_HOLE.  Some filesystems will return -EINVAL for values of origin
that are unknown.

Most of the filesystem-specific ->llseek() methods don't do any error
checking on "origin" because this is handled at the sys_llseek() level,
and hasn't changed in many years.

I assume this patch is also dependent upon the "remove default_llseek()"
patch, so that the implementation of SEEK_DATA and SEEK_HOLE can be done
in only generic_file_llseek()?

Finally, while looking through the various ->llseek() methods I notice
that many filesystems return "i_size" for SEEK_END, which clearly does
not make sense for filesystems like ext3/ext4 htree, btrfs, etc that
use hash keys instead of byte offsets for doing directory traversal.
The comment at generic_file_llseek() is that it is intended for use by
regular files.

Should the ext4_llseek() code be changed to return 0x7ffffffff for the
SEEK_END value?  That makes more sense compared to values returned for
SEEK_CUR so that an application can compare the current "offset" with
the final value for a progress bar.

Another interesting use is for N threads to process a large directory
in parallel by using max_off = llseek(dirfd, 0, SEEK_END) and then
each thread calls llseek(dirfd, thread_nr * max_off / N, SEEK_SET)
to process 1/N of the directory.


Cheers, Andreas






Cheers, Andreas





--
To unsubscribe from this list: send the line "unsubscribe linux-ext4" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Josef Bacik - May 25, 2011, 8:46 p.m.
On 05/25/2011 03:45 PM, Andreas Dilger wrote:
> On May 23, 2011, at 15:43, Josef Bacik wrote:
>> This just gets us ready to support the SEEK_HOLE and SEEK_DATA flags.  Turns out
>> using fiemap in things like cp cause more problems than it solves, so lets try
>> and give userspace an interface that doesn't suck.  We need to match solaris
>> here, and the definitions are
>>
>> diff --git a/fs/read_write.c b/fs/read_write.c
>> index 5520f8a..9c3b453 100644
>> --- a/fs/read_write.c
>> +++ b/fs/read_write.c
>> @@ -64,6 +64,23 @@ generic_file_llseek_unlocked(struct file *file, loff_t offset, int origin)
>> 			return file->f_pos;
>> 		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)
>> +			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 >= inode->i_size)
>> +			return -ENXIO;
>> +		offset = inode->i_size;
>> +		break;
>> 	}
> 
> What about all of the existing filesystems that currently just ignore
> values of "origin" that they don't understand?  Looking through those
> it appears that most of them will return "offset" for unknown values
> of "origin", which I guess is OK for SEEK_DATA, but is confusing for
> SEEK_HOLE.  Some filesystems will return -EINVAL for values of origin
> that are unknown.
> 

Yeah I just didn't want to do all that work until I was sure the base of
what I had was acceptable.  If people think this set is good to go then
I will go through and fix everybody who does their own lseek.

> Most of the filesystem-specific ->llseek() methods don't do any error
> checking on "origin" because this is handled at the sys_llseek() level,
> and hasn't changed in many years.
> 
> I assume this patch is also dependent upon the "remove default_llseek()"
> patch, so that the implementation of SEEK_DATA and SEEK_HOLE can be done
> in only generic_file_llseek()?
> 
> Finally, while looking through the various ->llseek() methods I notice
> that many filesystems return "i_size" for SEEK_END, which clearly does
> not make sense for filesystems like ext3/ext4 htree, btrfs, etc that
> use hash keys instead of byte offsets for doing directory traversal.
> The comment at generic_file_llseek() is that it is intended for use by
> regular files.
> 
> Should the ext4_llseek() code be changed to return 0x7ffffffff for the
> SEEK_END value?  That makes more sense compared to values returned for
> SEEK_CUR so that an application can compare the current "offset" with
> the final value for a progress bar.

So maybe we make SEEK_DATA/SEEK_HOLE only work on regular files and not
directories?  Sunil what does solaris do?  Thanks,

Josef
--
To unsubscribe from this list: send the line "unsubscribe linux-ext4" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Sunil Mushran - May 25, 2011, 10:07 p.m.
On 05/25/2011 01:46 PM, Josef Bacik wrote:
> On 05/25/2011 03:45 PM, Andreas Dilger wrote:
>> Most of the filesystem-specific ->llseek() methods don't do any error
>> checking on "origin" because this is handled at the sys_llseek() level,
>> and hasn't changed in many years.
>>
>> I assume this patch is also dependent upon the "remove default_llseek()"
>> patch, so that the implementation of SEEK_DATA and SEEK_HOLE can be done
>> in only generic_file_llseek()?
>>
>> Finally, while looking through the various ->llseek() methods I notice
>> that many filesystems return "i_size" for SEEK_END, which clearly does
>> not make sense for filesystems like ext3/ext4 htree, btrfs, etc that
>> use hash keys instead of byte offsets for doing directory traversal.
>> The comment at generic_file_llseek() is that it is intended for use by
>> regular files.
>>
>> Should the ext4_llseek() code be changed to return 0x7ffffffff for the
>> SEEK_END value?  That makes more sense compared to values returned for
>> SEEK_CUR so that an application can compare the current "offset" with
>> the final value for a progress bar.
> So maybe we make SEEK_DATA/SEEK_HOLE only work on regular files and not
> directories?  Sunil what does solaris do?  Thanks,

In Solaris the size of the directory appears to be equal to the number
of entries and the offset is the file#, so to speak. SEEK_DATA returns
the current offset and SEEK_HOLE the last one.

Just to be clear, I am not a Solaris expert. I just happen to have access
to it. ;)
--
To unsubscribe from this list: send the line "unsubscribe linux-ext4" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Patch

diff --git a/fs/read_write.c b/fs/read_write.c
index 5520f8a..9c3b453 100644
--- a/fs/read_write.c
+++ b/fs/read_write.c
@@ -64,6 +64,23 @@  generic_file_llseek_unlocked(struct file *file, loff_t offset, int origin)
 			return file->f_pos;
 		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)
+			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 >= inode->i_size)
+			return -ENXIO;
+		offset = inode->i_size;
+		break;
 	}
 
 	if (offset < 0 && !unsigned_offsets(file))
diff --git a/include/linux/fs.h b/include/linux/fs.h
index cdf9495..fe1e250 100644
--- a/include/linux/fs.h
+++ b/include/linux/fs.h
@@ -31,7 +31,9 @@ 
 #define SEEK_SET	0	/* seek relative to beginning of file */
 #define SEEK_CUR	1	/* seek relative to current file position */
 #define SEEK_END	2	/* seek relative to end of file */
-#define SEEK_MAX	SEEK_END
+#define SEEK_DATA	3	/* seek to the next data */
+#define SEEK_HOLE	4	/* seek to the next hole */
+#define SEEK_MAX	SEEK_HOLE
 
 struct fstrim_range {
 	__u64 start;