Re: [PATCH 05/11] afs: Fix to take ref on page when PG_private is set

[Matthew Wilcox <willy@infradead.org>]

On Wed, Oct 28, 2020 at 02:10:24PM +0000, David Howells wrote:
> +++ b/fs/afs/dir.c
> @@ -283,6 +283,7 @@ static struct afs_read *afs_read_dir(struct afs_vnode *dvnode, struct key *key)
>  
>  			set_page_private(req->pages[i], 1);
>  			SetPagePrivate(req->pages[i]);
> +			get_page(req->pages[i]);

Alternative spelling:

-			set_page_private(req->pages[i], 1);
-			SetPagePrivate(req->pages[i]);
+			attach_page_private(req->pages[i], (void *)1);

AFS is an anomaly; most filesystems actually stick a pointer in page->private.

> +++ b/fs/afs/write.c
> @@ -151,7 +151,8 @@ int afs_write_begin(struct file *file, struct address_space *mapping,
>  	priv |= f;
>  	trace_afs_page_dirty(vnode, tracepoint_string("begin"),
>  			     page->index, priv);
> -	SetPagePrivate(page);
> +	if (!TestSetPagePrivate(page))
> +		get_page(page);
>  	set_page_private(page, priv);
>  	_leave(" = 0");
>  	return 0;

There's an efficiency question here that I can't answer ... how often do
you call afs_write_begin() on a page which already has PagePrivate set?
It's fewer atomic ops to do:

	if (PagePrivate(page))
		set_page_private(page, priv);
	else
		attach_page_private(page, (void *)priv);

I have no objection to adding TestSetPagePrivate per se; I just don't
know if it's really what you want or not.