Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1463116 > unrolled thread

[PATCH 0/7] re-enable DAX PMD support

Back to article view | Back to linux.kernel

Contents

  [PATCH 0/7] re-enable DAX PMD support Ross Zwisler <ross.zwisler@linux.intel.com> - 2016-08-15 21:20 +0200
    [PATCH 5/7] dax: lock based on slot instead of [mapping, index] Ross Zwisler <ross.zwisler@linux.intel.com> - 2016-08-15 21:20 +0200
      Re: [PATCH 5/7] dax: lock based on slot instead of [mapping, index] Jan Kara <jack@suse.cz> - 2016-08-16 11:40 +0200
        Re: [PATCH 5/7] dax: lock based on slot instead of [mapping, index] Ross Zwisler <ross.zwisler@linux.intel.com> - 2016-08-17 22:30 +0200
          Re: [PATCH 5/7] dax: lock based on slot instead of [mapping, index] Jan Kara <jack@suse.cz> - 2016-08-18 16:20 +0200
    [PATCH 2/7] ext4: tell DAX the size of allocation holes Ross Zwisler <ross.zwisler@linux.intel.com> - 2016-08-15 21:20 +0200
      Re: [PATCH 2/7] ext4: tell DAX the size of allocation holes Jan Kara <jack@suse.cz> - 2016-08-16 11:20 +0200
    Re: [PATCH 0/7] re-enable DAX PMD support Dan Williams <dan.j.williams@intel.com> - 2016-08-15 22:30 +0200
      Re: [PATCH 0/7] re-enable DAX PMD support Ross Zwisler <ross.zwisler@linux.intel.com> - 2016-08-15 23:20 +0200
        Re: [PATCH 0/7] re-enable DAX PMD support Dan Williams <dan.j.williams@intel.com> - 2016-08-15 23:20 +0200
          Re: [PATCH 0/7] re-enable DAX PMD support Ross Zwisler <ross.zwisler@linux.intel.com> - 2016-08-17 18:30 +0200
            Re: [PATCH 0/7] re-enable DAX PMD support Jan Kara <jack@suse.cz> - 2016-08-17 19:30 +0200

#1463116 — [PATCH 0/7] re-enable DAX PMD support

DAX PMDs have been disabled since Jan Kara introduced DAX radix tree based
locking.  This series allows DAX PMDs to participate in the DAX radix tree
based locking scheme so that they can be re-enabled.

This series restores DAX PMD functionality back to what it was before it
was disabled.  There is still a known issue between DAX PMDs and hole
punch, which I am currently working on and which I plan to address with a
separate series.

Ross Zwisler (7):
  ext2: tell DAX the size of allocation holes
  ext4: tell DAX the size of allocation holes
  dax: remove buffer_size_valid()
  dax: rename 'ret' to 'entry' in grab_mapping_entry
  dax: lock based on slot instead of [mapping, index]
  dax: re-enable DAX PMD support
  dax: remove "depends on BROKEN" from FS_DAX_PMD

 fs/Kconfig          |   1 -
 fs/dax.c            | 301 ++++++++++++++++++++++++++--------------------------
 fs/ext2/inode.c     |   6 ++
 fs/ext4/inode.c     |   3 +
 include/linux/dax.h |  30 +++++-
 mm/filemap.c        |   7 +-
 6 files changed, 191 insertions(+), 157 deletions(-)

-- 
2.9.0

[toc] | [next] | [standalone]

#1463117 — [PATCH 5/7] dax: lock based on slot instead of [mapping, index]

DAX radix tree locking currently locks entries based on the unique
combination of the 'mapping' pointer and the pgoff_t 'index' for the entry.
This works for PTEs, but as we move to PMDs we will need to have all the
offsets within the range covered by the PMD to map to the same bit lock.
To accomplish this, lock based on the 'slot' pointer in the radix tree
instead of [mapping, index].

When a PMD entry is present in the tree, all offsets will map to the same
'slot' via radix tree lookups, and they will all share the same locking.

Signed-off-by: Ross Zwisler <ross.zwisler@linux.intel.com>
---
 fs/dax.c            | 59 +++++++++++++++++++++--------------------------------
 include/linux/dax.h |  3 +--
 mm/filemap.c        |  3 +--
 3 files changed, 25 insertions(+), 40 deletions(-)

diff --git a/fs/dax.c b/fs/dax.c
index fed6a52..0f1d053 100644
--- a/fs/dax.c
+++ b/fs/dax.c
@@ -62,11 +62,10 @@ static int __init init_dax_wait_table(void)
 }
 fs_initcall(init_dax_wait_table);
 
