On Wed, Aug 07, 2019 at 07:51:14AM -0700, Darrick J. Wong wrote:
> From: Darrick J. Wong <darrick.wong@xxxxxxxxxx>
> +/*
> + * Lock two pages, ensuring that we lock in offset order if the pages are from
> + * the same file.
> + */
> +static void vfs_lock_two_pages(struct page *page1, struct page *page2)
> +{
> + if (page1 == page2) {
> + lock_page(page1);
> + return;
> + }
> +
> + if (page1->mapping == page2->mapping && page1->index > page2->index)
> + swap(page1, page2);
I would do this even if the pages are on different mappings. That
way we don't expose a landmine if some other code locks two pages
from the same mappings in a different order...
> + lock_page(page1);
> + lock_page(page2);
> +}
> +
> /*
> * Compare extents of two files to see if they are the same.
> * Caller must have locked both inodes to prevent write races.
> @@ -1867,10 +1881,12 @@ static int vfs_dedupe_file_range_compare(struct inode *src, loff_t srcoff,
> dest_page = vfs_dedupe_get_page(dest, destoff);
> if (IS_ERR(dest_page)) {
> error = PTR_ERR(dest_page);
> - unlock_page(src_page);
> put_page(src_page);
> goto out_error;
> }
> +
> + vfs_lock_two_pages(src_page, dest_page);
> +
> src_addr = kmap_atomic(src_page);
> dest_addr = kmap_atomic(dest_page);
>
> @@ -1882,7 +1898,8 @@ static int vfs_dedupe_file_range_compare(struct inode *src, loff_t srcoff,
>
> kunmap_atomic(dest_addr);
> kunmap_atomic(src_addr);
> - unlock_page(dest_page);
> + if (dest_page != src_page)
> + unlock_page(dest_page);
> unlock_page(src_page);
Would it make sense for symmetry to wrap these in
vfs_unlock_two_pages()?
-Dave.
--
Dave Chinner
david@xxxxxxxxxxxxx
