On Thu, 26 Jul 2007 17:04:00 +0800 Joe Jin <[EMAIL PROTECTED]> wrote:
> This is the patch for check do_direct_IO() return val.
>
> At do_direct_IO(), sometimes dio_get_page() will return -EFAULT/-ENOMEM,
> according to orig source, it will go on left work. buf for dio_get_page()
> return a error will made many useful member of dio not initialized like
> dio->map_bh and others, at this point, kernel will panic.
>
> Signed-off-by: Joe Jin <[EMAIL PROTECTED]>
>
>
> ---
> --- linux-2.6.22/fs/direct-io.c.orig 2007-07-26 11:32:27.000000000 +0800
> +++ linux-2.6.22/fs/direct-io.c 2007-07-26 11:33:58.000000000 +0800
> @@ -1031,7 +1031,9 @@ direct_io_worker(int rw, struct kiocb *i
> ((dio->final_block_in_request - dio->block_in_file) <<
> blkbits);
>
> - if (ret) {
> + if (ret == -EFAULT || ret == -ENOMEM)
> + goto out;
> + else if (ret) {
> dio_cleanup(dio);
> break;
> }
> @@ -1113,6 +1115,7 @@ direct_io_worker(int rw, struct kiocb *i
> } else
> BUG_ON(ret != -EIOCBQUEUED);
>
> +out:
> return ret;
> }
>
I think we still want to run dio_cleanup() if do_direct_IO() failed?
Otherwise we can leak pages.
And there's nothing special about EFAULT or ENOMEM here: if do_direct_IO()
returns any error then that's it: we bale out, yes?
In fact I'm suspecting that this is what the code in there used to do.
Something like:
for (...) {
...
ret = do_direct_IO(...);
...
if (ret) {
dio_dleanup(dio);
break
}
}
return ret;
but then someone later came along and added more code between the end of
the for loop and the `return' statement.
<I do miss the BK diffviewer sometimes>
<does a binary search>
yep, that's exactly what happened. We broke it in the 2.5.45 release back
in October 2002. (Where "we" == "Badari")
So I think what we need to do is something close to this:
--- a/fs/direct-io.c~add-check-do_direct_io-return-val
+++ a/fs/direct-io.c
@@ -1033,7 +1033,7 @@ direct_io_worker(int rw, struct kiocb *i
if (ret) {
dio_cleanup(dio);
- break;
+ goto out;
}
} /* end iovec loop */
@@ -1112,7 +1112,7 @@ direct_io_worker(int rw, struct kiocb *i
kfree(dio);
} else
BUG_ON(ret != -EIOCBQUEUED);
-
+out:
return ret;
}
_
However I'd like to ask you guys to carefully review and test that please.
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
