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

[Andrew Morton]

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