| Message ID | 20180621184744.GA77164@jaegeuk-macbookpro.roam.corp.google.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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.
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,
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!
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)