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

[Matthew Wilcox <willy@xxxxxxxxxxxxx>]

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.