Re: [PATCH 1/2] fs: optimize mpage_readpage()

[Borislav Petkov <bp@alien8.de>]

From: Al Viro <viro@ZenIV.linux.org.uk>
Date: Fri, Jun 04, 2010 at 08:19:06AM +0100

> 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).

Right, the uninitialized warning above happens when you remove the NULL
assignment, i.e.

struct bio *bio;

...

bio = do_mpage_readpage(bio, ...)

I was trying to prove a point and obviously failed :). Thanks for
correcting me.

But this would normally be a false warning, similar to the one you
describe above - passing an address to a function even though the
function itself only writes to the variable at that address. Most
prominent example

u32 val;

..

pci_read_config_dword(..., &val);

This triggers at a couple of places in arch/x86/ with 2.6.35-rc1. For
example, see <arch/x86/kernel/quirks.c:ati_ixp4x0_rev()>, the u32 d
variable. And see also nvidia_force_enable_hpet() below where gcc is
being shut up with

u32 uninitialized_var(val);

But(!), in the mpage_readpage(), bio _absolutely_ has to be NULL because
it is checked if being so later in do_mpage_readpage(), so this one is a
complete different story.

To cut a long story short, you're correct, gcc is b0rked when warning
about passing addresses of variables to functions which only write to
them.

Such a warning might be valid if the variable doesn't have storage
allocated to it or if the receiving function reads it uninitialized but
I guess gcc can't "see" that across functions... Hmmm...

-- 
Regards/Gruss,
    Boris.