On Fri, 1 May 2009 09:32:26 -0400 Josef Bacik <josef@xxxxxxxxxx> 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@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? 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. -- To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
