Linux-EROFS Archive on lore.kernel.org
 help / color / Atom feed
From: Andrew Morton <akpm@linux-foundation.org>
To: Matthew Wilcox <willy@infradead.org>
Cc: linux-xfs@vger.kernel.org,
	William Kucharski <william.kucharski@oracle.com>,
	linux-kernel@vger.kernel.org,
	linux-f2fs-devel@lists.sourceforge.net, cluster-devel@redhat.com,
	linux-mm@kvack.org, ocfs2-devel@oss.oracle.com,
	linux-fsdevel@vger.kernel.org, linux-ext4@vger.kernel.org,
	linux-erofs@lists.ozlabs.org, Christoph Hellwig <hch@lst.de>,
	linux-btrfs@vger.kernel.org
Subject: Re: [PATCH v11 05/25] mm: Add new readahead_control API
Date: Tue, 14 Apr 2020 21:56:16 -0700
Message-ID: <20200414215616.f665d12f8549f52606784d1e@linux-foundation.org> (raw)
In-Reply-To: <20200415021808.GA5820@bombadil.infradead.org>

On Tue, 14 Apr 2020 19:18:08 -0700 Matthew Wilcox <willy@infradead.org> wrote:

> On Tue, Apr 14, 2020 at 06:17:05PM -0700, Andrew Morton wrote:
> > On Tue, 14 Apr 2020 08:02:13 -0700 Matthew Wilcox <willy@infradead.org> wrote:
> > > From: "Matthew Wilcox (Oracle)" <willy@infradead.org>
> > > 
> > > Filesystems which implement the upcoming ->readahead method will get
> > > their pages by calling readahead_page() or readahead_page_batch().
> > > These functions support large pages, even though none of the filesystems
> > > to be converted do yet.
> > > 
> > > +static inline struct page *readahead_page(struct readahead_control *rac)
> > > +static inline unsigned int __readahead_batch(struct readahead_control *rac,
> > > +		struct page **array, unsigned int array_sz)
> > 
> > These are large functions.  Was it correct to inline them?
> 
> Hmm.  They don't seem that big to me.

They're really big!

> readahead_page, stripped of its sanity checks:

Well, the sanity checks still count for cache footprint.

otoh, I think a function which is expected to be called from a single
site per filesystem is OK to be inlined, because there's not likely to
be much icache benefit unless different filesystem types are
simultaneously being used heavily, which sounds unlikely.  Although
there's still a bit of overall code size bloat.

> +       rac->_index += rac->_batch_count;
> +       if (!rac->_nr_pages) {
> +               rac->_batch_count = 0;
> +               return NULL;
> +       }
> +       page = xa_load(&rac->mapping->i_pages, rac->_index);
> +       rac->_batch_count = hpage_nr_pages(page);
> 
> __readahead_batch is much bigger, but it's only used by btrfs and fuse,
> and it seemed unfair to make everybody pay the cost for a function only
> used by two filesystems.

Do we expect more filesystems to use these in the future?

These function are really big!

> > The batching API only appears to be used by fuse?  If so, do we really
> > need it?  Does it provide some functional need, or is it a performance
> > thing?  If the latter, how significant is it?
> 
> I must confess to not knowing the performance impact.  If the code uses
> xa_load() repeatedly, it costs O(log n) each time as we walk down the tree
> (mitigated to a large extent by cache, of course).  Using xas_for_each()
> keeps us at the bottom of the tree and each iteration is O(1).
> I'm interested to see if filesystem maintainers start to use the batch
> function or if they're happier sticking with the individual lookups.
> 
> The batch API was originally written for use with btrfs, but it was a
> significant simplification to convert fuse to use it.

hm, OK.  It's not clear that its inclusion is justified?

> > The code adds quite a few (inlined!) VM_BUG_ONs.  Can we plan to remove
> > them at some stage?  Such as, before Linus shouts at us :)
> 
> I'd be happy to remove them.  Various reviewers said things like "are you
> sure this can't happen?"

Yeah, these things tend to live for ever.  Please add a todo to remove
them after the code has matured?


  reply index

