[PATCH] cleanup __generic_block_fiemap and fix a few minor issues

[PATCH] cleanup __generic_block_fiemap and fix a few minor issues

[Josef Bacik]

This patch keeps getting lost, so I'm posting it again.  Andrew has complained
several times about the format of this function, the type'ing used and such, so
this patch fixes all of this.  Also it addresses 2 issues

1) Some filesystems (gfs2) do not like being given non-block-aligned b_size's to
map, so make sure we block align the b_size.

2) If you have a file with say 50 blocks, and ask to map 25 of those blocks, and
block 23 is a hole and block 24 is data, block 24 will get dropped.  This fixes
the problem by making sure we add the extent to the fieinfo before returning.

I've tested this patch with my fiemap-tester, and it runs fine.  I also tested
with a file that was < blocksize (which is where gfs2 was having problems) and
it worked fine.  I also tested this with a 1 terabyte file, and it worked fine.

Signed-off-by: Josef Bacik <josef@redhat.com>
---
 fs/ioctl.c         |   88 ++++++++++++++++++++++++++++++++-------------------
 include/linux/fs.h |    5 ++-
 2 files changed, 58 insertions(+), 35 deletions(-)

diff --git a/fs/ioctl.c b/fs/ioctl.c
index 6c75110..4ebdc41 100644
--- a/fs/ioctl.c
+++ b/fs/ioctl.c
@@ -228,13 +228,22 @@ static int ioctl_fiemap(struct file *filp, unsigned long arg)
 
 #ifdef CONFIG_BLOCK
 
