fs: optimize mpage_readpage()
in [Kernel]
|
From: Borislav Petkov on 29 May 2010 08:20 From: Changli Gao <xiaosuo(a)gmail.com> Date: Sat, May 29, 2010 at 09:18:46AM +0800 > optimize mpage_readpage() > > we don't need to initialize bio, and pass it to do_mpage_readpage() as > parameter, as it is returned by do_mpage_readpage(). > > Signed-off-by: Changli Gao <xiaosuo(a)gmail.com> > ---- > fs/mpage.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > diff --git a/fs/mpage.c b/fs/mpage.c > index fd56ca2..94ff0d1 100644 > --- a/fs/mpage.c > +++ b/fs/mpage.c > @@ -409,14 +409,14 @@ EXPORT_SYMBOL(mpage_readpages); > */ > int mpage_readpage(struct page *page, get_block_t get_block) > { > - struct bio *bio = NULL; > + struct bio *bio; > sector_t last_block_in_bio = 0; > struct buffer_head map_bh; > unsigned long first_logical_block = 0; > > map_bh.b_state = 0; > map_bh.b_size = 0; > - bio = do_mpage_readpage(bio, page, 1, &last_block_in_bio, > + bio = do_mpage_readpage(NULL, page, 1, &last_block_in_bio, > &map_bh, &first_logical_block, get_block); > if (bio) > mpage_bio_submit(READ, bio); Nope, I don't think that's a good idea. On the one hand, this is a trick to shut up gcc: fs/mpage.c: In function 'mpage_readpage': fs/mpage.c:419: warning: 'bio' is used uninitialized in this function and, on the other hand, make sure bio is NULL and not some random stack value since bio is explicitly checked for NULL in do_mpage_readpage(). -- Regards/Gruss, Boris. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo(a)vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
From: Changli Gao on 29 May 2010 09:30 On Sat, May 29, 2010 at 8:10 PM, Borislav Petkov <bp(a)alien8.de> wrote: > From: Changli Gao <xiaosuo(a)gmail.com> > Date: Sat, May 29, 2010 at 09:18:46AM +0800 > >> optimize mpage_readpage() >> >> we don't need to initialize bio, and pass it to do_mpage_readpage() as >> parameter, as it is returned by do_mpage_readpage(). >> >> Signed-off-by: Changli Gao <xiaosuo(a)gmail.com> >> ---- >> fs/mpage.c | 4 ++-- >> 1 file changed, 2 insertions(+), 2 deletions(-) >> diff --git a/fs/mpage.c b/fs/mpage.c >> index fd56ca2..94ff0d1 100644 >> --- a/fs/mpage.c >> +++ b/fs/mpage.c >> @@ -409,14 +409,14 @@ EXPORT_SYMBOL(mpage_readpages); >> */ >> int mpage_readpage(struct page *page, get_block_t get_block) >> { >> - struct bio *bio = NULL; >> + struct bio *bio; >> sector_t last_block_in_bio = 0; >> struct buffer_head map_bh; >> unsigned long first_logical_block = 0; >> >> map_bh.b_state = 0; >> map_bh.b_size = 0; >> - bio = do_mpage_readpage(bio, page, 1, &last_block_in_bio, >> + bio = do_mpage_readpage(NULL, page, 1, &last_block_in_bio, >> &map_bh, &first_logical_block, get_block); >> if (bio) >> mpage_bio_submit(READ, bio); > > Nope, I don't think that's a good idea. > > On the one hand, this is a trick to shut up gcc: > > fs/mpage.c: In function 'mpage_readpage': > fs/mpage.c:419: warning: 'bio' is used uninitialized in this function > > and, on the other hand, make sure bio is NULL and not some random stack > value since bio is explicitly checked for NULL in do_mpage_readpage(). > Did the compiler warning appear after applying my patch? It doesn't happen when I testing it. And I don't pass bio to do_mpage_readpage(), but I pass NULL instead. -- Regards， Changli Gao(xiaosuo(a)gmail.com) -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo(a)vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
From: Borislav Petkov on 29 May 2010 09:50 From: Changli Gao <xiaosuo(a)gmail.com> Date: Sat, May 29, 2010 at 09:26:47PM +0800 > Did the compiler warning appear after applying my patch? It doesn't > happen when I testing it. yep, using gcc (Debian 4.4.4-2) 4.4.4 here. > And I don't pass bio to do_mpage_readpage(), but I pass NULL instead. True, but you still need that bio pointer so you might just as well set it to NULL as the original code does. -- Regards/Gruss, Boris. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo(a)vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
From: Changli Gao on 29 May 2010 10:20 On Sat, May 29, 2010 at 9:46 PM, Borislav Petkov <bp(a)alien8.de> wrote: > From: Changli Gao <xiaosuo(a)gmail.com> > Date: Sat, May 29, 2010 at 09:26:47PM +0800 > >> Did the compiler warning appear after applying my patch? It doesn't >> happen when I testing it. > > yep, using gcc (Debian 4.4.4-2) 4.4.4 here. > >> And I don't pass bio to do_mpage_readpage(), but I pass NULL instead. > > True, but you still need that bio pointer so you might just as well set > it to NULL as the original code does. > I'm using gcc 4.4.3(Gentoo). It's strange. on that line, bio is only used as left value. Maybe it is a compiler bug, or you didn't apply my patch properly. -- Regards， Changli Gao(xiaosuo(a)gmail.com) -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo(a)vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
From: Al Viro on 4 Jun 2010 03:20
On Sat, May 29, 2010 at 02:10:19PM +0200, Borislav Petkov wrote: > > - struct bio *bio = NULL; > > + struct bio *bio; > > sector_t last_block_in_bio = 0; > > struct buffer_head map_bh; > > unsigned long first_logical_block = 0; > > > > map_bh.b_state = 0; > > map_bh.b_size = 0; > > - bio = do_mpage_readpage(bio, page, 1, &last_block_in_bio, > > + bio = do_mpage_readpage(NULL, page, 1, &last_block_in_bio, > > &map_bh, &first_logical_block, get_block); > > if (bio) > > mpage_bio_submit(READ, bio); > > Nope, I don't think that's a good idea. > > On the one hand, this is a trick to shut up gcc: > > fs/mpage.c: In function ???mpage_readpage???: > fs/mpage.c:419: warning: ???bio??? is used uninitialized in this function File a bug against your version of gcc, then. The very first operation involving bio is assignment to it; if gcc complains about that, it's extremely fscked up. Said that, I don't see how could that be an optimization. Recent gcc is apparently b0rken in dead stores elimination, but that seems to be triggered by passing address of variable to another function later on [1]; nothing of that kind happens here. [1] gcc 4.3 and later (at least) fails to eliminate the first assignment in int foo(void) { extern int f(void); extern int g(int *); int x; x = 0; x = f(); return g(&x); } with any optimization level (and apparently on any target). -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo(a)vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/ |