From: Pramod Gurav <[email protected]>
Fixes below warning while compiling the kernel.
fs/direct-io.c: In function ‘__blockdev_direct_IO’:
fs/direct-io.c:1011:12: warning: ‘to’ may be used uninitialized in this function [-Wmaybe-uninitialized]
fs/direct-io.c:913:16: note: ‘to’ was declared here
fs/direct-io.c:1011:12: warning: ‘from’ may be used uninitialized in this function [-Wmaybe-uninitialized]
fs/direct-io.c:913:10: note: ‘from’ was declared here
Signed-off-by: Pramod Gurav <[email protected]>
CC: Alexander Viro <[email protected]>
CC: [email protected]
CC: [email protected]
---
fs/direct-io.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/fs/direct-io.c b/fs/direct-io.c
index 98040ba..6ad5d2c 100644
--- a/fs/direct-io.c
+++ b/fs/direct-io.c
@@ -910,7 +910,7 @@ static int do_direct_IO(struct dio *dio, struct dio_submit *sdio,
while (sdio->block_in_file < sdio->final_block_in_request) {
struct page *page;
- size_t from, to;
+ size_t from = 0, to = 0;
page = dio_get_page(dio, sdio, &from, &to);
if (IS_ERR(page)) {
ret = PTR_ERR(page);
--
1.7.9.5
Looks good to me,
Reviewed-by: Christoph Hellwig <[email protected]>
Al, can you pick this up? It's the only warnings in many of my usual
kernel builds at the moment.
Thanks Christoph for the review. :)
On Sun, Jul 13, 2014 at 5:20 PM, Christoph Hellwig <[email protected]> wrote:
> Looks good to me,
>
> Reviewed-by: Christoph Hellwig <[email protected]>
>
> Al, can you pick this up? It's the only warnings in many of my usual
> kernel builds at the moment.
>
--
Thanks and Regards
Pramod
On 07/13/2014 02:50 PM, Christoph Hellwig wrote:
> [email protected] wrote ...
>> diff --git a/fs/direct-io.c b/fs/direct-io.c
>> index 98040ba388ac..c0a9854d2bc7 100644
>> --- a/fs/direct-io.c
>> +++ b/fs/direct-io.c
>> @@ -910,7 +910,8 @@ static int do_direct_IO(struct dio *dio, struct dio_submit *sdio,
>>
>> while (sdio->block_in_file < sdio->final_block_in_request) {
>> struct page *page;
>> - size_t from, to;
>> + size_t from = 0;
>> + size_t to = 0;
>> page = dio_get_page(dio, sdio, &from, &to);
>> if (IS_ERR(page)) {
>> ret = PTR_ERR(page);
> Looks good to me,
>
> Reviewed-by: Christoph Hellwig <[email protected]>
>
> Al, can you pick this up? It's the only warnings in many of my usual
> kernel builds at the moment.
>
<>
Paul Bolle <[email protected]> wrote ...
<>
> @@ -911,11 +907,15 @@ static int do_direct_IO(struct dio *dio, struct dio_submit *sdio,
> while (sdio->block_in_file < sdio->final_block_in_request) {
> struct page *page;
> size_t from, to;
> - page = dio_get_page(dio, sdio, &from, &to);
> +
> + page = dio_get_page(dio, sdio);
> if (IS_ERR(page)) {
> ret = PTR_ERR(page);
> goto out;
> }
> + from = sdio->head ? 0 : sdio->from;
> + to = (sdio->head == sdio->tail - 1) ? sdio->to : PAGE_SIZE;
> + sdio->head++;
>
> while (from < to) {
> unsigned this_chunk_bytes; /* # of bytes mapped */
<>
This is the wrong fix. GCC is wrong here. As shown by Paul Bolle if
you move the from / to set from dio_get_page() to here the warning goes away.
The minimal fix must use uninitialized_var() in this case. See patch below
But I think the proper fix Is the one Paul Bolle <[email protected]> sent (above)
----
From: Boaz Harrosh <[email protected]>
Date: Wed, 16 Jul 2014 20:02:29 +0300
Subject: [PATCH] do_direct_IO: Fix compiler warning
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
This fixes
../fs/direct-io.c: In function ‘do_blockdev_direct_IO’:
../fs/direct-io.c:1011:12: warning: ‘from’ may be used uninitialized in this function [-Wmaybe-uninitialized]
u = (to - from) >> blkbits;
^
../fs/direct-io.c:1011:12: warning: ‘to’ may be used uninitialized in this function [-Wmaybe-uninitialized]
u = (to - from) >> blkbits;
I use: gcc version 4.8.3 20140624 (Red Hat 4.8.3-1)
GCC is wrong here so we should use the uninitialized_var() macro
and not silence it with = 0;
Signed-off-by: Boaz Harrosh <[email protected]>
---
fs/direct-io.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/fs/direct-io.c b/fs/direct-io.c
index 98040ba..156e6f0 100644
--- a/fs/direct-io.c
+++ b/fs/direct-io.c
@@ -910,7 +910,8 @@ static int do_direct_IO(struct dio *dio, struct dio_submit *sdio,
while (sdio->block_in_file < sdio->final_block_in_request) {
struct page *page;
- size_t from, to;
+ size_t uninitialized_var(from), uninitialized_var(to);
+
page = dio_get_page(dio, sdio, &from, &to);
if (IS_ERR(page)) {
ret = PTR_ERR(page);
--
1.9.3
On Wed, Jul 16, 2014 at 08:08:41PM +0300, Boaz Harrosh wrote:
> On 07/13/2014 02:50 PM, Christoph Hellwig wrote:
> > [email protected] wrote ...
> >> diff --git a/fs/direct-io.c b/fs/direct-io.c
> >> index 98040ba388ac..c0a9854d2bc7 100644
> >> --- a/fs/direct-io.c
> >> +++ b/fs/direct-io.c
> >> @@ -910,7 +910,8 @@ static int do_direct_IO(struct dio *dio, struct dio_submit *sdio,
> >>
> >> while (sdio->block_in_file < sdio->final_block_in_request) {
> >> struct page *page;
> >> - size_t from, to;
> >> + size_t from = 0;
> >> + size_t to = 0;
> >> page = dio_get_page(dio, sdio, &from, &to);
> >> if (IS_ERR(page)) {
> >> ret = PTR_ERR(page);
> > Looks good to me,
> >
> > Reviewed-by: Christoph Hellwig <[email protected]>
> >
> > Al, can you pick this up? It's the only warnings in many of my usual
> > kernel builds at the moment.
> >
> <>
>
> Paul Bolle <[email protected]> wrote ...
> <>
> > @@ -911,11 +907,15 @@ static int do_direct_IO(struct dio *dio, struct dio_submit *sdio,
> > while (sdio->block_in_file < sdio->final_block_in_request) {
> > struct page *page;
> > size_t from, to;
> > - page = dio_get_page(dio, sdio, &from, &to);
> > +
> > + page = dio_get_page(dio, sdio);
> > if (IS_ERR(page)) {
> > ret = PTR_ERR(page);
> > goto out;
> > }
> > + from = sdio->head ? 0 : sdio->from;
> > + to = (sdio->head == sdio->tail - 1) ? sdio->to : PAGE_SIZE;
> > + sdio->head++;
> >
> > while (from < to) {
> > unsigned this_chunk_bytes; /* # of bytes mapped */
> <>
>
> This is the wrong fix. GCC is wrong here. As shown by Paul Bolle if
> you move the from / to set from dio_get_page() to here the warning goes away.
Actually, that was me.
> The minimal fix must use uninitialized_var() in this case. See patch below
I didn't know about uninitialized_var() when I wrote the patch. As the
comment for it states:
/*
* A trick to suppress uninitialized variable warning without generating
* any code
*/
So,
> But I think the proper fix Is the one Paul Bolle <[email protected]> sent (above)
>
>
> ----
> From: Boaz Harrosh <[email protected]>
> Date: Wed, 16 Jul 2014 20:02:29 +0300
> Subject: [PATCH] do_direct_IO: Fix compiler warning
> MIME-Version: 1.0
> Content-Type: text/plain; charset=UTF-8
> Content-Transfer-Encoding: 8bit
>
> This fixes
> ../fs/direct-io.c: In function ‘do_blockdev_direct_IO’:
> ../fs/direct-io.c:1011:12: warning: ‘from’ may be used uninitialized in this function [-Wmaybe-uninitialized]
> u = (to - from) >> blkbits;
> ^
> ../fs/direct-io.c:1011:12: warning: ‘to’ may be used uninitialized in this function [-Wmaybe-uninitialized]
> u = (to - from) >> blkbits;
>
> I use: gcc version 4.8.3 20140624 (Red Hat 4.8.3-1)
>
> GCC is wrong here so we should use the uninitialized_var() macro
> and not silence it with = 0;
>
> Signed-off-by: Boaz Harrosh <[email protected]>
Acked-by: Jason Cooper <[email protected]>
thx,
Jason.
> ---
> fs/direct-io.c | 3 ++-
> 1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/fs/direct-io.c b/fs/direct-io.c
> index 98040ba..156e6f0 100644
> --- a/fs/direct-io.c
> +++ b/fs/direct-io.c
> @@ -910,7 +910,8 @@ static int do_direct_IO(struct dio *dio, struct dio_submit *sdio,
>
> while (sdio->block_in_file < sdio->final_block_in_request) {
> struct page *page;
> - size_t from, to;
> + size_t uninitialized_var(from), uninitialized_var(to);
> +
> page = dio_get_page(dio, sdio, &from, &to);
> if (IS_ERR(page)) {
> ret = PTR_ERR(page);
> --
> 1.9.3
>
On Wed, Jul 16, 2014 at 08:08:41PM +0300, Boaz Harrosh wrote:
> This is the wrong fix. GCC is wrong here. As shown by Paul Bolle if
> you move the from / to set from dio_get_page() to here the warning goes away.
>
> The minimal fix must use uninitialized_var() in this case. See patch below
>
> But I think the proper fix Is the one Paul Bolle <[email protected]> sent (above)
I don't think the initialization is wrong. The fix of moving the code
defintively looks nicer, while I think uninitialized_var is horrible
wart that won't get anywhere near my code.
Either way we should merge one of those fixes ASAP..
[Added Borislav and Sam to cc.]
Christoph Hellwig schreef op wo 16-07-2014 om 10:58 [-0700]:
> On Wed, Jul 16, 2014 at 08:08:41PM +0300, Boaz Harrosh wrote:
> > This is the wrong fix. GCC is wrong here. As shown by Paul Bolle if
> > you move the from / to set from dio_get_page() to here the warning goes away.
> >
> > The minimal fix must use uninitialized_var() in this case. See patch below
> >
> > But I think the proper fix Is the one Paul Bolle <[email protected]> sent (above)
>
> I don't think the initialization is wrong. The fix of moving the code
> defintively looks nicer,
Whether that was Jason's idea or mine doesn't matter much - though I do
think Boaz quoted part of my fix, but it was just a _draft_.
Anyhow, after all that I got involved in a short thread;
https://lkml.org/lkml/2014/7/8/168 is my contribution. My point is,
basically, that the false positives of -Wmaybe-uninitialized are a "code
smell". They tend to disappear if one reorganizes the code a bit.
Borislav disagrees quite strongly. I didn't really bother with keeping
that thread alive because I feared we'd mainly see more colorful
language.
> while I think uninitialized_var is horrible wart
Agree totally.
> that won't get anywhere near my code.
>
> Either way we should merge one of those fixes ASAP..
I'd like my builds to be warning free too. And it would be nice to know
whether there's consensus on how to deal with the false positives of
-Wmaybe-uninitialized that make it into mainline.
Paul Bolle
From: Paul Bolle <[email protected]>
The following warnings:
fs/direct-io.c: In function ‘__blockdev_direct_IO’:
fs/direct-io.c:1011:12: warning: ‘to’ may be used uninitialized in this function [-Wmaybe-uninitialized]
fs/direct-io.c:913:16: note: ‘to’ was declared here
fs/direct-io.c:1011:12: warning: ‘from’ may be used uninitialized in this function [-Wmaybe-uninitialized]
fs/direct-io.c:913:10: note: ‘from’ was declared here
are false positive because dio_get_page() either fails, or sets both
'from' and 'to'.
Maybe it's better to move initializing "to" and "from" out of
dio_get_page(). That _might_ make it easier for both the the reader and
the compiler to understand what's going on. Something like this:
Christoph Hellwig said ...
The fix of moving the code defintively looks nicer, while I think
uninitialized_var is horrible wart that won't get anywhere near my code.
Boaz Harrosh I agree with Christoph and Paul
Signed-off-by: Paul Bolle <[email protected]>
Signed-off-by: Boaz Harrosh <[email protected]>
---
fs/direct-io.c | 14 +++++++-------
1 file changed, 7 insertions(+), 7 deletions(-)
diff --git a/fs/direct-io.c b/fs/direct-io.c
index 98040ba..2f024fc 100644
--- a/fs/direct-io.c
+++ b/fs/direct-io.c
@@ -198,9 +198,8 @@ static inline int dio_refill_pages(struct dio *dio, struct dio_submit *sdio)
* L1 cache.
*/
static inline struct page *dio_get_page(struct dio *dio,
- struct dio_submit *sdio, size_t *from, size_t *to)
+ struct dio_submit *sdio)
{
- int n;
if (dio_pages_present(sdio) == 0) {
int ret;
@@ -209,10 +208,7 @@ static inline struct page *dio_get_page(struct dio *dio,
return ERR_PTR(ret);
BUG_ON(dio_pages_present(sdio) == 0);
}
- n = sdio->head++;
- *from = n ? 0 : sdio->from;
- *to = (n == sdio->tail - 1) ? sdio->to : PAGE_SIZE;
- return dio->pages[n];
+ return dio->pages[sdio->head];
}
/**
@@ -911,11 +907,15 @@ static int do_direct_IO(struct dio *dio, struct dio_submit *sdio,
while (sdio->block_in_file < sdio->final_block_in_request) {
struct page *page;
size_t from, to;
- page = dio_get_page(dio, sdio, &from, &to);
+
+ page = dio_get_page(dio, sdio);
if (IS_ERR(page)) {
ret = PTR_ERR(page);
goto out;
}
+ from = sdio->head ? 0 : sdio->from;
+ to = (sdio->head == sdio->tail - 1) ? sdio->to : PAGE_SIZE;
+ sdio->head++;
while (from < to) {
unsigned this_chunk_bytes; /* # of bytes mapped */
--
1.9.3
On 07/17/2014 12:37 PM, Boaz Harrosh wrote:
> From: Paul Bolle <[email protected]>
>
> The following warnings:
>
> fs/direct-io.c: In function ‘__blockdev_direct_IO’:
> fs/direct-io.c:1011:12: warning: ‘to’ may be used uninitialized in this function [-Wmaybe-uninitialized]
> fs/direct-io.c:913:16: note: ‘to’ was declared here
> fs/direct-io.c:1011:12: warning: ‘from’ may be used uninitialized in this function [-Wmaybe-uninitialized]
> fs/direct-io.c:913:10: note: ‘from’ was declared here
>
> are false positive because dio_get_page() either fails, or sets both
> 'from' and 'to'.
>
> Maybe it's better to move initializing "to" and "from" out of
> dio_get_page(). That _might_ make it easier for both the the reader and
> the compiler to understand what's going on. Something like this:
>
> Christoph Hellwig said ...
> The fix of moving the code defintively looks nicer, while I think
> uninitialized_var is horrible wart that won't get anywhere near my code.
>
> Boaz Harrosh I agree with Christoph and Paul
>
So Here, everyone likes this one (I think) please ACK/Review and let us merge
it.
> Signed-off-by: Paul Bolle <[email protected]>
Paul this is your patch I put you as Signed-off-by please acknowledge
> Signed-off-by: Boaz Harrosh <[email protected]>
Thanks
Boaz
> ---
> fs/direct-io.c | 14 +++++++-------
> 1 file changed, 7 insertions(+), 7 deletions(-)
>
> diff --git a/fs/direct-io.c b/fs/direct-io.c
> index 98040ba..2f024fc 100644
> --- a/fs/direct-io.c
> +++ b/fs/direct-io.c
> @@ -198,9 +198,8 @@ static inline int dio_refill_pages(struct dio *dio, struct dio_submit *sdio)
> * L1 cache.
> */
> static inline struct page *dio_get_page(struct dio *dio,
> - struct dio_submit *sdio, size_t *from, size_t *to)
> + struct dio_submit *sdio)
> {
> - int n;
> if (dio_pages_present(sdio) == 0) {
> int ret;
>
> @@ -209,10 +208,7 @@ static inline struct page *dio_get_page(struct dio *dio,
> return ERR_PTR(ret);
> BUG_ON(dio_pages_present(sdio) == 0);
> }
> - n = sdio->head++;
> - *from = n ? 0 : sdio->from;
> - *to = (n == sdio->tail - 1) ? sdio->to : PAGE_SIZE;
> - return dio->pages[n];
> + return dio->pages[sdio->head];
> }
>
> /**
> @@ -911,11 +907,15 @@ static int do_direct_IO(struct dio *dio, struct dio_submit *sdio,
> while (sdio->block_in_file < sdio->final_block_in_request) {
> struct page *page;
> size_t from, to;
> - page = dio_get_page(dio, sdio, &from, &to);
> +
> + page = dio_get_page(dio, sdio);
> if (IS_ERR(page)) {
> ret = PTR_ERR(page);
> goto out;
> }
> + from = sdio->head ? 0 : sdio->from;
> + to = (sdio->head == sdio->tail - 1) ? sdio->to : PAGE_SIZE;
> + sdio->head++;
>
> while (from < to) {
> unsigned this_chunk_bytes; /* # of bytes mapped */
>
Boaz,
On Thu, 2014-07-17 at 12:37 +0300, Boaz Harrosh wrote:
> From: Paul Bolle <[email protected]>
>
> The following warnings:
>
> fs/direct-io.c: In function ‘__blockdev_direct_IO’:
> fs/direct-io.c:1011:12: warning: ‘to’ may be used uninitialized in this function [-Wmaybe-uninitialized]
> fs/direct-io.c:913:16: note: ‘to’ was declared here
> fs/direct-io.c:1011:12: warning: ‘from’ may be used uninitialized in this function [-Wmaybe-uninitialized]
> fs/direct-io.c:913:10: note: ‘from’ was declared here
>
> are false positive because dio_get_page() either fails, or sets both
> 'from' and 'to'.
>
> Maybe it's better to move initializing "to" and "from" out of
> dio_get_page(). That _might_ make it easier for both the the reader and
> the compiler to understand what's going on. Something like this:
>
> Christoph Hellwig said ...
> The fix of moving the code defintively looks nicer, while I think
> uninitialized_var is horrible wart that won't get anywhere near my code.
>
> Boaz Harrosh I agree with Christoph and Paul
>
> Signed-off-by: Paul Bolle <[email protected]>
This is simply not coorect!
> Signed-off-by: Boaz Harrosh <[email protected]>
> ---
> fs/direct-io.c | 14 +++++++-------
> 1 file changed, 7 insertions(+), 7 deletions(-)
>
> diff --git a/fs/direct-io.c b/fs/direct-io.c
> index 98040ba..2f024fc 100644
> --- a/fs/direct-io.c
> +++ b/fs/direct-io.c
> @@ -198,9 +198,8 @@ static inline int dio_refill_pages(struct dio *dio, struct dio_submit *sdio)
> * L1 cache.
> */
> static inline struct page *dio_get_page(struct dio *dio,
> - struct dio_submit *sdio, size_t *from, size_t *to)
> + struct dio_submit *sdio)
> {
> - int n;
> if (dio_pages_present(sdio) == 0) {
> int ret;
>
> @@ -209,10 +208,7 @@ static inline struct page *dio_get_page(struct dio *dio,
> return ERR_PTR(ret);
> BUG_ON(dio_pages_present(sdio) == 0);
> }
> - n = sdio->head++;
> - *from = n ? 0 : sdio->from;
> - *to = (n == sdio->tail - 1) ? sdio->to : PAGE_SIZE;
> - return dio->pages[n];
> + return dio->pages[sdio->head];
> }
>
> /**
> @@ -911,11 +907,15 @@ static int do_direct_IO(struct dio *dio, struct dio_submit *sdio,
> while (sdio->block_in_file < sdio->final_block_in_request) {
> struct page *page;
> size_t from, to;
> - page = dio_get_page(dio, sdio, &from, &to);
> +
> + page = dio_get_page(dio, sdio);
> if (IS_ERR(page)) {
> ret = PTR_ERR(page);
> goto out;
> }
> + from = sdio->head ? 0 : sdio->from;
> + to = (sdio->head == sdio->tail - 1) ? sdio->to : PAGE_SIZE;
> + sdio->head++;
>
> while (from < to) {
> unsigned this_chunk_bytes; /* # of bytes mapped */
Paul Bolle
Boaz,
On Thu, 2014-07-17 at 12:40 +0300, Boaz Harrosh wrote:
> > Signed-off-by: Paul Bolle <[email protected]>
>
> Paul this is your patch I put you as Signed-off-by please acknowledge
I never signed off on that patch! Speaking from memory, I just sent a
message stating that "something like this" might silence the warning. I
also stated that I needed to have another look at it.
This touches code I did not write and do not yet understand well enough.
Getting there requires a clean schedule, clean head, and/or a clean
desk. I certainly won't sign off on it until one or more of those
conditions are met.
Please don't do this (adding a Signed-off-by on a patch I drafted)
again!
Paul Bolle
On 07/17/2014 12:48 PM, Paul Bolle wrote:
> Boaz,
>
> On Thu, 2014-07-17 at 12:37 +0300, Boaz Harrosh wrote:
>> From: Paul Bolle <[email protected]>
>>
>> The following warnings:
>>
>> fs/direct-io.c: In function ‘__blockdev_direct_IO’:
>> fs/direct-io.c:1011:12: warning: ‘to’ may be used uninitialized in this function [-Wmaybe-uninitialized]
>> fs/direct-io.c:913:16: note: ‘to’ was declared here
>> fs/direct-io.c:1011:12: warning: ‘from’ may be used uninitialized in this function [-Wmaybe-uninitialized]
>> fs/direct-io.c:913:10: note: ‘from’ was declared here
>>
>> are false positive because dio_get_page() either fails, or sets both
>> 'from' and 'to'.
>>
>> Maybe it's better to move initializing "to" and "from" out of
>> dio_get_page(). That _might_ make it easier for both the the reader and
>> the compiler to understand what's going on. Something like this:
>>
>> Christoph Hellwig said ...
>> The fix of moving the code defintively looks nicer, while I think
>> uninitialized_var is horrible wart that won't get anywhere near my code.
>>
>> Boaz Harrosh I agree with Christoph and Paul
>>
>> Signed-off-by: Paul Bolle <[email protected]>
>
> This is simply not coorect!
Sorry you are absolutely right, I only saw this after "send", is why
I ping you to make sure.
I have reviewed it fully, your initial code is correct *and tested).
Here is a new V2 patch without your sign-off
Thanks
Boaz
>
>> Signed-off-by: Boaz Harrosh <[email protected]>
>> ---
>> fs/direct-io.c | 14 +++++++-------
>> 1 file changed, 7 insertions(+), 7 deletions(-)
>>
>> diff --git a/fs/direct-io.c b/fs/direct-io.c
>> index 98040ba..2f024fc 100644
>> --- a/fs/direct-io.c
>> +++ b/fs/direct-io.c
>> @@ -198,9 +198,8 @@ static inline int dio_refill_pages(struct dio *dio, struct dio_submit *sdio)
>> * L1 cache.
>> */
>> static inline struct page *dio_get_page(struct dio *dio,
>> - struct dio_submit *sdio, size_t *from, size_t *to)
>> + struct dio_submit *sdio)
>> {
>> - int n;
>> if (dio_pages_present(sdio) == 0) {
>> int ret;
>>
>> @@ -209,10 +208,7 @@ static inline struct page *dio_get_page(struct dio *dio,
>> return ERR_PTR(ret);
>> BUG_ON(dio_pages_present(sdio) == 0);
>> }
>> - n = sdio->head++;
>> - *from = n ? 0 : sdio->from;
>> - *to = (n == sdio->tail - 1) ? sdio->to : PAGE_SIZE;
>> - return dio->pages[n];
>> + return dio->pages[sdio->head];
>> }
>>
>> /**
>> @@ -911,11 +907,15 @@ static int do_direct_IO(struct dio *dio, struct dio_submit *sdio,
>> while (sdio->block_in_file < sdio->final_block_in_request) {
>> struct page *page;
>> size_t from, to;
>> - page = dio_get_page(dio, sdio, &from, &to);
>> +
>> + page = dio_get_page(dio, sdio);
>> if (IS_ERR(page)) {
>> ret = PTR_ERR(page);
>> goto out;
>> }
>> + from = sdio->head ? 0 : sdio->from;
>> + to = (sdio->head == sdio->tail - 1) ? sdio->to : PAGE_SIZE;
>> + sdio->head++;
>>
>> while (from < to) {
>> unsigned this_chunk_bytes; /* # of bytes mapped */
>
>
> Paul Bolle
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
> the body of a message to [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
From: Paul Bolle <[email protected]>
The following warnings:
fs/direct-io.c: In function ‘__blockdev_direct_IO’:
fs/direct-io.c:1011:12: warning: ‘to’ may be used uninitialized in this function [-Wmaybe-uninitialized]
fs/direct-io.c:913:16: note: ‘to’ was declared here
fs/direct-io.c:1011:12: warning: ‘from’ may be used uninitialized in this function [-Wmaybe-uninitialized]
fs/direct-io.c:913:10: note: ‘from’ was declared here
are false positive because dio_get_page() either fails, or sets both
'from' and 'to'.
Maybe it's better to move initializing "to" and "from" out of
dio_get_page(). That _might_ make it easier for both the the reader and
the compiler to understand what's going on. Something like this:
Christoph Hellwig said ...
The fix of moving the code definitively looks nicer, while I think
uninitialized_var is horrible wart that won't get anywhere near my code.
Boaz Harrosh: I agree with Christoph and Paul
Signed-off-by: Boaz Harrosh <[email protected]>
---
fs/direct-io.c | 14 +++++++-------
1 file changed, 7 insertions(+), 7 deletions(-)
diff --git a/fs/direct-io.c b/fs/direct-io.c
index 98040ba..194d0d1 100644
--- a/fs/direct-io.c
+++ b/fs/direct-io.c
@@ -198,9 +198,8 @@ static inline int dio_refill_pages(struct dio *dio, struct dio_submit *sdio)
* L1 cache.
*/
static inline struct page *dio_get_page(struct dio *dio,
- struct dio_submit *sdio, size_t *from, size_t *to)
+ struct dio_submit *sdio)
{
- int n;
if (dio_pages_present(sdio) == 0) {
int ret;
@@ -209,10 +208,7 @@ static inline struct page *dio_get_page(struct dio *dio,
return ERR_PTR(ret);
BUG_ON(dio_pages_present(sdio) == 0);
}
- n = sdio->head++;
- *from = n ? 0 : sdio->from;
- *to = (n == sdio->tail - 1) ? sdio->to : PAGE_SIZE;
- return dio->pages[n];
+ return dio->pages[sdio->head];
}
/**
@@ -911,11 +907,15 @@ static int do_direct_IO(struct dio *dio, struct dio_submit *sdio,
while (sdio->block_in_file < sdio->final_block_in_request) {
struct page *page;
size_t from, to;
- page = dio_get_page(dio, sdio, &from, &to);
+
+ page = dio_get_page(dio, sdio);
if (IS_ERR(page)) {
ret = PTR_ERR(page);
goto out;
}
+ from = sdio->head ? 0 : sdio->from;
+ to = (sdio->head == sdio->tail - 1) ? sdio->to : PAGE_SIZE;
+ sdio->head++;
while (from < to) {
unsigned this_chunk_bytes; /* # of bytes mapped */
--
1.9.3
Boaz Harrosh schreef op do 17-07-2014 om 14:11 [+0300]:
> From: Paul Bolle <[email protected]>
Thanks for dropping my Signed-off-by tag.
Would you mind also dropping the From: line? I prefer you to also be
author of this patch.
Note that I'm pretty sure I can't claim copyright on the three chunks of
changes to the code nor on the few lines you copied into the commit
explanation. Besides, I'm perfectly OK with you using them in your patch
(especially since you've done review etc.).
Paul Bolle
From: Boaz Harrosh <[email protected]>
The following warnings:
fs/direct-io.c: In function ‘__blockdev_direct_IO’:
fs/direct-io.c:1011:12: warning: ‘to’ may be used uninitialized in this function [-Wmaybe-uninitialized]
fs/direct-io.c:913:16: note: ‘to’ was declared here
fs/direct-io.c:1011:12: warning: ‘from’ may be used uninitialized in this function [-Wmaybe-uninitialized]
fs/direct-io.c:913:10: note: ‘from’ was declared here
are false positive because dio_get_page() either fails, or sets both
'from' and 'to'.
Paul Bolle said ...
Maybe it's better to move initializing "to" and "from" out of
dio_get_page(). That _might_ make it easier for both the the reader and
the compiler to understand what's going on. Something like this:
Christoph Hellwig said ...
The fix of moving the code definitively looks nicer, while I think
uninitialized_var is horrible wart that won't get anywhere near my code.
Boaz Harrosh: I agree with Christoph and Paul
Signed-off-by: Boaz Harrosh <[email protected]>
---
fs/direct-io.c | 14 +++++++-------
1 file changed, 7 insertions(+), 7 deletions(-)
diff --git a/fs/direct-io.c b/fs/direct-io.c
index 98040ba..194d0d1 100644
--- a/fs/direct-io.c
+++ b/fs/direct-io.c
@@ -198,9 +198,8 @@ static inline int dio_refill_pages(struct dio *dio, struct dio_submit *sdio)
* L1 cache.
*/
static inline struct page *dio_get_page(struct dio *dio,
- struct dio_submit *sdio, size_t *from, size_t *to)
+ struct dio_submit *sdio)
{
- int n;
if (dio_pages_present(sdio) == 0) {
int ret;
@@ -209,10 +208,7 @@ static inline struct page *dio_get_page(struct dio *dio,
return ERR_PTR(ret);
BUG_ON(dio_pages_present(sdio) == 0);
}
- n = sdio->head++;
- *from = n ? 0 : sdio->from;
- *to = (n == sdio->tail - 1) ? sdio->to : PAGE_SIZE;
- return dio->pages[n];
+ return dio->pages[sdio->head];
}
/**
@@ -911,11 +907,15 @@ static int do_direct_IO(struct dio *dio, struct dio_submit *sdio,
while (sdio->block_in_file < sdio->final_block_in_request) {
struct page *page;
size_t from, to;
- page = dio_get_page(dio, sdio, &from, &to);
+
+ page = dio_get_page(dio, sdio);
if (IS_ERR(page)) {
ret = PTR_ERR(page);
goto out;
}
+ from = sdio->head ? 0 : sdio->from;
+ to = (sdio->head == sdio->tail - 1) ? sdio->to : PAGE_SIZE;
+ sdio->head++;
while (from < to) {
unsigned this_chunk_bytes; /* # of bytes mapped */
--
1.9.3
Looks good to me,
Reviewed-by: Christoph Hellwig <[email protected]>
While it looks obvious, did you make sure it passes xfstests, which
has a lot of direct I/O tests?
On 07/21/2014 02:36 PM, Christoph Hellwig wrote:
> Looks good to me,
>
> Reviewed-by: Christoph Hellwig <[email protected]>
>
> While it looks obvious, did you make sure it passes xfstests, which
> has a lot of direct I/O tests?
>
Thank you Christoph. OK So finally I did last night. I ran
ext4. Just that I'm not used to run ext4 or any of those
stuff so it took me time. I have the usual 2 failures I get
also from 3.15. So I'd say its good - tested
Thanks
Boaz