[PATCH] fiemap: fix problem with setting FIEMAP_EXTENT_LAST

[PATCH] fiemap: fix problem with setting FIEMAP_EXTENT_LAST

[Josef Bacik]

This patch fixes a problem where the generic block based fiemap stuff would not
properly set FIEMAP_EXTENT_LAST on the last extent.  I've reworked things to
keep track if we go past the EOF, and mark the last extent properly.  The
problem was reported by and tested by Eric Sandeen.

Tested-by: Eric Sandeen <sandeen@redhat.com>
Signed-off-by: Josef Bacik <jbacik@redhat.com>
---
 fs/ioctl.c |   75 ++++++++++++++++++++++++++++++++++++++++++++----------------
 1 files changed, 55 insertions(+), 20 deletions(-)

diff --git a/fs/ioctl.c b/fs/ioctl.c
index ac2d47e..82d9c42 100644
--- a/fs/ioctl.c
+++ b/fs/ioctl.c
@@ -258,7 +258,7 @@ int __generic_block_fiemap(struct inode *inode,
 	long long length = 0, map_len = 0;
 	u64 logical = 0, phys = 0, size = 0;
 	u32 flags = FIEMAP_EXTENT_MERGED;
-	int ret = 0;
+	int ret = 0, past_eof = 0, whole_file = 0;
 
 	if ((ret = fiemap_check_flags(fieinfo, FIEMAP_FLAG_SYNC)))
 		return ret;
@@ -266,6 +266,9 @@ int __generic_block_fiemap(struct inode *inode,
 	start_blk = logical_to_blk(inode, start);
 
 	length = (long long)min_t(u64, len, i_size_read(inode));
+	if (length < len)
+		whole_file = 1;
+
 	map_len = length;
 
 	do {
@@ -282,11 +285,26 @@ int __generic_block_fiemap(struct inode *inode,
 
 		/* HOLE */
 		if (!buffer_mapped(&tmp)) {
+			length -= blk_to_logical(inode, 1);
+			start_blk++;
+
+			/*
+			 * 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
+			 * marked with FIEMAP_EXTENT_LAST
+			 */
+			if (!past_eof &&
+			    blk_to_logical(inode, start_blk) >=
+			    blk_to_logical(inode, 0)+i_size_read(inode))
+				past_eof = 1;
+
 			/*
 			 * first hole after going past the EOF, this is our
 			 * last extent
 			 */
-			if (length <= 0) {
+			if (past_eof && size) {
 				flags = FIEMAP_EXTENT_MERGED|FIEMAP_EXTENT_LAST;
 				ret = fiemap_fill_next_extent(fieinfo, logical,
 							      phys, size,
@@ -294,15 +312,37 @@ int __generic_block_fiemap(struct inode *inode,
 				break;
 			}
 
-			length -= blk_to_logical(inode, 1);
-
 			/* if we have holes up to/past EOF then we're done */
-			if (length <= 0)
+			if (length <= 0 || past_eof)
 				break;
-
-			start_blk++;
 		} else {
-			if (length <= 0 && size) {
+			/*
+			 * 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.
+			 *
+			 * This is for the case where say we want to map all the
+			 * way up to the second to the last block in a file, but
+			 * the last block is a hole, making the second to last
+			 * block FIEMAP_EXTENT_LAST.  In this case we want to
+			 * see if there is a hole after the second to last block
+			 * so we can mark it properly.  If we found data after
+			 * we exceeded the length we were requesting, then we
+			 * are good to go, just add the extent to the fieinfo
+			 * and break
+			 */
+			if (length <= 0 && !whole_file) {
+				ret = fiemap_fill_next_extent(fieinfo, logical,
+							      phys, size,
+							      flags);
+				break;
+			}
+
+			/*
+			 * if size != 0 then we know we already have an extent
+			 * to add, so add it.
+			 */
+			if (size) {
 				ret = fiemap_fill_next_extent(fieinfo, logical,
 							      phys, size,
 							      flags);
@@ -319,19 +359,14 @@ int __generic_block_fiemap(struct inode *inode,
 			start_blk += logical_to_blk(inode, size);
 
 			/*
-			 * if we are past the EOF we need to loop again to see
-			 * if there is a hole so we can mark this extent as the
-			 * last one, and if not keep mapping things until we
-			 * find a hole, or we run out of slots in the extent
-			 * array
+			 * If we are past the EOF, then we need to make sure as
+			 * soon as we find a hole that the last extent we found
+			 * is marked with FIEMAP_EXTENT_LAST
 			 */
-			if (length <= 0)
-				continue;
-
-			ret = fiemap_fill_next_extent(fieinfo, logical, phys,
-						      size, flags);
-			if (ret)
-				break;
+			if (!past_eof &&
+			    logical+size >=
+			    blk_to_logical(inode, 0)+i_size_read(inode))
+				past_eof = 1;
 		}
 		cond_resched();
 	} while (1);
-- 
1.6.0.6

Re: [PATCH] fiemap: fix problem with setting FIEMAP_EXTENT_LAST

[Andrew Morton]

On Thu, 30 Apr 2009 13:44:51 -0400
Josef Bacik <jbacik@redhat.com> wrote:

> This patch fixes a problem where the generic block based fiemap stuff would not
> properly set FIEMAP_EXTENT_LAST on the last extent.  I've reworked things to
> keep track if we go past the EOF, and mark the last extent properly.  The
> problem was reported by and tested by Eric Sandeen.
> 

bleeearrggh.  __generic_block_fiemap() needs to be dragged out, shot,
stabbed and doused in gasoline.

- uses stupid helper macros (blk_to_logical, logical_to_blk), thus
  carefully obscuring the types of their incoming args and return value.

- has kerneldoc which documents non-existent arguments and fails to
  document the actual arguments.

- has a local variable called `tmp'.

- Uses random and seemingly irrational mixture of u64's and `long
  long's, thus carefully confusing readers who would prefer to see
  loff_t, sector_t, etc so they have a fighting chance of understanding
  what the hell the thing does.

- exists as useful counterexample for Documentation/CodingStyle

- has undocumented locals called "length", "len" and "map_len", to
  avoid any possibility of confusion.

- has a local called `start_blk' which has a suspiciously-small
  32-bit type.  Because of all the above intense obfuscation efforts I
  just cannot be assed working out whether or not if this is an actual
  bug.


Who the heck merged this turkey?

<checks>

Oh good, it wasn't me.

your patch:

- adds a string of booleans, unhelpfully typed with plain old `int'.

- seems to think that sentences start with lower-case letters.


Sigh.

Does this bugfix need to be backported into 2.6.29?  Earlier?  If so, why?

Thanks.

Re: [PATCH] fiemap: fix problem with setting FIEMAP_EXTENT_LAST

[Josef Bacik]

On Thu, Apr 30, 2009 at 03:40:47PM -0700, Andrew Morton wrote:
> On Thu, 30 Apr 2009 13:44:51 -0400
> Josef Bacik <jbacik@redhat.com> wrote:
> 
> > This patch fixes a problem where the generic block based fiemap stuff would not
> > properly set FIEMAP_EXTENT_LAST on the last extent.  I've reworked things to
> > keep track if we go past the EOF, and mark the last extent properly.  The
> > problem was reported by and tested by Eric Sandeen.
> > 
> 
> bleeearrggh.  __generic_block_fiemap() needs to be dragged out, shot,
> stabbed and doused in gasoline.
> 
> - uses stupid helper macros (blk_to_logical, logical_to_blk), thus
>   carefully obscuring the types of their incoming args and return value.
>

I did this to make it a bit more cleaner, so I didn't have a bunch of 

(blk << (inode)->i_blkbits)

type statements everywhere.  Do you have a better suggestion?  Would you like an
inline function, or do you want me to pepper the function with this stuff?
 
> - has kerneldoc which documents non-existent arguments and fails to
>   document the actual arguments.
>

Yes thats my fault, sorry.  Things got changed between my original submissions
and I never fixed that, I will take care of that now.
 
> - has a local variable called `tmp'.
>

Sorry, fixing that as well.
 
> - Uses random and seemingly irrational mixture of u64's and `long
>   long's, thus carefully confusing readers who would prefer to see
>   loff_t, sector_t, etc so they have a fighting chance of understanding
>   what the hell the thing does.
>

Hrm well I needed the long long to see if we mapped past where we wanted to, but
I can do that a different way.  I will fix this as well.
 
> - exists as useful counterexample for Documentation/CodingStyle
> 
> - has undocumented locals called "length", "len" and "map_len", to
>   avoid any possibility of confusion.
> 
> - has a local called `start_blk' which has a suspiciously-small
>   32-bit type.  Because of all the above intense obfuscation efforts I
>   just cannot be assed working out whether or not if this is an actual
>   bug.
> 
> 
> Who the heck merged this turkey?
> 
> <checks>
> 
> Oh good, it wasn't me.
> 
> your patch:
> 
> - adds a string of booleans, unhelpfully typed with plain old `int'.
> 
> - seems to think that sentences start with lower-case letters.
> 
>

I will fix all of this as well, sorry.
 
> Sigh.
> 
> Does this bugfix need to be backported into 2.6.29?  Earlier?  If so, why?
>

Yes it does, it will screw anybody up who tries to use fiemap beyond just the
simple cases.  I will send you a updated patch shortly.  Thanks for the review,

Josef 

Re: [PATCH] fiemap: fix problem with setting FIEMAP_EXTENT_LAST

[Andrew Morton]

On Fri, 1 May 2009 09:32:26 -0400 Josef Bacik <josef@redhat.com> wrote:

> On Thu, Apr 30, 2009 at 03:40:47PM -0700, Andrew Morton wrote:
> > On Thu, 30 Apr 2009 13:44:51 -0400
> > Josef Bacik <jbacik@redhat.com> wrote:
> > 
> > > This patch fixes a problem where the generic block based fiemap stuff would not
> > > properly set FIEMAP_EXTENT_LAST on the last extent.  I've reworked things to
> > > keep track if we go past the EOF, and mark the last extent properly.  The
> > > problem was reported by and tested by Eric Sandeen.
> > > 
> > 
> > bleeearrggh.  __generic_block_fiemap() needs to be dragged out, shot,
> > stabbed and doused in gasoline.
> > 
> > - uses stupid helper macros (blk_to_logical, logical_to_blk), thus
> >   carefully obscuring the types of their incoming args and return value.
> >
> 
> I did this to make it a bit more cleaner, so I didn't have a bunch of 
> 
> (blk << (inode)->i_blkbits)
> 
> type statements everywhere.  Do you have a better suggestion?  Would you like an
> inline function, or do you want me to pepper the function with this stuff?

An inline would be much better - the C type information in this sort of
code can be a big help if well-used.

> > - Uses random and seemingly irrational mixture of u64's and `long
> >   long's, thus carefully confusing readers who would prefer to see
> >   loff_t, sector_t, etc so they have a fighting chance of understanding
> >   what the hell the thing does.
> >
> 
> Hrm well I needed the long long to see if we mapped past where we wanted to, but
> I can do that a different way.  I will fix this as well.

It's useful to choose the appopriate type, if it exists.

What does this long-long actually contain?  A file offset?  If so, use
loff_t.  A block number?  Use sector_t.  etc.

Also, when you have a function which has a large number of locals (and
__generic_block_fiemap has 16!), comment them!  The thing which pips me
of about the

	int a, b, c, d, e;

style is that it leaves no room for comments.  Look:

	loff_t a;	/* offset in file */
	sector_t b;	/* relative block number in extent */

etc.

> > Does this bugfix need to be backported into 2.6.29?  Earlier?  If so, why?
> >
> 
> Yes it does, it will screw anybody up who tries to use fiemap beyond just the
> simple cases.

OK.

> I will send you a updated patch shortly.  Thanks for the review,

I don't believe the patch needed updating, did it?  At this stege we'd
be best off with a minimal fixup for -stable and for 2.6.30.

Re: [PATCH] fiemap: fix problem with setting FIEMAP_EXTENT_LAST

[Josef Bacik]

On Thu, Apr 30, 2009 at 03:40:47PM -0700, Andrew Morton wrote:
> On Thu, 30 Apr 2009 13:44:51 -0400
> Josef Bacik <jbacik@redhat.com> wrote:
> 
> > This patch fixes a problem where the generic block based fiemap stuff would not
> > properly set FIEMAP_EXTENT_LAST on the last extent.  I've reworked things to
> > keep track if we go past the EOF, and mark the last extent properly.  The
> > problem was reported by and tested by Eric Sandeen.
> > 
> 
> bleeearrggh.  __generic_block_fiemap() needs to be dragged out, shot,
> stabbed and doused in gasoline.
> 
> - uses stupid helper macros (blk_to_logical, logical_to_blk), thus
>   carefully obscuring the types of their incoming args and return value.
> 
> - has kerneldoc which documents non-existent arguments and fails to
>   document the actual arguments.
> 
> - has a local variable called `tmp'.
> 
> - Uses random and seemingly irrational mixture of u64's and `long
>   long's, thus carefully confusing readers who would prefer to see
>   loff_t, sector_t, etc so they have a fighting chance of understanding
>   what the hell the thing does.
> 
> - exists as useful counterexample for Documentation/CodingStyle
> 
> - has undocumented locals called "length", "len" and "map_len", to
>   avoid any possibility of confusion.
> 
> - has a local called `start_blk' which has a suspiciously-small
>   32-bit type.  Because of all the above intense obfuscation efforts I
>   just cannot be assed working out whether or not if this is an actual
>   bug.
> 
> 
> Who the heck merged this turkey?
> 
> <checks>
> 
> Oh good, it wasn't me.
> 
> your patch:
> 
> - adds a string of booleans, unhelpfully typed with plain old `int'.
> 
> - seems to think that sentences start with lower-case letters.
> 
> 
> Sigh.
> 
> Does this bugfix need to be backported into 2.6.29?  Earlier?  If so, why?
>

Here is an updated patch.  What am I supposed to do about these types?
inode->i_size is loff_t, bh->b_size is size_t, bh->b_blocknr is sector_t, not to
mention that all of the fieinfo fields are either u64 or u32.  Is what I've done
acceptable, or is there some other magic I'm supposed to be doing?  I've looked
around some and it seems that people just kind of ignore the types, they're just
there to explain the variables.  I just would like to better understand this so
I can avoid getting chewed out in the future.  Thanks,

Josef

diff --git a/fs/ioctl.c b/fs/ioctl.c
index ac2d47e..ee9fba0 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,43 +259,63 @@ 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 int start_blk;
-	long long length = 0, map_len = 0;
+	struct buffer_head map_bh;
+	sector_t start_blk;
+	loff_t end;
 	u64 logical = 0, phys = 0, size = 0;
 	u32 flags = FIEMAP_EXTENT_MERGED;
+	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));
-	map_len = length;
+	if (len >= i_size_read(inode)) {
+		whole_file = true;
+		len = i_size_read(inode);
+	}
+
+	end = start + len;
 
 	do {
 		/*
-		 * we set b_size to the total size we want so it will map as
+		 * 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)) {
+		if (!buffer_mapped(&map_bh)) {
+			start_blk++;
+
 			/*
-			 * first hole after going past the EOF, this is our
+			 * 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
+			 * marked with FIEMAP_EXTENT_LAST
+			 */
+			if (!past_eof &&
+			    blk_to_logical(inode, start_blk) >=
+			    blk_to_logical(inode, 0) + i_size_read(inode))
+				past_eof = true;
+
+			/*
+			 * First hole after going past the EOF, this is our
 			 * last extent
 			 */
-			if (length <= 0) {
+			if (past_eof && size) {
 				flags = FIEMAP_EXTENT_MERGED|FIEMAP_EXTENT_LAST;
 				ret = fiemap_fill_next_extent(fieinfo, logical,
 							      phys, size,
@@ -294,15 +323,39 @@ int __generic_block_fiemap(struct inode *inode,
 				break;
 			}
 
-			length -= blk_to_logical(inode, 1);
-
-			/* if we have holes up to/past EOF then we're done */
-			if (length <= 0)
+			/* If we have holes up to/past EOF then we're done */
+			if (blk_to_logical(inode, start_blk) >= end
+			    || past_eof)
 				break;
-
-			start_blk++;
 		} else {
-			if (length <= 0 && size) {
+			/*
+			 * 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.
+			 *
+			 * This is for the case where say we want to map all the
+			 * way up to the second to the last block in a file, but
+			 * the last block is a hole, making the second to last
+			 * block FIEMAP_EXTENT_LAST.  In this case we want to
+			 * see if there is a hole after the second to last block
+			 * so we can mark it properly.  If we found data after
+			 * we exceeded the length we were requesting, then we
+			 * are good to go, just add the extent to the fieinfo
+			 * and break
+			 */
+			if (blk_to_logical(inode, start_blk) >= end
+			    && !whole_file) {
+				ret = fiemap_fill_next_extent(fieinfo, logical,
+							      phys, size,
+							      flags);
+				break;
+			}
+
+			/*
+			 * If size != 0 then we know we already have an extent
+			 * to add, so add it.
+			 */
+			if (size) {
 				ret = fiemap_fill_next_extent(fieinfo, logical,
 							      phys, size,
 							      flags);
@@ -311,32 +364,26 @@ 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);
 
 			/*
-			 * if we are past the EOF we need to loop again to see
-			 * if there is a hole so we can mark this extent as the
-			 * last one, and if not keep mapping things until we
-			 * find a hole, or we run out of slots in the extent
-			 * array
+			 * If we are past the EOF, then we need to make sure as
+			 * soon as we find a hole that the last extent we found
+			 * is marked with FIEMAP_EXTENT_LAST
 			 */
-			if (length <= 0)
-				continue;
-
-			ret = fiemap_fill_next_extent(fieinfo, logical, phys,
-						      size, flags);
-			if (ret)
-				break;
+			if (!past_eof &&
+			    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 5bed436..b23c725 100644
--- a/include/linux/fs.h
+++ b/include/linux/fs.h
@@ -2305,8 +2305,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);

Re: [PATCH] fiemap: fix problem with setting FIEMAP_EXTENT_LAST

[Andrew Morton]

On Fri, 1 May 2009 10:35:17 -0400
Josef Bacik <josef@redhat.com> wrote:

> 
> Here is an updated patch.

This looks heaps better to me.

Officially we shouldn't do this sort of thing in a -stable/-rc4 patch. 
We should go for a minimal backportable fixup then do the cleanups
later, in an -rc1.

But we could sneak this in anyway, I expect.  Which way would you
prefer?

>  What am I supposed to do about these types?
> inode->i_size is loff_t, bh->b_size is size_t, bh->b_blocknr is sector_t, not to
> mention that all of the fieinfo fields are either u64 or u32.

We're missing a number of types and sometimes it's hard to find one
which is appropriate for the problem at hand.

It is appropriate that the fields which are communicated with userspace
have the bare u32/u64 types.  One way of handling these
information-free types is to name them well.  Another is to avoid using
them until the last possible moment - choose well-typed and well-named
locals and copy then to/from the target structure immediately
before/after the userspace copy in/out.

>  Is what I've done
> acceptable, or is there some other magic I'm supposed to be doing?

Looks better ;)

>  I've looked
> around some and it seems that people just kind of ignore the types, they're just
> there to explain the variables.

Yup, explaiing the variables is a major reason for the existence of 
these types.

This sort of code tends to get very confusing.  Look at
do_mpage_readpage() and weep.

>  I just would like to better understand this so
> I can avoid getting chewed out in the future.

yeah, well, sorry, that was a bit of a rant.  I saw the patch and tried
to understand it but this required understanding the surrounding code,
and it immediately became clear that this was going to take waaaay more
time that it should.

> --- 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);
> +}

That is so much better!

>  /**
>   * __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

space_to_map ;)

>   * @get_block - the fs's get_block function
>   *
>   * This does FIEMAP for block based inodes.  Basically it will just loop
> @@ -250,43 +259,63 @@ 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 int start_blk;
> -	long long length = 0, map_len = 0;
> +	struct buffer_head map_bh;

yep, map_bh is a well-known identifier for an on-stack bh which is
going to be used for get_block()ing.  Sticking to established patterns
is good.


> +	sector_t start_blk;
> +	loff_t end;

"end" of what?  file/sector/block/page/extent/etc?  The type loff_t is
a hint that it's measuring a file length.

>  	u64 logical = 0, phys = 0, size = 0;
>  	u32 flags = FIEMAP_EXTENT_MERGED;
> +	bool past_eof = false, whole_file = false;
>  	int ret = 0;
>
> ...
>

Are you really really confident that this is still correct?  No
mysterious shift overflows from type changes, nothing subtle like that?

Or would you be more confident if we ran with version 1 for now?

Re: [PATCH] fiemap: fix problem with setting FIEMAP_EXTENT_LAST

[Josef Bacik]

On Fri, May 01, 2009 at 12:50:43PM -0700, Andrew Morton wrote:
> On Fri, 1 May 2009 10:35:17 -0400
> Josef Bacik <josef@redhat.com> wrote:
> 
> > 
> > Here is an updated patch.
> 
> This looks heaps better to me.
> 
> Officially we shouldn't do this sort of thing in a -stable/-rc4 patch. 
> We should go for a minimal backportable fixup then do the cleanups
> later, in an -rc1.
> 
> But we could sneak this in anyway, I expect.  Which way would you
> prefer?
> 
> >  What am I supposed to do about these types?
> > inode->i_size is loff_t, bh->b_size is size_t, bh->b_blocknr is sector_t, not to
> > mention that all of the fieinfo fields are either u64 or u32.
> 
> We're missing a number of types and sometimes it's hard to find one
> which is appropriate for the problem at hand.
> 
> It is appropriate that the fields which are communicated with userspace
> have the bare u32/u64 types.  One way of handling these
> information-free types is to name them well.  Another is to avoid using
> them until the last possible moment - choose well-typed and well-named
> locals and copy then to/from the target structure immediately
> before/after the userspace copy in/out.
> 
> >  Is what I've done
> > acceptable, or is there some other magic I'm supposed to be doing?
> 
> Looks better ;)
> 
> >  I've looked
> > around some and it seems that people just kind of ignore the types, they're just
> > there to explain the variables.
> 
> Yup, explaiing the variables is a major reason for the existence of 
> these types.
> 
> This sort of code tends to get very confusing.  Look at
> do_mpage_readpage() and weep.
> 
> >  I just would like to better understand this so
> > I can avoid getting chewed out in the future.
> 
> yeah, well, sorry, that was a bit of a rant.  I saw the patch and tried
> to understand it but this required understanding the surrounding code,
> and it immediately became clear that this was going to take waaaay more
> time that it should.
> 
> > --- 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);
> > +}
> 
> That is so much better!
> 
> >  /**
> >   * __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
> 
> space_to_map ;)
> 
> >   * @get_block - the fs's get_block function
> >   *
> >   * This does FIEMAP for block based inodes.  Basically it will just loop
> > @@ -250,43 +259,63 @@ 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 int start_blk;
> > -	long long length = 0, map_len = 0;
> > +	struct buffer_head map_bh;
> 
> yep, map_bh is a well-known identifier for an on-stack bh which is
> going to be used for get_block()ing.  Sticking to established patterns
> is good.
> 
> 
> > +	sector_t start_blk;
> > +	loff_t end;
> 
> "end" of what?  file/sector/block/page/extent/etc?  The type loff_t is
> a hint that it's measuring a file length.
> 
> >  	u64 logical = 0, phys = 0, size = 0;
> >  	u32 flags = FIEMAP_EXTENT_MERGED;
> > +	bool past_eof = false, whole_file = false;
> >  	int ret = 0;
> >
> > ...
> >
> 
> Are you really really confident that this is still correct?  No
> mysterious shift overflows from type changes, nothing subtle like that?
> 
> Or would you be more confident if we ran with version 1 for now?

Thanks for the review, that was helpful.  I think this late in the game it's
probably best to just go with whats already in the kernel for now, and I'll
rework some of this stuff and work with Eric to get a good test suite set up to
make sure everything is working properly, and then shoot for -31.  Does that
sound good?  Thanks,

Josef

Re: [PATCH] fiemap: fix problem with setting FIEMAP_EXTENT_LAST

[Andrew Morton]

On Fri, 1 May 2009 16:16:46 -0400
Josef Bacik <josef@redhat.com> wrote:

> I think this late in the game it's
> probably best to just go with whats already in the kernel for now, and I'll
> rework some of this stuff and work with Eric to get a good test suite set up to
> make sure everything is working properly, and then shoot for -31.  Does that
> sound good?

yup, I agree.