-static wait_queue_head_t *dax_entry_waitqueue(struct address_space *mapping,
-					      pgoff_t index)
+static wait_queue_head_t *dax_entry_waitqueue(void **slot)
 {
-	unsigned long hash = hash_long((unsigned long)mapping ^ index,
-				       DAX_WAIT_TABLE_BITS);
+	unsigned long hash = hash_long((unsigned long)slot,
+					DAX_WAIT_TABLE_BITS);
 	return wait_table + hash;
 }
 
@@ -281,25 +280,19 @@ EXPORT_SYMBOL_GPL(dax_do_io);
 /*
  * DAX radix tree locking
  */
-struct exceptional_entry_key {
-	struct address_space *mapping;
-	unsigned long index;
-};
-
 struct wait_exceptional_entry_queue {
 	wait_queue_t wait;
-	struct exceptional_entry_key key;
+	void **slot;
 };
 
 static int wake_exceptional_entry_func(wait_queue_t *wait, unsigned int mode,
 				       int sync, void *keyp)
 {
-	struct exceptional_entry_key *key = keyp;
+	void **slot = keyp;
 	struct wait_exceptional_entry_queue *ewait =
 		container_of(wait, struct wait_exceptional_entry_queue, wait);
 
-	if (key->mapping != ewait->key.mapping ||
-	    key->index != ewait->key.index)
+	if (slot != ewait->slot)
 		return 0;
 	return autoremove_wake_function(wait, mode, sync, NULL);
 }
