Re: [PATCH] fs/netfs/fscache_io: remove the obsolete "using_pgpriv2" flag

[Jeff Layton]

On Mon, 2024-07-29 at 11:15 +0200, Max Kellermann wrote:
> This fixes a crash bug caused by commit ae678317b95e ("netfs: Remove
> deprecated use of PG_private_2 as a second writeback flag") by
> removing a leftover folio_end_private_2() call after all calls to
> folio_start_private_2() had been removed by the commit.
> 
> By calling folio_end_private_2() without folio_start_private_2(), the
> folio refcounter breaks and causes trouble like RCU stalls and general
> protection faults.
> 
> Signed-off-by: Max Kellermann <max.kellermann@xxxxxxxxx>
> Fixes: ae678317b95e ("netfs: Remove deprecated use of PG_private_2 as a second writeback flag")
> Link: https://lore.kernel.org/ceph-devel/CAKPOu+_DA8XiMAA2ApMj7Pyshve_YWknw8Hdt1=zCy9Y87R1qw@xxxxxxxxxxxxxx/
> Signed-off-by: Max Kellermann <max.kellermann@xxxxxxxxx>
> ---
>  fs/ceph/addr.c          |  2 +-
>  fs/netfs/fscache_io.c   | 29 +----------------------------
>  include/linux/fscache.h | 30 ++++--------------------------
>  3 files changed, 6 insertions(+), 55 deletions(-)
> 
> diff --git a/fs/ceph/addr.c b/fs/ceph/addr.c
> index 8c16bc5250ef..485cbd1730d1 100644
> --- a/fs/ceph/addr.c
> +++ b/fs/ceph/addr.c
> @@ -512,7 +512,7 @@ static void ceph_fscache_write_to_cache(struct inode *inode, u64 off, u64 len, b
>  	struct fscache_cookie *cookie = ceph_fscache_cookie(ci);
>  
>  	fscache_write_to_cache(cookie, inode->i_mapping, off, len, i_size_read(inode),
> -			       ceph_fscache_write_terminated, inode, true, caching);
> +			       ceph_fscache_write_terminated, inode, caching);
>  }
>  #else
>  static inline void ceph_fscache_write_to_cache(struct inode *inode, u64 off, u64 len, bool caching)
> diff --git a/fs/netfs/fscache_io.c b/fs/netfs/fscache_io.c
> index 38637e5c9b57..0d8f3f646598 100644
> --- a/fs/netfs/fscache_io.c
> +++ b/fs/netfs/fscache_io.c
> @@ -166,30 +166,10 @@ struct fscache_write_request {
>  	loff_t			start;
>  	size_t			len;
>  	bool			set_bits;
> -	bool			using_pgpriv2;
>  	netfs_io_terminated_t	term_func;
>  	void			*term_func_priv;
>  };
>  
> -void __fscache_clear_page_bits(struct address_space *mapping,
> -			       loff_t start, size_t len)
> -{
> -	pgoff_t first = start / PAGE_SIZE;
> -	pgoff_t last = (start + len - 1) / PAGE_SIZE;
> -	struct page *page;
> -
> -	if (len) {
> -		XA_STATE(xas, &mapping->i_pages, first);
> -
> -		rcu_read_lock();
> -		xas_for_each(&xas, page, last) {
> -			folio_end_private_2(page_folio(page));
> -		}
> -		rcu_read_unlock();
> -	}
> -}
> -EXPORT_SYMBOL(__fscache_clear_page_bits);
> -
>  /*
>   * Deal with the completion of writing the data to the cache.
>   */
> @@ -198,10 +178,6 @@ static void fscache_wreq_done(void *priv, ssize_t transferred_or_error,
>  {
>  	struct fscache_write_request *wreq = priv;
>  
> -	if (wreq->using_pgpriv2)
> -		fscache_clear_page_bits(wreq->mapping, wreq->start, wreq->len,
> -					wreq->set_bits);
> -
>  	if (wreq->term_func)
>  		wreq->term_func(wreq->term_func_priv, transferred_or_error,
>  				was_async);
> @@ -214,7 +190,7 @@ void __fscache_write_to_cache(struct fscache_cookie *cookie,
>  			      loff_t start, size_t len, loff_t i_size,
>  			      netfs_io_terminated_t term_func,
>  			      void *term_func_priv,
> -			      bool using_pgpriv2, bool cond)
> +			      bool cond)
>  {
>  	struct fscache_write_request *wreq;
>  	struct netfs_cache_resources *cres;
> @@ -232,7 +208,6 @@ void __fscache_write_to_cache(struct fscache_cookie *cookie,
>  	wreq->mapping		= mapping;
>  	wreq->start		= start;
>  	wreq->len		= len;
> -	wreq->using_pgpriv2	= using_pgpriv2;
>  	wreq->set_bits		= cond;
>  	wreq->term_func		= term_func;
>  	wreq->term_func_priv	= term_func_priv;
> @@ -260,8 +235,6 @@ void __fscache_write_to_cache(struct fscache_cookie *cookie,
>  abandon_free:
>  	kfree(wreq);
>  abandon:
> -	if (using_pgpriv2)
> -		fscache_clear_page_bits(mapping, start, len, cond);
>  	if (term_func)
>  		term_func(term_func_priv, ret, false);
>  }
> diff --git a/include/linux/fscache.h b/include/linux/fscache.h
> index 9de27643607f..f8c52bddaa15 100644
> --- a/include/linux/fscache.h
> +++ b/include/linux/fscache.h
> @@ -177,8 +177,7 @@ void __fscache_write_to_cache(struct fscache_cookie *cookie,
>  			      loff_t start, size_t len, loff_t i_size,
>  			      netfs_io_terminated_t term_func,
>  			      void *term_func_priv,
> -			      bool using_pgpriv2, bool cond);
> -extern void __fscache_clear_page_bits(struct address_space *, loff_t, size_t);
> +			      bool cond);
>  
>  /**
>   * fscache_acquire_volume - Register a volume as desiring caching services
> @@ -573,24 +572,6 @@ int fscache_write(struct netfs_cache_resources *cres,
>  	return ops->write(cres, start_pos, iter, term_func, term_func_priv);
>  }
>  
> -/**
> - * fscache_clear_page_bits - Clear the PG_fscache bits from a set of pages
> - * @mapping: The netfs inode to use as the source
> - * @start: The start position in @mapping
> - * @len: The amount of data to unlock
> - * @caching: If PG_fscache has been set
> - *
> - * Clear the PG_fscache flag from a sequence of pages and wake up anyone who's
> - * waiting.
> - */
> -static inline void fscache_clear_page_bits(struct address_space *mapping,
> -					   loff_t start, size_t len,
> -					   bool caching)
> -{
> -	if (caching)
> -		__fscache_clear_page_bits(mapping, start, len);
> -}
> -
>  /**
>   * fscache_write_to_cache - Save a write to the cache and clear PG_fscache
>   * @cookie: The cookie representing the cache object
> @@ -600,7 +581,6 @@ static inline void fscache_clear_page_bits(struct address_space *mapping,
>   * @i_size: The new size of the inode
>   * @term_func: The function to call upon completion
>   * @term_func_priv: The private data for @term_func
> - * @using_pgpriv2: If we're using PG_private_2 to mark in-progress write
>   * @caching: If we actually want to do the caching
>   *
>   * Helper function for a netfs to write dirty data from an inode into the cache
> @@ -612,21 +592,19 @@ static inline void fscache_clear_page_bits(struct address_space *mapping,
>   * marked with PG_fscache.
>   *
>   * If given, @term_func will be called upon completion and supplied with
> - * @term_func_priv.  Note that if @using_pgpriv2 is set, the PG_private_2 flags
> - * will have been cleared by this point, so the netfs must retain its own pin
> - * on the mapping.
> + * @term_func_priv.
>   */
>  static inline void fscache_write_to_cache(struct fscache_cookie *cookie,
>  					  struct address_space *mapping,
>  					  loff_t start, size_t len, loff_t i_size,
>  					  netfs_io_terminated_t term_func,
>  					  void *term_func_priv,
> -					  bool using_pgpriv2, bool caching)
> +					  bool caching)
>  {
>  	if (caching)
>  		__fscache_write_to_cache(cookie, mapping, start, len, i_size,
>  					 term_func, term_func_priv,
> -					 using_pgpriv2, caching);
> +					 caching);
>  	else if (term_func)
>  		term_func(term_func_priv, -ENOBUFS, false);
>  


(cc'ing the cephfs maintainers too)

Nice work! I'd prefer this patch over the first one. It looks like the
Fixes: commit went into v6.10. Did it go into earlier kernels too?

If so, what might be best is to take both of your patches. Have the
simple one first that just flips the flag, and mark that one for
stable. Then we can add the second patch on top to remove all of this
stuff for mainline.

Either way, you can add this to both patches:

Reviewed-by: Jeff Layton <jlayton@xxxxxxxxxx>