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;
Date:   Thu, 29 Jul 2021 10:19:24 -0500
From:   Goldwyn Rodrigues <rgoldwyn@suse.de>
To:     NeilBrown <neilb@suse.de>
Cc:     Matthew Wilcox <willy@infradead.org>,
        linux-fsdevel@vger.kernel.org, linux-btrfs@vger.kernel.org,
        linux-fscrypt@vger.kernel.org, linux-nfs@vger.kernel.org,
        codalist@coda.cs.cmu.edu
Subject: Re: [PATCH] fs: reduce pointers while using file_ra_state_init()
Message-ID: <20210729151924.ncwwsz6x6jknyk6t@fiona>
References: <20210726164647.brx3l2ykwv3zz7vr@fiona>
 <162753473650.21659.5563242071693885551@noble.neil.brown.name>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <162753473650.21659.5563242071693885551@noble.neil.brown.name>
Precedence: bulk

On 14:58 29/07, NeilBrown wrote:
> 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.
> 

Thanks Neil. Coda seems to be the only filesystem to manipulate
inode->i_mapping to support the mmap() operation and eventually resets
it back on release().

Not sure if this hack should be put in just for coda, or just leave the
function prototype as it is to accept address_space *. I will send out
another patch to see what others feel about it.


-- 
Goldwyn