Re: [PATCH v4] vfs: fix page locking deadlocks when deduping files

Matthew Wilcox
2019-08-15T18:18:06Z

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@oracle.com>

Reviewed-by: Matthew Wilcox (Oracle) <willy@infradead.org>

(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.