-#define blk_to_logical(inode, blk) (blk << (inode)->i_blkbits)
-#define logical_to_blk(inode, offset) (offset >> (inode)->i_blkbits);
+static inline sector_t logical_to_blk(struct inode *inode, loff_t offset)
+{
+	return (offset >> inode->i_blkbits);
+}
+
+static inline loff_t blk_to_logical(struct inode *inode, sector_t blk)
+{
+	return (blk << inode->i_blkbits);
+}
 
 /**
  * __generic_block_fiemap - FIEMAP for block based inodes (no locking)
  * @inode - the inode to map
- * @arg - the pointer to userspace where we copy everything to
+ * @fieinfo - the fiemap info struct that will be passed back to userspace
+ * @start - where to start mapping in the inode
+ * @len - how much space to map
  * @get_block - the fs's get_block function
  *
  * This does FIEMAP for block based inodes.  Basically it will just loop
@@ -250,46 +259,55 @@ static int ioctl_fiemap(struct file *filp, unsigned long arg)
  */
 
 int __generic_block_fiemap(struct inode *inode,
-			   struct fiemap_extent_info *fieinfo, u64 start,
-			   u64 len, get_block_t *get_block)
+			   struct fiemap_extent_info *fieinfo, loff_t start,
+			   loff_t len, get_block_t *get_block)
 {
-	struct buffer_head tmp;
-	unsigned long long start_blk;
-	long long length = 0, map_len = 0;
+	struct buffer_head map_bh;
+	sector_t start_blk;
+	loff_t map_end;
 	u64 logical = 0, phys = 0, size = 0;
 	u32 flags = FIEMAP_EXTENT_MERGED;
-	int ret = 0, past_eof = 0, whole_file = 0;
+	bool past_eof = false, whole_file = false;
+	int ret = 0;
 
-	if ((ret = fiemap_check_flags(fieinfo, FIEMAP_FLAG_SYNC)))
+	ret = fiemap_check_flags(fieinfo, FIEMAP_FLAG_SYNC);
+	if (ret)
 		return ret;
 
 	start_blk = logical_to_blk(inode, start);
 
-	length = (long long)min_t(u64, len, i_size_read(inode));
-	if (length < len)
-		whole_file = 1;
+	if (len >= i_size_read(inode)) {
+		whole_file = true;
+		len = i_size_read(inode);
+	}
 
-	map_len = length;
+	/*
+	 * Some filesystems (gfs2) don't deal with being told to map lengths
+	 * that are not blocksize aligned, so make the length blocksized
+	 * aligned, since b_size should always just come back as the real len.
+	 */
+	len = (len + (1 << inode->i_blkbits) - 1) &
+		~((1 << inode->i_blkbits) - 1);
+	map_end = start + len;
 
 	do {
 		/*
 		 * we set b_size to the total size we want so it will map as
 		 * many contiguous blocks as possible at once
 		 */
-		memset(&tmp, 0, sizeof(struct buffer_head));
-		tmp.b_size = map_len;
+		memset(&map_bh, 0, sizeof(struct buffer_head));
+		map_bh.b_size = len;
 
-		ret = get_block(inode, start_blk, &tmp, 0);
+		ret = get_block(inode, start_blk, &map_bh, 0);
 		if (ret)
 			break;
 
 		/* HOLE */
-		if (!buffer_mapped(&tmp)) {
-			length -= blk_to_logical(inode, 1);
+		if (!buffer_mapped(&map_bh)) {
 			start_blk++;
 
 			/*
-			 * we want to handle the case where there is an
+			 * We want to handle the case where there is an
 			 * allocated block at the front of the file, and then
 			 * nothing but holes up to the end of the file properly,
 			 * to make sure that extent at the front gets properly
@@ -297,11 +315,11 @@ int __generic_block_fiemap(struct inode *inode,
 			 */
 			if (!past_eof &&
 			    blk_to_logical(inode, start_blk) >=
-			    blk_to_logical(inode, 0)+i_size_read(inode))
+			    blk_to_logical(inode, 0) + i_size_read(inode))
 				past_eof = 1;
 
 			/*
-			 * first hole after going past the EOF, this is our
+			 * First hole after going past the EOF, this is our
 			 * last extent
 			 */
 			if (past_eof && size) {
@@ -309,15 +327,19 @@ int __generic_block_fiemap(struct inode *inode,
 				ret = fiemap_fill_next_extent(fieinfo, logical,
 							      phys, size,
 							      flags);
-				break;
+			} else if (size) {
+				ret = fiemap_fill_next_extent(fieinfo, logical,
+							      phys, size, flags);
+				size = 0;
 			}
 
 			/* if we have holes up to/past EOF then we're done */
-			if (length <= 0 || past_eof)
+			if (blk_to_logical(inode, start_blk) > map_end ||
+			    past_eof || ret)
 				break;
 		} else {
 			/*
-			 * we have gone over the length of what we wanted to
+			 * We have gone over the length of what we wanted to
 			 * map, and it wasn't the entire file, so add the extent
 			 * we got last time and exit.
 			 *
@@ -331,7 +353,8 @@ int __generic_block_fiemap(struct inode *inode,
 			 * are good to go, just add the extent to the fieinfo
 			 * and break
 			 */
-			if (length <= 0 && !whole_file) {
+			if (blk_to_logical(inode, start_blk) > map_end &&
+			    !whole_file) {
 				ret = fiemap_fill_next_extent(fieinfo, logical,
 							      phys, size,
 							      flags);
@@ -351,11 +374,10 @@ int __generic_block_fiemap(struct inode *inode,
 			}
 
 			logical = blk_to_logical(inode, start_blk);
-			phys = blk_to_logical(inode, tmp.b_blocknr);
-			size = tmp.b_size;
+			phys = blk_to_logical(inode, map_bh.b_blocknr);
+			size = map_bh.b_size;
 			flags = FIEMAP_EXTENT_MERGED;
 
-			length -= tmp.b_size;
 			start_blk += logical_to_blk(inode, size);
 
 			/*
@@ -364,14 +386,14 @@ int __generic_block_fiemap(struct inode *inode,
 			 * is marked with FIEMAP_EXTENT_LAST
 			 */
 			if (!past_eof &&
-			    logical+size >=
-			    blk_to_logical(inode, 0)+i_size_read(inode))
-				past_eof = 1;
+			    logical + size >=
+			    blk_to_logical(inode, 0) + i_size_read(inode))
+				past_eof = true;
 		}
 		cond_resched();
 	} while (1);
 
-	/* if ret is 1 then we just hit the end of the extent array */
+	/* If ret is 1 then we just hit the end of the extent array */
 	if (ret == 1)
 		ret = 0;
 
diff --git a/include/linux/fs.h b/include/linux/fs.h
index ebb1cd5..7e6e47f 100644
--- a/include/linux/fs.h
+++ b/include/linux/fs.h
@@ -2313,8 +2313,9 @@ extern int vfs_fstatat(int , char __user *, struct kstat *, int);
 extern int do_vfs_ioctl(struct file *filp, unsigned int fd, unsigned int cmd,
 		    unsigned long arg);
 extern int __generic_block_fiemap(struct inode *inode,
-				  struct fiemap_extent_info *fieinfo, u64 start,
-				  u64 len, get_block_t *get_block);
+				  struct fiemap_extent_info *fieinfo,
+				  loff_t start, loff_t len,
+				  get_block_t *get_block);
 extern int generic_block_fiemap(struct inode *inode,
 				struct fiemap_extent_info *fieinfo, u64 start,
 				u64 len, get_block_t *get_block);
-- 
1.6.5.rc2

Re: [PATCH] cleanup __generic_block_fiemap and fix a few minor issues

[Linus Torvalds]

On Wed, 3 Mar 2010, Josef Bacik wrote:
> 
> 1) Some filesystems (gfs2) do not like being given non-block-aligned b_size's to
> map, so make sure we block align the b_size.

As far as I can tell, that alignment is wrong.

You can't just align "len". If "start" wasn't aligned, then just aligning 
"len" is wrong. And if start is guaranteed to be aligned, how?

Also, the alignment itself is ugly as hell, doing

> +	len = (len + (1 << inode->i_blkbits) - 1) &
> +		~((1 << inode->i_blkbits) - 1);
> +	map_end = start + len;

When what you probably should have done is to do

	map_end = start + len;

_before_ rounding 'start', and then after that you can now map 'start' and 
'map_end' just using the regular "logical_to_blk" that you already use (up 
or down depending on whether the thing should be inclusive or exclusive of 
partial blocks). No?

IOW, the whole thing should probably look something like

	sector_t start_blk, last_blk;

	start_blk = logical_to_blk(inode, start);
	last_blk = logical_to_blk(inode, start + len - 1);

and you're done. Short, sweet, and to the point (note that "last_blk" 
really is the _last_ block, so if you want the length in blocks, it would 
be "last_blk - start_blk + 1").

			Linus

