[PATCH] dax: Flush partial PMDs correctly

[PATCH] dax: Flush partial PMDs correctly

[Matthew Wilcox]

The radix tree would rewind the index in an iterator to the lowest index
of a multi-slot entry.  The XArray iterators instead leave the index
unchanged, but I overlooked that when converting DAX from the radix tree
to the XArray.  Adjust the index that we use for flushing to the start
of the PMD range.

Fixes: c1901cd33cf4 "page cache: Convert find_get_entries_tag to XArray"
Reported-by: Piotr Balcer <piotr.balcer@intel.com>
Tested-by: Dan Williams <dan.j.williams@intel.com>
Signed-off-by: Matthew Wilcox <willy@infradead.org>
---
 fs/dax.c | 17 ++++++++---------
 1 file changed, 8 insertions(+), 9 deletions(-)

diff --git a/fs/dax.c b/fs/dax.c
index 6959837cc465..f7a7af766efe 100644
--- a/fs/dax.c
+++ b/fs/dax.c
@@ -843,9 +843,8 @@ static void dax_entry_mkclean(struct address_space *mapping, pgoff_t index,
 static int dax_writeback_one(struct xa_state *xas, struct dax_device *dax_dev,
 		struct address_space *mapping, void *entry)
 {
-	unsigned long pfn;
+	unsigned long pfn, index, count;
 	long ret = 0;
-	size_t size;
 
 	/*
 	 * A page got tagged dirty in DAX mapping? Something is seriously
@@ -894,17 +893,18 @@ static int dax_writeback_one(struct xa_state *xas, struct dax_device *dax_dev,
 	xas_unlock_irq(xas);
 
 	/*
-	 * Even if dax_writeback_mapping_range() was given a wbc->range_start
-	 * in the middle of a PMD, the 'index' we are given will be aligned to
-	 * the start index of the PMD, as will the pfn we pull from 'entry'.
+	 * If dax_writeback_mapping_range() was given a wbc->range_start
+	 * in the middle of a PMD, the 'index' we are given needs to be
+	 * aligned to the start index of the PMD.
 	 * This allows us to flush for PMD_SIZE and not have to worry about
 	 * partial PMD writebacks.
 	 */
 	pfn = dax_to_pfn(entry);
-	size = PAGE_SIZE << dax_entry_order(entry);
+	count = 1UL << dax_entry_order(entry);
+	index = xas->xa_index &~ (count - 1);
 
 	dax_entry_mkclean(mapping, xas->xa_index, pfn);
-	dax_flush(dax_dev, page_address(pfn_to_page(pfn)), size);
+	dax_flush(dax_dev, page_address(pfn_to_page(pfn)), count * PAGE_SIZE);
 	/*
 	 * After we have flushed the cache, we can clear the dirty tag. There
 	 * cannot be new dirty data in the pfn after the flush has completed as
@@ -917,8 +917,7 @@ static int dax_writeback_one(struct xa_state *xas, struct dax_device *dax_dev,
 	xas_clear_mark(xas, PAGECACHE_TAG_DIRTY);
 	dax_wake_entry(xas, entry, false);
 
-	trace_dax_writeback_one(mapping->host, xas->xa_index,
-			size >> PAGE_SHIFT);
+	trace_dax_writeback_one(mapping->host, xas->xa_index, count);
 	return ret;
 
  put_unlocked:
-- 
2.20.1

Re: [PATCH] dax: Flush partial PMDs correctly

[Jan Kara]

On Thu 28-02-19 20:24:48, Matthew Wilcox wrote:
> The radix tree would rewind the index in an iterator to the lowest index
> of a multi-slot entry.  The XArray iterators instead leave the index
> unchanged, but I overlooked that when converting DAX from the radix tree
> to the XArray.  Adjust the index that we use for flushing to the start
> of the PMD range.
> 
> Fixes: c1901cd33cf4 "page cache: Convert find_get_entries_tag to XArray"
> Reported-by: Piotr Balcer <piotr.balcer@intel.com>
> Tested-by: Dan Williams <dan.j.williams@intel.com>
> Signed-off-by: Matthew Wilcox <willy@infradead.org>
> ---

Thanks for the patch! One comment below:

> @@ -894,17 +893,18 @@ static int dax_writeback_one(struct xa_state *xas, struct dax_device *dax_dev,
>  	xas_unlock_irq(xas);
>  
>  	/*
> -	 * Even if dax_writeback_mapping_range() was given a wbc->range_start
> -	 * in the middle of a PMD, the 'index' we are given will be aligned to
> -	 * the start index of the PMD, as will the pfn we pull from 'entry'.
> +	 * If dax_writeback_mapping_range() was given a wbc->range_start
> +	 * in the middle of a PMD, the 'index' we are given needs to be
> +	 * aligned to the start index of the PMD.
>  	 * This allows us to flush for PMD_SIZE and not have to worry about
>  	 * partial PMD writebacks.
>  	 */
>  	pfn = dax_to_pfn(entry);
> -	size = PAGE_SIZE << dax_entry_order(entry);
> +	count = 1UL << dax_entry_order(entry);
> +	index = xas->xa_index &~ (count - 1);

Hum, why do you compute 'index' here when you actually never use it? The
whole patch looks fishy since it is effectively a noop AFAICT...


								Honza

>  
>  	dax_entry_mkclean(mapping, xas->xa_index, pfn);
> -	dax_flush(dax_dev, page_address(pfn_to_page(pfn)), size);
> +	dax_flush(dax_dev, page_address(pfn_to_page(pfn)), count * PAGE_SIZE);
>  	/*
>  	 * After we have flushed the cache, we can clear the dirty tag. There
>  	 * cannot be new dirty data in the pfn after the flush has completed as
> @@ -917,8 +917,7 @@ static int dax_writeback_one(struct xa_state *xas, struct dax_device *dax_dev,
>  	xas_clear_mark(xas, PAGECACHE_TAG_DIRTY);
>  	dax_wake_entry(xas, entry, false);
>  
> -	trace_dax_writeback_one(mapping->host, xas->xa_index,
> -			size >> PAGE_SHIFT);
> +	trace_dax_writeback_one(mapping->host, xas->xa_index, count);
>  	return ret;
>  
>   put_unlocked:
> -- 
> 2.20.1
> 
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

Re: [PATCH] dax: Flush partial PMDs correctly

[Matthew Wilcox]

On Fri, Mar 01, 2019 at 02:49:54PM +0100, Jan Kara wrote:
> >  	pfn = dax_to_pfn(entry);
> > -	size = PAGE_SIZE << dax_entry_order(entry);
> > +	count = 1UL << dax_entry_order(entry);
> > +	index = xas->xa_index &~ (count - 1);
> 
> Hum, why do you compute 'index' here when you actually never use it? The
> whole patch looks fishy since it is effectively a noop AFAICT...

*facepalm*.  I lost the change to the next line while I was backing out
some extraneous changes.

> >  	dax_entry_mkclean(mapping, xas->xa_index, pfn);

Replacement patch sent.

[PATCH] dax: Flush partial PMDs correctly

[Matthew Wilcox]

The radix tree would rewind the index in an iterator to the lowest index
of a multi-slot entry.  The XArray iterators instead leave the index
unchanged, but I overlooked that when converting DAX from the radix tree
to the XArray.  Adjust the index that we use for flushing to the start
of the PMD range.

Fixes: c1901cd33cf4 "page cache: Convert find_get_entries_tag to XArray"
Reported-by: Piotr Balcer <piotr.balcer@intel.com>
Tested-by: Dan Williams <dan.j.williams@intel.com>
Signed-off-by: Matthew Wilcox <willy@infradead.org>
---
 fs/dax.c | 19 +++++++++----------
 1 file changed, 9 insertions(+), 10 deletions(-)

diff --git a/fs/dax.c b/fs/dax.c
index 6959837cc465..4116cd9f55dd 100644
--- a/fs/dax.c
+++ b/fs/dax.c
@@ -843,9 +843,8 @@ static void dax_entry_mkclean(struct address_space *mapping, pgoff_t index,
 static int dax_writeback_one(struct xa_state *xas, struct dax_device *dax_dev,
 		struct address_space *mapping, void *entry)
 {
-	unsigned long pfn;
+	unsigned long pfn, index, count;
 	long ret = 0;
-	size_t size;
 
 	/*
 	 * A page got tagged dirty in DAX mapping? Something is seriously
@@ -894,17 +893,18 @@ static int dax_writeback_one(struct xa_state *xas, struct dax_device *dax_dev,
 	xas_unlock_irq(xas);
 
 	/*
-	 * Even if dax_writeback_mapping_range() was given a wbc->range_start
-	 * in the middle of a PMD, the 'index' we are given will be aligned to
-	 * the start index of the PMD, as will the pfn we pull from 'entry'.
+	 * If dax_writeback_mapping_range() was given a wbc->range_start
+	 * in the middle of a PMD, the 'index' we use needs to be
+	 * aligned to the start of the PMD.
 	 * This allows us to flush for PMD_SIZE and not have to worry about
 	 * partial PMD writebacks.
 	 */
 	pfn = dax_to_pfn(entry);
-	size = PAGE_SIZE << dax_entry_order(entry);
+	count = 1UL << dax_entry_order(entry);
+	index = xas->xa_index & ~(count - 1);
 
-	dax_entry_mkclean(mapping, xas->xa_index, pfn);
-	dax_flush(dax_dev, page_address(pfn_to_page(pfn)), size);
+	dax_entry_mkclean(mapping, index, pfn);
+	dax_flush(dax_dev, page_address(pfn_to_page(pfn)), count * PAGE_SIZE);
 	/*
 	 * After we have flushed the cache, we can clear the dirty tag. There
 	 * cannot be new dirty data in the pfn after the flush has completed as
@@ -917,8 +917,7 @@ static int dax_writeback_one(struct xa_state *xas, struct dax_device *dax_dev,
 	xas_clear_mark(xas, PAGECACHE_TAG_DIRTY);
 	dax_wake_entry(xas, entry, false);
 
-	trace_dax_writeback_one(mapping->host, xas->xa_index,
-			size >> PAGE_SHIFT);
+	trace_dax_writeback_one(mapping->host, xas->xa_index, count);
 	return ret;
 
  put_unlocked:
-- 
2.20.1

Re: [PATCH] dax: Flush partial PMDs correctly

[Dan Williams]

On Fri, Mar 1, 2019 at 11:12 AM Matthew Wilcox <willy@infradead.org> wrote:
>
> The radix tree would rewind the index in an iterator to the lowest index
> of a multi-slot entry.  The XArray iterators instead leave the index
> unchanged, but I overlooked that when converting DAX from the radix tree
> to the XArray.  Adjust the index that we use for flushing to the start
> of the PMD range.
>
> Fixes: c1901cd33cf4 "page cache: Convert find_get_entries_tag to XArray"
> Reported-by: Piotr Balcer <piotr.balcer@intel.com>
> Tested-by: Dan Williams <dan.j.williams@intel.com>

Given this is a new patch my previous tested-by is invalid.

...that said, this one works.

Tested-by: Dan Williams <dan.j.williams@intel.com>

Re: [PATCH] dax: Flush partial PMDs correctly

[Jan Kara]

On Fri 01-03-19 11:12:41, Matthew Wilcox wrote:
> The radix tree would rewind the index in an iterator to the lowest index
> of a multi-slot entry.  The XArray iterators instead leave the index
> unchanged, but I overlooked that when converting DAX from the radix tree
> to the XArray.  Adjust the index that we use for flushing to the start
> of the PMD range.
> 
> Fixes: c1901cd33cf4 "page cache: Convert find_get_entries_tag to XArray"
> Reported-by: Piotr Balcer <piotr.balcer@intel.com>
> Tested-by: Dan Williams <dan.j.williams@intel.com>
> Signed-off-by: Matthew Wilcox <willy@infradead.org>
> ---
...
> @@ -917,8 +917,7 @@ static int dax_writeback_one(struct xa_state *xas, struct dax_device *dax_dev,
>  	xas_clear_mark(xas, PAGECACHE_TAG_DIRTY);
>  	dax_wake_entry(xas, entry, false);
>  
> -	trace_dax_writeback_one(mapping->host, xas->xa_index,
> -			size >> PAGE_SHIFT);
> +	trace_dax_writeback_one(mapping->host, xas->xa_index, count);

I think here should be 'index' as well, shouldn't it? Otherwise the tracing
data will be somewhat misleading... Besides this the patch looks good to me
now so feel free to add:

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

								Honza
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

Re: [PATCH] dax: Flush partial PMDs correctly

[Dan Williams]

On Mon, Mar 11, 2019 at 2:06 AM Jan Kara <jack@suse.cz> wrote:
>
> On Fri 01-03-19 11:12:41, Matthew Wilcox wrote:
> > The radix tree would rewind the index in an iterator to the lowest index
> > of a multi-slot entry.  The XArray iterators instead leave the index
> > unchanged, but I overlooked that when converting DAX from the radix tree
> > to the XArray.  Adjust the index that we use for flushing to the start
> > of the PMD range.
> >
> > Fixes: c1901cd33cf4 "page cache: Convert find_get_entries_tag to XArray"
> > Reported-by: Piotr Balcer <piotr.balcer@intel.com>
> > Tested-by: Dan Williams <dan.j.williams@intel.com>
> > Signed-off-by: Matthew Wilcox <willy@infradead.org>
> > ---
> ...
> > @@ -917,8 +917,7 @@ static int dax_writeback_one(struct xa_state *xas, struct dax_device *dax_dev,
> >       xas_clear_mark(xas, PAGECACHE_TAG_DIRTY);
> >       dax_wake_entry(xas, entry, false);
> >
> > -     trace_dax_writeback_one(mapping->host, xas->xa_index,
> > -                     size >> PAGE_SHIFT);
> > +     trace_dax_writeback_one(mapping->host, xas->xa_index, count);
>
> I think here should be 'index' as well, shouldn't it? Otherwise the tracing
> data will be somewhat misleading... Besides this the patch looks good to me
> now so feel free to add:
>
> Reviewed-by: Jan Kara <jack@suse.cz>

I agree on the 'index' change, I'll fix that up.

Thanks!