Re: [PATCH v7 14/24] btrfs: Convert from readpages to readahead
On Mon, Feb 24, 2020 at 01:54:14PM -0800, Matthew Wilcox wrote: > > First I think the implicit ARRAY_SIZE in readahead_page_batch is highly > > dangerous, as it will do the wrong thing when passing a pointer or > > function argument. > > somebody already thought of that ;-) > > #define ARRAY_SIZE(arr) (sizeof(arr) / sizeof((arr)[0]) + > __must_be_array(arr)) Ok. Still find it pretty weird to design a primary interface that just works with an array type.
Re: [PATCH v7 14/24] btrfs: Convert from readpages to readahead
On Mon, Feb 24, 2020 at 01:43:47PM -0800, Christoph Hellwig wrote: > On Thu, Feb 20, 2020 at 07:57:27AM -0800, Christoph Hellwig wrote: > > On Thu, Feb 20, 2020 at 07:54:52AM -0800, Matthew Wilcox wrote: > > > On Thu, Feb 20, 2020 at 07:46:58AM -0800, Christoph Hellwig wrote: > > > > On Thu, Feb 20, 2020 at 05:48:49AM -0800, Matthew Wilcox wrote: > > > > > btrfs: Convert from readpages to readahead > > > > > > > > > > Implement the new readahead method in btrfs. Add a > > > > > readahead_page_batch() > > > > > to optimise fetching a batch of pages at once. > > > > > > > > Shouldn't this readahead_page_batch heper go into a separate patch so > > > > that it clearly stands out? > > > > > > I'll move it into 'Put readahead pages in cache earlier' for v8 (the > > > same patch where we add readahead_page()) > > > > One argument for keeping it in a patch of its own is that btrfs appears > > to be the only user, and Goldwyn has a WIP conversion of btrfs to iomap, > > so it might go away pretty soon and we could just revert the commit. > > > > But this starts to get into really minor details, so I'll shut up now :) > > So looking at this again I have another comment and a question. > > First I think the implicit ARRAY_SIZE in readahead_page_batch is highly > dangerous, as it will do the wrong thing when passing a pointer or > function argument. somebody already thought of that ;-) #define ARRAY_SIZE(arr) (sizeof(arr) / sizeof((arr)[0]) + __must_be_array(arr)) > Second I wonder іf it would be worth to also switch to a batched > operation in iomap if the xarray overhead is high enough. That should > be pretty trivial, but we don't really need to do it in this series. I've also considered keeping a small array of pointers inside the readahead_control so nobody needs to have a readahead_page_batch() operation. Even keeping 10 pointers in there will reduce the XArray overhead by 90%. But this fit the current btrfs model well, and it lets us play with different approaches by abstracting everything away. I'm sure this won't be the last patch that touches the readahead code ;-)
Re: [PATCH v7 14/24] btrfs: Convert from readpages to readahead
On Thu, Feb 20, 2020 at 07:57:27AM -0800, Christoph Hellwig wrote: > On Thu, Feb 20, 2020 at 07:54:52AM -0800, Matthew Wilcox wrote: > > On Thu, Feb 20, 2020 at 07:46:58AM -0800, Christoph Hellwig wrote: > > > On Thu, Feb 20, 2020 at 05:48:49AM -0800, Matthew Wilcox wrote: > > > > btrfs: Convert from readpages to readahead > > > > > > > > Implement the new readahead method in btrfs. Add a > > > > readahead_page_batch() > > > > to optimise fetching a batch of pages at once. > > > > > > Shouldn't this readahead_page_batch heper go into a separate patch so > > > that it clearly stands out? > > > > I'll move it into 'Put readahead pages in cache earlier' for v8 (the > > same patch where we add readahead_page()) > > One argument for keeping it in a patch of its own is that btrfs appears > to be the only user, and Goldwyn has a WIP conversion of btrfs to iomap, > so it might go away pretty soon and we could just revert the commit. > > But this starts to get into really minor details, so I'll shut up now :) So looking at this again I have another comment and a question. First I think the implicit ARRAY_SIZE in readahead_page_batch is highly dangerous, as it will do the wrong thing when passing a pointer or function argument. Second I wonder іf it would be worth to also switch to a batched operation in iomap if the xarray overhead is high enough. That should be pretty trivial, but we don't really need to do it in this series.
Re: [PATCH v7 14/24] btrfs: Convert from readpages to readahead
On Thu, Feb 20, 2020 at 07:54:52AM -0800, Matthew Wilcox wrote: > On Thu, Feb 20, 2020 at 07:46:58AM -0800, Christoph Hellwig wrote: > > On Thu, Feb 20, 2020 at 05:48:49AM -0800, Matthew Wilcox wrote: > > > btrfs: Convert from readpages to readahead > > > > > > Implement the new readahead method in btrfs. Add a readahead_page_batch() > > > to optimise fetching a batch of pages at once. > > > > Shouldn't this readahead_page_batch heper go into a separate patch so > > that it clearly stands out? > > I'll move it into 'Put readahead pages in cache earlier' for v8 (the > same patch where we add readahead_page()) One argument for keeping it in a patch of its own is that btrfs appears to be the only user, and Goldwyn has a WIP conversion of btrfs to iomap, so it might go away pretty soon and we could just revert the commit. But this starts to get into really minor details, so I'll shut up now :)
Re: [PATCH v7 14/24] btrfs: Convert from readpages to readahead
On Thu, Feb 20, 2020 at 05:48:49AM -0800, Matthew Wilcox wrote: > btrfs: Convert from readpages to readahead > > Implement the new readahead method in btrfs. Add a readahead_page_batch() > to optimise fetching a batch of pages at once. Shouldn't this readahead_page_batch heper go into a separate patch so that it clearly stands out?
Re: [PATCH v7 14/24] btrfs: Convert from readpages to readahead
On Thu, Feb 20, 2020 at 09:42:19AM +, Johannes Thumshirn wrote:
> On 19/02/2020 22:03, Matthew Wilcox wrote:
> > From: "Matthew Wilcox (Oracle)"
> >
> > Use the new readahead operation in btrfs. Add a
> > readahead_for_each_batch() iterator to optimise the loop in the XArray.
>
> OK I must admit I haven't followed this series closely, but what
> happened to said readahead_for_each_batch()?
>
> As far as I can see it's now:
>
> [...]
> > + while ((nr = readahead_page_batch(rac, pagepool))) {
Oops, forgot to update the changelog there. Yes, that's exactly what it
changed to. That discussion was here:
https://lore.kernel.org/linux-fsdevel/20200219144117.gp24...@bombadil.infradead.org/
... and then Christoph pointed out the iterators weren't really adding
much value at that point, so they got deleted. New changelog for
this patch:
btrfs: Convert from readpages to readahead
Implement the new readahead method in btrfs. Add a readahead_page_batch()
to optimise fetching a batch of pages at once.
Re: [PATCH v7 14/24] btrfs: Convert from readpages to readahead
On 19/02/2020 22:03, Matthew Wilcox wrote:
> From: "Matthew Wilcox (Oracle)"
>
> Use the new readahead operation in btrfs. Add a
> readahead_for_each_batch() iterator to optimise the loop in the XArray.
OK I must admit I haven't followed this series closely, but what
happened to said readahead_for_each_batch()?
As far as I can see it's now:
[...]
> + while ((nr = readahead_page_batch(rac, pagepool))) {
[PATCH v7 14/24] btrfs: Convert from readpages to readahead
From: "Matthew Wilcox (Oracle)"
Use the new readahead operation in btrfs. Add a
readahead_for_each_batch() iterator to optimise the loop in the XArray.
Signed-off-by: Matthew Wilcox (Oracle)
---
fs/btrfs/extent_io.c| 46 +
fs/btrfs/extent_io.h| 3 +--
fs/btrfs/inode.c| 16 +++---
include/linux/pagemap.h | 37 +
4 files changed, 59 insertions(+), 43 deletions(-)
diff --git a/fs/btrfs/extent_io.c b/fs/btrfs/extent_io.c
index c0f202741e09..e70f14c1de60 100644
--- a/fs/btrfs/extent_io.c
+++ b/fs/btrfs/extent_io.c
@@ -4278,52 +4278,34 @@ int extent_writepages(struct address_space *mapping,
return ret;
}
-int extent_readpages(struct address_space *mapping, struct list_head *pages,
-unsigned nr_pages)
+void extent_readahead(struct readahead_control *rac)
{
struct bio *bio = NULL;
unsigned long bio_flags = 0;
struct page *pagepool[16];
struct extent_map *em_cached = NULL;
- struct extent_io_tree *tree = _I(mapping->host)->io_tree;
- int nr = 0;
+ struct extent_io_tree *tree = _I(rac->mapping->host)->io_tree;
u64 prev_em_start = (u64)-1;
+ int nr;
- while (!list_empty(pages)) {
- u64 contig_end = 0;
-
- for (nr = 0; nr < ARRAY_SIZE(pagepool) && !list_empty(pages);) {
- struct page *page = lru_to_page(pages);
-
- prefetchw(>flags);
- list_del(>lru);
- if (add_to_page_cache_lru(page, mapping, page->index,
- readahead_gfp_mask(mapping))) {
- put_page(page);
- break;
- }
-
- pagepool[nr++] = page;
- contig_end = page_offset(page) + PAGE_SIZE - 1;
- }
+ while ((nr = readahead_page_batch(rac, pagepool))) {
+ u64 contig_start = page_offset(pagepool[0]);
+ u64 contig_end = page_offset(pagepool[nr - 1]) + PAGE_SIZE - 1;
- if (nr) {
- u64 contig_start = page_offset(pagepool[0]);
+ ASSERT(contig_start + nr * PAGE_SIZE - 1 == contig_end);
- ASSERT(contig_start + nr * PAGE_SIZE - 1 == contig_end);
-
- contiguous_readpages(tree, pagepool, nr, contig_start,
-contig_end, _cached, , _flags,
-_em_start);
- }
+ contiguous_readpages(tree, pagepool, nr, contig_start,
+ contig_end, _cached, , _flags,
+ _em_start);
}
if (em_cached)
free_extent_map(em_cached);
- if (bio)
- return submit_one_bio(bio, 0, bio_flags);
- return 0;
+ if (bio) {
+ if (submit_one_bio(bio, 0, bio_flags))
+ return;
+ }
}
/*
diff --git a/fs/btrfs/extent_io.h b/fs/btrfs/extent_io.h
index 5d205bbaafdc..bddac32948c7 100644
--- a/fs/btrfs/extent_io.h
+++ b/fs/btrfs/extent_io.h
@@ -198,8 +198,7 @@ int extent_writepages(struct address_space *mapping,
struct writeback_control *wbc);
int btree_write_cache_pages(struct address_space *mapping,
struct writeback_control *wbc);
-int extent_readpages(struct address_space *mapping, struct list_head *pages,
-unsigned nr_pages);
+void extent_readahead(struct readahead_control *rac);
int extent_fiemap(struct inode *inode, struct fiemap_extent_info *fieinfo,
__u64 start, __u64 len);
void set_page_extent_mapped(struct page *page);
diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
index 7d26b4bfb2c6..61d5137ce4e9 100644
--- a/fs/btrfs/inode.c
+++ b/fs/btrfs/inode.c
@@ -4802,8 +4802,8 @@ static void evict_inode_truncate_pages(struct inode
*inode)
/*
* Keep looping until we have no more ranges in the io tree.
-* We can have ongoing bios started by readpages (called from readahead)
-* that have their endio callback (extent_io.c:end_bio_extent_readpage)
+* We can have ongoing bios started by readahead that have
+* their endio callback (extent_io.c:end_bio_extent_readpage)
* still in progress (unlocked the pages in the bio but did not yet
* unlocked the ranges in the io tree). Therefore this means some
* ranges can still be locked and eviction started because before
@@ -7004,11 +7004,11 @@ static int lock_extent_direct(struct inode *inode, u64
lockstart, u64 lockend,
* for it to complete) and then invalidate the pages for
* this range (through invalidate_inode_pages2_range()),
* but that can lead us to a
