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@xxxxxxxxxx> 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 
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/