@@ -357,12 +350,10 @@ static void *get_unlocked_mapping_entry(struct address_space *mapping,
 {
 	void *ret, **slot;
 	struct wait_exceptional_entry_queue ewait;
-	wait_queue_head_t *wq = dax_entry_waitqueue(mapping, index);
+	wait_queue_head_t *wq;
 
 	init_wait(&ewait.wait);
 	ewait.wait.func = wake_exceptional_entry_func;
-	ewait.key.mapping = mapping;
-	ewait.key.index = index;
 
 	for (;;) {
 		ret = __radix_tree_lookup(&mapping->page_tree, index, NULL,
@@ -373,6 +364,9 @@ static void *get_unlocked_mapping_entry(struct address_space *mapping,
 				*slotp = slot;
 			return ret;
 		}
+
+		wq = dax_entry_waitqueue(slot);
+		ewait.slot = slot;
 		prepare_to_wait_exclusive(wq, &ewait.wait,
 					  TASK_UNINTERRUPTIBLE);
 		spin_unlock_irq(&mapping->tree_lock);
@@ -445,10 +439,9 @@ restart:
 	return entry;
 }
 
-void dax_wake_mapping_entry_waiter(struct address_space *mapping,
-				   pgoff_t index, bool wake_all)
+void dax_wake_mapping_entry_waiter(void **slot, bool wake_all)
 {
-	wait_queue_head_t *wq = dax_entry_waitqueue(mapping, index);
+	wait_queue_head_t *wq = dax_entry_waitqueue(slot);
 
 	/*
 	 * Checking for locked entry and prepare_to_wait_exclusive() happens
@@ -456,13 +449,8 @@ void dax_wake_mapping_entry_waiter(struct address_space *mapping,
 	 * So at this point all tasks that could have seen our entry locked
 	 * must be in the waitqueue and the following check will see them.
 	 */
-	if (waitqueue_active(wq)) {
-		struct exceptional_entry_key key;
-
-		key.mapping = mapping;
-		key.index = index;
-		__wake_up(wq, TASK_NORMAL, wake_all ? 0 : 1, &key);
-	}
+	if (waitqueue_active(wq))
+		__wake_up(wq, TASK_NORMAL, wake_all ? 0 : 1, slot);
 }
 
 void dax_unlock_mapping_entry(struct address_space *mapping, pgoff_t index)
@@ -478,7 +466,7 @@ void dax_unlock_mapping_entry(struct address_space *mapping, pgoff_t index)
 	}
 	unlock_slot(mapping, slot);
 	spin_unlock_irq(&mapping->tree_lock);
-	dax_wake_mapping_entry_waiter(mapping, index, false);
+	dax_wake_mapping_entry_waiter(slot, false);
 }
 
 static void put_locked_mapping_entry(struct address_space *mapping,
@@ -496,14 +484,13 @@ static void put_locked_mapping_entry(struct address_space *mapping,
  * Called when we are done with radix tree entry we looked up via
  * get_unlocked_mapping_entry() and which we didn't lock in the end.
  */
-static void put_unlocked_mapping_entry(struct address_space *mapping,
-				       pgoff_t index, void *entry)
+static void put_unlocked_mapping_entry(void **slot, void *entry)
 {
 	if (!radix_tree_exceptional_entry(entry))
 		return;
 
 	/* We have to wake up next waiter for the radix tree entry lock */
-	dax_wake_mapping_entry_waiter(mapping, index, false);
+	dax_wake_mapping_entry_waiter(slot, false);
 }
 
 /*
@@ -512,10 +499,10 @@ static void put_unlocked_mapping_entry(struct address_space *mapping,
  */
 int dax_delete_mapping_entry(struct address_space *mapping, pgoff_t index)
 {
-	void *entry;
+	void *entry, **slot;
 
 	spin_lock_irq(&mapping->tree_lock);
-	entry = get_unlocked_mapping_entry(mapping, index, NULL);
+	entry = get_unlocked_mapping_entry(mapping, index, &slot);
 	/*
 	 * This gets called from truncate / punch_hole path. As such, the caller
 	 * must hold locks protecting against concurrent modifications of the
@@ -530,7 +517,7 @@ int dax_delete_mapping_entry(struct address_space *mapping, pgoff_t index)
 	radix_tree_delete(&mapping->page_tree, index);
 	mapping->nrexceptional--;
 	spin_unlock_irq(&mapping->tree_lock);
-	dax_wake_mapping_entry_waiter(mapping, index, true);
+	dax_wake_mapping_entry_waiter(slot, true);
 
 	return 1;
 }
@@ -1118,15 +1105,15 @@ int dax_pfn_mkwrite(struct vm_area_struct *vma, struct vm_fault *vmf)
 {
 	struct file *file = vma->vm_file;
 	struct address_space *mapping = file->f_mapping;
-	void *entry;
+	void *entry, **slot;
 	pgoff_t index = vmf->pgoff;
 
 	spin_lock_irq(&mapping->tree_lock);
-	entry = get_unlocked_mapping_entry(mapping, index, NULL);
+	entry = get_unlocked_mapping_entry(mapping, index, &slot);
 	if (!entry || !radix_tree_exceptional_entry(entry))
 		goto out;
 	radix_tree_tag_set(&mapping->page_tree, index, PAGECACHE_TAG_DIRTY);
-	put_unlocked_mapping_entry(mapping, index, entry);
+	put_unlocked_mapping_entry(slot, entry);
 out:
 	spin_unlock_irq(&mapping->tree_lock);
 	return VM_FAULT_NOPAGE;
diff --git a/include/linux/dax.h b/include/linux/dax.h
index 9c6dc77..8bcb852 100644
--- a/include/linux/dax.h
+++ b/include/linux/dax.h
@@ -15,8 +15,7 @@ int dax_zero_page_range(struct inode *, loff_t from, unsigned len, get_block_t);
 int dax_truncate_page(struct inode *, loff_t from, get_block_t);
 int dax_fault(struct vm_area_struct *, struct vm_fault *, get_block_t);
 int dax_delete_mapping_entry(struct address_space *mapping, pgoff_t index);
-void dax_wake_mapping_entry_waiter(struct address_space *mapping,
-				   pgoff_t index, bool wake_all);
+void dax_wake_mapping_entry_waiter(void **slot, bool wake_all);
 
 #ifdef CONFIG_FS_DAX
 struct page *read_dax_sector(struct block_device *bdev, sector_t n);
diff --git a/mm/filemap.c b/mm/filemap.c
index 8a287df..56c4ac7 100644
--- a/mm/filemap.c
+++ b/mm/filemap.c
@@ -617,8 +617,7 @@ static int page_cache_tree_insert(struct address_space *mapping,
 			if (node)
 				workingset_node_pages_dec(node);
 			/* Wakeup waiters for exceptional entry lock */
-			dax_wake_mapping_entry_waiter(mapping, page->index,
-						      false);
+			dax_wake_mapping_entry_waiter(slot, false);
 		}
 	}
 	radix_tree_replace_slot(slot, page);
-- 
2.9.0

[toc] | [prev] | [next] | [standalone]

#1463612 — Re: [PATCH 5/7] dax: lock based on slot instead of [mapping, index]

On Mon 15-08-16 13:09:16, Ross Zwisler wrote:
> DAX radix tree locking currently locks entries based on the unique
> combination of the 'mapping' pointer and the pgoff_t 'index' for the entry.
> This works for PTEs, but as we move to PMDs we will need to have all the
> offsets within the range covered by the PMD to map to the same bit lock.
> To accomplish this, lock based on the 'slot' pointer in the radix tree
> instead of [mapping, index].

I'm not convinced this is safe. What makes the slot pointer still valid
after you drop tree_lock? At least radix_tree_shrink() or
radix_tree_expand() could move your slot without letting the waiter know
and he would be never woken.

								Honza

> 
> When a PMD entry is present in the tree, all offsets will map to the same
> 'slot' via radix tree lookups, and they will all share the same locking.
> 
> Signed-off-by: Ross Zwisler <ross.zwisler@linux.intel.com>
> ---
>  fs/dax.c            | 59 +++++++++++++++++++++--------------------------------
>  include/linux/dax.h |  3 +--
>  mm/filemap.c        |  3 +--
>  3 files changed, 25 insertions(+), 40 deletions(-)
> 
> diff --git a/fs/dax.c b/fs/dax.c
> index fed6a52..0f1d053 100644
> --- a/fs/dax.c
> +++ b/fs/dax.c
> @@ -62,11 +62,10 @@ static int __init init_dax_wait_table(void)
>  }
>  fs_initcall(init_dax_wait_table);
>  
> -static wait_queue_head_t *dax_entry_waitqueue(struct address_space *mapping,
> -					      pgoff_t index)
> +static wait_queue_head_t *dax_entry_waitqueue(void **slot)
>  {
> -	unsigned long hash = hash_long((unsigned long)mapping ^ index,
> -				       DAX_WAIT_TABLE_BITS);
> +	unsigned long hash = hash_long((unsigned long)slot,
> +					DAX_WAIT_TABLE_BITS);
>  	return wait_table + hash;
>  }
>  
> @@ -281,25 +280,19 @@ EXPORT_SYMBOL_GPL(dax_do_io);
>  /*
>   * DAX radix tree locking
>   */
> -struct exceptional_entry_key {
> -	struct address_space *mapping;
> -	unsigned long index;
> -};
> -
>  struct wait_exceptional_entry_queue {
>  	wait_queue_t wait;
> -	struct exceptional_entry_key key;
> +	void **slot;
>  };
>  
>  static int wake_exceptional_entry_func(wait_queue_t *wait, unsigned int mode,
>  				       int sync, void *keyp)
>  {
> -	struct exceptional_entry_key *key = keyp;
> +	void **slot = keyp;
>  	struct wait_exceptional_entry_queue *ewait =
>  		container_of(wait, struct wait_exceptional_entry_queue, wait);
>  
> -	if (key->mapping != ewait->key.mapping ||
> -	    key->index != ewait->key.index)
> +	if (slot != ewait->slot)
>  		return 0;
>  	return autoremove_wake_function(wait, mode, sync, NULL);
>  }
> @@ -357,12 +350,10 @@ static void *get_unlocked_mapping_entry(struct address_space *mapping,
>  {
>  	void *ret, **slot;
>  	struct wait_exceptional_entry_queue ewait;
> -	wait_queue_head_t *wq = dax_entry_waitqueue(mapping, index);
> +	wait_queue_head_t *wq;
>  
>  	init_wait(&ewait.wait);
>  	ewait.wait.func = wake_exceptional_entry_func;
> -	ewait.key.mapping = mapping;
> -	ewait.key.index = index;
>  
>  	for (;;) {
>  		ret = __radix_tree_lookup(&mapping->page_tree, index, NULL,
> @@ -373,6 +364,9 @@ static void *get_unlocked_mapping_entry(struct address_space *mapping,
>  				*slotp = slot;
>  			return ret;
>  		}
> +
> +		wq = dax_entry_waitqueue(slot);
> +		ewait.slot = slot;
>  		prepare_to_wait_exclusive(wq, &ewait.wait,
>  					  TASK_UNINTERRUPTIBLE);
>  		spin_unlock_irq(&mapping->tree_lock);
> @@ -445,10 +439,9 @@ restart:
>  	return entry;
>  }
>  
> -void dax_wake_mapping_entry_waiter(struct address_space *mapping,
> -				   pgoff_t index, bool wake_all)
> +void dax_wake_mapping_entry_waiter(void **slot, bool wake_all)
>  {
> -	wait_queue_head_t *wq = dax_entry_waitqueue(mapping, index);
> +	wait_queue_head_t *wq = dax_entry_waitqueue(slot);
>  
>  	/*
>  	 * Checking for locked entry and prepare_to_wait_exclusive() happens
> @@ -456,13 +449,8 @@ void dax_wake_mapping_entry_waiter(struct address_space *mapping,
>  	 * So at this point all tasks that could have seen our entry locked
>  	 * must be in the waitqueue and the following check will see them.
>  	 */
> -	if (waitqueue_active(wq)) {
> -		struct exceptional_entry_key key;
> -
> -		key.mapping = mapping;
> -		key.index = index;
> -		__wake_up(wq, TASK_NORMAL, wake_all ? 0 : 1, &key);
> -	}
> +	if (waitqueue_active(wq))
> +		__wake_up(wq, TASK_NORMAL, wake_all ? 0 : 1, slot);
>  }
>  
>  void dax_unlock_mapping_entry(struct address_space *mapping, pgoff_t index)
> @@ -478,7 +466,7 @@ void dax_unlock_mapping_entry(struct address_space *mapping, pgoff_t index)
>  	}
>  	unlock_slot(mapping, slot);
>  	spin_unlock_irq(&mapping->tree_lock);
> -	dax_wake_mapping_entry_waiter(mapping, index, false);
> +	dax_wake_mapping_entry_waiter(slot, false);
>  }
>  
>  static void put_locked_mapping_entry(struct address_space *mapping,
> @@ -496,14 +484,13 @@ static void put_locked_mapping_entry(struct address_space *mapping,
>   * Called when we are done with radix tree entry we looked up via
>   * get_unlocked_mapping_entry() and which we didn't lock in the end.
>   */
> -static void put_unlocked_mapping_entry(struct address_space *mapping,
> -				       pgoff_t index, void *entry)
> +static void put_unlocked_mapping_entry(void **slot, void *entry)
>  {
>  	if (!radix_tree_exceptional_entry(entry))
>  		return;
>  
>  	/* We have to wake up next waiter for the radix tree entry lock */
> -	dax_wake_mapping_entry_waiter(mapping, index, false);
> +	dax_wake_mapping_entry_waiter(slot, false);
>  }
>  
>  /*
> @@ -512,10 +499,10 @@ static void put_unlocked_mapping_entry(struct address_space *mapping,
>   */
>  int dax_delete_mapping_entry(struct address_space *mapping, pgoff_t index)
>  {
> -	void *entry;
> +	void *entry, **slot;
>  
>  	spin_lock_irq(&mapping->tree_lock);
> -	entry = get_unlocked_mapping_entry(mapping, index, NULL);
> +	entry = get_unlocked_mapping_entry(mapping, index, &slot);
>  	/*
>  	 * This gets called from truncate / punch_hole path. As such, the caller
>  	 * must hold locks protecting against concurrent modifications of the
> @@ -530,7 +517,7 @@ int dax_delete_mapping_entry(struct address_space *mapping, pgoff_t index)
>  	radix_tree_delete(&mapping->page_tree, index);
>  	mapping->nrexceptional--;
>  	spin_unlock_irq(&mapping->tree_lock);
> -	dax_wake_mapping_entry_waiter(mapping, index, true);
> +	dax_wake_mapping_entry_waiter(slot, true);
>  
>  	return 1;
>  }
> @@ -1118,15 +1105,15 @@ int dax_pfn_mkwrite(struct vm_area_struct *vma, struct vm_fault *vmf)
>  {
>  	struct file *file = vma->vm_file;
>  	struct address_space *mapping = file->f_mapping;
> -	void *entry;
> +	void *entry, **slot;
>  	pgoff_t index = vmf->pgoff;
>  
>  	spin_lock_irq(&mapping->tree_lock);
> -	entry = get_unlocked_mapping_entry(mapping, index, NULL);
> +	entry = get_unlocked_mapping_entry(mapping, index, &slot);
>  	if (!entry || !radix_tree_exceptional_entry(entry))
>  		goto out;
>  	radix_tree_tag_set(&mapping->page_tree, index, PAGECACHE_TAG_DIRTY);
> -	put_unlocked_mapping_entry(mapping, index, entry);
> +	put_unlocked_mapping_entry(slot, entry);
>  out:
>  	spin_unlock_irq(&mapping->tree_lock);
>  	return VM_FAULT_NOPAGE;
> diff --git a/include/linux/dax.h b/include/linux/dax.h
> index 9c6dc77..8bcb852 100644
> --- a/include/linux/dax.h
> +++ b/include/linux/dax.h
> @@ -15,8 +15,7 @@ int dax_zero_page_range(struct inode *, loff_t from, unsigned len, get_block_t);
>  int dax_truncate_page(struct inode *, loff_t from, get_block_t);
>  int dax_fault(struct vm_area_struct *, struct vm_fault *, get_block_t);
>  int dax_delete_mapping_entry(struct address_space *mapping, pgoff_t index);
> -void dax_wake_mapping_entry_waiter(struct address_space *mapping,
> -				   pgoff_t index, bool wake_all);
> +void dax_wake_mapping_entry_waiter(void **slot, bool wake_all);
>  
>  #ifdef CONFIG_FS_DAX
>  struct page *read_dax_sector(struct block_device *bdev, sector_t n);
> diff --git a/mm/filemap.c b/mm/filemap.c
> index 8a287df..56c4ac7 100644
> --- a/mm/filemap.c
> +++ b/mm/filemap.c
> @@ -617,8 +617,7 @@ static int page_cache_tree_insert(struct address_space *mapping,
>  			if (node)
>  				workingset_node_pages_dec(node);
>  			/* Wakeup waiters for exceptional entry lock */
> -			dax_wake_mapping_entry_waiter(mapping, page->index,
> -						      false);
> +			dax_wake_mapping_entry_waiter(slot, false);
>  		}
>  	}
>  	radix_tree_replace_slot(slot, page);
> -- 
> 2.9.0
> 
> 
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

[toc] | [prev] | [next] | [standalone]

#1464786 — Re: [PATCH 5/7] dax: lock based on slot instead of [mapping, index]

On Tue, Aug 16, 2016 at 11:28:16AM +0200, Jan Kara wrote:
> On Mon 15-08-16 13:09:16, Ross Zwisler wrote:
> > DAX radix tree locking currently locks entries based on the unique
> > combination of the 'mapping' pointer and the pgoff_t 'index' for the entry.
> > This works for PTEs, but as we move to PMDs we will need to have all the
> > offsets within the range covered by the PMD to map to the same bit lock.
> > To accomplish this, lock based on the 'slot' pointer in the radix tree
> > instead of [mapping, index].
> 
> I'm not convinced this is safe. What makes the slot pointer still valid
> after you drop tree_lock? At least radix_tree_shrink() or
> radix_tree_expand() could move your slot without letting the waiter know
> and he would be never woken.
> 
> 								Honza

Yep, you're right, thanks for catching that.

Given that we can't rely on 'slot' being stable, my next idea is to use a
combination of [mapping, index], but tweak 'index' so that it's always the
beginning of the entry.  So for 4k entries we'd leave it alone, but for 2MiB
entries we'd mask it down to the appropriate 2MiB barrier.

Let me hack on that for a bit, unless you've a better idea.

[toc] | [prev] | [next] | [standalone]

#1465468 — Re: [PATCH 5/7] dax: lock based on slot instead of [mapping, index]

On Wed 17-08-16 14:25:56, Ross Zwisler wrote:
> On Tue, Aug 16, 2016 at 11:28:16AM +0200, Jan Kara wrote:
> > On Mon 15-08-16 13:09:16, Ross Zwisler wrote:
> > > DAX radix tree locking currently locks entries based on the unique
> > > combination of the 'mapping' pointer and the pgoff_t 'index' for the entry.
> > > This works for PTEs, but as we move to PMDs we will need to have all the
> > > offsets within the range covered by the PMD to map to the same bit lock.
> > > To accomplish this, lock based on the 'slot' pointer in the radix tree
> > > instead of [mapping, index].
> > 
> > I'm not convinced this is safe. What makes the slot pointer still valid
> > after you drop tree_lock? At least radix_tree_shrink() or
> > radix_tree_expand() could move your slot without letting the waiter know
> > and he would be never woken.
> > 
> > 								Honza
> 
> Yep, you're right, thanks for catching that.
> 
> Given that we can't rely on 'slot' being stable, my next idea is to use a
> combination of [mapping, index], but tweak 'index' so that it's always the
> beginning of the entry.  So for 4k entries we'd leave it alone, but for 2MiB
> entries we'd mask it down to the appropriate 2MiB barrier.
> 
> Let me hack on that for a bit, unless you've a better idea.

No, that's what I'd do as well.

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

[toc] | [prev] | [next] | [standalone]

#1463118 — [PATCH 2/7] ext4: tell DAX the size of allocation holes

When DAX calls _ext4_get_block() and the file offset points to a hole we
currently don't set bh->b_size.  When we re-enable PMD faults DAX will
need bh->b_size to tell it the size of the hole so it can decide whether to
fault in a 4 KiB zero page or a 2 MiB zero page.

_ext4_get_block() has the hole size information from ext4_map_blocks(), so
populate bh->b_size.

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

diff --git a/fs/ext4/inode.c b/fs/ext4/inode.c
index 3131747..1808013 100644
--- a/fs/ext4/inode.c
+++ b/fs/ext4/inode.c
@@ -759,6 +759,9 @@ static int _ext4_get_block(struct inode *inode, sector_t iblock,
 		ext4_update_bh_state(bh, map.m_flags);
 		bh->b_size = inode->i_sb->s_blocksize * map.m_len;
 		ret = 0;
+	} else if (ret == 0) {
+		/* hole case, need to fill in bh->b_size */
+		bh->b_size = inode->i_sb->s_blocksize * map.m_len;
 	}
 	return ret;
 }
-- 
2.9.0

[toc] | [prev] | [next] | [standalone]

#1463570 — Re: [PATCH 2/7] ext4: tell DAX the size of allocation holes

On Mon 15-08-16 13:09:13, Ross Zwisler wrote:
> When DAX calls _ext4_get_block() and the file offset points to a hole we
> currently don't set bh->b_size.  When we re-enable PMD faults DAX will
> need bh->b_size to tell it the size of the hole so it can decide whether to
> fault in a 4 KiB zero page or a 2 MiB zero page.
> 
> _ext4_get_block() has the hole size information from ext4_map_blocks(), so
> populate bh->b_size.
> 
> Signed-off-by: Ross Zwisler <ross.zwisler@linux.intel.com>

Looks good. You can add:

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

								Honza

> ---
>  fs/ext4/inode.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/fs/ext4/inode.c b/fs/ext4/inode.c
> index 3131747..1808013 100644
> --- a/fs/ext4/inode.c
> +++ b/fs/ext4/inode.c
> @@ -759,6 +759,9 @@ static int _ext4_get_block(struct inode *inode, sector_t iblock,
>  		ext4_update_bh_state(bh, map.m_flags);
>  		bh->b_size = inode->i_sb->s_blocksize * map.m_len;
>  		ret = 0;
> +	} else if (ret == 0) {
> +		/* hole case, need to fill in bh->b_size */
> +		bh->b_size = inode->i_sb->s_blocksize * map.m_len;
>  	}
>  	return ret;
>  }
> -- 
> 2.9.0
> 
> 
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

[toc] | [prev] | [next] | [standalone]

#1463155

On Mon, Aug 15, 2016 at 12:09 PM, Ross Zwisler
<ross.zwisler@linux.intel.com> wrote:
> DAX PMDs have been disabled since Jan Kara introduced DAX radix tree based
> locking.  This series allows DAX PMDs to participate in the DAX radix tree
> based locking scheme so that they can be re-enabled.

Looks good to me.

> This series restores DAX PMD functionality back to what it was before it
> was disabled.  There is still a known issue between DAX PMDs and hole
> punch, which I am currently working on and which I plan to address with a
> separate series.

Perhaps we should hold off on applying patch 6 and 7 until after the
hole-punch fix is ready?

[toc] | [prev] | [next] | [standalone]

#1463175

On Mon, Aug 15, 2016 at 01:21:47PM -0700, Dan Williams wrote:
> On Mon, Aug 15, 2016 at 12:09 PM, Ross Zwisler
> <ross.zwisler@linux.intel.com> wrote:
> > DAX PMDs have been disabled since Jan Kara introduced DAX radix tree based
> > locking.  This series allows DAX PMDs to participate in the DAX radix tree
> > based locking scheme so that they can be re-enabled.
> 
> Looks good to me.
> 
> > This series restores DAX PMD functionality back to what it was before it
> > was disabled.  There is still a known issue between DAX PMDs and hole
> > punch, which I am currently working on and which I plan to address with a
> > separate series.
> 
> Perhaps we should hold off on applying patch 6 and 7 until after the
> hole-punch fix is ready?

Sure, I'm cool with holding off on patch 7 (the Kconfig change) until after
the hole punch fix is ready.

I don't see a reason to hold off on patch 6, though?  It stands on it's own,
implements the correct locking, and doesn't break anything.

[toc] | [prev] | [next] | [standalone]

#1463176

On Mon, Aug 15, 2016 at 2:11 PM, Ross Zwisler
<ross.zwisler@linux.intel.com> wrote:
> On Mon, Aug 15, 2016 at 01:21:47PM -0700, Dan Williams wrote:
>> On Mon, Aug 15, 2016 at 12:09 PM, Ross Zwisler
>> <ross.zwisler@linux.intel.com> wrote:
>> > DAX PMDs have been disabled since Jan Kara introduced DAX radix tree based
>> > locking.  This series allows DAX PMDs to participate in the DAX radix tree
>> > based locking scheme so that they can be re-enabled.
>>
>> Looks good to me.
>>
>> > This series restores DAX PMD functionality back to what it was before it
>> > was disabled.  There is still a known issue between DAX PMDs and hole
>> > punch, which I am currently working on and which I plan to address with a
>> > separate series.
>>
>> Perhaps we should hold off on applying patch 6 and 7 until after the
>> hole-punch fix is ready?
>
> Sure, I'm cool with holding off on patch 7 (the Kconfig change) until after
> the hole punch fix is ready.
>
> I don't see a reason to hold off on patch 6, though?  It stands on it's own,
> implements the correct locking, and doesn't break anything.

Whoops, I just meant 7.

[toc] | [prev] | [next] | [standalone]

#1464646

On Mon, Aug 15, 2016 at 02:14:14PM -0700, Dan Williams wrote:
> On Mon, Aug 15, 2016 at 2:11 PM, Ross Zwisler
> <ross.zwisler@linux.intel.com> wrote:
> > On Mon, Aug 15, 2016 at 01:21:47PM -0700, Dan Williams wrote:
> >> On Mon, Aug 15, 2016 at 12:09 PM, Ross Zwisler
> >> <ross.zwisler@linux.intel.com> wrote:
> >> > DAX PMDs have been disabled since Jan Kara introduced DAX radix tree based
> >> > locking.  This series allows DAX PMDs to participate in the DAX radix tree
> >> > based locking scheme so that they can be re-enabled.
> >>
> >> Looks good to me.
> >>
> >> > This series restores DAX PMD functionality back to what it was before it
> >> > was disabled.  There is still a known issue between DAX PMDs and hole
> >> > punch, which I am currently working on and which I plan to address with a
> >> > separate series.
> >>
> >> Perhaps we should hold off on applying patch 6 and 7 until after the
> >> hole-punch fix is ready?
> >
> > Sure, I'm cool with holding off on patch 7 (the Kconfig change) until after
> > the hole punch fix is ready.
> >
> > I don't see a reason to hold off on patch 6, though?  It stands on it's own,
> > implements the correct locking, and doesn't break anything.
> 
> Whoops, I just meant 7.

Well, it looks like the hole punch case is much improved since I tested it
last!  :)  I used to be able to generate a few different kernel BUGs when hole
punching DAX PMDs, but those have apparently been fixed in the mm layer since
I was last testing, which admittedly was quite a long time ago (February?).

