Re: [PATCH v4 02/28] mm: Add an unlock function for PG_private_2/PG_fscache

From: Matthew Wilcox
Date: Tue Mar 23 2021 - 09:53:00 EST


On Tue, Mar 23, 2021 at 01:17:20PM +0000, David Howells wrote:
> +++ b/fs/afs/write.c
> @@ -846,7 +846,7 @@ vm_fault_t afs_page_mkwrite(struct vm_fault *vmf)
> */
> #ifdef CONFIG_AFS_FSCACHE
> if (PageFsCache(page) &&
> - wait_on_page_bit_killable(page, PG_fscache) < 0)
> + wait_on_page_fscache_killable(page) < 0)
> return VM_FAULT_RETRY;
> #endif
>
> @@ -861,7 +861,8 @@ vm_fault_t afs_page_mkwrite(struct vm_fault *vmf)
> * details the portion of the page we need to write back and we might
> * need to redirty the page if there's a problem.
> */
> - wait_on_page_writeback(page);
> + if (wait_on_page_writeback_killable(page) < 0)
> + return VM_FAULT_RETRY | VM_FAULT_LOCKED;

You forgot to unlock the page. Also, if you're waiting killably here,
do you need to wait before you get the page lock? Ditto for waiting on
fscache -- do you want to do that before or after you get the page lock?

Also, I never quite understood why you needed to wait for fscache
writes to finish before allowing the page to be dirtied. Is this a
wait_for_stable_page() kind of situation, where the cache might be
calculating a checksum on it? Because as far as I can tell, once the
page is dirty in RAM, the contents of the on-disk cache are irrelevant ...
unless they're part of a RAID 5 checksum kind of situation.

I didn't spot any other problems ...