Linux-EROFS Archive on lore.kernel.org
 help / color / Atom feed
From: Matthew Wilcox <willy@infradead.org>
To: Dave Chinner <david@fromorbit.com>
Cc: linux-xfs@vger.kernel.org, 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, linux-btrfs@vger.kernel.org
Subject: Re: [PATCH v5 04/13] mm: Add readahead address space operation
Date: Tue, 11 Feb 2020 04:54:13 -0800
Message-ID: <20200211125413.GU8731@bombadil.infradead.org> (raw)
In-Reply-To: <20200211045230.GD10776@dread.disaster.area>

On Tue, Feb 11, 2020 at 03:52:30PM +1100, Dave Chinner wrote:
> > +struct readahead_control {
> > +	struct file *file;
> > +	struct address_space *mapping;
> > +/* private: use the readahead_* accessors instead */
> > +	pgoff_t start;
> > +	unsigned int nr_pages;
> > +	unsigned int batch_count;
> > +};
> > +
> > +static inline struct page *readahead_page(struct readahead_control *rac)
> > +{
> > +	struct page *page;
> > +
> > +	if (!rac->nr_pages)
> > +		return NULL;
> > +
> > +	page = xa_load(&rac->mapping->i_pages, rac->start);
> > +	VM_BUG_ON_PAGE(!PageLocked(page), page);
> > +	rac->batch_count = hpage_nr_pages(page);
> > +	rac->start += rac->batch_count;
> 
> There's no mention of large page support in the patch description
> and I don't recall this sort of large page batching in previous
> iterations.
> 
> This seems like new functionality to me, not directly related to
> the initial ->readahead API change? What have I missed?

I had a crisis of confidence when I was working on this -- the loop
originally looked like this:

#define readahead_for_each(rac, page)                                   \
        for (; (page = readahead_page(rac)); rac->nr_pages--)

and then I started thinking about what I'd need to do to support large
pages, and that turned into

#define readahead_for_each(rac, page)                                   \
        for (; (page = readahead_page(rac));				\
		rac->nr_pages -= hpage_nr_pages(page))

but I realised that was potentially a use-after-free because 'page' has
certainly had put_page() called on it by then.  I had a brief period
where I looked at moving put_page() away from being the filesystem's
responsibility and into the iterator, but that would introduce more
changes into the patchset, as well as causing problems for filesystems
that want to break out of the loop.

By this point, I was also looking at the readahead_for_each_batch()
iterator that btrfs uses, and so we have the batch count anyway, and we
might as well use it to store the number of subpages of the large page.
And so it became easier to just put the whole ball of wax into the initial
patch set, rather than introduce the iterator now and then fix it up in
the patch set that I'm basing on this.

So yes, there's a certain amount of excess functionality in this patch
set ... I can remove it for the next release.

  reply index

Thread overview: 31+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-02-11  1:03 [PATCH v5 00/13] Change readahead API Matthew Wilcox
2020-02-11  1:03 ` [PATCH v5 01/13] mm: Fix the return type of __do_page_cache_readahead Matthew Wilcox
2020-02-11  8:19   ` Johannes Thumshirn
2020-02-11 12:34     ` Matthew Wilcox
2020-02-12 18:13   ` Christoph Hellwig
2020-02-14  3:19   ` John Hubbard
2020-02-14  4:21     ` Matthew Wilcox
2020-02-14  4:33       ` John Hubbard
2020-02-14 19:50   ` Matthew Wilcox
2020-02-11  1:03 ` [PATCH v5 02/13] mm: Ignore return value of ->readpages Matthew Wilcox
2020-02-12 18:13   ` Christoph Hellwig
2020-02-11  1:03 ` [PATCH v5 03/13] mm: Put readahead pages in cache earlier Matthew Wilcox
2020-02-14  3:36   ` John Hubbard
2020-02-15  1:15     ` Matthew Wilcox
2020-02-11  1:03 ` [PATCH v5 04/13] mm: Add readahead address space operation Matthew Wilcox
2020-02-11  4:52   ` Dave Chinner
2020-02-11 12:54     ` Matthew Wilcox [this message]
2020-02-11 20:08       ` Dave Chinner
2020-02-12 18:18   ` Christoph Hellwig
2020-02-14  5:36   ` John Hubbard
2020-02-15  1:15     ` Matthew Wilcox
2020-02-11  1:03 ` [PATCH v5 05/13] mm: Add page_cache_readahead_limit Matthew Wilcox
2020-02-11  1:03 ` [PATCH v5 06/13] fs: Convert mpage_readpages to mpage_readahead Matthew Wilcox
2020-02-13 22:09   ` Junxiao Bi
2020-02-11  1:03 ` [PATCH v5 07/13] btrfs: Convert from readpages to readahead Matthew Wilcox
2020-02-11  1:03 ` [PATCH v5 08/13] erofs: Convert uncompressed files " Matthew Wilcox
2020-02-11  1:03 ` [PATCH v5 09/13] erofs: Convert compressed " Matthew Wilcox
2020-02-11  1:03 ` [PATCH v5 10/13] ext4: Convert " Matthew Wilcox
2020-02-11  1:03 ` [PATCH v5 11/13] f2fs: " Matthew Wilcox
2020-02-11  1:03 ` [PATCH v5 12/13] fuse: " Matthew Wilcox
2020-02-11  1:03 ` [PATCH v5 13/13] iomap: " Matthew Wilcox

Reply instructions:

You may reply publically 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=20200211125413.GU8731@bombadil.infradead.org \
    --to=willy@infradead.org \
    --cc=cluster-devel@redhat.com \
    --cc=david@fromorbit.com \
    --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 \
    /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