The only issue I was able to find with DAX PMD hole punching was that ext4
wasn't properly doing a writeback before the hole was unmapped and the radix
tree entries were removed.  This issue applies equally to the 4k case, so I've
submitted a bug fix for v4.8:

https://lists.01.org/pipermail/linux-nvdimm/2016-August/006621.html

With that applied, I don't know of any more issues related to DAX PMDs and
hole punch.  I've tested ext4 and XFS (ext2 doesn't support hole punch), and
they both properly do a writeback of all affected PMDs, fully unmap all
affected PMDs, and remove the radix tree entries.  I've tested that new page
faults for addresses previously covered by the old PMDs generate new page
faults, and 4k pages are now faulted in because the block allocator no longer
has 2MiB contiguous allocations.

One question (probably for Jan): should the above ext4 fix be marked for
stable?

Thanks,
- Ross

[toc] | [prev] | [next] | [standalone]

#1464679

On Wed 17-08-16 10:21:24, Ross Zwisler wrote:
> On Mon, Aug 15, 2016 at 02:14:14PM -0700, Dan Williams wrote:
> > On Mon, Aug 15, 2016 at 2:11 PM, Ross Zwisler
> > <ross.zwisler@linux.intel.com> wrote:
> > > On Mon, Aug 15, 2016 at 01:21:47PM -0700, Dan Williams wrote:
> > >> On Mon, Aug 15, 2016 at 12:09 PM, Ross Zwisler
> > >> <ross.zwisler@linux.intel.com> wrote:
> > >> > DAX PMDs have been disabled since Jan Kara introduced DAX radix tree based
> > >> > locking.  This series allows DAX PMDs to participate in the DAX radix tree
> > >> > based locking scheme so that they can be re-enabled.
> > >>
> > >> Looks good to me.
> > >>
> > >> > This series restores DAX PMD functionality back to what it was before it
> > >> > was disabled.  There is still a known issue between DAX PMDs and hole
> > >> > punch, which I am currently working on and which I plan to address with a
> > >> > separate series.
> > >>
> > >> Perhaps we should hold off on applying patch 6 and 7 until after the
> > >> hole-punch fix is ready?
> > >
> > > Sure, I'm cool with holding off on patch 7 (the Kconfig change) until after
> > > the hole punch fix is ready.
> > >
> > > I don't see a reason to hold off on patch 6, though?  It stands on it's own,
> > > implements the correct locking, and doesn't break anything.
> > 
> > Whoops, I just meant 7.
> 
> Well, it looks like the hole punch case is much improved since I tested it
> last!  :)  I used to be able to generate a few different kernel BUGs when hole
> punching DAX PMDs, but those have apparently been fixed in the mm layer since
> I was last testing, which admittedly was quite a long time ago (February?).
> 
> The only issue I was able to find with DAX PMD hole punching was that ext4
> wasn't properly doing a writeback before the hole was unmapped and the radix
> tree entries were removed.  This issue applies equally to the 4k case, so I've
> submitted a bug fix for v4.8:
> 
> https://lists.01.org/pipermail/linux-nvdimm/2016-August/006621.html
> 
> With that applied, I don't know of any more issues related to DAX PMDs and
> hole punch.  I've tested ext4 and XFS (ext2 doesn't support hole punch), and
> they both properly do a writeback of all affected PMDs, fully unmap all
> affected PMDs, and remove the radix tree entries.  I've tested that new page
> faults for addresses previously covered by the old PMDs generate new page
> faults, and 4k pages are now faulted in because the block allocator no longer
> has 2MiB contiguous allocations.
> 
> One question (probably for Jan): should the above ext4 fix be marked for
> stable?

Yes, probably it should be.

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

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web