Thread overview: 40+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-04-14 15:02 [PATCH v11 00/25] Change readahead API Matthew Wilcox
2020-04-14 15:02 ` [PATCH v11 01/25] mm: Move readahead prototypes from mm.h Matthew Wilcox
2020-04-15  9:10   ` Johannes Thumshirn
2020-04-14 15:02 ` [PATCH v11 02/25] mm: Return void from various readahead functions Matthew Wilcox
2020-04-14 15:02 ` [PATCH v11 03/25] mm: Ignore return value of ->readpages Matthew Wilcox
2020-04-15  9:17   ` Johannes Thumshirn
2020-04-14 15:02 ` [PATCH v11 04/25] mm: Move readahead nr_pages check into read_pages Matthew Wilcox
2020-04-15  9:19   ` Johannes Thumshirn
2020-04-14 15:02 ` [PATCH v11 05/25] mm: Add new readahead_control API Matthew Wilcox
2020-04-15  1:17   ` Andrew Morton
2020-04-15  2:18     ` Matthew Wilcox
2020-04-15  4:56       ` Andrew Morton [this message]
2020-04-15 11:22         ` Matthew Wilcox
2020-04-14 15:02 ` [PATCH v11 06/25] mm: Use readahead_control to pass arguments Matthew Wilcox
2020-04-15  9:30   ` Johannes Thumshirn
2020-04-14 15:02 ` [PATCH v11 07/25] mm: Rename various 'offset' parameters to 'index' Matthew Wilcox
2020-04-14 15:02 ` [PATCH v11 08/25] mm: rename readahead loop variable to 'i' Matthew Wilcox
2020-04-15  9:31   ` Johannes Thumshirn
2020-04-14 15:02 ` [PATCH v11 09/25] mm: Remove 'page_offset' from readahead loop Matthew Wilcox
2020-04-14 15:02 ` [PATCH v11 10/25] mm: Put readahead pages in cache earlier Matthew Wilcox
2020-04-14 15:02 ` [PATCH v11 11/25] mm: Add readahead address space operation Matthew Wilcox
2020-04-14 15:02 ` [PATCH v11 12/25] mm: Move end_index check out of readahead loop Matthew Wilcox
2020-04-14 15:02 ` [PATCH v11 13/25] mm: Add page_cache_readahead_unbounded Matthew Wilcox
2020-04-14 15:02 ` [PATCH v11 14/25] mm: Document why we don't set PageReadahead Matthew Wilcox
2020-04-14 15:02 ` [PATCH v11 15/25] mm: Use memalloc_nofs_save in readahead path Matthew Wilcox
2020-04-14 15:02 ` [PATCH v11 16/25] fs: Convert mpage_readpages to mpage_readahead Matthew Wilcox
2020-04-14 15:02 ` [PATCH v11 17/25] btrfs: Convert from readpages to readahead Matthew Wilcox
2020-04-14 15:02 ` [PATCH v11 18/25] erofs: Convert uncompressed files " Matthew Wilcox
2020-04-14 15:02 ` [PATCH v11 19/25] erofs: Convert compressed " Matthew Wilcox
2020-04-21  5:42   ` Andrew Morton
2020-04-21  7:28     ` Gao Xiang via Linux-erofs
2020-04-14 15:02 ` [PATCH v11 20/25] ext4: Convert " Matthew Wilcox
2020-04-14 15:02 ` [PATCH v11 21/25] ext4: Pass the inode to ext4_mpage_readpages Matthew Wilcox
2020-04-14 15:02 ` [PATCH v11 22/25] f2fs: Convert from readpages to readahead Matthew Wilcox
2020-04-14 15:02 ` [PATCH v11 23/25] f2fs: Pass the inode to f2fs_mpage_readpages Matthew Wilcox
2020-04-14 15:02 ` [PATCH v11 24/25] fuse: Convert from readpages to readahead Matthew Wilcox
2020-04-20 11:14   ` Miklos Szeredi
2020-04-20 11:43     ` Matthew Wilcox
2020-04-20 11:54       ` Miklos Szeredi
2020-04-14 15:02 ` [PATCH v11 25/25] iomap: " Matthew Wilcox

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20200414215616.f665d12f8549f52606784d1e@linux-foundation.org \
    --to=akpm@linux-foundation.org \
    --cc=cluster-devel@redhat.com \
    --cc=hch@lst.de \
    --cc=linux-btrfs@vger.kernel.org \
    --cc=linux-erofs@lists.ozlabs.org \
    --cc=linux-ext4@vger.kernel.org \
    --cc=linux-f2fs-devel@lists.sourceforge.net \
    --cc=linux-fsdevel@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-mm@kvack.org \
    --cc=linux-xfs@vger.kernel.org \
    --cc=ocfs2-devel@oss.oracle.com \
    --cc=william.kucharski@oracle.com \
    --cc=willy@infradead.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

Linux-EROFS Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-erofs/0 linux-erofs/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-erofs linux-erofs/ https://lore.kernel.org/linux-erofs \
		linux-erofs@lists.ozlabs.org linux-erofs@ozlabs.org
	public-inbox-index linux-erofs

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.ozlabs.lists.linux-erofs


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git