[PATCH v4 0/4] Submit ->readpages() IO as read-ahead

[PATCH v4 0/4] Submit ->readpages() IO as read-ahead

[Jens Axboe]

The only caller of ->readpages() is from read-ahead, yet we don't
submit IO flagged with REQ_RAHEAD. This means we don't see it in
blktrace, for instance, which is a shame. Additionally, it's
preventing further functional changes in the block layer for
deadling with read-ahead more intelligently. We already make assumptions
about ->readpages() just being for read-ahead in the mpage
implementation, using readahead_gfp_mask(mapping) as out GFP mask of
choice.

This small series fixes up mpage_readpages() to submit with REQ_RAHEAD,
which takes care of file systems using mpage_readpages().  The first
patch is a prep patch, that makes do_mpage_readpage() take an argument
structure.

Changes since v3:

- Add comments on ->readpages() purely being read-ahead
- Rebase on current -git

Changes since v2:

- Get rid of 'gfp' passing once we have is_readahead
- Pack struct better, makes it 8 bytes smaller.

Changes since v1:

- Fix ext4_mpage_readpages() also being used for regular reads
- Add prep patch with struct arguments for do_mpage_readpage()

 fs/btrfs/extent_io.c |   2 +-
 fs/ext4/ext4.h       |   2 +-
 fs/ext4/inode.c      |   5 +-
 fs/ext4/readpage.c   |   5 +-
 fs/f2fs/data.c       |   5 ++
 fs/mpage.c           | 115 ++++++++++++++++++++++++-------------------
 include/linux/fs.h   |   4 ++
 7 files changed, 80 insertions(+), 58 deletions(-)

-- 
Jens Axboe

[PATCH 1/4] mpage: add argument structure for do_mpage_readpage()

[Jens Axboe]

We're currently passing 8 arguments to this function, clean it up a
bit by packing the arguments in an args structure we pass to it.

No intentional functional changes in this patch.

Signed-off-by: Jens Axboe <axboe@kernel.dk>
---
 fs/mpage.c | 106 +++++++++++++++++++++++++++--------------------------
 1 file changed, 54 insertions(+), 52 deletions(-)

diff --git a/fs/mpage.c b/fs/mpage.c
index b7e7f570733a..0ecac5c410f4 100644
--- a/fs/mpage.c
+++ b/fs/mpage.c
@@ -133,6 +133,17 @@ map_buffer_to_page(struct page *page, struct buffer_head *bh, int page_block)
 	} while (page_bh != head);
 }
 
+struct mpage_readpage_args {
+	struct bio *bio;
+	struct page *page;
+	unsigned nr_pages;
+	sector_t last_block_in_bio;
+	struct buffer_head map_bh;
+	unsigned long first_logical_block;
+	get_block_t *get_block;
+	gfp_t gfp;
+};
+
 /*
  * This is the worker routine which does all the work of mapping the disk
  * blocks and constructs largest possible bios, submits them for IO if the
@@ -142,16 +153,14 @@ map_buffer_to_page(struct page *page, struct buffer_head *bh, int page_block)
  * represent the validity of its disk mapping and to decide when to do the next
  * get_block() call.
  */