Re: [PATCH] cleanup __generic_block_fiemap and fix a few minor issues

[Josef Bacik]

On Thu, Mar 04, 2010 at 08:03:55AM -0800, Linus Torvalds wrote:
> 
> 
> On Wed, 3 Mar 2010, Josef Bacik wrote:
> > 
> > 1) Some filesystems (gfs2) do not like being given non-block-aligned b_size's to
> > map, so make sure we block align the b_size.
> 
> As far as I can tell, that alignment is wrong.
> 
> You can't just align "len". If "start" wasn't aligned, then just aligning 
> "len" is wrong. And if start is guaranteed to be aligned, how?
> 

Hmm I said that wrong.  GFS2 can't handle < blocksize map requests, so making
the length blocksize aligned fixed that problem, I just described it wrong.

> Also, the alignment itself is ugly as hell, doing
> 
> > +	len = (len + (1 << inode->i_blkbits) - 1) &
> > +		~((1 << inode->i_blkbits) - 1);
> > +	map_end = start + len;
> 
> When what you probably should have done is to do
> 
> 	map_end = start + len;
> 
> _before_ rounding 'start', and then after that you can now map 'start' and 
> 'map_end' just using the regular "logical_to_blk" that you already use (up 
> or down depending on whether the thing should be inclusive or exclusive of 
> partial blocks). No?
> 
> IOW, the whole thing should probably look something like
> 
> 	sector_t start_blk, last_blk;
> 
> 	start_blk = logical_to_blk(inode, start);
> 	last_blk = logical_to_blk(inode, start + len - 1);
> 
> and you're done. Short, sweet, and to the point (note that "last_blk" 
> really is the _last_ block, so if you want the length in blocks, it would 
> be "last_blk - start_blk + 1").
> 

Ahh yes thats much nicer, thank you.  Here is my updated patch with your
suggestions.  Thanks,

Signed-off-by: Josef Bacik <josef@redhat.com>
---
 fs/ioctl.c         |   81 +++++++++++++++++++++++++++++----------------------
 include/linux/fs.h |    5 ++-
 2 files changed, 49 insertions(+), 37 deletions(-)

diff --git a/fs/ioctl.c b/fs/ioctl.c
index 6c75110..64fed0b 100644
--- a/fs/ioctl.c
+++ b/fs/ioctl.c
@@ -228,13 +228,22 @@ static int ioctl_fiemap(struct file *filp, unsigned long arg)
 
 #ifdef CONFIG_BLOCK
 
