This BUG() is suspicious, it makes its following statements
unreachable, and it seems to be useless, since the caller
of this function already handles the failure properly.
Remove it.
Signed-off-by: WANG Cong <[email protected]>
Cc: Alexander Viro <[email protected]>
Cc: Jens Axboe <[email protected]>
Cc: Nick Piggin <[email protected]>
Cc: "Theodore Ts'o" <[email protected]>
---
diff --git a/fs/buffer.c b/fs/buffer.c
index 6fa5302..ac111d7 100644
--- a/fs/buffer.c
+++ b/fs/buffer.c
@@ -1041,7 +1041,6 @@ grow_dev_page(struct block_device *bdev, sector_t block,
return page;
failed:
- BUG();
unlock_page(page);
page_cache_release(page);
return NULL;
On Mon, Nov 30, 2009 at 09:34:14PM -0500, Amerigo Wang wrote:
> This BUG() is suspicious, it makes its following statements
> unreachable,
only when CONFIG_BUG=y
> and it seems to be useless, since the caller
> of this function already handles the failure properly.
because this function can return NULL in other codepath
> Remove it.
I don't know why this BUG() is there (and maybe it's not really
needed), but your rationale is wrong.
> Signed-off-by: WANG Cong <[email protected]>
> Cc: Alexander Viro <[email protected]>
> Cc: Jens Axboe <[email protected]>
> Cc: Nick Piggin <[email protected]>
> Cc: "Theodore Ts'o" <[email protected]>
>
> ---
> diff --git a/fs/buffer.c b/fs/buffer.c
> index 6fa5302..ac111d7 100644
> --- a/fs/buffer.c
> +++ b/fs/buffer.c
> @@ -1041,7 +1041,6 @@ grow_dev_page(struct block_device *bdev, sector_t block,
> return page;
>
> failed:
> - BUG();
> unlock_page(page);
> page_cache_release(page);
> return NULL;
> --
On Tue, Dec 01, 2009 at 05:55:11PM +0100, Marcin Slusarz wrote:
> On Mon, Nov 30, 2009 at 09:34:14PM -0500, Amerigo Wang wrote:
> > This BUG() is suspicious, it makes its following statements
> > unreachable,
> only when CONFIG_BUG=y
Which is true for all kernels except for the very rare embedded case.
> > and it seems to be useless, since the caller
> > of this function already handles the failure properly.
> because this function can return NULL in other codepath
>
> > Remove it.
> I don't know why this BUG() is there (and maybe it's not really
> needed), but your rationale is wrong.
Your reply is a bit snarky, IMHO. It might have been nicer and more
courteous if you had bothered to take a closer look at the patch
before firing off a reply.
In fact, it's good to avoid BUG() if at all possible, especially if it
can happen in the normally course of events --- such as running out of
memory. Having code which triggers an BUG in an low memory situation
is very bad form.
Looks good to me.
Signed-off-by: "Theodore Ts'o" <[email protected]>
- Ted
On Tue, Dec 01, 2009 at 01:52:25PM -0500, [email protected] wrote:
> On Tue, Dec 01, 2009 at 05:55:11PM +0100, Marcin Slusarz wrote:
> > On Mon, Nov 30, 2009 at 09:34:14PM -0500, Amerigo Wang wrote:
> > > This BUG() is suspicious, it makes its following statements
> > > unreachable,
> > only when CONFIG_BUG=y
>
> Which is true for all kernels except for the very rare embedded case.
>
> > > and it seems to be useless, since the caller
> > > of this function already handles the failure properly.
> > because this function can return NULL in other codepath
> >
> > > Remove it.
> > I don't know why this BUG() is there (and maybe it's not really
> > needed), but your rationale is wrong.
>
> Your reply is a bit snarky, IMHO. It might have been nicer and more
> courteous if you had bothered to take a closer look at the patch
> before firing off a reply.
>
> In fact, it's good to avoid BUG() if at all possible, especially if it
> can happen in the normally course of events --- such as running out of
> memory. Having code which triggers an BUG in an low memory situation
> is very bad form.
>
> Looks good to me.
>
> Signed-off-by: "Theodore Ts'o" <[email protected]>
Maybe convert it to a warning?
On Mon, 30 Nov 2009 21:34:14 -0500
Amerigo Wang <[email protected]> wrote:
>
> This BUG() is suspicious, it makes its following statements
> unreachable, and it seems to be useless, since the caller
> of this function already handles the failure properly.
> Remove it.
>
> Signed-off-by: WANG Cong <[email protected]>
> Cc: Alexander Viro <[email protected]>
> Cc: Jens Axboe <[email protected]>
> Cc: Nick Piggin <[email protected]>
> Cc: "Theodore Ts'o" <[email protected]>
>
> ---
> diff --git a/fs/buffer.c b/fs/buffer.c
> index 6fa5302..ac111d7 100644
> --- a/fs/buffer.c
> +++ b/fs/buffer.c
> @@ -1041,7 +1041,6 @@ grow_dev_page(struct block_device *bdev, sector_t block,
> return page;
>
> failed:
> - BUG();
> unlock_page(page);
> page_cache_release(page);
> return NULL;
The caller doesn't handle this properly. If we return zero here,
grow_buffers() will say sheesh and will retry and the kernel goes into
an infinite retry loop.
If there is a blockdev page which is sitting in pagecache and for some
reason it has buffers and we cannot release them, we're kind of stuck
and don't know what to do. Going BUG() is a decent thing to do here.
I don't think I've ever seen a report of the BUG triggering. It could
happen as a result of memory corruption or a missed bh_put() or
whatever.
I think a better patch would be to remove the
unlock_page()/page_cache_release(), add a comment (culled from the
above) and leave the BUG() there.
Andrew Morton wrote:
> On Mon, 30 Nov 2009 21:34:14 -0500
> Amerigo Wang <[email protected]> wrote:
>
>> This BUG() is suspicious, it makes its following statements
>> unreachable, and it seems to be useless, since the caller
>> of this function already handles the failure properly.
>> Remove it.
>>
>> Signed-off-by: WANG Cong <[email protected]>
>> Cc: Alexander Viro <[email protected]>
>> Cc: Jens Axboe <[email protected]>
>> Cc: Nick Piggin <[email protected]>
>> Cc: "Theodore Ts'o" <[email protected]>
>>
>> ---
>> diff --git a/fs/buffer.c b/fs/buffer.c
>> index 6fa5302..ac111d7 100644
>> --- a/fs/buffer.c
>> +++ b/fs/buffer.c
>> @@ -1041,7 +1041,6 @@ grow_dev_page(struct block_device *bdev, sector_t block,
>> return page;
>>
>> failed:
>> - BUG();
>> unlock_page(page);
>> page_cache_release(page);
>> return NULL;
>
> The caller doesn't handle this properly. If we return zero here,
> grow_buffers() will say sheesh and will retry and the kernel goes into
> an infinite retry loop.
>
> If there is a blockdev page which is sitting in pagecache and for some
> reason it has buffers and we cannot release them, we're kind of stuck
> and don't know what to do. Going BUG() is a decent thing to do here.
>
> I don't think I've ever seen a report of the BUG triggering. It could
> happen as a result of memory corruption or a missed bh_put() or
> whatever.
>
Oh, good explanation!
> I think a better patch would be to remove the
> unlock_page()/page_cache_release(), add a comment (culled from the
> above) and leave the BUG() there.
>
Ok, I will prepare a patch tomorrow.
Thanks!