Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Date:   Wed, 17 Mar 2021 18:39:16 +0100
From:   Christoph Hellwig <hch@infradead.org>
To:     "Matthew Wilcox (Oracle)" <willy@infradead.org>
Cc:     linux-mm@kvack.org, linux-kernel@vger.kernel.org,
        linux-fsdevel@vger.kernel.org
Subject: Re: [PATCH v4 20/25] mm/filemap: Convert wait_on_page_bit to
 wait_on_folio_bit
Message-ID: <YFI+xNeN+NrgszI7@infradead.org>
References: <20210305041901.2396498-1-willy@infradead.org>
 <20210305041901.2396498-21-willy@infradead.org>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20210305041901.2396498-21-willy@infradead.org>
Precedence: bulk

> +	if (FolioWriteback(folio) &&
> +	    wait_on_folio_bit_killable(folio, PG_writeback) < 0)
>  		return VM_FAULT_RETRY;

This really screams for a proper wait_on_page_writeback_killable helper
rather than hardcoding the PG_* bit in a random file system.  It also
seems to have missed the conversion to a while loop in
wait_on_page_writeback.

Also this patch seems to be different in style to other by not for now
always using page wrappers in the file systems.  Not that I really care
either way, but it seems inconsistent with the rest.

>  /*
> - * This is exported only for wait_on_page_locked/wait_on_page_writeback, etc.,
> + * This is exported only for wait_on_folio_locked/wait_on_folio_writeback, etc.,
>   * and should not be used directly.
>   */
> -extern void wait_on_page_bit(struct page *page, int bit_nr);
> -extern int wait_on_page_bit_killable(struct page *page, int bit_nr);
> +extern void wait_on_folio_bit(struct folio *folio, int bit_nr);
> +extern int wait_on_folio_bit_killable(struct folio *folio, int bit_nr);

Well, the above code obviously ignored this comment :(  Maybe an
__ prefix is a bit more of hint?