[PATCH 0/2] ext4: fix DAX dma vs truncate/hole-punch

[PATCH 0/2] ext4: fix DAX dma vs truncate/hole-punch

[Ross Zwisler]

This series from Dan:

https://lists.01.org/pipermail/linux-nvdimm/2018-March/014913.html

added synchronization between DAX dma and truncate/hole-punch in XFS.
This short series adds analogous support to ext4.

I've added calls to ext4_break_layouts() everywhere that ext4 removes
blocks from an inode's map.

The timings in XFS are such that it's difficult to hit this race.  Dan
was able to show the race by manually introducing delays in the direct
I/O path.

For ext4, though, its trivial to hit this race, and a hit will result in
a trigger of this WARN_ON_ONCE() in dax_disassociate_entry():

	WARN_ON_ONCE(trunc && page_ref_count(page) > 1);

I've made an xfstest which tests all the paths where we now call
ext4_break_layouts(), with the exception of the site in
ext4_truncate_failed_write() which is an error path.  Each of the other
4 paths easily hits this race many times in my test setup with the
xfstest.  I'll post this test shortly.

With these patches applied, I've still seen occasional hits of the above
WARN_ON_ONCE(), which tells me that we still have some work to do.  I'll
continue looking at these more rare hits while we review this set.

Ross Zwisler (2):
  dax: dax_layout_busy_page() warn on !exceptional
  ext4: handle layout changes to pinned DAX mappings

 fs/dax.c           |  9 ++++++++-
 fs/ext4/ext4.h     |  1 +
 fs/ext4/extents.c  | 12 ++++++++++++
 fs/ext4/inode.c    | 48 ++++++++++++++++++++++++++++++++++++++++++++++++
 fs/ext4/truncate.h |  1 +
 5 files changed, 70 insertions(+), 1 deletion(-)

-- 
2.14.4

_______________________________________________
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm

[PATCH 1/2] dax: dax_layout_busy_page() warn on !exceptional

[Ross Zwisler]

Inodes using DAX should only ever have exceptional entries in their page
caches.  Make this clear by warning if the iteration in
dax_layout_busy_page() ever sees a non-exceptional entry, and by adding a
comment for the pagevec_release() call which only deals with struct page
pointers.

Signed-off-by: Ross Zwisler <ross.zwisler@linux.intel.com>
---
 fs/dax.c | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/fs/dax.c b/fs/dax.c
index 641192808bb6..4a5e31f8a2d4 100644
--- a/fs/dax.c
+++ b/fs/dax.c
@@ -566,7 +566,7 @@ struct page *dax_layout_busy_page(struct address_space *mapping)
 			if (index >= end)
 				break;
 
-			if (!radix_tree_exceptional_entry(pvec_ent))
+			if (WARN_ON_ONCE(!radix_tree_exceptional_entry(pvec_ent)))
 				continue;
 
 			xa_lock_irq(&mapping->i_pages);
@@ -578,6 +578,13 @@ struct page *dax_layout_busy_page(struct address_space *mapping)
 			if (page)
 				break;
 		}
+
+		/*
+		 * We don't expect normal struct page entries to exist in our
+		 * tree, but we keep these pagevec calls so that this code is
+		 * consistent with the common pattern for handling pagevecs
+		 * throughout the kernel.
+		 */
 		pagevec_remove_exceptionals(&pvec);
 		pagevec_release(&pvec);
 		index++;
-- 
2.14.4

_______________________________________________
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm

[PATCH 2/2] ext4: handle layout changes to pinned DAX mappings

[Ross Zwisler]

Follow the lead of xfs_break_dax_layouts() and add synchronization between
operations in ext4 which remove blocks from an inode (hole punch, truncate
down, etc.) and pages which are pinned due to DAX DMA operations.

Signed-off-by: Ross Zwisler <ross.zwisler@linux.intel.com>
---
 fs/ext4/ext4.h     |  1 +
 fs/ext4/extents.c  | 12 ++++++++++++
 fs/ext4/inode.c    | 48 ++++++++++++++++++++++++++++++++++++++++++++++++
 fs/ext4/truncate.h |  1 +
 4 files changed, 62 insertions(+)

