vfs: fix page locking deadlocks when deduping files
PATCH 1/1 vfs: fix page locking deadlocks when deduping files
From: Darrick J. Wong <[email protected]>
When dedupe wants to use the page cache to compare parts of two files for dedupe, we must be very careful to handle locking correctly. The current code doesn't do this. It must lock and unlock the page only once if the two pages are the same, since the overlapping range check doesn't catch this when blocksize < pagesize. If the pages are distinct but from the same file, we must observe page locking order and lock them in order of increasing offset to avoid clashing with writeback locking.
--- fs/read_write.c | 31 ++++++++++++++++++++++++------- 1 file changed, 24 insertions(+), 7 deletions(-)
fs/read_write.c +24 -7
@@ -1811,10 +1811,7 @@ static int generic_remap_check_len(struct inode *inode_in,return (remap_flags &REMAP_FILE_DEDUP ) ? -EBADE : -EINVAL ; } -/* -* Read a page's worth of file data into the page cache. Return the page -* locked. -*/ +/* Read a page's worth of file data into the page cache. */ static struct page *vfs_dedupe_get_page (struct inode *inode,loff_t offset) {struct page *page;
@@ -1826,10 +1823,27 @@ static struct page *vfs_dedupe_get_page(struct inode *inode, loff_t offset)put_page (page);return ERR_PTR (-EIO ); } -lock_page (page);return page; } +/* +* 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); + +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);put_page (dest_page);put_page (src_page);
Quoted 4 lines · Darrick J. Wong
From: Darrick J. Wong <[email protected]>
fs/read_write.c +3
-0+/* +* Lock two pages, ensuring that we lock in offset order if the pages are from +* the same file.
fs/read_write.c static struct page *vfs_dedupe_get_page(struct inode *inode, loff_t offset) · line 1841
Quoted 10 lines
fs/read_write.c +10
-0+*/ +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...
Quoted 22 lines
fs/read_write.c +7
-1+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);
fs/read_write.c static int vfs_dedupe_file_range_compare(struct inode *src, loff_t srcoff, · line 1903
Quoted 7 lines
fs/read_write.c +2
-1@@ -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 invfs_unlock_two_pages()?
-Dave.
Signature · 2 lines
Dave Chinner [email protected]
Quoted 11 lines · Dave Chinner
Quoted 10 lines · Darrick J. Wong
From: Darrick J. Wong <[email protected]>
fs/read_write.c +9
-0+/* +* 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 ;
Quoted 8 lines
fs/read_write.c static struct page *vfs_dedupe_get_page(struct inode *inode, loff_t offset) · line 1841
Quoted 4 lines
fs/read_write.c +4
-0+ } + +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...
Sure.
Quoted 23 lines
Quoted 23 lines
fs/read_write.c +7
-1+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,
Quoted 9 lines
fs/read_write.c static int vfs_dedupe_file_range_compare(struct inode *src, loff_t srcoff, · line 1903
Quoted 6 lines
fs/read_write.c +2
-1kunmap_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()?
Sure.
--D
Quoted 4 lines
-Dave.
Signature · 2 lines
Dave Chinner [email protected]