diff mbox

[1/2,v2] libext2fs: introduce lseek SEEK_DATA/HOLE

Message ID 1358173111-10511-1-git-send-email-wenqing.lz@taobao.com
State Superseded, archived
Headers show

Commit Message

Zheng Liu Jan. 14, 2013, 2:18 p.m. UTC 
From: Zheng Liu <wenqing.lz@taobao.com>

ext2fs_file_llseek is extented to introduce SEEK_DATA/HOLE.  In *_data()
function it will find the next data, and in *_hole() function it will find the
next hole.  A new error code called EXT2_ET_SEEK_BEYOND_EOF is define to
indicate that the offset is beyond the end of file.

Here they need to solve a problem that the caller can not dereference
ext2_file_t->pos because this structure is hidden by a typedef.  Thus,
EXT2_SEEK_OFFSET_INVALID is define to indicate whether or not it will
find the data/hole from ext2_file_t->pos.

CC: "Theodore Ts'o" <tytso@mit.edu>
Signed-off-by: Zheng Liu <wenqing.lz@taobao.com>
---
Hi Ted,

ext2fs_file_llseek_data/hole() seem to be weird because ext2_file_t structure is
hidden by a typedef.  The caller can not dereference it.  So I define a marco
called EXT2_SEEK_OFFSET_INVALID to let the caller indicate that it find the
data/hole from ext2_file_t->pos or from offset.  What do you think?

Thanks,
						- Zheng

 lib/ext2fs/ext2_err.et.in |  3 ++
 lib/ext2fs/ext2fs.h       |  4 +++
 lib/ext2fs/fileio.c       | 80 ++++++++++++++++++++++++++++++++++++++++++++++-
 3 files changed, 86 insertions(+), 1 deletion(-)

Comments

Theodore Ts'o Jan. 15, 2013, 3:23 a.m. UTC | #1 
On Mon, Jan 14, 2013 at 10:18:30PM +0800, Zheng Liu wrote:
> 
> ext2fs_file_llseek_data/hole() seem to be weird because ext2_file_t
> structure is hidden by a typedef.  The caller can not dereference
> it.  So I define a marco called EXT2_SEEK_OFFSET_INVALID to let the
> caller indicate that it find the data/hole from ext2_file_t->pos or
> from offset.  What do you think?

Is the problem you're worried about is that the user can't get current
location?

That's pretty easy to solve.  You can get it the same way it works
with the lseek(2) system call.

     retval = ext2fs_file_llseek(file, 0, SEEK_CUR, &pos);

Upon the return, pos will be contain the current file offset.

     	 	     	     	     	 	 - Ted
--
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
Zheng Liu Jan. 15, 2013, 1:02 p.m. UTC | #2 
On Mon, Jan 14, 2013 at 10:23:29PM -0500, Theodore Ts'o wrote:
> On Mon, Jan 14, 2013 at 10:18:30PM +0800, Zheng Liu wrote:
> > 
> > ext2fs_file_llseek_data/hole() seem to be weird because ext2_file_t
> > structure is hidden by a typedef.  The caller can not dereference
> > it.  So I define a marco called EXT2_SEEK_OFFSET_INVALID to let the
> > caller indicate that it find the data/hole from ext2_file_t->pos or
> > from offset.  What do you think?
> 
> Is the problem you're worried about is that the user can't get current
> location?
> 
> That's pretty easy to solve.  You can get it the same way it works
> with the lseek(2) system call.
> 
>      retval = ext2fs_file_llseek(file, 0, SEEK_CUR, &pos);
> 
> Upon the return, pos will be contain the current file offset.

Ah, I see.  I will remove EXT2_SEEK_OFFSET_INVALID flag in next version.

Thanks,
                                                - Zheng