diff --git a/fs/ext4/ext4.h b/fs/ext4/ext4.h
index 0b127853c584..34bccd64d83d 100644
--- a/fs/ext4/ext4.h
+++ b/fs/ext4/ext4.h
@@ -2460,6 +2460,7 @@ extern int ext4_get_inode_loc(struct inode *, struct ext4_iloc *);
 extern int ext4_inode_attach_jinode(struct inode *inode);
 extern int ext4_can_truncate(struct inode *inode);
 extern int ext4_truncate(struct inode *);
+extern int ext4_break_layouts(struct inode *);
 extern int ext4_punch_hole(struct inode *inode, loff_t offset, loff_t length);
 extern int ext4_truncate_restart_trans(handle_t *, struct inode *, int nblocks);
 extern void ext4_set_inode_flags(struct inode *);
diff --git a/fs/ext4/extents.c b/fs/ext4/extents.c
index 0057fe3f248d..a6aef06f455b 100644
--- a/fs/ext4/extents.c
+++ b/fs/ext4/extents.c
@@ -4820,6 +4820,13 @@ static long ext4_zero_range(struct file *file, loff_t offset,
 		 * released from page cache.
 		 */
 		down_write(&EXT4_I(inode)->i_mmap_sem);
+
+		ret = ext4_break_layouts(inode);
+		if (ret) {
+			up_write(&EXT4_I(inode)->i_mmap_sem);
+			goto out_mutex;
+		}
+
 		ret = ext4_update_disksize_before_punch(inode, offset, len);
 		if (ret) {
 			up_write(&EXT4_I(inode)->i_mmap_sem);
@@ -5493,6 +5500,11 @@ int ext4_collapse_range(struct inode *inode, loff_t offset, loff_t len)
 	 * page cache.
 	 */
 	down_write(&EXT4_I(inode)->i_mmap_sem);
+
+	ret = ext4_break_layouts(inode);
+	if (ret)
+		goto out_mmap;
+
 	/*
 	 * Need to round down offset to be aligned with page size boundary
 	 * for page size > block size.
diff --git a/fs/ext4/inode.c b/fs/ext4/inode.c
index 2ea07efbe016..c795e5118745 100644
--- a/fs/ext4/inode.c
+++ b/fs/ext4/inode.c
@@ -4193,6 +4193,41 @@ int ext4_update_disksize_before_punch(struct inode *inode, loff_t offset,
 	return 0;
 }
 
+static void ext4_wait_dax_page(struct ext4_inode_info *ei, bool *did_unlock)
+{
+	*did_unlock = true;
+	up_write(&ei->i_mmap_sem);
+	schedule();
+	down_write(&ei->i_mmap_sem);
+}
+
+int ext4_break_layouts(struct inode *inode)
+{
+	struct ext4_inode_info *ei = EXT4_I(inode);
+	struct page *page;
+	bool retry;
+	int error;
+
+	if (unlikely(!rwsem_is_locked(&ei->i_mmap_sem))) {
+		WARN_ON_ONCE(1);
+		return -EINVAL;
+	}
+
+	do {
+		retry = false;
+		page = dax_layout_busy_page(inode->i_mapping);
+		if (!page)
+			return 0;
+
+		error = ___wait_var_event(&page->_refcount,
+				atomic_read(&page->_refcount) == 1,
+				TASK_INTERRUPTIBLE, 0, 0,
+				ext4_wait_dax_page(ei, &retry));
+	} while (error == 0 && retry);
+
+	return error;
+}
+
 /*
  * ext4_punch_hole: punches a hole in a file by releasing the blocks
  * associated with the given offset and length
@@ -4266,6 +4301,11 @@ int ext4_punch_hole(struct inode *inode, loff_t offset, loff_t length)
 	 * page cache.
 	 */
 	down_write(&EXT4_I(inode)->i_mmap_sem);
+
+	ret = ext4_break_layouts(inode);
+	if (ret)
+		goto out_dio;
+
 	first_block_offset = round_up(offset, sb->s_blocksize);
 	last_block_offset = round_down((offset + length), sb->s_blocksize) - 1;
 
@@ -5554,6 +5594,14 @@ int ext4_setattr(struct dentry *dentry, struct iattr *attr)
 				ext4_wait_for_tail_page_commit(inode);
 		}
 		down_write(&EXT4_I(inode)->i_mmap_sem);
+
+		rc = ext4_break_layouts(inode);
+		if (rc) {
+			up_write(&EXT4_I(inode)->i_mmap_sem);
+			error = rc;
+			goto err_out;
+		}
+
 		/*
 		 * Truncate pagecache after we've waited for commit
 		 * in data=journal mode to make pages freeable.
diff --git a/fs/ext4/truncate.h b/fs/ext4/truncate.h
index 0cb13badf473..a3b78241e9f6 100644
--- a/fs/ext4/truncate.h
+++ b/fs/ext4/truncate.h
@@ -12,6 +12,7 @@
 static inline void ext4_truncate_failed_write(struct inode *inode)
 {
 	down_write(&EXT4_I(inode)->i_mmap_sem);
+	ext4_break_layouts(inode);
 	truncate_inode_pages(inode->i_mapping, inode->i_size);
 	ext4_truncate(inode);
 	up_write(&EXT4_I(inode)->i_mmap_sem);
-- 
2.14.4

_______________________________________________
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm

Re: [PATCH 1/2] dax: dax_layout_busy_page() warn on !exceptional

[Jan Kara]

On Wed 20-06-18 16:15:02, Ross Zwisler wrote:
> Inodes using DAX should only ever have exceptional entries in their page
> caches.  Make this clear by warning if the iteration in
> dax_layout_busy_page() ever sees a non-exceptional entry, and by adding a
> comment for the pagevec_release() call which only deals with struct page
> pointers.
> 
> Signed-off-by: Ross Zwisler <ross.zwisler@linux.intel.com>
> ---
>  fs/dax.c | 9 ++++++++-
>  1 file changed, 8 insertions(+), 1 deletion(-)
> 
> diff --git a/fs/dax.c b/fs/dax.c
> index 641192808bb6..4a5e31f8a2d4 100644
> --- a/fs/dax.c
> +++ b/fs/dax.c
> @@ -566,7 +566,7 @@ struct page *dax_layout_busy_page(struct address_space *mapping)
>  			if (index >= end)
>  				break;
>  
> -			if (!radix_tree_exceptional_entry(pvec_ent))
> +			if (WARN_ON_ONCE(!radix_tree_exceptional_entry(pvec_ent)))

Please wrap this line. Otherwise the patch looks good. You can add:

Reviewed-by: Jan Kara <jack@suse.cz>

								Honza
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR
_______________________________________________
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm

Re: [PATCH 2/2] ext4: handle layout changes to pinned DAX mappings

[Jan Kara]

On Wed 20-06-18 16:15:03, Ross Zwisler wrote:
> Follow the lead of xfs_break_dax_layouts() and add synchronization between
> operations in ext4 which remove blocks from an inode (hole punch, truncate
> down, etc.) and pages which are pinned due to DAX DMA operations.
> 
> Signed-off-by: Ross Zwisler <ross.zwisler@linux.intel.com>
> ---
>  fs/ext4/ext4.h     |  1 +
>  fs/ext4/extents.c  | 12 ++++++++++++
>  fs/ext4/inode.c    | 48 ++++++++++++++++++++++++++++++++++++++++++++++++
>  fs/ext4/truncate.h |  1 +
>  4 files changed, 62 insertions(+)
> 
> diff --git a/fs/ext4/ext4.h b/fs/ext4/ext4.h
> index 0b127853c584..34bccd64d83d 100644
> --- a/fs/ext4/ext4.h
> +++ b/fs/ext4/ext4.h
> @@ -2460,6 +2460,7 @@ extern int ext4_get_inode_loc(struct inode *, struct ext4_iloc *);
>  extern int ext4_inode_attach_jinode(struct inode *inode);
>  extern int ext4_can_truncate(struct inode *inode);
>  extern int ext4_truncate(struct inode *);
> +extern int ext4_break_layouts(struct inode *);
>  extern int ext4_punch_hole(struct inode *inode, loff_t offset, loff_t length);
>  extern int ext4_truncate_restart_trans(handle_t *, struct inode *, int nblocks);
>  extern void ext4_set_inode_flags(struct inode *);
> diff --git a/fs/ext4/extents.c b/fs/ext4/extents.c
> index 0057fe3f248d..a6aef06f455b 100644
> --- a/fs/ext4/extents.c
> +++ b/fs/ext4/extents.c
> @@ -4820,6 +4820,13 @@ static long ext4_zero_range(struct file *file, loff_t offset,
>  		 * released from page cache.
>  		 */
>  		down_write(&EXT4_I(inode)->i_mmap_sem);
> +
> +		ret = ext4_break_layouts(inode);
> +		if (ret) {
> +			up_write(&EXT4_I(inode)->i_mmap_sem);
> +			goto out_mutex;
> +		}
> +
>  		ret = ext4_update_disksize_before_punch(inode, offset, len);
>  		if (ret) {
>  			up_write(&EXT4_I(inode)->i_mmap_sem);
> @@ -5493,6 +5500,11 @@ int ext4_collapse_range(struct inode *inode, loff_t offset, loff_t len)
>  	 * page cache.
>  	 */
>  	down_write(&EXT4_I(inode)->i_mmap_sem);
> +
> +	ret = ext4_break_layouts(inode);
> +	if (ret)
> +		goto out_mmap;
> +
>  	/*
>  	 * Need to round down offset to be aligned with page size boundary
>  	 * for page size > block size.
> diff --git a/fs/ext4/inode.c b/fs/ext4/inode.c
> index 2ea07efbe016..c795e5118745 100644
> --- a/fs/ext4/inode.c
> +++ b/fs/ext4/inode.c
> @@ -4193,6 +4193,41 @@ int ext4_update_disksize_before_punch(struct inode *inode, loff_t offset,
>  	return 0;
>  }
>  
> +static void ext4_wait_dax_page(struct ext4_inode_info *ei, bool *did_unlock)
> +{
> +	*did_unlock = true;
> +	up_write(&ei->i_mmap_sem);
> +	schedule();
> +	down_write(&ei->i_mmap_sem);
> +}
> +
> +int ext4_break_layouts(struct inode *inode)
> +{
> +	struct ext4_inode_info *ei = EXT4_I(inode);
> +	struct page *page;
> +	bool retry;
> +	int error;
> +
> +	if (unlikely(!rwsem_is_locked(&ei->i_mmap_sem))) {
> +		WARN_ON_ONCE(1);
> +		return -EINVAL;
> +	}

This could be shortened as:

if (WARN_ON_ONCE(!rwsem_is_locked(&ei->i_mmap_sem))) {
}

couldn't it?

Besides that the patch looks to me. You can add:

Reviewed-by: Jan Kara <jack@suse.cz>

And I'm really wondering which protection are we still missing that you are
still able to hit the warning with these patches applied.

								Honza
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR
_______________________________________________
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm

Re: [PATCH 2/2] ext4: handle layout changes to pinned DAX mappings

[Jan Kara]

On Wed 20-06-18 16:15:03, Ross Zwisler wrote:
> diff --git a/fs/ext4/truncate.h b/fs/ext4/truncate.h
> index 0cb13badf473..a3b78241e9f6 100644
> --- a/fs/ext4/truncate.h
> +++ b/fs/ext4/truncate.h
> @@ -12,6 +12,7 @@
>  static inline void ext4_truncate_failed_write(struct inode *inode)
>  {
>  	down_write(&EXT4_I(inode)->i_mmap_sem);
> +	ext4_break_layouts(inode);
>  	truncate_inode_pages(inode->i_mapping, inode->i_size);
>  	ext4_truncate(inode);
>  	up_write(&EXT4_I(inode)->i_mmap_sem);

One comment here: I don't think ext4_break_layouts() is necessary here.
ext4_truncate_failed_write() exists to truncate blocks beyond EOF. As such
these blocks could never be even mmaped, let alone pinned by GUP. Maybe
just add a comment that blocks we are truncating here were never visible to
userspace and so we don't need ext4_break_layouts() protection.

								Honza
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR
_______________________________________________
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm

Re: [PATCH 2/2] ext4: handle layout changes to pinned DAX mappings

[Ross Zwisler]

On Fri, Jun 22, 2018 at 10:19:15AM +0200, Jan Kara wrote:
> On Wed 20-06-18 16:15:03, Ross Zwisler wrote:
> > Follow the lead of xfs_break_dax_layouts() and add synchronization between
> > operations in ext4 which remove blocks from an inode (hole punch, truncate
> > down, etc.) and pages which are pinned due to DAX DMA operations.
> > 
> > Signed-off-by: Ross Zwisler <ross.zwisler@linux.intel.com>
> > ---
> >  fs/ext4/ext4.h     |  1 +
> >  fs/ext4/extents.c  | 12 ++++++++++++
> >  fs/ext4/inode.c    | 48 ++++++++++++++++++++++++++++++++++++++++++++++++
> >  fs/ext4/truncate.h |  1 +
> >  4 files changed, 62 insertions(+)
> > 
> > diff --git a/fs/ext4/ext4.h b/fs/ext4/ext4.h
> > index 0b127853c584..34bccd64d83d 100644
> > --- a/fs/ext4/ext4.h
> > +++ b/fs/ext4/ext4.h
> > @@ -2460,6 +2460,7 @@ extern int ext4_get_inode_loc(struct inode *, struct ext4_iloc *);
> >  extern int ext4_inode_attach_jinode(struct inode *inode);
> >  extern int ext4_can_truncate(struct inode *inode);
> >  extern int ext4_truncate(struct inode *);
> > +extern int ext4_break_layouts(struct inode *);
> >  extern int ext4_punch_hole(struct inode *inode, loff_t offset, loff_t length);
> >  extern int ext4_truncate_restart_trans(handle_t *, struct inode *, int nblocks);
> >  extern void ext4_set_inode_flags(struct inode *);
> > diff --git a/fs/ext4/extents.c b/fs/ext4/extents.c
> > index 0057fe3f248d..a6aef06f455b 100644
> > --- a/fs/ext4/extents.c
> > +++ b/fs/ext4/extents.c
> > @@ -4820,6 +4820,13 @@ static long ext4_zero_range(struct file *file, loff_t offset,
> >  		 * released from page cache.
> >  		 */
> >  		down_write(&EXT4_I(inode)->i_mmap_sem);
> > +
> > +		ret = ext4_break_layouts(inode);
> > +		if (ret) {
> > +			up_write(&EXT4_I(inode)->i_mmap_sem);
> > +			goto out_mutex;
> > +		}
> > +
> >  		ret = ext4_update_disksize_before_punch(inode, offset, len);
> >  		if (ret) {
> >  			up_write(&EXT4_I(inode)->i_mmap_sem);
> > @@ -5493,6 +5500,11 @@ int ext4_collapse_range(struct inode *inode, loff_t offset, loff_t len)
> >  	 * page cache.
> >  	 */
> >  	down_write(&EXT4_I(inode)->i_mmap_sem);
> > +
> > +	ret = ext4_break_layouts(inode);
> > +	if (ret)
> > +		goto out_mmap;
> > +
> >  	/*
> >  	 * Need to round down offset to be aligned with page size boundary
> >  	 * for page size > block size.
> > diff --git a/fs/ext4/inode.c b/fs/ext4/inode.c
> > index 2ea07efbe016..c795e5118745 100644
> > --- a/fs/ext4/inode.c
> > +++ b/fs/ext4/inode.c
> > @@ -4193,6 +4193,41 @@ int ext4_update_disksize_before_punch(struct inode *inode, loff_t offset,
> >  	return 0;
> >  }
> >  
> > +static void ext4_wait_dax_page(struct ext4_inode_info *ei, bool *did_unlock)
> > +{
> > +	*did_unlock = true;
> > +	up_write(&ei->i_mmap_sem);
> > +	schedule();
> > +	down_write(&ei->i_mmap_sem);
> > +}
> > +
> > +int ext4_break_layouts(struct inode *inode)
> > +{
> > +	struct ext4_inode_info *ei = EXT4_I(inode);
> > +	struct page *page;
> > +	bool retry;
> > +	int error;
> > +
> > +	if (unlikely(!rwsem_is_locked(&ei->i_mmap_sem))) {
> > +		WARN_ON_ONCE(1);
> > +		return -EINVAL;
> > +	}
> 
> This could be shortened as:
> 
> if (WARN_ON_ONCE(!rwsem_is_locked(&ei->i_mmap_sem))) {
> }
> 
> couldn't it?

Yep, that's much better.  Thank you for the review.

> Besides that the patch looks to me. You can add:
> 
> Reviewed-by: Jan Kara <jack@suse.cz>
> 
> And I'm really wondering which protection are we still missing that you are
> still able to hit the warning with these patches applied.

I'm also very curious and am going to dig into that this week.
_______________________________________________
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm