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@xxxxxxxxxx>
> On Mar 26, 2020, at 6:44 AM, Matthew Wilcox <willy@xxxxxxxxxxxxx> 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@xxxxxxxxxxxxx>
>>>
>>> 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@xxxxxxxxxxxxx>
>>
>> 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@xxxxxxx>
>
> 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.
>