-#define blk_to_logical(inode, blk) (blk << (inode)->i_blkbits)
-#define logical_to_blk(inode, offset) (offset >> (inode)->i_blkbits);
+static inline sector_t logical_to_blk(struct inode *inode, loff_t offset)
+{
+	return (offset >> inode->i_blkbits);
+}
+
+static inline loff_t blk_to_logical(struct inode *inode, sector_t blk)
+{
+	return (blk << inode->i_blkbits);
+}
 
 /**
  * __generic_block_fiemap - FIEMAP for block based inodes (no locking)
  * @inode - the inode to map
- * @arg - the pointer to userspace where we copy everything to
+ * @fieinfo - the fiemap info struct that will be passed back to userspace
+ * @start - where to start mapping in the inode
+ * @len - how much space to map
  * @get_block - the fs's get_block function
  *
  * This does FIEMAP for block based inodes.  Basically it will just loop
@@ -250,46 +259,46 @@ static int ioctl_fiemap(struct file *filp, unsigned long arg)
  */
 
 int __generic_block_fiemap(struct inode *inode,
-			   struct fiemap_extent_info *fieinfo, u64 start,
-			   u64 len, get_block_t *get_block)
+			   struct fiemap_extent_info *fieinfo, loff_t start,
+			   loff_t len, get_block_t *get_block)
 {
-	struct buffer_head tmp;
-	unsigned long long start_blk;
-	long long length = 0, map_len = 0;
+	struct buffer_head map_bh;
+	sector_t start_blk, last_blk;
 	u64 logical = 0, phys = 0, size = 0;
 	u32 flags = FIEMAP_EXTENT_MERGED;
-	int ret = 0, past_eof = 0, whole_file = 0;
+	bool past_eof = false, whole_file = false;
+	int ret = 0;
 
-	if ((ret = fiemap_check_flags(fieinfo, FIEMAP_FLAG_SYNC)))
+	ret = fiemap_check_flags(fieinfo, FIEMAP_FLAG_SYNC);
+	if (ret)
 		return ret;
 
-	start_blk = logical_to_blk(inode, start);
-
-	length = (long long)min_t(u64, len, i_size_read(inode));
-	if (length < len)
-		whole_file = 1;
+	if (len >= i_size_read(inode)) {
+		whole_file = true;
+		len = i_size_read(inode);
+	}
 
-	map_len = length;
+	start_blk = logical_to_blk(inode, start);
+	last_blk = logical_to_blk(inode, start + len - 1);
 
 	do {
 		/*
 		 * we set b_size to the total size we want so it will map as
 		 * many contiguous blocks as possible at once
 		 */
-		memset(&tmp, 0, sizeof(struct buffer_head));
-		tmp.b_size = map_len;
+		memset(&map_bh, 0, sizeof(struct buffer_head));
+		map_bh.b_size = len;
 
-		ret = get_block(inode, start_blk, &tmp, 0);
+		ret = get_block(inode, start_blk, &map_bh, 0);
 		if (ret)
 			break;
 
 		/* HOLE */
-		if (!buffer_mapped(&tmp)) {
-			length -= blk_to_logical(inode, 1);
+		if (!buffer_mapped(&map_bh)) {
 			start_blk++;
 
 			/*
-			 * we want to handle the case where there is an
+			 * We want to handle the case where there is an
 			 * allocated block at the front of the file, and then
 			 * nothing but holes up to the end of the file properly,
 			 * to make sure that extent at the front gets properly
@@ -297,11 +306,11 @@ int __generic_block_fiemap(struct inode *inode,
 			 */
 			if (!past_eof &&
 			    blk_to_logical(inode, start_blk) >=
-			    blk_to_logical(inode, 0)+i_size_read(inode))
+			    blk_to_logical(inode, 0) + i_size_read(inode))
 				past_eof = 1;
 
 			/*
-			 * first hole after going past the EOF, this is our
+			 * First hole after going past the EOF, this is our
 			 * last extent
 			 */
 			if (past_eof && size) {
@@ -309,15 +318,18 @@ int __generic_block_fiemap(struct inode *inode,
 				ret = fiemap_fill_next_extent(fieinfo, logical,
 							      phys, size,
 							      flags);
-				break;
+			} else if (size) {
+				ret = fiemap_fill_next_extent(fieinfo, logical,
+							      phys, size, flags);
+				size = 0;
 			}
 
 			/* if we have holes up to/past EOF then we're done */
-			if (length <= 0 || past_eof)
+			if (start_blk > last_blk || past_eof || ret)
 				break;
 		} else {
 			/*
-			 * we have gone over the length of what we wanted to
+			 * We have gone over the length of what we wanted to
 			 * map, and it wasn't the entire file, so add the extent
 			 * we got last time and exit.
 			 *
@@ -331,7 +343,7 @@ int __generic_block_fiemap(struct inode *inode,
 			 * are good to go, just add the extent to the fieinfo
 			 * and break
 			 */
-			if (length <= 0 && !whole_file) {
+			if (start_blk > last_blk && !whole_file) {
 				ret = fiemap_fill_next_extent(fieinfo, logical,
 							      phys, size,
 							      flags);
@@ -351,11 +363,10 @@ int __generic_block_fiemap(struct inode *inode,
 			}
 
 			logical = blk_to_logical(inode, start_blk);
-			phys = blk_to_logical(inode, tmp.b_blocknr);
-			size = tmp.b_size;
+			phys = blk_to_logical(inode, map_bh.b_blocknr);
+			size = map_bh.b_size;
 			flags = FIEMAP_EXTENT_MERGED;
 
-			length -= tmp.b_size;
 			start_blk += logical_to_blk(inode, size);
 
 			/*
@@ -364,14 +375,14 @@ int __generic_block_fiemap(struct inode *inode,
 			 * is marked with FIEMAP_EXTENT_LAST
 			 */
 			if (!past_eof &&
-			    logical+size >=
-			    blk_to_logical(inode, 0)+i_size_read(inode))
-				past_eof = 1;
+			    logical + size >=
+			    blk_to_logical(inode, 0) + i_size_read(inode))
+				past_eof = true;
 		}
 		cond_resched();
 	} while (1);
 
-	/* if ret is 1 then we just hit the end of the extent array */
+	/* If ret is 1 then we just hit the end of the extent array */
 	if (ret == 1)
 		ret = 0;
 
diff --git a/include/linux/fs.h b/include/linux/fs.h
index ebb1cd5..7e6e47f 100644
--- a/include/linux/fs.h
+++ b/include/linux/fs.h
@@ -2313,8 +2313,9 @@ extern int vfs_fstatat(int , char __user *, struct kstat *, int);
 extern int do_vfs_ioctl(struct file *filp, unsigned int fd, unsigned int cmd,
 		    unsigned long arg);
 extern int __generic_block_fiemap(struct inode *inode,
-				  struct fiemap_extent_info *fieinfo, u64 start,
-				  u64 len, get_block_t *get_block);
+				  struct fiemap_extent_info *fieinfo,
+				  loff_t start, loff_t len,
+				  get_block_t *get_block);
 extern int generic_block_fiemap(struct inode *inode,
 				struct fiemap_extent_info *fieinfo, u64 start,
 				u64 len, get_block_t *get_block);
-- 
1.6.5.rc2

Re: [PATCH] cleanup __generic_block_fiemap and fix a few minor issues

[Randy Dunlap]

On Thu, 4 Mar 2010 11:20:40 -0500 Josef Bacik wrote:

> Ahh yes thats much nicer, thank you.  Here is my updated patch with your
> suggestions.  Thanks,
> 
> Signed-off-by: Josef Bacik <josef@redhat.com>
> ---
>  fs/ioctl.c         |   81 +++++++++++++++++++++++++++++----------------------
>  include/linux/fs.h |    5 ++-
>  2 files changed, 49 insertions(+), 37 deletions(-)
> 
> diff --git a/fs/ioctl.c b/fs/ioctl.c
> index 6c75110..64fed0b 100644
> --- a/fs/ioctl.c
> +++ b/fs/ioctl.c
> @@ -228,13 +228,22 @@ static int ioctl_fiemap(struct file *filp, unsigned long arg)
>  
>  #ifdef CONFIG_BLOCK
>  
> -#define blk_to_logical(inode, blk) (blk << (inode)->i_blkbits)
> -#define logical_to_blk(inode, offset) (offset >> (inode)->i_blkbits);
> +static inline sector_t logical_to_blk(struct inode *inode, loff_t offset)
> +{
> +	return (offset >> inode->i_blkbits);
> +}
> +
> +static inline loff_t blk_to_logical(struct inode *inode, sector_t blk)
> +{
> +	return (blk << inode->i_blkbits);
> +}
>  
>  /**
>   * __generic_block_fiemap - FIEMAP for block based inodes (no locking)
>   * @inode - the inode to map
> - * @arg - the pointer to userspace where we copy everything to
> + * @fieinfo - the fiemap info struct that will be passed back to userspace
> + * @start - where to start mapping in the inode
> + * @len - how much space to map
>   * @get_block - the fs's get_block function
>   *
>   * This does FIEMAP for block based inodes.  Basically it will just loop

Please fix the kernel-doc notation.  Function parameter notation is like:

 * @start: where to start mapping in the inode

thanks,
---
~Randy

Re: [PATCH] cleanup __generic_block_fiemap and fix a few minor issues

[Josef Bacik]

On Thu, Mar 04, 2010 at 08:50:21AM -0800, Randy Dunlap wrote:
> On Thu, 4 Mar 2010 11:20:40 -0500 Josef Bacik wrote:
> 
> > Ahh yes thats much nicer, thank you.  Here is my updated patch with your
> > suggestions.  Thanks,
> > 
> > Signed-off-by: Josef Bacik <josef@redhat.com>
> > ---
> >  fs/ioctl.c         |   81 +++++++++++++++++++++++++++++----------------------
> >  include/linux/fs.h |    5 ++-
> >  2 files changed, 49 insertions(+), 37 deletions(-)
> > 
> > diff --git a/fs/ioctl.c b/fs/ioctl.c
> > index 6c75110..64fed0b 100644
> > --- a/fs/ioctl.c
> > +++ b/fs/ioctl.c
> > @@ -228,13 +228,22 @@ static int ioctl_fiemap(struct file *filp, unsigned long arg)
> >  
> >  #ifdef CONFIG_BLOCK
> >  
> > -#define blk_to_logical(inode, blk) (blk << (inode)->i_blkbits)
> > -#define logical_to_blk(inode, offset) (offset >> (inode)->i_blkbits);
> > +static inline sector_t logical_to_blk(struct inode *inode, loff_t offset)
> > +{
> > +	return (offset >> inode->i_blkbits);
> > +}
> > +
> > +static inline loff_t blk_to_logical(struct inode *inode, sector_t blk)
> > +{
> > +	return (blk << inode->i_blkbits);
> > +}
> >  
> >  /**
> >   * __generic_block_fiemap - FIEMAP for block based inodes (no locking)
> >   * @inode - the inode to map
> > - * @arg - the pointer to userspace where we copy everything to
> > + * @fieinfo - the fiemap info struct that will be passed back to userspace
> > + * @start - where to start mapping in the inode
> > + * @len - how much space to map
> >   * @get_block - the fs's get_block function
> >   *
> >   * This does FIEMAP for block based inodes.  Basically it will just loop
> 
> Please fix the kernel-doc notation.  Function parameter notation is like:
> 
>  * @start: where to start mapping in the inode

Sounds good, fixed below.  Thanks,

Signed-off-by: Josef Bacik <josef@redhat.com>
---
 fs/ioctl.c         |   85 +++++++++++++++++++++++++++++----------------------
 include/linux/fs.h |    5 ++-
 2 files changed, 51 insertions(+), 39 deletions(-)

diff --git a/fs/ioctl.c b/fs/ioctl.c
index 6c75110..f330fec 100644
--- a/fs/ioctl.c
+++ b/fs/ioctl.c
@@ -228,14 +228,23 @@ static int ioctl_fiemap(struct file *filp, unsigned long arg)
 
 #ifdef CONFIG_BLOCK
 
-#define blk_to_logical(inode, blk) (blk << (inode)->i_blkbits)
-#define logical_to_blk(inode, offset) (offset >> (inode)->i_blkbits);
+static inline sector_t logical_to_blk(struct inode *inode, loff_t offset)
+{
+	return (offset >> inode->i_blkbits);
+}
+
+static inline loff_t blk_to_logical(struct inode *inode, sector_t blk)
+{
+	return (blk << inode->i_blkbits);
+}
 
 /**
  * __generic_block_fiemap - FIEMAP for block based inodes (no locking)
- * @inode - the inode to map
- * @arg - the pointer to userspace where we copy everything to
- * @get_block - the fs's get_block function
+ * @inode: the inode to map
+ * @fieinfo: the fiemap info struct that will be passed back to userspace
+ * @start: where to start mapping in the inode
+ * @len: how much space to map
+ * @get_block: the fs's get_block function
  *
  * This does FIEMAP for block based inodes.  Basically it will just loop
  * through get_block until we hit the number of extents we want to map, or we
@@ -250,46 +259,46 @@ static int ioctl_fiemap(struct file *filp, unsigned long arg)
  */
 
 int __generic_block_fiemap(struct inode *inode,
-			   struct fiemap_extent_info *fieinfo, u64 start,
-			   u64 len, get_block_t *get_block)
+			   struct fiemap_extent_info *fieinfo, loff_t start,
+			   loff_t len, get_block_t *get_block)
 {
-	struct buffer_head tmp;
-	unsigned long long start_blk;
-	long long length = 0, map_len = 0;
+	struct buffer_head map_bh;
+	sector_t start_blk, last_blk;
 	u64 logical = 0, phys = 0, size = 0;
 	u32 flags = FIEMAP_EXTENT_MERGED;
-	int ret = 0, past_eof = 0, whole_file = 0;
+	bool past_eof = false, whole_file = false;
+	int ret = 0;
 
-	if ((ret = fiemap_check_flags(fieinfo, FIEMAP_FLAG_SYNC)))
+	ret = fiemap_check_flags(fieinfo, FIEMAP_FLAG_SYNC);
+	if (ret)
 		return ret;
 
-	start_blk = logical_to_blk(inode, start);
-
-	length = (long long)min_t(u64, len, i_size_read(inode));
-	if (length < len)
-		whole_file = 1;
+	if (len >= i_size_read(inode)) {
+		whole_file = true;
+		len = i_size_read(inode);
+	}
 
-	map_len = length;
+	start_blk = logical_to_blk(inode, start);
+	last_blk = logical_to_blk(inode, start + len - 1);
 
 	do {
 		/*
 		 * we set b_size to the total size we want so it will map as
 		 * many contiguous blocks as possible at once
 		 */
-		memset(&tmp, 0, sizeof(struct buffer_head));
-		tmp.b_size = map_len;
+		memset(&map_bh, 0, sizeof(struct buffer_head));
+		map_bh.b_size = len;
 
-		ret = get_block(inode, start_blk, &tmp, 0);
+		ret = get_block(inode, start_blk, &map_bh, 0);
 		if (ret)
 			break;
 
 		/* HOLE */
-		if (!buffer_mapped(&tmp)) {
-			length -= blk_to_logical(inode, 1);
+		if (!buffer_mapped(&map_bh)) {
 			start_blk++;
 
 			/*
-			 * we want to handle the case where there is an
+			 * We want to handle the case where there is an
 			 * allocated block at the front of the file, and then
 			 * nothing but holes up to the end of the file properly,
 			 * to make sure that extent at the front gets properly
@@ -297,11 +306,11 @@ int __generic_block_fiemap(struct inode *inode,
 			 */
 			if (!past_eof &&
 			    blk_to_logical(inode, start_blk) >=
-			    blk_to_logical(inode, 0)+i_size_read(inode))
+			    blk_to_logical(inode, 0) + i_size_read(inode))
 				past_eof = 1;
 
 			/*
-			 * first hole after going past the EOF, this is our
+			 * First hole after going past the EOF, this is our
 			 * last extent
 			 */
 			if (past_eof && size) {
@@ -309,15 +318,18 @@ int __generic_block_fiemap(struct inode *inode,
 				ret = fiemap_fill_next_extent(fieinfo, logical,
 							      phys, size,
 							      flags);
-				break;
+			} else if (size) {
+				ret = fiemap_fill_next_extent(fieinfo, logical,
+							      phys, size, flags);
+				size = 0;
 			}
 
 			/* if we have holes up to/past EOF then we're done */
-			if (length <= 0 || past_eof)
+			if (start_blk > last_blk || past_eof || ret)
 				break;
 		} else {
 			/*
-			 * we have gone over the length of what we wanted to
+			 * We have gone over the length of what we wanted to
 			 * map, and it wasn't the entire file, so add the extent
 			 * we got last time and exit.
 			 *
@@ -331,7 +343,7 @@ int __generic_block_fiemap(struct inode *inode,
 			 * are good to go, just add the extent to the fieinfo
 			 * and break
 			 */
-			if (length <= 0 && !whole_file) {
+			if (start_blk > last_blk && !whole_file) {
 				ret = fiemap_fill_next_extent(fieinfo, logical,
 							      phys, size,
 							      flags);
@@ -351,11 +363,10 @@ int __generic_block_fiemap(struct inode *inode,
 			}
 
 			logical = blk_to_logical(inode, start_blk);
-			phys = blk_to_logical(inode, tmp.b_blocknr);
-			size = tmp.b_size;
+			phys = blk_to_logical(inode, map_bh.b_blocknr);
+			size = map_bh.b_size;
 			flags = FIEMAP_EXTENT_MERGED;
 
-			length -= tmp.b_size;
 			start_blk += logical_to_blk(inode, size);
 
 			/*
@@ -364,14 +375,14 @@ int __generic_block_fiemap(struct inode *inode,
 			 * is marked with FIEMAP_EXTENT_LAST
 			 */
 			if (!past_eof &&
-			    logical+size >=
-			    blk_to_logical(inode, 0)+i_size_read(inode))
-				past_eof = 1;
+			    logical + size >=
+			    blk_to_logical(inode, 0) + i_size_read(inode))
+				past_eof = true;
 		}
 		cond_resched();
 	} while (1);
 
-	/* if ret is 1 then we just hit the end of the extent array */
+	/* If ret is 1 then we just hit the end of the extent array */
 	if (ret == 1)
 		ret = 0;
 
diff --git a/include/linux/fs.h b/include/linux/fs.h
index ebb1cd5..7e6e47f 100644
--- a/include/linux/fs.h
+++ b/include/linux/fs.h
@@ -2313,8 +2313,9 @@ extern int vfs_fstatat(int , char __user *, struct kstat *, int);
 extern int do_vfs_ioctl(struct file *filp, unsigned int fd, unsigned int cmd,
 		    unsigned long arg);
 extern int __generic_block_fiemap(struct inode *inode,
-				  struct fiemap_extent_info *fieinfo, u64 start,
-				  u64 len, get_block_t *get_block);
+				  struct fiemap_extent_info *fieinfo,
+				  loff_t start, loff_t len,
+				  get_block_t *get_block);
 extern int generic_block_fiemap(struct inode *inode,
 				struct fiemap_extent_info *fieinfo, u64 start,
 				u64 len, get_block_t *get_block);
-- 
1.6.5.rc2

Re: [PATCH] cleanup __generic_block_fiemap and fix a few minor issues

[Randy Dunlap]

On Thu, 4 Mar 2010 12:12:54 -0500 Josef Bacik wrote:

> diff --git a/fs/ioctl.c b/fs/ioctl.c
> index 6c75110..f330fec 100644
> --- a/fs/ioctl.c
> +++ b/fs/ioctl.c
> @@ -228,14 +228,23 @@ static int ioctl_fiemap(struct file *filp, unsigned long arg)
>  
>  
>  /**
>   * __generic_block_fiemap - FIEMAP for block based inodes (no locking)
> - * @inode - the inode to map
> - * @arg - the pointer to userspace where we copy everything to
> - * @get_block - the fs's get_block function
> + * @inode: the inode to map
> + * @fieinfo: the fiemap info struct that will be passed back to userspace
> + * @start: where to start mapping in the inode
> + * @len: how much space to map
> + * @get_block: the fs's get_block function
>   *
>   * This does FIEMAP for block based inodes.  Basically it will just loop
>   * through get_block until we hit the number of extents we want to map, or we

That's good.  Thank you.

---
~Randy

Re: [PATCH] cleanup __generic_block_fiemap and fix a few minor issues

[Josef Bacik]

On Thu, Mar 04, 2010 at 12:12:54PM -0500, Josef Bacik wrote:
> On Thu, Mar 04, 2010 at 08:50:21AM -0800, Randy Dunlap wrote:
> > On Thu, 4 Mar 2010 11:20:40 -0500 Josef Bacik wrote:
> > 
> > > Ahh yes thats much nicer, thank you.  Here is my updated patch with your
> > > suggestions.  Thanks,
> > > 
> > > Signed-off-by: Josef Bacik <josef@redhat.com>
> > > ---
> > >  fs/ioctl.c         |   81 +++++++++++++++++++++++++++++----------------------
> > >  include/linux/fs.h |    5 ++-
> > >  2 files changed, 49 insertions(+), 37 deletions(-)
> > > 
> > > diff --git a/fs/ioctl.c b/fs/ioctl.c
> > > index 6c75110..64fed0b 100644
> > > --- a/fs/ioctl.c
> > > +++ b/fs/ioctl.c
> > > @@ -228,13 +228,22 @@ static int ioctl_fiemap(struct file *filp, unsigned long arg)
> > >  
> > >  #ifdef CONFIG_BLOCK
> > >  
> > > -#define blk_to_logical(inode, blk) (blk << (inode)->i_blkbits)
> > > -#define logical_to_blk(inode, offset) (offset >> (inode)->i_blkbits);
> > > +static inline sector_t logical_to_blk(struct inode *inode, loff_t offset)
> > > +{
> > > +	return (offset >> inode->i_blkbits);
> > > +}
> > > +
> > > +static inline loff_t blk_to_logical(struct inode *inode, sector_t blk)
> > > +{
> > > +	return (blk << inode->i_blkbits);
> > > +}
> > >  
> > >  /**
> > >   * __generic_block_fiemap - FIEMAP for block based inodes (no locking)
> > >   * @inode - the inode to map
> > > - * @arg - the pointer to userspace where we copy everything to
> > > + * @fieinfo - the fiemap info struct that will be passed back to userspace
> > > + * @start - where to start mapping in the inode
> > > + * @len - how much space to map
> > >   * @get_block - the fs's get_block function
> > >   *
> > >   * This does FIEMAP for block based inodes.  Basically it will just loop
> > 
> > Please fix the kernel-doc notation.  Function parameter notation is like:
> > 
> >  * @start: where to start mapping in the inode
> 
> Sounds good, fixed below.  Thanks,
>

Is there anything else that needs to be done with this?  I just noticed it did
not get pulled in yet, just wanted to make sure there are no outstanding issues.
Thanks,

Josef