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

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

[Changli Gao]

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@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);

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

[Borislav Petkov]

From: Changli Gao <xiaosuo@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@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.

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

[Changli Gao]

On Sat, May 29, 2010 at 8:10 PM, Borislav Petkov <bp@alien8.de> wrote:
> From: Changli Gao <xiaosuo@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@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@gmail.com)
--
To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Borislav Petkov]

From: Changli Gao <xiaosuo@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.

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

[Changli Gao]

On Sat, May 29, 2010 at 9:46 PM, Borislav Petkov <bp@alien8.de> wrote:
> From: Changli Gao <xiaosuo@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@gmail.com)
--
To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Al Viro]

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

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

[Borislav Petkov]

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.

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

[Al Viro]

On Fri, Jun 04, 2010 at 10:13:22AM +0200, Borislav Petkov wrote:
> > > > -	bio = do_mpage_readpage(bio, page, 1, &last_block_in_bio,
> > > > +	bio = do_mpage_readpage(NULL, page, 1, &last_block_in_bio,

> Right, the uninitialized warning above happens when you remove the NULL
> assignment, i.e.
> 
> struct bio *bio;
> 
> ...
> 
> bio = do_mpage_readpage(bio, ...)

WTF?  His patch does *NOT* leave you with bio = do_mpage_readpage(bio, ...),
it replaces that with bio = do_mpage_readpage(NULL, ...).

Which variant has produced a warning?

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

To make it even shorter, you've misapplied the patch.  Correct?

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

[Borislav Petkov]

From: Al Viro <viro@ZenIV.linux.org.uk>
Date: Fri, Jun 04, 2010 at 09:30:31AM +0100

> On Fri, Jun 04, 2010 at 10:13:22AM +0200, Borislav Petkov wrote:
> > > > > -	bio = do_mpage_readpage(bio, page, 1, &last_block_in_bio,
> > > > > +	bio = do_mpage_readpage(NULL, page, 1, &last_block_in_bio,
> 
> > Right, the uninitialized warning above happens when you remove the NULL
> > assignment, i.e.
> > 
> > struct bio *bio;
> > 
> > ...
> > 
> > bio = do_mpage_readpage(bio, ...)
> 
> WTF?  His patch does *NOT* leave you with bio = do_mpage_readpage(bio, ...),
> it replaces that with bio = do_mpage_readpage(NULL, ...).
> 
> Which variant has produced a warning?
> 
> > 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.
> 
> To make it even shorter, you've misapplied the patch.  Correct?

Yes.

-- 
Regards/Gruss,
Boris.