lkml.org 
[lkml]   [2009]   [May]   [1]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH] fiemap: fix problem with setting FIEMAP_EXTENT_LAST
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.



\
 
 \ /
  Last update: 2009-05-01 20:35    [W:0.089 / U:0.108 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site