Re: [PATCH 2/2] mm: Use clear_bit_unlock_is_negative_byte for PageWriteback

[William Kucharski <william.kucharski@oracle.com>]

I like it explicit in the code before __clear_page_writeback() is called,
and given it's the only caller I think it adds clarity to have it in
end_page_writeback() - but either approach is fine with me.

For the series:

Reviewed-by: William Kucharski <william.kucharski@oracle.com>

> On Mar 26, 2020, at 6:44 AM, Matthew Wilcox <willy@infradead.org> wrote:
> 
> On Thu, Mar 26, 2020 at 01:40:47PM +0100, Jan Kara wrote:
>> On Thu 26-03-20 05:24:29, Matthew Wilcox wrote:
>>> From: "Matthew Wilcox (Oracle)" <willy@infradead.org>
>>> 
>>> By moving PG_writeback down into the low bits of the page flags, we can
>>> use clear_bit_unlock_is_negative_byte() for writeback as well as the
>>> lock bit.  wake_up_page() then has no more callers.  Given the other
>>> code being executed between the clear and the test, this is not going
>>> to be as dramatic a win as it was for PageLocked, but symmetry between
>>> the two is nice and lets us remove some code.
>>> 
>>> Signed-off-by: Matthew Wilcox (Oracle) <willy@infradead.org>
>> 
>> The patch looks good to me. Just one nit:
>> 
>>> +	VM_BUG_ON_PAGE(!PageWriteback(page), page);
>>> +	if (__clear_page_writeback(page))
>>> +		wake_up_page_bit(page, PG_writeback);
>> 
>> Since __clear_page_writeback() isn't really prepared for PageWriteback()
>> not being set, can we move the VM_BUG_ON_PAGE() there? Otherwise feel free
>> to add:
>> 
>> Reviewed-by: Jan Kara <jack@suse.cz>
> 
> Thanks!  I put it there to be parallel to the PageLocked equivalent:
> 
> void unlock_page(struct page *page)
> {
>        BUILD_BUG_ON(PG_waiters != 7);
>        BUILD_BUG_ON(PG_locked > 7);
>        page = compound_head(page);
>        VM_BUG_ON_PAGE(!PageLocked(page), page);
>        if (clear_bit_unlock_is_negative_byte(PG_locked, &page->flags))
>                wake_up_page_bit(page, PG_locked);
> }
> 
> but one could equally well argue it should be in __clear_page_writeback
> instead.
>