-static struct bio *
-do_mpage_readpage(struct bio *bio, struct page *page, unsigned nr_pages,
-		sector_t *last_block_in_bio, struct buffer_head *map_bh,
-		unsigned long *first_logical_block, get_block_t get_block,
-		gfp_t gfp)
+static struct bio *do_mpage_readpage(struct mpage_readpage_args *args)
 {
+	struct page *page = args->page;
 	struct inode *inode = page->mapping->host;
 	const unsigned blkbits = inode->i_blkbits;
 	const unsigned blocks_per_page = PAGE_SIZE >> blkbits;
 	const unsigned blocksize = 1 << blkbits;
+	struct buffer_head *map_bh = &args->map_bh;
 	sector_t block_in_file;
 	sector_t last_block;
 	sector_t last_block_in_file;
@@ -168,7 +177,7 @@ do_mpage_readpage(struct bio *bio, struct page *page, unsigned nr_pages,
 		goto confused;
 
 	block_in_file = (sector_t)page->index << (PAGE_SHIFT - blkbits);
-	last_block = block_in_file + nr_pages * blocks_per_page;
+	last_block = block_in_file + args->nr_pages * blocks_per_page;
 	last_block_in_file = (i_size_read(inode) + blocksize - 1) >> blkbits;
 	if (last_block > last_block_in_file)
 		last_block = last_block_in_file;
@@ -178,9 +187,9 @@ do_mpage_readpage(struct bio *bio, struct page *page, unsigned nr_pages,
 	 * Map blocks using the result from the previous get_blocks call first.
 	 */
 	nblocks = map_bh->b_size >> blkbits;
-	if (buffer_mapped(map_bh) && block_in_file > *first_logical_block &&
-			block_in_file < (*first_logical_block + nblocks)) {
-		unsigned map_offset = block_in_file - *first_logical_block;
+	if (buffer_mapped(map_bh) && block_in_file > args->first_logical_block &&
+			block_in_file < (args->first_logical_block + nblocks)) {
+		unsigned map_offset = block_in_file - args->first_logical_block;
 		unsigned last = nblocks - map_offset;
 
 		for (relative_block = 0; ; relative_block++) {
@@ -208,9 +217,9 @@ do_mpage_readpage(struct bio *bio, struct page *page, unsigned nr_pages,
 
 		if (block_in_file < last_block) {
 			map_bh->b_size = (last_block-block_in_file) << blkbits;
-			if (get_block(inode, block_in_file, map_bh, 0))
+			if (args->get_block(inode, block_in_file, map_bh, 0))
 				goto confused;
-			*first_logical_block = block_in_file;
+			args->first_logical_block = block_in_file;
 		}
 
 		if (!buffer_mapped(map_bh)) {
@@ -273,43 +282,43 @@ do_mpage_readpage(struct bio *bio, struct page *page, unsigned nr_pages,
 	/*
 	 * This page will go to BIO.  Do we need to send this BIO off first?
 	 */
-	if (bio && (*last_block_in_bio != blocks[0] - 1))
-		bio = mpage_bio_submit(REQ_OP_READ, 0, bio);
+	if (args->bio && (args->last_block_in_bio != blocks[0] - 1))
+		args->bio = mpage_bio_submit(REQ_OP_READ, 0, args->bio);
 
 alloc_new:
-	if (bio == NULL) {
+	if (args->bio == NULL) {
 		if (first_hole == blocks_per_page) {
 			if (!bdev_read_page(bdev, blocks[0] << (blkbits - 9),
 								page))
 				goto out;
 		}
-		bio = mpage_alloc(bdev, blocks[0] << (blkbits - 9),
-				min_t(int, nr_pages, BIO_MAX_PAGES), gfp);
-		if (bio == NULL)
+		args->bio = mpage_alloc(bdev, blocks[0] << (blkbits - 9),
+				min_t(int, args->nr_pages, BIO_MAX_PAGES), args->gfp);
+		if (args->bio == NULL)
 			goto confused;
 	}
 
 	length = first_hole << blkbits;
-	if (bio_add_page(bio, page, length, 0) < length) {
-		bio = mpage_bio_submit(REQ_OP_READ, 0, bio);
+	if (bio_add_page(args->bio, page, length, 0) < length) {
+		args->bio = mpage_bio_submit(REQ_OP_READ, 0, args->bio);
 		goto alloc_new;
 	}
 
-	relative_block = block_in_file - *first_logical_block;
+	relative_block = block_in_file - args->first_logical_block;
 	nblocks = map_bh->b_size >> blkbits;
 	if ((buffer_boundary(map_bh) && relative_block == nblocks) ||
 	    (first_hole != blocks_per_page))
-		bio = mpage_bio_submit(REQ_OP_READ, 0, bio);
+		args->bio = mpage_bio_submit(REQ_OP_READ, 0, args->bio);
 	else
-		*last_block_in_bio = blocks[blocks_per_page - 1];
+		args->last_block_in_bio = blocks[blocks_per_page - 1];
 out:
-	return bio;
+	return args->bio;
 
 confused:
-	if (bio)
-		bio = mpage_bio_submit(REQ_OP_READ, 0, bio);
+	if (args->bio)
+		args->bio = mpage_bio_submit(REQ_OP_READ, 0, args->bio);
 	if (!PageUptodate(page))
-	        block_read_full_page(page, get_block);
+	        block_read_full_page(page, args->get_block);
 	else
 		unlock_page(page);
 	goto out;
@@ -363,15 +372,12 @@ int
 mpage_readpages(struct address_space *mapping, struct list_head *pages,
 				unsigned nr_pages, get_block_t get_block)
 {
-	struct bio *bio = NULL;
+	struct mpage_readpage_args args = {
+		.get_block = get_block,
+		.gfp = readahead_gfp_mask(mapping),
+	};
 	unsigned page_idx;
-	sector_t last_block_in_bio = 0;
-	struct buffer_head map_bh;
-	unsigned long first_logical_block = 0;
-	gfp_t gfp = readahead_gfp_mask(mapping);
 
-	map_bh.b_state = 0;
-	map_bh.b_size = 0;
 	for (page_idx = 0; page_idx < nr_pages; page_idx++) {
 		struct page *page = lru_to_page(pages);
 
@@ -379,18 +385,16 @@ mpage_readpages(struct address_space *mapping, struct list_head *pages,
 		list_del(&page->lru);
 		if (!add_to_page_cache_lru(page, mapping,
 					page->index,
-					gfp)) {
-			bio = do_mpage_readpage(bio, page,
-					nr_pages - page_idx,
-					&last_block_in_bio, &map_bh,
-					&first_logical_block,
-					get_block, gfp);
+					args.gfp)) {
+			args.page = page;
+			args.nr_pages = nr_pages - page_idx;
+			args.bio = do_mpage_readpage(&args);
 		}
 		put_page(page);
 	}
 	BUG_ON(!list_empty(pages));
-	if (bio)
-		mpage_bio_submit(REQ_OP_READ, 0, bio);
+	if (args.bio)
+		mpage_bio_submit(REQ_OP_READ, 0, args.bio);
 	return 0;
 }
 EXPORT_SYMBOL(mpage_readpages);
@@ -400,18 +404,16 @@ EXPORT_SYMBOL(mpage_readpages);
  */
 int mpage_readpage(struct page *page, get_block_t get_block)
 {
-	struct bio *bio = NULL;
-	sector_t last_block_in_bio = 0;
-	struct buffer_head map_bh;
-	unsigned long first_logical_block = 0;
-	gfp_t gfp = mapping_gfp_constraint(page->mapping, GFP_KERNEL);
+	struct mpage_readpage_args args = {
+		.page = page,
+		.nr_pages = 1,
+		.get_block = get_block,
+		.gfp = mapping_gfp_constraint(page->mapping, GFP_KERNEL),
+	};
 
-	map_bh.b_state = 0;
-	map_bh.b_size = 0;
-	bio = do_mpage_readpage(bio, page, 1, &last_block_in_bio,
-			&map_bh, &first_logical_block, get_block, gfp);
-	if (bio)
-		mpage_bio_submit(REQ_OP_READ, 0, bio);
+	args.bio = do_mpage_readpage(&args);
+	if (args.bio)
+		mpage_bio_submit(REQ_OP_READ, 0, args.bio);
 	return 0;
 }
 EXPORT_SYMBOL(mpage_readpage);
-- 
2.17.1

[PATCH 2/4] mpage: mpage_readpages() should submit IO as read-ahead

[Jens Axboe]

a_ops->readpages() is only ever used for read-ahead, yet we don't
flag the IO being submitted as such. Fix that up. Any file system
that uses mpage_readpages() as its ->readpages() implementation
will now get this right.

Since we're passing in whether the IO is read-ahead or not, we
don't need to pass in the 'gfp' separately, as it is dependent
on the IO being read-ahead. Kill off that member.

Add some documentation notes on ->readpages() being purely for
read-ahead.

Signed-off-by: Jens Axboe <axboe@kernel.dk>
---
 fs/f2fs/data.c     |  5 +++++
 fs/mpage.c         | 29 +++++++++++++++++++----------
 include/linux/fs.h |  4 ++++
 3 files changed, 28 insertions(+), 10 deletions(-)

diff --git a/fs/f2fs/data.c b/fs/f2fs/data.c
index 8f931d699287..b7c9b58acf3e 100644
--- a/fs/f2fs/data.c
+++ b/fs/f2fs/data.c
@@ -1421,6 +1421,11 @@ int f2fs_fiemap(struct inode *inode, struct fiemap_extent_info *fieinfo,
 /*
  * This function was originally taken from fs/mpage.c, and customized for f2fs.
  * Major change was from block_size == page_size in f2fs by default.
+ *
+ * Note that the aops->readpages() function is ONLY used for read-ahead. If
+ * this function ever deviates from doing just read-ahead, it should either
+ * use ->readpage() or do the necessary surgery to decouple ->readpages()
+ * readom read-ahead.
  */
 static int f2fs_mpage_readpages(struct address_space *mapping,
 			struct list_head *pages, struct page *page,
diff --git a/fs/mpage.c b/fs/mpage.c
index 0ecac5c410f4..b0f9de977526 100644
--- a/fs/mpage.c
+++ b/fs/mpage.c
@@ -137,11 +137,11 @@ struct mpage_readpage_args {
 	struct bio *bio;
 	struct page *page;
 	unsigned nr_pages;
+	bool is_readahead;
 	sector_t last_block_in_bio;
 	struct buffer_head map_bh;
 	unsigned long first_logical_block;
 	get_block_t *get_block;
-	gfp_t gfp;
 };
 
 /*
@@ -170,8 +170,18 @@ static struct bio *do_mpage_readpage(struct mpage_readpage_args *args)
 	struct block_device *bdev = NULL;
 	int length;
 	int fully_mapped = 1;
+	int op_flags;
 	unsigned nblocks;
 	unsigned relative_block;
+	gfp_t gfp;
+
+	if (args->is_readahead) {
+		op_flags = REQ_RAHEAD;
+		gfp = readahead_gfp_mask(page->mapping);
+	} else {
+		op_flags = 0;
+		gfp = mapping_gfp_constraint(page->mapping, GFP_KERNEL);
+	}
 
 	if (page_has_buffers(page))
 		goto confused;
@@ -283,7 +293,7 @@ static struct bio *do_mpage_readpage(struct mpage_readpage_args *args)
 	 * This page will go to BIO.  Do we need to send this BIO off first?
 	 */
 	if (args->bio && (args->last_block_in_bio != blocks[0] - 1))
-		args->bio = mpage_bio_submit(REQ_OP_READ, 0, args->bio);
+		args->bio = mpage_bio_submit(REQ_OP_READ, op_flags, args->bio);
 
 alloc_new:
 	if (args->bio == NULL) {
@@ -293,14 +303,14 @@ static struct bio *do_mpage_readpage(struct mpage_readpage_args *args)
 				goto out;
 		}
 		args->bio = mpage_alloc(bdev, blocks[0] << (blkbits - 9),
-				min_t(int, args->nr_pages, BIO_MAX_PAGES), args->gfp);
+				min_t(int, args->nr_pages, BIO_MAX_PAGES), gfp);
 		if (args->bio == NULL)
 			goto confused;
 	}
 
 	length = first_hole << blkbits;
 	if (bio_add_page(args->bio, page, length, 0) < length) {
-		args->bio = mpage_bio_submit(REQ_OP_READ, 0, args->bio);
+		args->bio = mpage_bio_submit(REQ_OP_READ, op_flags, args->bio);
 		goto alloc_new;
 	}
 
@@ -308,7 +318,7 @@ static struct bio *do_mpage_readpage(struct mpage_readpage_args *args)
 	nblocks = map_bh->b_size >> blkbits;
 	if ((buffer_boundary(map_bh) && relative_block == nblocks) ||
 	    (first_hole != blocks_per_page))
-		args->bio = mpage_bio_submit(REQ_OP_READ, 0, args->bio);
+		args->bio = mpage_bio_submit(REQ_OP_READ, op_flags, args->bio);
 	else
 		args->last_block_in_bio = blocks[blocks_per_page - 1];
 out:
@@ -316,7 +326,7 @@ static struct bio *do_mpage_readpage(struct mpage_readpage_args *args)
 
 confused:
 	if (args->bio)
-		args->bio = mpage_bio_submit(REQ_OP_READ, 0, args->bio);
+		args->bio = mpage_bio_submit(REQ_OP_READ, op_flags, args->bio);
 	if (!PageUptodate(page))
 	        block_read_full_page(page, args->get_block);
 	else
@@ -374,7 +384,7 @@ mpage_readpages(struct address_space *mapping, struct list_head *pages,
 {
 	struct mpage_readpage_args args = {
 		.get_block = get_block,
-		.gfp = readahead_gfp_mask(mapping),
+		.is_readahead = true,
 	};
 	unsigned page_idx;
 
@@ -385,7 +395,7 @@ mpage_readpages(struct address_space *mapping, struct list_head *pages,
 		list_del(&page->lru);
 		if (!add_to_page_cache_lru(page, mapping,
 					page->index,
-					args.gfp)) {
+					readahead_gfp_mask(mapping))) {
 			args.page = page;
 			args.nr_pages = nr_pages - page_idx;
 			args.bio = do_mpage_readpage(&args);
@@ -394,7 +404,7 @@ mpage_readpages(struct address_space *mapping, struct list_head *pages,
 	}
 	BUG_ON(!list_empty(pages));
 	if (args.bio)
-		mpage_bio_submit(REQ_OP_READ, 0, args.bio);
+		mpage_bio_submit(REQ_OP_READ, REQ_RAHEAD, args.bio);
 	return 0;
 }
 EXPORT_SYMBOL(mpage_readpages);
@@ -408,7 +418,6 @@ int mpage_readpage(struct page *page, get_block_t get_block)
 		.page = page,
 		.nr_pages = 1,
 		.get_block = get_block,
-		.gfp = mapping_gfp_constraint(page->mapping, GFP_KERNEL),
 	};
 
 	args.bio = do_mpage_readpage(&args);
diff --git a/include/linux/fs.h b/include/linux/fs.h
index 5c91108846db..b45c34a9b28d 100644
--- a/include/linux/fs.h
+++ b/include/linux/fs.h
@@ -341,6 +341,10 @@ struct address_space_operations {
 	/* Set a page dirty.  Return true if this dirtied it */
 	int (*set_page_dirty)(struct page *page);
 
+	/*
+	 * Reads in the requested pages. Unlike ->readpage(), this is
+	 * PURELY used for read-ahead!.
+	 */
 	int (*readpages)(struct file *filp, struct address_space *mapping,
 			struct list_head *pages, unsigned nr_pages);
 
-- 
2.17.1

[PATCH 3/4] btrfs: readpages() should submit IO as read-ahead

[Jens Axboe]

a_ops->readpages() is only ever used for read-ahead. Ensure that we
pass this information down to the block layer.

Signed-off-by: Jens Axboe <axboe@kernel.dk>
---
 fs/btrfs/extent_io.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/fs/btrfs/extent_io.c b/fs/btrfs/extent_io.c
index cce6087d6880..440e2bdc518d 100644
--- a/fs/btrfs/extent_io.c
+++ b/fs/btrfs/extent_io.c
@@ -3116,7 +3116,7 @@ static inline void __do_contiguous_readpages(struct extent_io_tree *tree,
 
 	for (index = 0; index < nr_pages; index++) {
 		__do_readpage(tree, pages[index], btrfs_get_extent, em_cached,
-				bio, 0, bio_flags, 0, prev_em_start);
+				bio, 0, bio_flags, REQ_RAHEAD, prev_em_start);
 		put_page(pages[index]);
 	}
 }
-- 
2.17.1

[PATCH 4/4] ext4: readpages() should submit IO as read-ahead

[Jens Axboe]

a_ops->readpages() is only ever used for read-ahead. Ensure that we
pass this information down to the block layer.

Signed-off-by: Jens Axboe <axboe@kernel.dk>
---
 fs/ext4/ext4.h     | 2 +-
 fs/ext4/inode.c    | 5 +++--
 fs/ext4/readpage.c | 5 +++--
 3 files changed, 7 insertions(+), 5 deletions(-)

diff --git a/fs/ext4/ext4.h b/fs/ext4/ext4.h
index 0b127853c584..e416cb0a2b8c 100644
--- a/fs/ext4/ext4.h
+++ b/fs/ext4/ext4.h
@@ -3065,7 +3065,7 @@ static inline void ext4_set_de_type(struct super_block *sb,
 /* readpages.c */
 extern int ext4_mpage_readpages(struct address_space *mapping,
 				struct list_head *pages, struct page *page,
-				unsigned nr_pages);
+				unsigned nr_pages, bool is_readahead);
 
 /* symlink.c */
 extern const struct inode_operations ext4_encrypted_symlink_inode_operations;
diff --git a/fs/ext4/inode.c b/fs/ext4/inode.c
index 2ea07efbe016..de1235fa1bfb 100644
--- a/fs/ext4/inode.c
+++ b/fs/ext4/inode.c
@@ -3327,7 +3327,8 @@ static int ext4_readpage(struct file *file, struct page *page)
 		ret = ext4_readpage_inline(inode, page);
 
 	if (ret == -EAGAIN)
-		return ext4_mpage_readpages(page->mapping, NULL, page, 1);
+		return ext4_mpage_readpages(page->mapping, NULL, page, 1,
+						false);
 
 	return ret;
 }
@@ -3342,7 +3343,7 @@ ext4_readpages(struct file *file, struct address_space *mapping,
 	if (ext4_has_inline_data(inode))
 		return 0;
 
-	return ext4_mpage_readpages(mapping, pages, NULL, nr_pages);
+	return ext4_mpage_readpages(mapping, pages, NULL, nr_pages, true);
 }
 
 static void ext4_invalidatepage(struct page *page, unsigned int offset,
diff --git a/fs/ext4/readpage.c b/fs/ext4/readpage.c
index 19b87a8de6ff..f461d75ac049 100644
--- a/fs/ext4/readpage.c
+++ b/fs/ext4/readpage.c
@@ -98,7 +98,7 @@ static void mpage_end_io(struct bio *bio)
 
 int ext4_mpage_readpages(struct address_space *mapping,
 			 struct list_head *pages, struct page *page,
-			 unsigned nr_pages)
+			 unsigned nr_pages, bool is_readahead)
 {
 	struct bio *bio = NULL;
 	sector_t last_block_in_bio = 0;
@@ -259,7 +259,8 @@ int ext4_mpage_readpages(struct address_space *mapping,
 			bio->bi_iter.bi_sector = blocks[0] << (blkbits - 9);
 			bio->bi_end_io = mpage_end_io;
 			bio->bi_private = ctx;
-			bio_set_op_attrs(bio, REQ_OP_READ, 0);
+			bio_set_op_attrs(bio, REQ_OP_READ,
+						is_readahead ? REQ_RAHEAD : 0);
 		}
 
 		length = first_hole << blkbits;
-- 
2.17.1

Re: [PATCH 2/4] mpage: mpage_readpages() should submit IO as read-ahead

[Randy Dunlap]

On 06/20/2018 06:07 PM, Jens Axboe wrote:
> a_ops->readpages() is only ever used for read-ahead, yet we don't
> flag the IO being submitted as such. Fix that up. Any file system
> that uses mpage_readpages() as its ->readpages() implementation
> will now get this right.
> 
> Since we're passing in whether the IO is read-ahead or not, we
> don't need to pass in the 'gfp' separately, as it is dependent
> on the IO being read-ahead. Kill off that member.
> 
> Add some documentation notes on ->readpages() being purely for
> read-ahead.
> 
> Signed-off-by: Jens Axboe <axboe@kernel.dk>
> ---
>  fs/f2fs/data.c     |  5 +++++
>  fs/mpage.c         | 29 +++++++++++++++++++----------
>  include/linux/fs.h |  4 ++++
>  3 files changed, 28 insertions(+), 10 deletions(-)
> 
> diff --git a/fs/f2fs/data.c b/fs/f2fs/data.c
> index 8f931d699287..b7c9b58acf3e 100644
> --- a/fs/f2fs/data.c
> +++ b/fs/f2fs/data.c
> @@ -1421,6 +1421,11 @@ int f2fs_fiemap(struct inode *inode, struct fiemap_extent_info *fieinfo,
>  /*
>   * This function was originally taken from fs/mpage.c, and customized for f2fs.
>   * Major change was from block_size == page_size in f2fs by default.
> + *
> + * Note that the aops->readpages() function is ONLY used for read-ahead. If
> + * this function ever deviates from doing just read-ahead, it should either
> + * use ->readpage() or do the necessary surgery to decouple ->readpages()
> + * readom read-ahead.

      eh?

>   */


-- 
~Randy

Re: [PATCH 2/4] mpage: mpage_readpages() should submit IO as read-ahead

[Jens Axboe]

On 6/21/18 11:27 AM, Randy Dunlap wrote:
> On 06/20/2018 06:07 PM, Jens Axboe wrote:
>> a_ops->readpages() is only ever used for read-ahead, yet we don't
>> flag the IO being submitted as such. Fix that up. Any file system
>> that uses mpage_readpages() as its ->readpages() implementation
>> will now get this right.
>>
>> Since we're passing in whether the IO is read-ahead or not, we
>> don't need to pass in the 'gfp' separately, as it is dependent
>> on the IO being read-ahead. Kill off that member.
>>
>> Add some documentation notes on ->readpages() being purely for
>> read-ahead.
>>
>> Signed-off-by: Jens Axboe <axboe@kernel.dk>
>> ---
>>  fs/f2fs/data.c     |  5 +++++
>>  fs/mpage.c         | 29 +++++++++++++++++++----------
>>  include/linux/fs.h |  4 ++++
>>  3 files changed, 28 insertions(+), 10 deletions(-)
>>
>> diff --git a/fs/f2fs/data.c b/fs/f2fs/data.c
>> index 8f931d699287..b7c9b58acf3e 100644
>> --- a/fs/f2fs/data.c
>> +++ b/fs/f2fs/data.c
>> @@ -1421,6 +1421,11 @@ int f2fs_fiemap(struct inode *inode, struct fiemap_extent_info *fieinfo,
>>  /*
>>   * This function was originally taken from fs/mpage.c, and customized for f2fs.
>>   * Major change was from block_size == page_size in f2fs by default.
>> + *
>> + * Note that the aops->readpages() function is ONLY used for read-ahead. If
>> + * this function ever deviates from doing just read-ahead, it should either
>> + * use ->readpage() or do the necessary surgery to decouple ->readpages()
>> + * readom read-ahead.
> 
>       eh?

Huh weird, must be touchpad and typing. Should be 'from read-ahead'.

-- 
Jens Axboe

Re: [PATCH 2/4] mpage: mpage_readpages() should submit IO as read-ahead

[Jaegeuk Kim]

On 06/20, Jens Axboe wrote:
> a_ops->readpages() is only ever used for read-ahead, yet we don't
> flag the IO being submitted as such. Fix that up. Any file system
> that uses mpage_readpages() as its ->readpages() implementation
> will now get this right.
> 
> Since we're passing in whether the IO is read-ahead or not, we
> don't need to pass in the 'gfp' separately, as it is dependent
> on the IO being read-ahead. Kill off that member.
> 
> Add some documentation notes on ->readpages() being purely for
> read-ahead.
> 
> Signed-off-by: Jens Axboe <axboe@kernel.dk>
> ---
>  fs/f2fs/data.c     |  5 +++++
>  fs/mpage.c         | 29 +++++++++++++++++++----------
>  include/linux/fs.h |  4 ++++
>  3 files changed, 28 insertions(+), 10 deletions(-)
> 
> diff --git a/fs/f2fs/data.c b/fs/f2fs/data.c
> index 8f931d699287..b7c9b58acf3e 100644
> --- a/fs/f2fs/data.c
> +++ b/fs/f2fs/data.c
> @@ -1421,6 +1421,11 @@ int f2fs_fiemap(struct inode *inode, struct fiemap_extent_info *fieinfo,
>  /*
>   * This function was originally taken from fs/mpage.c, and customized for f2fs.
>   * Major change was from block_size == page_size in f2fs by default.
> + *
> + * Note that the aops->readpages() function is ONLY used for read-ahead. If
> + * this function ever deviates from doing just read-ahead, it should either
> + * use ->readpage() or do the necessary surgery to decouple ->readpages()
> + * readom read-ahead.
>   */
>  static int f2fs_mpage_readpages(struct address_space *mapping,
>  			struct list_head *pages, struct page *page,

Hi Jens,

Could you please consider the below change to address your concern?

---
 fs/f2fs/data.c | 15 ++++++++-------
 1 file changed, 8 insertions(+), 7 deletions(-)

diff --git a/fs/f2fs/data.c b/fs/f2fs/data.c
index 8f931d699287..c1266b231f62 100644
--- a/fs/f2fs/data.c
+++ b/fs/f2fs/data.c
@@ -534,7 +534,7 @@ void f2fs_submit_page_write(struct f2fs_io_info *fio)
 }
 
 static struct bio *f2fs_grab_read_bio(struct inode *inode, block_t blkaddr,
-							 unsigned nr_pages)
+					unsigned nr_pages, unsigned op_flag)
 {
 	struct f2fs_sb_info *sbi = F2FS_I_SB(inode);
 	struct bio *bio;
@@ -546,7 +546,7 @@ static struct bio *f2fs_grab_read_bio(struct inode *inode, block_t blkaddr,
 		return ERR_PTR(-ENOMEM);
 	f2fs_target_device(sbi, blkaddr, bio);
 	bio->bi_end_io = f2fs_read_end_io;
-	bio_set_op_attrs(bio, REQ_OP_READ, 0);
+	bio_set_op_attrs(bio, REQ_OP_READ, op_flag);
 
 	if (f2fs_encrypted_file(inode))
 		post_read_steps |= 1 << STEP_DECRYPT;
@@ -571,7 +571,7 @@ static struct bio *f2fs_grab_read_bio(struct inode *inode, block_t blkaddr,
 static int f2fs_submit_page_read(struct inode *inode, struct page *page,
 							block_t blkaddr)
 {
-	struct bio *bio = f2fs_grab_read_bio(inode, blkaddr, 1);
+	struct bio *bio = f2fs_grab_read_bio(inode, blkaddr, 1, 0);
 
 	if (IS_ERR(bio))
 		return PTR_ERR(bio);
@@ -1424,7 +1424,7 @@ int f2fs_fiemap(struct inode *inode, struct fiemap_extent_info *fieinfo,
  */
 static int f2fs_mpage_readpages(struct address_space *mapping,
 			struct list_head *pages, struct page *page,
-			unsigned nr_pages)
+			unsigned nr_pages, bool is_readahead)
 {
 	struct bio *bio = NULL;
 	sector_t last_block_in_bio = 0;
@@ -1514,7 +1514,8 @@ static int f2fs_mpage_readpages(struct address_space *mapping,
 			bio = NULL;
 		}
 		if (bio == NULL) {
-			bio = f2fs_grab_read_bio(inode, block_nr, nr_pages);
+			bio = f2fs_grab_read_bio(inode, block_nr, nr_pages,
+					is_readahead ? REQ_RAHEAD : 0);
 			if (IS_ERR(bio)) {
 				bio = NULL;
 				goto set_error_page;
@@ -1558,7 +1559,7 @@ static int f2fs_read_data_page(struct file *file, struct page *page)
 	if (f2fs_has_inline_data(inode))
 		ret = f2fs_read_inline_data(inode, page);
 	if (ret == -EAGAIN)
-		ret = f2fs_mpage_readpages(page->mapping, NULL, page, 1);
+		ret = f2fs_mpage_readpages(page->mapping, NULL, page, 1, false);
 	return ret;
 }
 
@@ -1575,7 +1576,7 @@ static int f2fs_read_data_pages(struct file *file,
 	if (f2fs_has_inline_data(inode))
 		return 0;
 
-	return f2fs_mpage_readpages(mapping, pages, NULL, nr_pages);
+	return f2fs_mpage_readpages(mapping, pages, NULL, nr_pages, true);
 }
 
 static int encrypt_one_page(struct f2fs_io_info *fio)
-- 
2.17.0.441.gb46fe60e1d-goog

Re: [PATCH 2/4] mpage: mpage_readpages() should submit IO as read-ahead

[Jens Axboe]

On 6/21/18 12:47 PM, Jaegeuk Kim wrote:
> On 06/20, Jens Axboe wrote:
>> a_ops->readpages() is only ever used for read-ahead, yet we don't
>> flag the IO being submitted as such. Fix that up. Any file system
>> that uses mpage_readpages() as its ->readpages() implementation
>> will now get this right.
>>
>> Since we're passing in whether the IO is read-ahead or not, we
>> don't need to pass in the 'gfp' separately, as it is dependent
>> on the IO being read-ahead. Kill off that member.
>>
>> Add some documentation notes on ->readpages() being purely for
>> read-ahead.
>>
>> Signed-off-by: Jens Axboe <axboe@kernel.dk>
>> ---
>>  fs/f2fs/data.c     |  5 +++++
>>  fs/mpage.c         | 29 +++++++++++++++++++----------
>>  include/linux/fs.h |  4 ++++
>>  3 files changed, 28 insertions(+), 10 deletions(-)
>>
>> diff --git a/fs/f2fs/data.c b/fs/f2fs/data.c
>> index 8f931d699287..b7c9b58acf3e 100644
>> --- a/fs/f2fs/data.c
>> +++ b/fs/f2fs/data.c
>> @@ -1421,6 +1421,11 @@ int f2fs_fiemap(struct inode *inode, struct fiemap_extent_info *fieinfo,
>>  /*
>>   * This function was originally taken from fs/mpage.c, and customized for f2fs.
>>   * Major change was from block_size == page_size in f2fs by default.
>> + *
>> + * Note that the aops->readpages() function is ONLY used for read-ahead. If
>> + * this function ever deviates from doing just read-ahead, it should either
>> + * use ->readpage() or do the necessary surgery to decouple ->readpages()
>> + * readom read-ahead.
>>   */
>>  static int f2fs_mpage_readpages(struct address_space *mapping,
>>  			struct list_head *pages, struct page *page,
> 
> Hi Jens,
> 
> Could you please consider the below change to address your concern?

Looks good to me.

-- 
Jens Axboe

Re: [PATCH 2/4] mpage: mpage_readpages() should submit IO as read-ahead

[Jaegeuk Kim]

On 06/21, Jens Axboe wrote:
> On 6/21/18 12:47 PM, Jaegeuk Kim wrote:
> > On 06/20, Jens Axboe wrote:
> >> a_ops->readpages() is only ever used for read-ahead, yet we don't
> >> flag the IO being submitted as such. Fix that up. Any file system
> >> that uses mpage_readpages() as its ->readpages() implementation
> >> will now get this right.
> >>
> >> Since we're passing in whether the IO is read-ahead or not, we
> >> don't need to pass in the 'gfp' separately, as it is dependent
> >> on the IO being read-ahead. Kill off that member.
> >>
> >> Add some documentation notes on ->readpages() being purely for
> >> read-ahead.
> >>
> >> Signed-off-by: Jens Axboe <axboe@kernel.dk>
> >> ---
> >>  fs/f2fs/data.c     |  5 +++++
> >>  fs/mpage.c         | 29 +++++++++++++++++++----------
> >>  include/linux/fs.h |  4 ++++
> >>  3 files changed, 28 insertions(+), 10 deletions(-)
> >>
> >> diff --git a/fs/f2fs/data.c b/fs/f2fs/data.c
> >> index 8f931d699287..b7c9b58acf3e 100644
> >> --- a/fs/f2fs/data.c
> >> +++ b/fs/f2fs/data.c
> >> @@ -1421,6 +1421,11 @@ int f2fs_fiemap(struct inode *inode, struct fiemap_extent_info *fieinfo,
> >>  /*
> >>   * This function was originally taken from fs/mpage.c, and customized for f2fs.
> >>   * Major change was from block_size == page_size in f2fs by default.
> >> + *
> >> + * Note that the aops->readpages() function is ONLY used for read-ahead. If
> >> + * this function ever deviates from doing just read-ahead, it should either
> >> + * use ->readpage() or do the necessary surgery to decouple ->readpages()
> >> + * readom read-ahead.
> >>   */
> >>  static int f2fs_mpage_readpages(struct address_space *mapping,
> >>  			struct list_head *pages, struct page *page,
> > 
> > Hi Jens,
> > 
> > Could you please consider the below change to address your concern?
> 
> Looks good to me.

Let me add this change in f2fs tree.

Thanks,

Re: [PATCH 2/4] mpage: mpage_readpages() should submit IO as read-ahead

[Jens Axboe]

On 6/21/18 1:32 PM, Jaegeuk Kim wrote:
> On 06/21, Jens Axboe wrote:
>> On 6/21/18 12:47 PM, Jaegeuk Kim wrote:
>>> On 06/20, Jens Axboe wrote:
>>>> a_ops->readpages() is only ever used for read-ahead, yet we don't
>>>> flag the IO being submitted as such. Fix that up. Any file system
>>>> that uses mpage_readpages() as its ->readpages() implementation
>>>> will now get this right.
>>>>
>>>> Since we're passing in whether the IO is read-ahead or not, we
>>>> don't need to pass in the 'gfp' separately, as it is dependent
>>>> on the IO being read-ahead. Kill off that member.
>>>>
>>>> Add some documentation notes on ->readpages() being purely for
>>>> read-ahead.
>>>>
>>>> Signed-off-by: Jens Axboe <axboe@kernel.dk>
>>>> ---
>>>>  fs/f2fs/data.c     |  5 +++++
>>>>  fs/mpage.c         | 29 +++++++++++++++++++----------
>>>>  include/linux/fs.h |  4 ++++
>>>>  3 files changed, 28 insertions(+), 10 deletions(-)
>>>>
>>>> diff --git a/fs/f2fs/data.c b/fs/f2fs/data.c
>>>> index 8f931d699287..b7c9b58acf3e 100644
>>>> --- a/fs/f2fs/data.c
>>>> +++ b/fs/f2fs/data.c
>>>> @@ -1421,6 +1421,11 @@ int f2fs_fiemap(struct inode *inode, struct fiemap_extent_info *fieinfo,
>>>>  /*
>>>>   * This function was originally taken from fs/mpage.c, and customized for f2fs.
>>>>   * Major change was from block_size == page_size in f2fs by default.
>>>> + *
>>>> + * Note that the aops->readpages() function is ONLY used for read-ahead. If
>>>> + * this function ever deviates from doing just read-ahead, it should either
>>>> + * use ->readpage() or do the necessary surgery to decouple ->readpages()
>>>> + * readom read-ahead.
>>>>   */
>>>>  static int f2fs_mpage_readpages(struct address_space *mapping,
>>>>  			struct list_head *pages, struct page *page,
>>>
>>> Hi Jens,
>>>
>>> Could you please consider the below change to address your concern?
>>
>> Looks good to me.
> 
> Let me add this change in f2fs tree.

Thanks!

-- 
Jens Axboe

[PATCH 2/4] mpage: mpage_readpages() should submit IO as read-ahead

[Jens Axboe]

a_ops->readpages() is only ever used for read-ahead, yet we don't
flag the IO being submitted as such. Fix that up. Any file system
that uses mpage_readpages() as it's ->readpages() implementation
will now get this right.

Since we're passing in whether the IO is read-ahead or not, we
don't need to pass in the 'gfp' separately, as it is dependent
on the IO being read-ahead. Kill off that member.

Signed-off-by: Jens Axboe <axboe@kernel.dk>
---
 fs/mpage.c | 29 +++++++++++++++++++----------
 1 file changed, 19 insertions(+), 10 deletions(-)

diff --git a/fs/mpage.c b/fs/mpage.c
index 0ecac5c410f4..b0f9de977526 100644
--- a/fs/mpage.c
+++ b/fs/mpage.c
@@ -137,11 +137,11 @@ struct mpage_readpage_args {
 	struct bio *bio;
 	struct page *page;
 	unsigned nr_pages;
+	bool is_readahead;
 	sector_t last_block_in_bio;
 	struct buffer_head map_bh;
 	unsigned long first_logical_block;
 	get_block_t *get_block;
-	gfp_t gfp;
 };
 
 /*
@@ -170,8 +170,18 @@ static struct bio *do_mpage_readpage(struct mpage_readpage_args *args)
 	struct block_device *bdev = NULL;
 	int length;
 	int fully_mapped = 1;
+	int op_flags;
 	unsigned nblocks;
 	unsigned relative_block;
+	gfp_t gfp;
+
+	if (args->is_readahead) {
+		op_flags = REQ_RAHEAD;
+		gfp = readahead_gfp_mask(page->mapping);
+	} else {
+		op_flags = 0;
+		gfp = mapping_gfp_constraint(page->mapping, GFP_KERNEL);
+	}
 
 	if (page_has_buffers(page))
 		goto confused;
@@ -283,7 +293,7 @@ static struct bio *do_mpage_readpage(struct mpage_readpage_args *args)
 	 * This page will go to BIO.  Do we need to send this BIO off first?
 	 */
 	if (args->bio && (args->last_block_in_bio != blocks[0] - 1))
-		args->bio = mpage_bio_submit(REQ_OP_READ, 0, args->bio);
+		args->bio = mpage_bio_submit(REQ_OP_READ, op_flags, args->bio);
 
 alloc_new:
 	if (args->bio == NULL) {
@@ -293,14 +303,14 @@ static struct bio *do_mpage_readpage(struct mpage_readpage_args *args)
 				goto out;
 		}
 		args->bio = mpage_alloc(bdev, blocks[0] << (blkbits - 9),
-				min_t(int, args->nr_pages, BIO_MAX_PAGES), args->gfp);
+				min_t(int, args->nr_pages, BIO_MAX_PAGES), gfp);
 		if (args->bio == NULL)
 			goto confused;
 	}
 
 	length = first_hole << blkbits;
 	if (bio_add_page(args->bio, page, length, 0) < length) {
-		args->bio = mpage_bio_submit(REQ_OP_READ, 0, args->bio);
+		args->bio = mpage_bio_submit(REQ_OP_READ, op_flags, args->bio);
 		goto alloc_new;
 	}
 
@@ -308,7 +318,7 @@ static struct bio *do_mpage_readpage(struct mpage_readpage_args *args)
 	nblocks = map_bh->b_size >> blkbits;
 	if ((buffer_boundary(map_bh) && relative_block == nblocks) ||
 	    (first_hole != blocks_per_page))
-		args->bio = mpage_bio_submit(REQ_OP_READ, 0, args->bio);
+		args->bio = mpage_bio_submit(REQ_OP_READ, op_flags, args->bio);
 	else
 		args->last_block_in_bio = blocks[blocks_per_page - 1];
 out:
@@ -316,7 +326,7 @@ static struct bio *do_mpage_readpage(struct mpage_readpage_args *args)
 
 confused:
 	if (args->bio)
-		args->bio = mpage_bio_submit(REQ_OP_READ, 0, args->bio);
+		args->bio = mpage_bio_submit(REQ_OP_READ, op_flags, args->bio);
 	if (!PageUptodate(page))
 	        block_read_full_page(page, args->get_block);
 	else
@@ -374,7 +384,7 @@ mpage_readpages(struct address_space *mapping, struct list_head *pages,
 {
 	struct mpage_readpage_args args = {
 		.get_block = get_block,
-		.gfp = readahead_gfp_mask(mapping),
+		.is_readahead = true,
 	};
 	unsigned page_idx;
 
@@ -385,7 +395,7 @@ mpage_readpages(struct address_space *mapping, struct list_head *pages,
 		list_del(&page->lru);
 		if (!add_to_page_cache_lru(page, mapping,
 					page->index,
-					args.gfp)) {
+					readahead_gfp_mask(mapping))) {
 			args.page = page;
 			args.nr_pages = nr_pages - page_idx;
 			args.bio = do_mpage_readpage(&args);
@@ -394,7 +404,7 @@ mpage_readpages(struct address_space *mapping, struct list_head *pages,
 	}
 	BUG_ON(!list_empty(pages));
 	if (args.bio)
-		mpage_bio_submit(REQ_OP_READ, 0, args.bio);
+		mpage_bio_submit(REQ_OP_READ, REQ_RAHEAD, args.bio);
 	return 0;
 }
 EXPORT_SYMBOL(mpage_readpages);
@@ -408,7 +418,6 @@ int mpage_readpage(struct page *page, get_block_t get_block)
 		.page = page,
 		.nr_pages = 1,
 		.get_block = get_block,
-		.gfp = mapping_gfp_constraint(page->mapping, GFP_KERNEL),
 	};
 
 	args.bio = do_mpage_readpage(&args);
-- 
2.7.4

[PATCH 2/4] mpage: mpage_readpages() should submit IO as read-ahead

[Jens Axboe]

a_ops->readpages() is only ever used for read-ahead, yet we don't
flag the IO being submitted as such. Fix that up. Any file system
that uses mpage_readpages() as it's ->readpages() implementation
will now get this right.

Signed-off-by: Jens Axboe <axboe@kernel.dk>
---
 fs/mpage.c | 13 ++++++++-----
 1 file changed, 8 insertions(+), 5 deletions(-)

diff --git a/fs/mpage.c b/fs/mpage.c
index 0ecac5c410f4..a6344996f924 100644
--- a/fs/mpage.c
+++ b/fs/mpage.c
@@ -142,6 +142,7 @@ struct mpage_readpage_args {
 	unsigned long first_logical_block;
 	get_block_t *get_block;
 	gfp_t gfp;
+	bool is_readahead;
 };
 
 /*
@@ -170,6 +171,7 @@ static struct bio *do_mpage_readpage(struct mpage_readpage_args *args)
 	struct block_device *bdev = NULL;
 	int length;
 	int fully_mapped = 1;
+	int op_flags = args->is_readahead ? REQ_RAHEAD : 0;
 	unsigned nblocks;
 	unsigned relative_block;
 
@@ -283,7 +285,7 @@ static struct bio *do_mpage_readpage(struct mpage_readpage_args *args)
 	 * This page will go to BIO.  Do we need to send this BIO off first?
 	 */
 	if (args->bio && (args->last_block_in_bio != blocks[0] - 1))
-		args->bio = mpage_bio_submit(REQ_OP_READ, 0, args->bio);
+		args->bio = mpage_bio_submit(REQ_OP_READ, op_flags, args->bio);
 
 alloc_new:
 	if (args->bio == NULL) {
@@ -300,7 +302,7 @@ static struct bio *do_mpage_readpage(struct mpage_readpage_args *args)
 
 	length = first_hole << blkbits;
 	if (bio_add_page(args->bio, page, length, 0) < length) {
-		args->bio = mpage_bio_submit(REQ_OP_READ, 0, args->bio);
+		args->bio = mpage_bio_submit(REQ_OP_READ, op_flags, args->bio);
 		goto alloc_new;
 	}
 
@@ -308,7 +310,7 @@ static struct bio *do_mpage_readpage(struct mpage_readpage_args *args)
 	nblocks = map_bh->b_size >> blkbits;
 	if ((buffer_boundary(map_bh) && relative_block == nblocks) ||
 	    (first_hole != blocks_per_page))
-		args->bio = mpage_bio_submit(REQ_OP_READ, 0, args->bio);
+		args->bio = mpage_bio_submit(REQ_OP_READ, op_flags, args->bio);
 	else
 		args->last_block_in_bio = blocks[blocks_per_page - 1];
 out:
@@ -316,7 +318,7 @@ static struct bio *do_mpage_readpage(struct mpage_readpage_args *args)
 
 confused:
 	if (args->bio)
-		args->bio = mpage_bio_submit(REQ_OP_READ, 0, args->bio);
+		args->bio = mpage_bio_submit(REQ_OP_READ, op_flags, args->bio);
 	if (!PageUptodate(page))
 	        block_read_full_page(page, args->get_block);
 	else
@@ -375,6 +377,7 @@ mpage_readpages(struct address_space *mapping, struct list_head *pages,
 	struct mpage_readpage_args args = {
 		.get_block = get_block,
 		.gfp = readahead_gfp_mask(mapping),
+		.is_readahead = true,
 	};
 	unsigned page_idx;
 
@@ -394,7 +397,7 @@ mpage_readpages(struct address_space *mapping, struct list_head *pages,
 	}
 	BUG_ON(!list_empty(pages));
 	if (args.bio)
-		mpage_bio_submit(REQ_OP_READ, 0, args.bio);
+		mpage_bio_submit(REQ_OP_READ, REQ_RAHEAD, args.bio);
 	return 0;
 }
 EXPORT_SYMBOL(mpage_readpages);
-- 
2.7.4