Received-SPF: pass (google.com: domain of linux-nfs-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Content-Type: text/plain; charset="utf-8"
Content-Transfer-Encoding: 7bit
MIME-Version: 1.0
From:   "NeilBrown" <neilb@suse.de>
To:     "Goldwyn Rodrigues" <rgoldwyn@suse.de>,
        Matthew Wilcox <willy@infradead.org>
Cc:     linux-fsdevel@vger.kernel.org, linux-btrfs@vger.kernel.org,
        linux-fscrypt@vger.kernel.org, linux-nfs@vger.kernel.org
Subject: Re: [PATCH] fs: reduce pointers while using file_ra_state_init()
In-reply-to: <20210726164647.brx3l2ykwv3zz7vr@fiona>
References: <20210726164647.brx3l2ykwv3zz7vr@fiona>
Date:   Thu, 29 Jul 2021 14:58:56 +1000
Message-id: <162753473650.21659.5563242071693885551@noble.neil.brown.name>
Precedence: bulk

On Tue, 27 Jul 2021, Goldwyn Rodrigues wrote:
> Simplification.
> 
> file_ra_state_init() take struct address_space *, just to use inode
> pointer by dereferencing from mapping->host.
> 
> The callers also derive mapping either by file->f_mapping, or
> even file->f_mapping->host->i_mapping.
> 
> Change file_ra_state_init() to accept struct inode * to reduce pointer
> dereferencing, both in the callee and the caller.
> 
> Signed-off-by: Goldwyn Rodrigues <rgoldwyn@suse.com>
> ---
....

> diff --git a/mm/readahead.c b/mm/readahead.c
> index d589f147f4c2..3541941df5e7 100644
> --- a/mm/readahead.c
> +++ b/mm/readahead.c
> @@ -31,9 +31,9 @@
>   * memset *ra to zero.
>   */
>  void
> -file_ra_state_init(struct file_ra_state *ra, struct address_space *mapping)
> +file_ra_state_init(struct file_ra_state *ra, struct inode *inode)
>  {
> -	ra->ra_pages = inode_to_bdi(mapping->host)->ra_pages;
> +	ra->ra_pages = inode_to_bdi(inode)->ra_pages;
>  	ra->prev_pos = -1;

I think this patch can be made OK by adding:

  if (unlikely(inode->i_mapping != &inode->i_data))
	inode = inode->i_mapping->host;

The "unlikely" is mostly for documentation.
Loading "inode->i_mapping" is nearly free as that cache line needs to be
loaded to get i_sb, which inode_to_bdi() needs.  Calculating &->i_data
is trivial.  So this adds minimal cost, and preserves correctness.

NeilBrown


>  }
>  EXPORT_SYMBOL_GPL(file_ra_state_init);
> -- 
> 2.32.0
> 
> 
> -- 
> Goldwyn
> 
>