--
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
Theodore Ts'o Jan. 15, 2013, 6:55 p.m. UTC | #3 
On Mon, Jan 14, 2013 at 10:18:30PM +0800, Zheng Liu wrote:
> +static errcode_t ext2fs_file_llseek_data(ext2_file_t file, __u64 offset)
> +{
> +	int		ret_flags, flag = 1;
> +	ext2_filsys	fs = file->fs;
> +	errcode_t	retval;
> +
> +	/*
> +	 * If offset == EXT2_SEEK_OFFSET_INVALID, that means that
> +	 * the caller wants to find the data from file->pos.
> +	 */
> +	offset = offset != EXT2_SEEK_OFFSET_INVALID ? offset : file->pos;
> +	file->blockno = offset / fs->blocksize;
> +	while (file->blockno * fs->blocksize < EXT2_I_SIZE(&file->inode)) {
> +		retval = ext2fs_bmap2(fs, file->ino, &file->inode,
> +				      BMAP_BUFFER, 0, file->blockno,
> +				      &ret_flags, &file->physblock);

Using bmap will certainly work, although as an optimization we can do
much better for extents if we use the extents interface.  I won't
insist on this, though.  (We can always optimize this later).

       	  		     	 	- Ted
--
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
Zheng Liu Jan. 16, 2013, 12:04 p.m. UTC | #4 
On Tue, Jan 15, 2013 at 01:55:09PM -0500, Theodore Ts'o wrote:
> On Mon, Jan 14, 2013 at 10:18:30PM +0800, Zheng Liu wrote:
> > +static errcode_t ext2fs_file_llseek_data(ext2_file_t file, __u64 offset)
> > +{
> > +	int		ret_flags, flag = 1;
> > +	ext2_filsys	fs = file->fs;
> > +	errcode_t	retval;
> > +
> > +	/*
> > +	 * If offset == EXT2_SEEK_OFFSET_INVALID, that means that
> > +	 * the caller wants to find the data from file->pos.
> > +	 */
> > +	offset = offset != EXT2_SEEK_OFFSET_INVALID ? offset : file->pos;
> > +	file->blockno = offset / fs->blocksize;
> > +	while (file->blockno * fs->blocksize < EXT2_I_SIZE(&file->inode)) {
> > +		retval = ext2fs_bmap2(fs, file->ino, &file->inode,
> > +				      BMAP_BUFFER, 0, file->blockno,
> > +				      &ret_flags, &file->physblock);
> 
> Using bmap will certainly work, although as an optimization we can do
> much better for extents if we use the extents interface.  I won't
> insist on this, though.  (We can always optimize this later).

Yeah, it allows us to skip to the next data/hole directly if the extents
interface is used.  But if we do that, we will need to handle
extent-based file and indirect-based file resptively like this.

        if (inode->i_flags & EXT4_EXTENTS_FL) {
                ext2fs_file_ext_llseek_data();
                ...
        } else {
                ext2fs_file_ind_llseek_data();
                ...
        }

I am not sure whether it is too complicated or not for us.  What do you
think?

Thanks,
                                                - Zheng
--
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
Theodore Ts'o Jan. 16, 2013, 2:59 p.m. UTC | #5 
On Wed, Jan 16, 2013 at 08:04:56PM +0800, Zheng Liu wrote:
> Yeah, it allows us to skip to the next data/hole directly if the extents
> interface is used.  But if we do that, we will need to handle
> extent-based file and indirect-based file resptively like this.
> 
>         if (inode->i_flags & EXT4_EXTENTS_FL) {
>                 ext2fs_file_ext_llseek_data();
>                 ...
>         } else {
>                 ext2fs_file_ind_llseek_data();
>                 ...
>         }
> 
> I am not sure whether it is too complicated or not for us.  What do you
> think?

I'm not too worried about the performance issues for debugfs.  But for
clients who are accessing ext[234] using libext2fs and FUSE, they
would probably notice in at least some circumstances.  I'm not that
worried about the complexity, but it's also not a high priority thing,
either --- it's a "nice to have", not a "must have".

Regards,

					- Ted
--
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
Zheng Liu Jan. 18, 2013, 10:07 a.m. UTC | #6 
On Wed, Jan 16, 2013 at 09:59:18AM -0500, Theodore Ts'o wrote:
> On Wed, Jan 16, 2013 at 08:04:56PM +0800, Zheng Liu wrote:
> > Yeah, it allows us to skip to the next data/hole directly if the extents
> > interface is used.  But if we do that, we will need to handle
> > extent-based file and indirect-based file resptively like this.
> > 
> >         if (inode->i_flags & EXT4_EXTENTS_FL) {
> >                 ext2fs_file_ext_llseek_data();
> >                 ...
> >         } else {
> >                 ext2fs_file_ind_llseek_data();
> >                 ...
> >         }
> > 
> > I am not sure whether it is too complicated or not for us.  What do you
> > think?
> 
> I'm not too worried about the performance issues for debugfs.  But for
> clients who are accessing ext[234] using libext2fs and FUSE, they
> would probably notice in at least some circumstances.  I'm not that
> worried about the complexity, but it's also not a high priority thing,
> either --- it's a "nice to have", not a "must have".

Understood.  Let me try it.

Thanks,
                                                - Zheng
--
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
diff mbox

Patch

diff --git a/lib/ext2fs/ext2_err.et.in b/lib/ext2fs/ext2_err.et.in
index c99a167..f763938 100644
--- a/lib/ext2fs/ext2_err.et.in
+++ b/lib/ext2fs/ext2_err.et.in
@@ -473,4 +473,7 @@  ec	EXT2_ET_UNKNOWN_CSUM,
 ec	EXT2_ET_MMP_CSUM_INVALID,
 	"MMP block checksum does not match MMP block"
 
+ec	EXT2_ET_SEEK_BEYOND_EOF,
+	"lseek beyond the EOF"
+
 	end
diff --git a/lib/ext2fs/ext2fs.h b/lib/ext2fs/ext2fs.h
index 7139b4d..474049f 100644
--- a/lib/ext2fs/ext2fs.h
+++ b/lib/ext2fs/ext2fs.h
@@ -160,6 +160,10 @@  typedef struct ext2_file *ext2_file_t;
 #define EXT2_SEEK_SET	0
 #define EXT2_SEEK_CUR	1
 #define EXT2_SEEK_END	2
+#define EXT2_SEEK_DATA	3
+#define EXT2_SEEK_HOLE	4
+
+#define EXT2_SEEK_OFFSET_INVALID	-1
 
 /*
  * Flags for the ext2_filsys structure and for ext2fs_open()
diff --git a/lib/ext2fs/fileio.c b/lib/ext2fs/fileio.c
index 1f7002c..ab33290 100644
--- a/lib/ext2fs/fileio.c
+++ b/lib/ext2fs/fileio.c
@@ -312,10 +312,84 @@  fail:
 	return retval;
 }
 
+static errcode_t ext2fs_file_llseek_data(ext2_file_t file, __u64 offset)
+{
+	int		ret_flags, flag = 1;
+	ext2_filsys	fs = file->fs;
+	errcode_t	retval;
+
+	/*
+	 * If offset == EXT2_SEEK_OFFSET_INVALID, that means that
+	 * the caller wants to find the data from file->pos.
+	 */
+	offset = offset != EXT2_SEEK_OFFSET_INVALID ? offset : file->pos;
+	file->blockno = offset / fs->blocksize;
+	while (file->blockno * fs->blocksize < EXT2_I_SIZE(&file->inode)) {
+		retval = ext2fs_bmap2(fs, file->ino, &file->inode,
+				      BMAP_BUFFER, 0, file->blockno,
+				      &ret_flags, &file->physblock);
+		if (retval)
+			return retval;
+		if (file->physblock == 0 || (ret_flags & BMAP_RET_UNINIT)) {
+			file->blockno++;
+			flag = 0;
+		} else {
+			if (flag)
+				file->pos = offset;
+			else
+				file->pos = file->blockno * fs->blocksize;
+			break;
+		}
+	}
+
+	/* notify the caller that there is no any data */
+	if (file->blockno * fs->blocksize >= EXT2_I_SIZE(&file->inode))
+		return EXT2_ET_SEEK_BEYOND_EOF;
+
+	return 0;
+}
+
+static errcode_t ext2fs_file_llseek_hole(ext2_file_t file, __u64 offset)
+{
+	int		ret_flags, flag = 1;
+	ext2_filsys	fs = file->fs;
+	errcode_t	retval;
+
+	/*
+	 * If offset == EXT2_SEEK_OFFSET_INVALID, that means that
+	 * the caller wants to find the hole from file->pos.
+	 */
+	offset = offset != EXT2_SEEK_OFFSET_INVALID ? offset : file->pos;
+	if (offset >= EXT2_I_SIZE(&file->inode))
+		return EXT2_ET_SEEK_BEYOND_EOF;
+
+	file->blockno = offset / fs->blocksize;
+	while (file->blockno * fs->blocksize < EXT2_I_SIZE(&file->inode)) {
+		retval = ext2fs_bmap2(fs, file->ino, &file->inode,
+				      BMAP_BUFFER, 0, file->blockno,
+				      &ret_flags, &file->physblock);
+		if (retval)
+			return retval;
+		if (file->physblock == 0 || (ret_flags & BMAP_RET_UNINIT)) {
+			if (flag)
+				file->pos = offset;
+			else
+				file->pos = file->blockno * fs->blocksize;
+			break;
+		} else {
+			file->blockno++;
+			flag = 0;
+		}
+	}
+
+	return 0;
+}
+
 errcode_t ext2fs_file_llseek(ext2_file_t file, __u64 offset,
 			    int whence, __u64 *ret_pos)
 {
 	EXT2_CHECK_MAGIC(file, EXT2_ET_MAGIC_EXT2_FILE);
+	errcode_t retval = 0;
 
 	if (whence == EXT2_SEEK_SET)
 		file->pos = offset;
@@ -323,13 +397,17 @@  errcode_t ext2fs_file_llseek(ext2_file_t file, __u64 offset,
 		file->pos += offset;
 	else if (whence == EXT2_SEEK_END)
 		file->pos = EXT2_I_SIZE(&file->inode) + offset;
+	else if (whence == EXT2_SEEK_DATA)
+		retval = ext2fs_file_llseek_data(file, offset);
+	else if (whence == EXT2_SEEK_HOLE)
+		retval = ext2fs_file_llseek_hole(file, offset);
 	else
 		return EXT2_ET_INVALID_ARGUMENT;
 
 	if (ret_pos)
 		*ret_pos = file->pos;
 
-	return 0;
+	return retval;
 }
 
 errcode_t ext2fs_file_lseek(ext2_file_t file, ext2_off_t offset,