On Thu, Aug 15, 2019 at 09:49:40AM -0700, Darrick J. Wong wrote:
> Fixes: 876bec6f9bbfcb3 ("vfs: refactor clone/dedupe_file_range common functions")
> Signed-off-by: Darrick J. Wong <darrick.wong@xxxxxxxxxx>
Reviewed-by: Matthew Wilcox (Oracle) <willy@xxxxxxxxxxxxx>
(I approve of this patch going upstream)
However, I think there are further simplifications to be made here.
With this patch applied, vfs_dedupe_get_page() now looks like this:
static struct page *vfs_dedupe_get_page(struct inode *inode, loff_t offset)
{
struct page *page;
page = read_mapping_page(inode->i_mapping, offset >> PAGE_SHIFT, NULL);
if (IS_ERR(page))
return page;
if (!PageUptodate(page)) {
put_page(page);
return ERR_PTR(-EIO);
}
return page;
}
But I don't think read_mapping_page() can return a page which doesn't have
PageUptodate set. Follow the path down through read_cache_page() into
do_read_cache_page().
Other than the locations which return an ERR_PTR, the only return point
is at the out: label. Three of the gotos to 'out' are guarded by 'if
(PageUptodate)'. The fourth is after calling wait_on_page_read(). Which
will return ERR_PTR(-EIO) if the page isn't Uptodate after being unlocked.
Subtracting that never-exercised check from the routine leaves us with:
static struct page *vfs_dedupe_get_page(struct inode *inode, loff_t offset)
{
struct page *page;
page = read_mapping_page(inode->i_mapping, offset >> PAGE_SHIFT, NULL);
if (IS_ERR(page))
return page;
return page;
}
which is fundamentally just:
static struct page *vfs_dedupe_get_page(struct inode *inode, loff_t offset)
{
return read_mapping_page(inode->i_mapping, offset >> PAGE_SHIFT, NULL);
}
which seems like it might have a better name and be located in pagemap.h?
I might also like to see
out:
+ VM_BUG_ON(!PageUptodate(page));
mark_page_accessed(page);
at the end of do_read_cache_page(), just to be sure nobody ever screws
that up.
