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
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 <[email protected]>
SUSE Labs, CR
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
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 [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
--
Jan Kara <[email protected]>
SUSE Labs, CR
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
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 [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html