* dead/wrong code in ext3/4_releasepage()
@ 2012-06-29 16:52 Andrew Perepechko
2012-07-02 17:09 ` Jan Kara
0 siblings, 1 reply; 6+ messages in thread
From: Andrew Perepechko @ 2012-06-29 16:52 UTC (permalink / raw)
To: linux-ext4
Hello!
The implementation of ext4_releasepage() for many kernel versions
(as well as current git) is the following:
static int ext4_releasepage(struct page *page, gfp_t wait)
{
journal_t *journal = EXT4_JOURNAL(page->mapping->host);
trace_ext4_releasepage(page);
WARN_ON(PageChecked(page));
if (!page_has_buffers(page))
return 0;
if (journal)
return jbd2_journal_try_to_free_buffers(journal, page,
wait);
else
return try_to_free_buffers(page);
}
The "if (!page_has_buffers(page))" check seems to be attempting to
handle the "nobh" case. However, the correct return value for this case
seems to be 1 (success), not 0 (failure).
This does not lead to oom or any similar issue since calls to
try_to_release_page()
are accompanied by page_has_private() checks.
If ->release_page() can be called without a prior check, then
the return code is wrong. Otherwise, the check is dead code.
What do you think?
Thank you,
Andrew
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: dead/wrong code in ext3/4_releasepage()
2012-06-29 16:52 dead/wrong code in ext3/4_releasepage() Andrew Perepechko
@ 2012-07-02 17:09 ` Jan Kara
2012-07-02 17:14 ` Andrew Perepechko
0 siblings, 1 reply; 6+ messages in thread
From: Jan Kara @ 2012-07-02 17:09 UTC (permalink / raw)
To: Andrew Perepechko; +Cc: linux-ext4
On Fri 29-06-12 20:52:51, Andrew Perepechko wrote:
> Hello!
>
> The implementation of ext4_releasepage() for many kernel versions
> (as well as current git) is the following:
>
> static int ext4_releasepage(struct page *page, gfp_t wait)
> {
> journal_t *journal = EXT4_JOURNAL(page->mapping->host);
>
> trace_ext4_releasepage(page);
>
> WARN_ON(PageChecked(page));
> if (!page_has_buffers(page))
> return 0;
> if (journal)
> return jbd2_journal_try_to_free_buffers(journal,
> page, wait);
> else
> return try_to_free_buffers(page);
> }
>
> The "if (!page_has_buffers(page))" check seems to be attempting to
> handle the "nobh" case. However, the correct return value for this case
> seems to be 1 (success), not 0 (failure).
>
> This does not lead to oom or any similar issue since calls to
> try_to_release_page()
> are accompanied by page_has_private() checks.
>
> If ->release_page() can be called without a prior check, then
> the return code is wrong. Otherwise, the check is dead code.
>
> What do you think?
The check is a dead code because ->release_page() is called only if
PagePrivate bit is set.
Honza
--
Jan Kara <jack@suse.cz>
SUSE Labs, CR
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: dead/wrong code in ext3/4_releasepage()
2012-07-02 17:09 ` Jan Kara
@ 2012-07-02 17:14 ` Andrew Perepechko
2012-07-02 17:33 ` Jan Kara
0 siblings, 1 reply; 6+ messages in thread
From: Andrew Perepechko @ 2012-07-02 17:14 UTC (permalink / raw)
To: Jan Kara; +Cc: linux-ext4
Hi Jan!
So you think the correct fix would be to remove the check completely
since every try_to_release_page() caller is obliged to perform the
page_has_private() check?
Thank you,
Andrew
On 07/02/2012 09:09 PM, Jan Kara wrote:
> On Fri 29-06-12 20:52:51, Andrew Perepechko wrote:
>> This does not lead to oom or any similar issue since calls to
>> try_to_release_page()
>> are accompanied by page_has_private() checks.
>>
>>
> The check is a dead code because ->release_page() is called only if
> PagePrivate bit is set.
>
> Honza
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: dead/wrong code in ext3/4_releasepage()
2012-07-02 17:14 ` Andrew Perepechko
@ 2012-07-02 17:33 ` Jan Kara
2012-07-02 18:17 ` Theodore Ts'o
0 siblings, 1 reply; 6+ messages in thread
From: Jan Kara @ 2012-07-02 17:33 UTC (permalink / raw)
To: Andrew Perepechko; +Cc: Jan Kara, linux-ext4
Hi,
On Mon 02-07-12 21:14:58, Andrew Perepechko wrote:
> So you think the correct fix would be to remove the check completely
> since every try_to_release_page() caller is obliged to perform the
> page_has_private() check?
Yes, I think that would be reasonable. XFS already relies on this so
it should be safe.
Honza
> On 07/02/2012 09:09 PM, Jan Kara wrote:
> >On Fri 29-06-12 20:52:51, Andrew Perepechko wrote:
> >>This does not lead to oom or any similar issue since calls to
> >>try_to_release_page()
> >>are accompanied by page_has_private() checks.
> >>
> >>
> > The check is a dead code because ->release_page() is called only if
> >PagePrivate bit is set.
> >
> > Honza
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-ext4" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
--
Jan Kara <jack@suse.cz>
SUSE Labs, CR
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: dead/wrong code in ext3/4_releasepage()
2012-07-02 17:33 ` Jan Kara
@ 2012-07-02 18:17 ` Theodore Ts'o
2012-07-02 18:19 ` Andrew Perepechko
0 siblings, 1 reply; 6+ messages in thread
From: Theodore Ts'o @ 2012-07-02 18:17 UTC (permalink / raw)
To: Jan Kara; +Cc: Andrew Perepechko, linux-ext4
On Mon, Jul 02, 2012 at 07:33:26PM +0200, Jan Kara wrote:
> Hi,
>
> On Mon 02-07-12 21:14:58, Andrew Perepechko wrote:
> > So you think the correct fix would be to remove the check completely
> > since every try_to_release_page() caller is obliged to perform the
> > page_has_private() check?
> Yes, I think that would be reasonable. XFS already relies on this so
> it should be safe.
Andrew, can you send me a patch?
Thanks,
- Ted
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: dead/wrong code in ext3/4_releasepage()
2012-07-02 18:17 ` Theodore Ts'o
@ 2012-07-02 18:19 ` Andrew Perepechko
0 siblings, 0 replies; 6+ messages in thread
From: Andrew Perepechko @ 2012-07-02 18:19 UTC (permalink / raw)
To: Theodore Ts'o; +Cc: Jan Kara, linux-ext4
Hi Theodore!
Sent.
Thank you,
Andrew
On 07/02/2012 10:17 PM, Theodore Ts'o wrote:
> On Mon, Jul 02, 2012 at 07:33:26PM +0200, Jan Kara wrote:
>> Hi,
>>
>> On Mon 02-07-12 21:14:58, Andrew Perepechko wrote:
>>> So you think the correct fix would be to remove the check completely
>>> since every try_to_release_page() caller is obliged to perform the
>>> page_has_private() check?
>> Yes, I think that would be reasonable. XFS already relies on this so
>> it should be safe.
>
> Andrew, can you send me a patch?
>
> Thanks,
>
> - Ted
> --
> To unsubscribe from this list: send the line "unsubscribe linux-ext4" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2012-07-02 18:30 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2012-06-29 16:52 dead/wrong code in ext3/4_releasepage() Andrew Perepechko
2012-07-02 17:09 ` Jan Kara
2012-07-02 17:14 ` Andrew Perepechko
2012-07-02 17:33 ` Jan Kara
2012-07-02 18:17 ` Theodore Ts'o
2012-07-02 18:19 ` Andrew Perepechko
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox