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

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

[Darrick J. Wong]

From: Darrick J. Wong <darrick.wong@oracle.com>

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.

Fixes: 876bec6f9bbfcb3 ("vfs: refactor clone/dedupe_file_range common functions")
Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
Reviewed-by: Bill O'Donnell <billodo@redhat.com>
---
v3: revalidate page after locking it
v2: provide an unlock helper
---
 fs/read_write.c |   50 ++++++++++++++++++++++++++++++++++++++++++--------
 1 file changed, 42 insertions(+), 8 deletions(-)

diff --git a/fs/read_write.c b/fs/read_write.c
index 1f5088dec566..da341eb3033c 100644
--- a/fs/read_write.c
+++ b/fs/read_write.c
@@ -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,32 @@ 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)
+{
+	/* Always lock in order of increasing index. */
+	if (page1->index > page2->index)
+		swap(page1, page2);
+
+	lock_page(page1);
+	if (page1 != page2)
+		lock_page(page2);
+}
+
+/* Unlock two pages, being careful not to unlock the same page twice. */
+static void vfs_unlock_two_pages(struct page *page1, struct page *page2)
+{
+	unlock_page(page1);
+	if (page1 != page2)
+		unlock_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 +1886,25 @@ 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);
+
+		/*
+		 * Now that we've locked both pages, make sure they still
+		 * represent the data we're interested in.  If not, someone
+		 * is invalidating pages on us and we lose.
+		 */
+		if (src_page->mapping != src->i_mapping ||
+		    src_page->index != srcoff >> PAGE_SHIFT ||
+		    dest_page->mapping != dest->i_mapping ||
+		    dest_page->index != destoff >> PAGE_SHIFT) {
+			same = false;
+			goto unlock;
+		}
+
 		src_addr = kmap_atomic(src_page);
 		dest_addr = kmap_atomic(dest_page);
 
@@ -1882,8 +1916,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);
-		unlock_page(src_page);
+unlock:
+		vfs_unlock_two_pages(src_page, dest_page);
 		put_page(dest_page);
 		put_page(src_page);
 

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

[Matthew Wilcox]

On Tue, Aug 13, 2019 at 08:14:34AM -0700, Darrick J. Wong wrote:
> +		/*
> +		 * Now that we've locked both pages, make sure they still
> +		 * represent the data we're interested in.  If not, someone
> +		 * is invalidating pages on us and we lose.
> +		 */
> +		if (src_page->mapping != src->i_mapping ||
> +		    src_page->index != srcoff >> PAGE_SHIFT ||
> +		    dest_page->mapping != dest->i_mapping ||
> +		    dest_page->index != destoff >> PAGE_SHIFT) {
> +			same = false;
> +			goto unlock;
> +		}

It is my understanding that you don't need to check the ->index here.
If I'm wrong about that, I'd really appreciate being corrected, because
the page cache locking is subtle.

You call read_mapping_page() which returns the page with an elevated
refcount.  That means the page can't go back to the page allocator and
be allocated again.  It can, because it's unlocked, still be truncated,
so the check for ->mapping after locking it is needed.  But the check
for ->index being correct was done by find_get_entry().

See pagecache_get_page() -- if we specify FGP_LOCK, then it will lock
the page, check the ->mapping but not check ->index.  OK, it does check
->index, but in a VM_BUG_ON(), so it's not something that ought to be
able to be wrong.

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

[Filipe Manana]

On Tue, Aug 13, 2019 at 4:15 PM Darrick J. Wong <darrick.wong@oracle.com> wrote:
>
> From: Darrick J. Wong <darrick.wong@oracle.com>
>
> 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.
>
> Fixes: 876bec6f9bbfcb3 ("vfs: refactor clone/dedupe_file_range common functions")
> Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> Reviewed-by: Bill O'Donnell <billodo@redhat.com>

Reviewed-by: Filipe Manana <fdmanana@suse.com>

We actually had the same bug in btrfs, before we had cloning/dedupe in
vfs/xfs/etc, and fixed it back in 2017 [1].
I totally missed this behaviour in the vfs helpers when I updated
btrfs to use them some months ago.
Thanks.

[1] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=b1517622f2524f531113b12c27b9a0ea69c38983


> ---
> v3: revalidate page after locking it
> v2: provide an unlock helper
> ---
>  fs/read_write.c |   50 ++++++++++++++++++++++++++++++++++++++++++--------
>  1 file changed, 42 insertions(+), 8 deletions(-)
>
> diff --git a/fs/read_write.c b/fs/read_write.c
> index 1f5088dec566..da341eb3033c 100644
> --- a/fs/read_write.c
> +++ b/fs/read_write.c
> @@ -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,32 @@ 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)
> +{
> +       /* Always lock in order of increasing index. */
> +       if (page1->index > page2->index)
> +               swap(page1, page2);
> +
> +       lock_page(page1);
> +       if (page1 != page2)
> +               lock_page(page2);
> +}
> +
> +/* Unlock two pages, being careful not to unlock the same page twice. */
> +static void vfs_unlock_two_pages(struct page *page1, struct page *page2)
> +{
> +       unlock_page(page1);
> +       if (page1 != page2)
> +               unlock_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 +1886,25 @@ 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);
> +
> +               /*
> +                * Now that we've locked both pages, make sure they still
> +                * represent the data we're interested in.  If not, someone
> +                * is invalidating pages on us and we lose.
> +                */
> +               if (src_page->mapping != src->i_mapping ||
> +                   src_page->index != srcoff >> PAGE_SHIFT ||
> +                   dest_page->mapping != dest->i_mapping ||
> +                   dest_page->index != destoff >> PAGE_SHIFT) {
> +                       same = false;
> +                       goto unlock;
> +               }
> +
>                 src_addr = kmap_atomic(src_page);
>                 dest_addr = kmap_atomic(dest_page);
>
> @@ -1882,8 +1916,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);
> -               unlock_page(src_page);
> +unlock:
> +               vfs_unlock_two_pages(src_page, dest_page);
>                 put_page(dest_page);
>                 put_page(src_page);
>


-- 
Filipe David Manana,

“Whether you think you can, or you think you can't — you're right.”

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

[Gao Xiang]

On Tue, Aug 13, 2019 at 08:40:10AM -0700, Matthew Wilcox wrote:
> On Tue, Aug 13, 2019 at 08:14:34AM -0700, Darrick J. Wong wrote:
> > +		/*
> > +		 * Now that we've locked both pages, make sure they still
> > +		 * represent the data we're interested in.  If not, someone
> > +		 * is invalidating pages on us and we lose.
> > +		 */
> > +		if (src_page->mapping != src->i_mapping ||
> > +		    src_page->index != srcoff >> PAGE_SHIFT ||
> > +		    dest_page->mapping != dest->i_mapping ||
> > +		    dest_page->index != destoff >> PAGE_SHIFT) {
> > +			same = false;
> > +			goto unlock;
> > +		}
> 
> It is my understanding that you don't need to check the ->index here.
> If I'm wrong about that, I'd really appreciate being corrected, because
> the page cache locking is subtle.
> 
> You call read_mapping_page() which returns the page with an elevated
> refcount.  That means the page can't go back to the page allocator and
> be allocated again.  It can, because it's unlocked, still be truncated,
> so the check for ->mapping after locking it is needed.  But the check
> for ->index being correct was done by find_get_entry().
> 
> See pagecache_get_page() -- if we specify FGP_LOCK, then it will lock
> the page, check the ->mapping but not check ->index.  OK, it does check
> ->index, but in a VM_BUG_ON(), so it's not something that ought to be
> able to be wrong.

That is my understanding as well. In details...

The page data get ready after read_mapping_page() is successfully
returned. However, if someone needs to get a stable untruncated page,
lock_page() and recheck page->mapping are needed as well.

I have no idea how page->index can be changed safely without reallocating
the page, even some paths could keep using some truncated page temporarily
with some refcounts held but I think those paths cannot add these pages
directly to some page cache again without freeing since it seems really
unsafe.....

Thanks,
Gao Xiang

> 

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

[Gao Xiang]

On Wed, Aug 14, 2019 at 03:03:21PM +0800, Gao Xiang wrote:
> On Tue, Aug 13, 2019 at 08:40:10AM -0700, Matthew Wilcox wrote:
> > On Tue, Aug 13, 2019 at 08:14:34AM -0700, Darrick J. Wong wrote:
> > > +		/*
> > > +		 * Now that we've locked both pages, make sure they still
> > > +		 * represent the data we're interested in.  If not, someone
> > > +		 * is invalidating pages on us and we lose.
> > > +		 */
> > > +		if (src_page->mapping != src->i_mapping ||
> > > +		    src_page->index != srcoff >> PAGE_SHIFT ||
> > > +		    dest_page->mapping != dest->i_mapping ||
> > > +		    dest_page->index != destoff >> PAGE_SHIFT) {
> > > +			same = false;
> > > +			goto unlock;
> > > +		}
> > 
> > It is my understanding that you don't need to check the ->index here.
> > If I'm wrong about that, I'd really appreciate being corrected, because
> > the page cache locking is subtle.
> > 
> > You call read_mapping_page() which returns the page with an elevated
> > refcount.  That means the page can't go back to the page allocator and
> > be allocated again.  It can, because it's unlocked, still be truncated,
> > so the check for ->mapping after locking it is needed.  But the check
> > for ->index being correct was done by find_get_entry().
> > 
> > See pagecache_get_page() -- if we specify FGP_LOCK, then it will lock
> > the page, check the ->mapping but not check ->index.  OK, it does check
> > ->index, but in a VM_BUG_ON(), so it's not something that ought to be
> > able to be wrong.
> 
> That is my understanding as well. In details...
> 
> The page data get ready after read_mapping_page() is successfully
> returned. However, if someone needs to get a stable untruncated page,
> lock_page() and recheck page->mapping are needed as well.
> 
> I have no idea how page->index can be changed safely without reallocating
> the page, even some paths could keep using some truncated page temporarily
> with some refcounts held but I think those paths cannot add these pages

Such a case is like that even if the page can be truncated
at the same time without locking, some paths only needs to
get its page data unstrictly (and note that these pages
should be Uptodated before). Therefore those paths can
only take a refcount without PG_lock... But such refcounts
should be used temporarily, those pages cannot be added to
page cache again without reallocating...

Thanks,
Gao Xiang

> directly to some page cache again without freeing since it seems really
> unsafe.....
> 
> Thanks,
> Gao Xiang
> 
> > 

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

[Dave Chinner]

On Tue, Aug 13, 2019 at 08:40:10AM -0700, Matthew Wilcox wrote:
> On Tue, Aug 13, 2019 at 08:14:34AM -0700, Darrick J. Wong wrote:
> > +		/*
> > +		 * Now that we've locked both pages, make sure they still
> > +		 * represent the data we're interested in.  If not, someone
> > +		 * is invalidating pages on us and we lose.
> > +		 */
> > +		if (src_page->mapping != src->i_mapping ||
> > +		    src_page->index != srcoff >> PAGE_SHIFT ||
> > +		    dest_page->mapping != dest->i_mapping ||
> > +		    dest_page->index != destoff >> PAGE_SHIFT) {
> > +			same = false;
> > +			goto unlock;
> > +		}
> 
> It is my understanding that you don't need to check the ->index here.
> If I'm wrong about that, I'd really appreciate being corrected, because
> the page cache locking is subtle.

Ah, when talking to Darrick about this, I didn't notice the code
took references on the page, so it probably doesn't need the index
check - the page can't be recycled out from under us here an
inserted into a new mapping until we drop the reference.

What I was mainly concerned about here is that we only have a shared
inode lock on the src inode, so this code can be running
concurrently with both invalidation and insertion into the mapping.
e.g. direct write io does invalidation, buffered read does
insertion. Hence we have to be really careful about the data in the
source page being valid and stable while we run the comparison.

And on further thought, I don't think shared locking is actually
safe here. A shared lock doesn't stop new direct IO from being
submitted, so inode_dio_wait() just drains IO at that point in time
and but doesn't provide any guarantee that there isn't concurrent
DIO running.

Hence we could do the comparison here, see the data is the same,
drop the page lock, a DIO write then invalidates the page and writes
new data while we are comparing the rest of page(s) in the range. By
the time we've checked the whole range, the data at the start is no
longer the same, and the comparison is stale.

And then we do the dedupe operation oblivious to the fact the data
on disk doesn't actually match anymore, and we corrupt the data in
the destination file as it gets linked to mismatched data in the
source file....

Darrick?

> You call read_mapping_page() which returns the page with an elevated
> refcount.  That means the page can't go back to the page allocator and
> be allocated again.  It can, because it's unlocked, still be truncated,
> so the check for ->mapping after locking it is needed.  But the check
> for ->index being correct was done by find_get_entry().
> 
> See pagecache_get_page() -- if we specify FGP_LOCK, then it will lock
> the page, check the ->mapping but not check ->index.  OK, it does check
> ->index, but in a VM_BUG_ON(), so it's not something that ought to be
> able to be wrong.

Yeah, we used to have to play tricks in the old XFS writeback
clustering code to do our own non-blocking page cache lookups adn
this was one of the things we needed to be careful about until
the pagevec_lookup* interfaces came along and solved all the
problems for us. Funny how the brain remembers old gotchas with
also reminding you that the problems went away almost as long
ago.....


Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

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

[Darrick J. Wong]

On Wed, Aug 14, 2019 at 07:54:48PM +1000, Dave Chinner wrote:
> On Tue, Aug 13, 2019 at 08:40:10AM -0700, Matthew Wilcox wrote:
> > On Tue, Aug 13, 2019 at 08:14:34AM -0700, Darrick J. Wong wrote:
> > > +		/*
> > > +		 * Now that we've locked both pages, make sure they still
> > > +		 * represent the data we're interested in.  If not, someone
> > > +		 * is invalidating pages on us and we lose.
> > > +		 */
> > > +		if (src_page->mapping != src->i_mapping ||
> > > +		    src_page->index != srcoff >> PAGE_SHIFT ||
> > > +		    dest_page->mapping != dest->i_mapping ||
> > > +		    dest_page->index != destoff >> PAGE_SHIFT) {
> > > +			same = false;
> > > +			goto unlock;
> > > +		}
> > 
> > It is my understanding that you don't need to check the ->index here.
> > If I'm wrong about that, I'd really appreciate being corrected, because
> > the page cache locking is subtle.
> 
> Ah, when talking to Darrick about this, I didn't notice the code
> took references on the page, so it probably doesn't need the index
> check - the page can't be recycled out from under us here an
> inserted into a new mapping until we drop the reference.
> 
> What I was mainly concerned about here is that we only have a shared
> inode lock on the src inode, so this code can be running
> concurrently with both invalidation and insertion into the mapping.
> e.g. direct write io does invalidation, buffered read does
> insertion. Hence we have to be really careful about the data in the
> source page being valid and stable while we run the comparison.
> 
> And on further thought, I don't think shared locking is actually
> safe here. A shared lock doesn't stop new direct IO from being
> submitted, so inode_dio_wait() just drains IO at that point in time
> and but doesn't provide any guarantee that there isn't concurrent
> DIO running.
> 
> Hence we could do the comparison here, see the data is the same,
> drop the page lock, a DIO write then invalidates the page and writes
> new data while we are comparing the rest of page(s) in the range. By
> the time we've checked the whole range, the data at the start is no
> longer the same, and the comparison is stale.
> 
> And then we do the dedupe operation oblivious to the fact the data
> on disk doesn't actually match anymore, and we corrupt the data in
> the destination file as it gets linked to mismatched data in the
> source file....

<urrrrrrk> Yeah, that looks like a bug to me.  I didn't realize that
directio writes were IOLOCK_SHARED and therefore reflink has to take
IOLOCK_EXCL to block them.

Related question: do we need to do the same for MMAPLOCK?  I think the
answer is yes because xfs_filemap_fault can call page_mkwrite with
MMAPLOCK_SHARED.

--D

> Darrick?
> 
> > You call read_mapping_page() which returns the page with an elevated
> > refcount.  That means the page can't go back to the page allocator and
> > be allocated again.  It can, because it's unlocked, still be truncated,
> > so the check for ->mapping after locking it is needed.  But the check
> > for ->index being correct was done by find_get_entry().
> > 
> > See pagecache_get_page() -- if we specify FGP_LOCK, then it will lock
> > the page, check the ->mapping but not check ->index.  OK, it does check
> > ->index, but in a VM_BUG_ON(), so it's not something that ought to be
> > able to be wrong.
> 
> Yeah, we used to have to play tricks in the old XFS writeback
> clustering code to do our own non-blocking page cache lookups adn
> this was one of the things we needed to be careful about until
> the pagevec_lookup* interfaces came along and solved all the
> problems for us. Funny how the brain remembers old gotchas with
> also reminding you that the problems went away almost as long
> ago.....
> 
> 
> Cheers,
> 
> Dave.
> -- 
> Dave Chinner
> david@fromorbit.com

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

[Dave Chinner]

On Wed, Aug 14, 2019 at 08:33:49AM -0700, Darrick J. Wong wrote:
> On Wed, Aug 14, 2019 at 07:54:48PM +1000, Dave Chinner wrote:
> > On Tue, Aug 13, 2019 at 08:40:10AM -0700, Matthew Wilcox wrote:
> > > On Tue, Aug 13, 2019 at 08:14:34AM -0700, Darrick J. Wong wrote:
> > > > +		/*
> > > > +		 * Now that we've locked both pages, make sure they still
> > > > +		 * represent the data we're interested in.  If not, someone
> > > > +		 * is invalidating pages on us and we lose.
> > > > +		 */
> > > > +		if (src_page->mapping != src->i_mapping ||
> > > > +		    src_page->index != srcoff >> PAGE_SHIFT ||
> > > > +		    dest_page->mapping != dest->i_mapping ||
> > > > +		    dest_page->index != destoff >> PAGE_SHIFT) {
> > > > +			same = false;
> > > > +			goto unlock;
> > > > +		}
> > > 
> > > It is my understanding that you don't need to check the ->index here.
> > > If I'm wrong about that, I'd really appreciate being corrected, because
> > > the page cache locking is subtle.
> > 
> > Ah, when talking to Darrick about this, I didn't notice the code
> > took references on the page, so it probably doesn't need the index
> > check - the page can't be recycled out from under us here an
> > inserted into a new mapping until we drop the reference.
> > 
> > What I was mainly concerned about here is that we only have a shared
> > inode lock on the src inode, so this code can be running
> > concurrently with both invalidation and insertion into the mapping.
> > e.g. direct write io does invalidation, buffered read does
> > insertion. Hence we have to be really careful about the data in the
> > source page being valid and stable while we run the comparison.
> > 
> > And on further thought, I don't think shared locking is actually
> > safe here. A shared lock doesn't stop new direct IO from being
> > submitted, so inode_dio_wait() just drains IO at that point in time
> > and but doesn't provide any guarantee that there isn't concurrent
> > DIO running.
> > 
> > Hence we could do the comparison here, see the data is the same,
> > drop the page lock, a DIO write then invalidates the page and writes
> > new data while we are comparing the rest of page(s) in the range. By
> > the time we've checked the whole range, the data at the start is no
> > longer the same, and the comparison is stale.
> > 
> > And then we do the dedupe operation oblivious to the fact the data
> > on disk doesn't actually match anymore, and we corrupt the data in
> > the destination file as it gets linked to mismatched data in the
> > source file....
> 
> <urrrrrrk> Yeah, that looks like a bug to me.  I didn't realize that
> directio writes were IOLOCK_SHARED and therefore reflink has to take
> IOLOCK_EXCL to block them.
> 
> Related question: do we need to do the same for MMAPLOCK?  I think the
> answer is yes because xfs_filemap_fault can call page_mkwrite with
> MMAPLOCK_SHARED.

Hmmmm. Yes, you are right, but I don't just holding MMAPLOCK_EXCL is
enough. Holding the MMAPLOCK will hold off page faults while we have
the lock, but it won't prevent pages that already have writeable
ptes from being modified as they don't require another page fault
until they've been cleaned.

So it seems to me that if we need to ensure that the file range is
not being concurrently modified, we have to:

	a) inode lock exclusive
	b) mmap lock exclusive
	c) break layouts(*)
	d) wait for dio
	e) clean all the dirty pages

On both the source and destination files. And then, because the
locks we hold form a barrier against newly dirtied pages, will all
attempts to modify the data be blocked. And so now the data
comparison can be done safely.

(*) The break layouts bit is necessary to handle co-ordination with
RDMA, NVMEoF, P2P DMA, pNFS, etc that manipulate data directly via
the block device rather than through file pages or DIO...

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

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

[Darrick J. Wong]

On Thu, Aug 15, 2019 at 07:28:33AM +1000, Dave Chinner wrote:
> On Wed, Aug 14, 2019 at 08:33:49AM -0700, Darrick J. Wong wrote:
> > On Wed, Aug 14, 2019 at 07:54:48PM +1000, Dave Chinner wrote:
> > > On Tue, Aug 13, 2019 at 08:40:10AM -0700, Matthew Wilcox wrote:
> > > > On Tue, Aug 13, 2019 at 08:14:34AM -0700, Darrick J. Wong wrote:
> > > > > +		/*
> > > > > +		 * Now that we've locked both pages, make sure they still
> > > > > +		 * represent the data we're interested in.  If not, someone
> > > > > +		 * is invalidating pages on us and we lose.
> > > > > +		 */
> > > > > +		if (src_page->mapping != src->i_mapping ||
> > > > > +		    src_page->index != srcoff >> PAGE_SHIFT ||
> > > > > +		    dest_page->mapping != dest->i_mapping ||
> > > > > +		    dest_page->index != destoff >> PAGE_SHIFT) {
> > > > > +			same = false;
> > > > > +			goto unlock;
> > > > > +		}
> > > > 
> > > > It is my understanding that you don't need to check the ->index here.
> > > > If I'm wrong about that, I'd really appreciate being corrected, because
> > > > the page cache locking is subtle.
> > > 
> > > Ah, when talking to Darrick about this, I didn't notice the code
> > > took references on the page, so it probably doesn't need the index
> > > check - the page can't be recycled out from under us here an
> > > inserted into a new mapping until we drop the reference.
> > > 
> > > What I was mainly concerned about here is that we only have a shared
> > > inode lock on the src inode, so this code can be running
> > > concurrently with both invalidation and insertion into the mapping.
> > > e.g. direct write io does invalidation, buffered read does
> > > insertion. Hence we have to be really careful about the data in the
> > > source page being valid and stable while we run the comparison.
> > > 
> > > And on further thought, I don't think shared locking is actually
> > > safe here. A shared lock doesn't stop new direct IO from being
> > > submitted, so inode_dio_wait() just drains IO at that point in time
> > > and but doesn't provide any guarantee that there isn't concurrent
> > > DIO running.
> > > 
> > > Hence we could do the comparison here, see the data is the same,
> > > drop the page lock, a DIO write then invalidates the page and writes
> > > new data while we are comparing the rest of page(s) in the range. By
> > > the time we've checked the whole range, the data at the start is no
> > > longer the same, and the comparison is stale.
> > > 
> > > And then we do the dedupe operation oblivious to the fact the data
> > > on disk doesn't actually match anymore, and we corrupt the data in
> > > the destination file as it gets linked to mismatched data in the
> > > source file....
> > 
> > <urrrrrrk> Yeah, that looks like a bug to me.  I didn't realize that
> > directio writes were IOLOCK_SHARED and therefore reflink has to take
> > IOLOCK_EXCL to block them.
> > 
> > Related question: do we need to do the same for MMAPLOCK?  I think the
> > answer is yes because xfs_filemap_fault can call page_mkwrite with
> > MMAPLOCK_SHARED.
> 
> Hmmmm. Yes, you are right, but I don't just holding MMAPLOCK_EXCL is
> enough. Holding the MMAPLOCK will hold off page faults while we have
> the lock, but it won't prevent pages that already have writeable
> ptes from being modified as they don't require another page fault
> until they've been cleaned.
> 
> So it seems to me that if we need to ensure that the file range is
> not being concurrently modified, we have to:
> 
> 	a) inode lock exclusive
> 	b) mmap lock exclusive
> 	c) break layouts(*)
> 	d) wait for dio
> 	e) clean all the dirty pages
> 
> On both the source and destination files. And then, because the
> locks we hold form a barrier against newly dirtied pages, will all
> attempts to modify the data be blocked. And so now the data
> comparison can be done safely.

I think xfs already proceeds in this order (a-e), it's just that we
aren't correctly taking IOLOCK_EXCL/MMAPLOCK_EXCL on the source file to
prevent some other thread from starting a directio write or dirtying an
mmap'd page.  But let's try my crappy little patch that fixes the shared
locks and see what other smoke comes out of the machine...

--D

> (*) The break layouts bit is necessary to handle co-ordination with
> RDMA, NVMEoF, P2P DMA, pNFS, etc that manipulate data directly via
> the block device rather than through file pages or DIO...
> 
> Cheers,
> 
> Dave.
> -- 
> Dave Chinner
> david@fromorbit.com