Re: [PATCH 01/21] fs: readahead_begin() to call before locking folio

Re: [PATCH 01/21] fs: readahead_begin() to call before locking folio

[Christoph Hellwig]

On Thu, Mar 02, 2023 at 04:24:46PM -0600, Goldwyn Rodrigues wrote:
> From: Goldwyn Rodrigues <rgoldwyn@suse.com>
> 
> The btrfs filesystem needs to lock the extents before locking folios
> to be read from disk. So, introduce a function in
> address_space_operaitons, called btrfs_readahead_begin() which is called
> before the folio are allocateed and locked.

Please Cc the mm and fsdevel and willy on these kinds of changes.

But I'd also like to take this opportunity to ask what the rationale
behind the extent locking for reads in btrfs is to start with.

All other file systems rely on filemap_invalidate_lock_shared for
locking page reads vs invalidates and it seems to work great.  btrfs
creates a lot of overhead with the extent locking, and introduces
a lot of additional trouble like the readahead code here, or the problem
with O_DIRECT writes that read from the same region that Boris recently
fixed.

Maybe we can think really hard and find a way to normalize the locking
and simply both btrfs and common infrastructure?

> ---
>  include/linux/fs.h | 1 +
>  mm/readahead.c     | 3 +++
>  2 files changed, 4 insertions(+)
> 
> diff --git a/include/linux/fs.h b/include/linux/fs.h
> index c1769a2c5d70..6b650db57ca3 100644
> --- a/include/linux/fs.h
> +++ b/include/linux/fs.h
> @@ -363,6 +363,7 @@ struct address_space_operations {
>  	/* Mark a folio dirty.  Return true if this dirtied it */
>  	bool (*dirty_folio)(struct address_space *, struct folio *);
>  
> +	void (*readahead_begin)(struct readahead_control *);
>  	void (*readahead)(struct readahead_control *);
>  
>  	int (*write_begin)(struct file *, struct address_space *mapping,
> diff --git a/mm/readahead.c b/mm/readahead.c
> index b10f0cf81d80..6924d5fed350 100644
> --- a/mm/readahead.c
> +++ b/mm/readahead.c
> @@ -520,6 +520,9 @@ void page_cache_ra_order(struct readahead_control *ractl,
>  			new_order--;
>  	}
>  
> +	if (mapping->a_ops->readahead_begin)
> +		mapping->a_ops->readahead_begin(ractl);
> +
>  	filemap_invalidate_lock_shared(mapping);
>  	while (index <= limit) {
>  		unsigned int order = new_order;
> -- 
> 2.39.2
> 
---end quoted text---