[PATCH 0/2] Add updated DAX locking to ext2

[PATCH 0/2] Add updated DAX locking to ext2

[Ross Zwisler]

The first patch in this series is a somewhat related bug fix.  The second patch
adds new locking to ext2 to isolate DAX faults (page faults, PMD faults, page
mkwrite and pfn mkwrite) from ext2 operations that modify a given inode's data
block allocations.

In my first attempt at this fix I attempted to expand the protection offered by
ext2_inode_info->truncate_mutex so that we took truncate_mutex before our DAX
fault paths were entered.  The DAX fault handlers would then call a version of
ext2_get_block() that assumed truncate_mutex was already held.

This *almost* works, but it turns out that it introduces a lock ordering
inversion between truncate_mutex and a given page's page_lock.  With my series
as it is, the lock ordering looks like this:

mmap_sem (MM)
  ext2_inode_info->dax_sem
    sb_start_pagefault (vfs, freeze - taken in DAX)
      address_space->i_mmap_rwsem or page_lock (mutually exclusive in DAX)
        ext2_inode_info->truncate_mutex

With truncate_mutex being taken before the page fault handler, as in my initial
attempt, our lock ordering is instead:

mmap_sem (MM)
  ext2_inode_info->truncate_mutex
    sb_start_pagefault (vfs, freeze - taken in DAX)
      address_space->i_mmap_rwsem or page_lock (mutually exclusive in DAX)

This is fine for DAX, but it turns out that you can't actually take
truncate_mutex before a page_lock because some operations call ext2_get_block()
while already holding page_lock from calls higher in the stack.  This means
that whatever locks are taken in ext2_get_block() must be below page_lock in
the locking order, else you can deadlock.  To solve this I had to introduce a
new lock that would sit above page_lock in the locking order.  This is the new
"dax_sem" semaphore.

Folks with ext2 experience, have I missed any other cases that need to be
protected by dax_sem?  I took a hard look at all the cases in XFS that are
protected by i_mmaplock and convinced myself that truncate was the only one of
them present in ext2.

I've tested this series using xfstests with DAX both enabled and disabled.
Lockdep was on and working, and didn't have any complaints.

Ross Zwisler (2):
  dax: dax_pfn_mkwrite() truncate race check
  ext2: Add locking for DAX faults

 fs/dax.c        | 13 +++++++++++--
 fs/ext2/ext2.h  |  1 +
 fs/ext2/file.c  | 51 +++++++++++++++++++++++++++++++++++++++++++++++----
 fs/ext2/inode.c |  9 +++++++++
 fs/ext2/super.c |  1 +
 5 files changed, 69 insertions(+), 6 deletions(-)

-- 
2.1.0

[PATCH 0/2] Add updated DAX locking to ext2

[Ross Zwisler]

The first patch in this series is a somewhat related bug fix.  The second patch
adds new locking to ext2 to isolate DAX faults (page faults, PMD faults, page
mkwrite and pfn mkwrite) from ext2 operations that modify a given inode's data
block allocations.

In my first attempt at this fix I attempted to expand the protection offered by
ext2_inode_info->truncate_mutex so that we took truncate_mutex before our DAX
fault paths were entered.  The DAX fault handlers would then call a version of
ext2_get_block() that assumed truncate_mutex was already held.

This *almost* works, but it turns out that it introduces a lock ordering
inversion between truncate_mutex and a given page's page_lock.  With my series
as it is, the lock ordering looks like this:

mmap_sem (MM)
  ext2_inode_info->dax_sem
    sb_start_pagefault (vfs, freeze - taken in DAX)
      address_space->i_mmap_rwsem or page_lock (mutually exclusive in DAX)
        ext2_inode_info->truncate_mutex

With truncate_mutex being taken before the page fault handler, as in my initial
attempt, our lock ordering is instead:

mmap_sem (MM)
  ext2_inode_info->truncate_mutex
    sb_start_pagefault (vfs, freeze - taken in DAX)
      address_space->i_mmap_rwsem or page_lock (mutually exclusive in DAX)

This is fine for DAX, but it turns out that you can't actually take
truncate_mutex before a page_lock because some operations call ext2_get_block()
while already holding page_lock from calls higher in the stack.  This means
that whatever locks are taken in ext2_get_block() must be below page_lock in
the locking order, else you can deadlock.  To solve this I had to introduce a
new lock that would sit above page_lock in the locking order.  This is the new
"dax_sem" semaphore.

Folks with ext2 experience, have I missed any other cases that need to be
protected by dax_sem?  I took a hard look at all the cases in XFS that are
protected by i_mmaplock and convinced myself that truncate was the only one of
them present in ext2.

I've tested this series using xfstests with DAX both enabled and disabled.
Lockdep was on and working, and didn't have any complaints.

Ross Zwisler (2):
  dax: dax_pfn_mkwrite() truncate race check
  ext2: Add locking for DAX faults

 fs/dax.c        | 13 +++++++++++--
 fs/ext2/ext2.h  |  1 +
 fs/ext2/file.c  | 51 +++++++++++++++++++++++++++++++++++++++++++++++----
 fs/ext2/inode.c |  9 +++++++++
 fs/ext2/super.c |  1 +
 5 files changed, 69 insertions(+), 6 deletions(-)

-- 
2.1.0

[PATCH 1/2] dax: dax_pfn_mkwrite() truncate race check

[Ross Zwisler]

Update dax_pfn_mkwrite() so that it validates i_size before returning.
This is necessary to ensure that the page fault has not raced with truncate
and is now pointing to a region beyond the end of the current file.

This change is based on a similar outstanding patch for XFS from Dave
Chinner entitled "xfs: add ->pfn_mkwrite support for DAX".

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

diff --git a/fs/dax.c b/fs/dax.c
index 131fd35a..82be6e4 100644
--- a/fs/dax.c
+++ b/fs/dax.c
@@ -693,12 +693,21 @@ EXPORT_SYMBOL_GPL(dax_pmd_fault);
  */
 int dax_pfn_mkwrite(struct vm_area_struct *vma, struct vm_fault *vmf)
 {
-	struct super_block *sb = file_inode(vma->vm_file)->i_sb;
+	struct inode *inode = file_inode(vma->vm_file);
+	struct super_block *sb = inode->i_sb;
+	int ret = VM_FAULT_NOPAGE;
+	loff_t size;
 
 	sb_start_pagefault(sb);
 	file_update_time(vma->vm_file);
+
+	/* check that the faulting page hasn't raced with truncate */
+	size = (i_size_read(inode) + PAGE_SIZE - 1) >> PAGE_SHIFT;
+	if (vmf->pgoff >= size)
+		ret = VM_FAULT_SIGBUS;
+
 	sb_end_pagefault(sb);
-	return VM_FAULT_NOPAGE;
+	return ret;
 }
 EXPORT_SYMBOL_GPL(dax_pfn_mkwrite);
 
-- 
2.1.0

[PATCH 1/2] dax: dax_pfn_mkwrite() truncate race check

[Ross Zwisler]

Update dax_pfn_mkwrite() so that it validates i_size before returning.
This is necessary to ensure that the page fault has not raced with truncate
and is now pointing to a region beyond the end of the current file.

This change is based on a similar outstanding patch for XFS from Dave
Chinner entitled "xfs: add ->pfn_mkwrite support for DAX".

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

diff --git a/fs/dax.c b/fs/dax.c
index 131fd35a..82be6e4 100644
--- a/fs/dax.c
+++ b/fs/dax.c
@@ -693,12 +693,21 @@ EXPORT_SYMBOL_GPL(dax_pmd_fault);
  */
 int dax_pfn_mkwrite(struct vm_area_struct *vma, struct vm_fault *vmf)
 {
-	struct super_block *sb = file_inode(vma->vm_file)->i_sb;
+	struct inode *inode = file_inode(vma->vm_file);
+	struct super_block *sb = inode->i_sb;
+	int ret = VM_FAULT_NOPAGE;
+	loff_t size;
 
 	sb_start_pagefault(sb);
 	file_update_time(vma->vm_file);
+
+	/* check that the faulting page hasn't raced with truncate */
+	size = (i_size_read(inode) + PAGE_SIZE - 1) >> PAGE_SHIFT;
+	if (vmf->pgoff >= size)
+		ret = VM_FAULT_SIGBUS;
+
 	sb_end_pagefault(sb);
-	return VM_FAULT_NOPAGE;
+	return ret;
 }
 EXPORT_SYMBOL_GPL(dax_pfn_mkwrite);
 
-- 
2.1.0

[PATCH 2/2] ext2: Add locking for DAX faults

[Ross Zwisler]

Add locking to ensure that DAX faults are isolated from ext2 operations
that modify the data blocks allocation for an inode.  This is intended to
be analogous to the work being done in XFS by Dave Chinner:

http://www.spinics.net/lists/linux-fsdevel/msg90260.html

Compared with XFS the ext2 case is greatly simplified by the fact that ext2
already allocates and zeros new blocks before they are returned as part of
ext2_get_block(), so DAX doesn't need to worry about getting unmapped or
unwritten buffer heads.

This means that the only work we need to do in ext2 is to isolate the DAX
faults from inode block allocation changes.  I believe this just means that
we need to isolate the DAX faults from truncate operations.

The newly introduced dax_sem is intended to replicate the protection
offered by i_mmaplock in XFS.  In addition to truncate the i_mmaplock also
protects XFS operations like hole punching, fallocate down, extent
manipulation IOCTLS like xfs_ioc_space() and extent swapping.  Truncate is
the only one of these operations supported by ext2.

Signed-off-by: Ross Zwisler <ross.zwisler@linux.intel.com>
---
 fs/ext2/ext2.h  |  1 +
 fs/ext2/file.c  | 51 +++++++++++++++++++++++++++++++++++++++++++++++----
 fs/ext2/inode.c |  9 +++++++++
 fs/ext2/super.c |  1 +
 4 files changed, 58 insertions(+), 4 deletions(-)

diff --git a/fs/ext2/ext2.h b/fs/ext2/ext2.h
index 8d15feb..ec3cd02 100644
--- a/fs/ext2/ext2.h
+++ b/fs/ext2/ext2.h
@@ -684,6 +684,7 @@ struct ext2_inode_info {
 	struct rw_semaphore xattr_sem;
 #endif
 	rwlock_t i_meta_lock;
+	struct rw_semaphore dax_sem;
 
 	/*
 	 * truncate_mutex is for serialising ext2_truncate() against
diff --git a/fs/ext2/file.c b/fs/ext2/file.c
index 1982c3f..389c5d5 100644
--- a/fs/ext2/file.c
+++ b/fs/ext2/file.c
@@ -27,27 +27,70 @@
 #include "acl.h"
 
 #ifdef CONFIG_FS_DAX
+/*
+ * The lock ordering for ext2 DAX fault paths is:
+ *
+ * mmap_sem (MM)
+ *   ext2_inode_info->dax_sem
+ *     sb_start_pagefault (vfs, freeze - taken in DAX)
+ *       address_space->i_mmap_rwsem or page_lock (mutually exclusive in DAX)
+ *         ext2_inode_info->truncate_mutex
+ */
 static int ext2_dax_fault(struct vm_area_struct *vma, struct vm_fault *vmf)
 {
-	return dax_fault(vma, vmf, ext2_get_block, NULL);
+	struct inode *inode = file_inode(vma->vm_file);
+	struct ext2_inode_info *ei = EXT2_I(inode);
+	int ret;
+
+	down_read(&ei->dax_sem);
+	ret = dax_fault(vma, vmf, ext2_get_block, NULL);
+	up_read(&ei->dax_sem);
+	return ret;
 }
 
 static int ext2_dax_pmd_fault(struct vm_area_struct *vma, unsigned long addr,
 						pmd_t *pmd, unsigned int flags)
 {
-	return dax_pmd_fault(vma, addr, pmd, flags, ext2_get_block, NULL);
+	struct inode *inode = file_inode(vma->vm_file);
+	struct ext2_inode_info *ei = EXT2_I(inode);
+	int ret;
+
+	down_read(&ei->dax_sem);
+	ret = dax_pmd_fault(vma, addr, pmd, flags, ext2_get_block, NULL);
+	up_read(&ei->dax_sem);
+	return ret;
 }
 
 static int ext2_dax_mkwrite(struct vm_area_struct *vma, struct vm_fault *vmf)
 {
-	return dax_mkwrite(vma, vmf, ext2_get_block, NULL);
+	struct inode *inode = file_inode(vma->vm_file);
+	struct ext2_inode_info *ei = EXT2_I(inode);
+	int ret;
+
+	down_read(&ei->dax_sem);
+	ret = dax_mkwrite(vma, vmf, ext2_get_block, NULL);
+	up_read(&ei->dax_sem);
+	return ret;
+}
+
+static int ext2_dax_pfn_mkwrite(struct vm_area_struct *vma,
+		struct vm_fault *vmf)
+{
+	struct inode *inode = file_inode(vma->vm_file);
+	struct ext2_inode_info *ei = EXT2_I(inode);
+	int ret;
+
+	down_read(&ei->dax_sem);
+	ret = dax_pfn_mkwrite(vma, vmf);
+	up_read(&ei->dax_sem);
+	return ret;
 }
 
 static const struct vm_operations_struct ext2_dax_vm_ops = {
 	.fault		= ext2_dax_fault,
 	.pmd_fault	= ext2_dax_pmd_fault,
 	.page_mkwrite	= ext2_dax_mkwrite,
-	.pfn_mkwrite	= dax_pfn_mkwrite,
+	.pfn_mkwrite	= ext2_dax_pfn_mkwrite,
 };
 
 static int ext2_file_mmap(struct file *file, struct vm_area_struct *vma)
diff --git a/fs/ext2/inode.c b/fs/ext2/inode.c
index c60a248..2b974fc 100644
--- a/fs/ext2/inode.c
+++ b/fs/ext2/inode.c
@@ -1085,6 +1085,7 @@ static void ext2_free_branches(struct inode *inode, __le32 *p, __le32 *q, int de
 		ext2_free_data(inode, p, q);
 }
 
+/* dax_sem must be held when calling this function */
 static void __ext2_truncate_blocks(struct inode *inode, loff_t offset)
 {
 	__le32 *i_data = EXT2_I(inode)->i_data;
@@ -1170,6 +1171,8 @@ do_indirects:
 
 static void ext2_truncate_blocks(struct inode *inode, loff_t offset)
 {
+	struct ext2_inode_info *ei = EXT2_I(inode);
+
 	/*
 	 * XXX: it seems like a bug here that we don't allow
 	 * IS_APPEND inode to have blocks-past-i_size trimmed off.
@@ -1185,11 +1188,15 @@ static void ext2_truncate_blocks(struct inode *inode, loff_t offset)
 		return;
 	if (IS_APPEND(inode) || IS_IMMUTABLE(inode))
 		return;
+
+	down_write(&ei->dax_sem);
 	__ext2_truncate_blocks(inode, offset);
+	up_write(&ei->dax_sem);
 }
 
 static int ext2_setsize(struct inode *inode, loff_t newsize)
 {
+	struct ext2_inode_info *ei = EXT2_I(inode);
 	int error;
 
 	if (!(S_ISREG(inode->i_mode) || S_ISDIR(inode->i_mode) ||
@@ -1213,8 +1220,10 @@ static int ext2_setsize(struct inode *inode, loff_t newsize)
 	if (error)
 		return error;
 
+	down_write(&ei->dax_sem);
 	truncate_setsize(inode, newsize);
 	__ext2_truncate_blocks(inode, newsize);
+	up_write(&ei->dax_sem);
 
 	inode->i_mtime = inode->i_ctime = CURRENT_TIME_SEC;
 	if (inode_needs_sync(inode)) {
diff --git a/fs/ext2/super.c b/fs/ext2/super.c
index 900e19c..0f3fedc 100644
--- a/fs/ext2/super.c
+++ b/fs/ext2/super.c
@@ -192,6 +192,7 @@ static void init_once(void *foo)
 	init_rwsem(&ei->xattr_sem);
 #endif
 	mutex_init(&ei->truncate_mutex);
+	init_rwsem(&ei->dax_sem);
 	inode_init_once(&ei->vfs_inode);
 }
 
-- 
2.1.0

[PATCH 2/2] ext2: Add locking for DAX faults

[Ross Zwisler]

Add locking to ensure that DAX faults are isolated from ext2 operations
that modify the data blocks allocation for an inode.  This is intended to
be analogous to the work being done in XFS by Dave Chinner:

http://www.spinics.net/lists/linux-fsdevel/msg90260.html

Compared with XFS the ext2 case is greatly simplified by the fact that ext2
already allocates and zeros new blocks before they are returned as part of
ext2_get_block(), so DAX doesn't need to worry about getting unmapped or
unwritten buffer heads.

This means that the only work we need to do in ext2 is to isolate the DAX
faults from inode block allocation changes.  I believe this just means that
we need to isolate the DAX faults from truncate operations.

The newly introduced dax_sem is intended to replicate the protection
offered by i_mmaplock in XFS.  In addition to truncate the i_mmaplock also
protects XFS operations like hole punching, fallocate down, extent
manipulation IOCTLS like xfs_ioc_space() and extent swapping.  Truncate is
the only one of these operations supported by ext2.

Signed-off-by: Ross Zwisler <ross.zwisler@linux.intel.com>
---
 fs/ext2/ext2.h  |  1 +
 fs/ext2/file.c  | 51 +++++++++++++++++++++++++++++++++++++++++++++++----
 fs/ext2/inode.c |  9 +++++++++
 fs/ext2/super.c |  1 +
 4 files changed, 58 insertions(+), 4 deletions(-)

diff --git a/fs/ext2/ext2.h b/fs/ext2/ext2.h
index 8d15feb..ec3cd02 100644
--- a/fs/ext2/ext2.h
+++ b/fs/ext2/ext2.h
@@ -684,6 +684,7 @@ struct ext2_inode_info {
 	struct rw_semaphore xattr_sem;
 #endif
 	rwlock_t i_meta_lock;
+	struct rw_semaphore dax_sem;
 
 	/*
 	 * truncate_mutex is for serialising ext2_truncate() against
diff --git a/fs/ext2/file.c b/fs/ext2/file.c
index 1982c3f..389c5d5 100644
--- a/fs/ext2/file.c
+++ b/fs/ext2/file.c
@@ -27,27 +27,70 @@
 #include "acl.h"
 
 #ifdef CONFIG_FS_DAX
+/*
+ * The lock ordering for ext2 DAX fault paths is:
+ *
+ * mmap_sem (MM)
+ *   ext2_inode_info->dax_sem
+ *     sb_start_pagefault (vfs, freeze - taken in DAX)
+ *       address_space->i_mmap_rwsem or page_lock (mutually exclusive in DAX)
+ *         ext2_inode_info->truncate_mutex
+ */
 static int ext2_dax_fault(struct vm_area_struct *vma, struct vm_fault *vmf)
 {
-	return dax_fault(vma, vmf, ext2_get_block, NULL);
+	struct inode *inode = file_inode(vma->vm_file);
+	struct ext2_inode_info *ei = EXT2_I(inode);
+	int ret;
+
+	down_read(&ei->dax_sem);
+	ret = dax_fault(vma, vmf, ext2_get_block, NULL);
+	up_read(&ei->dax_sem);
+	return ret;
 }
 
 static int ext2_dax_pmd_fault(struct vm_area_struct *vma, unsigned long addr,
 						pmd_t *pmd, unsigned int flags)
 {
-	return dax_pmd_fault(vma, addr, pmd, flags, ext2_get_block, NULL);
+	struct inode *inode = file_inode(vma->vm_file);
+	struct ext2_inode_info *ei = EXT2_I(inode);
+	int ret;
+
+	down_read(&ei->dax_sem);
+	ret = dax_pmd_fault(vma, addr, pmd, flags, ext2_get_block, NULL);
+	up_read(&ei->dax_sem);
+	return ret;
 }
 
 static int ext2_dax_mkwrite(struct vm_area_struct *vma, struct vm_fault *vmf)
 {
-	return dax_mkwrite(vma, vmf, ext2_get_block, NULL);
+	struct inode *inode = file_inode(vma->vm_file);
+	struct ext2_inode_info *ei = EXT2_I(inode);
+	int ret;
+
+	down_read(&ei->dax_sem);
+	ret = dax_mkwrite(vma, vmf, ext2_get_block, NULL);
+	up_read(&ei->dax_sem);
+	return ret;
+}
+
+static int ext2_dax_pfn_mkwrite(struct vm_area_struct *vma,
+		struct vm_fault *vmf)
+{
+	struct inode *inode = file_inode(vma->vm_file);
+	struct ext2_inode_info *ei = EXT2_I(inode);
+	int ret;
+
+	down_read(&ei->dax_sem);
+	ret = dax_pfn_mkwrite(vma, vmf);
+	up_read(&ei->dax_sem);
+	return ret;
 }
 
 static const struct vm_operations_struct ext2_dax_vm_ops = {
 	.fault		= ext2_dax_fault,
 	.pmd_fault	= ext2_dax_pmd_fault,
 	.page_mkwrite	= ext2_dax_mkwrite,
-	.pfn_mkwrite	= dax_pfn_mkwrite,
+	.pfn_mkwrite	= ext2_dax_pfn_mkwrite,
 };
 
 static int ext2_file_mmap(struct file *file, struct vm_area_struct *vma)
diff --git a/fs/ext2/inode.c b/fs/ext2/inode.c
index c60a248..2b974fc 100644
--- a/fs/ext2/inode.c
+++ b/fs/ext2/inode.c
@@ -1085,6 +1085,7 @@ static void ext2_free_branches(struct inode *inode, __le32 *p, __le32 *q, int de
 		ext2_free_data(inode, p, q);
 }
 
+/* dax_sem must be held when calling this function */
 static void __ext2_truncate_blocks(struct inode *inode, loff_t offset)
 {
 	__le32 *i_data = EXT2_I(inode)->i_data;
@@ -1170,6 +1171,8 @@ do_indirects:
 
 static void ext2_truncate_blocks(struct inode *inode, loff_t offset)
 {
+	struct ext2_inode_info *ei = EXT2_I(inode);
+
 	/*
 	 * XXX: it seems like a bug here that we don't allow
 	 * IS_APPEND inode to have blocks-past-i_size trimmed off.
@@ -1185,11 +1188,15 @@ static void ext2_truncate_blocks(struct inode *inode, loff_t offset)
 		return;
 	if (IS_APPEND(inode) || IS_IMMUTABLE(inode))
 		return;
+
+	down_write(&ei->dax_sem);
 	__ext2_truncate_blocks(inode, offset);
+	up_write(&ei->dax_sem);
 }
 
 static int ext2_setsize(struct inode *inode, loff_t newsize)
 {
+	struct ext2_inode_info *ei = EXT2_I(inode);
 	int error;
 
 	if (!(S_ISREG(inode->i_mode) || S_ISDIR(inode->i_mode) ||
@@ -1213,8 +1220,10 @@ static int ext2_setsize(struct inode *inode, loff_t newsize)
 	if (error)
 		return error;
 
+	down_write(&ei->dax_sem);
 	truncate_setsize(inode, newsize);
 	__ext2_truncate_blocks(inode, newsize);
+	up_write(&ei->dax_sem);
 
 	inode->i_mtime = inode->i_ctime = CURRENT_TIME_SEC;
 	if (inode_needs_sync(inode)) {
diff --git a/fs/ext2/super.c b/fs/ext2/super.c
index 900e19c..0f3fedc 100644
--- a/fs/ext2/super.c
+++ b/fs/ext2/super.c
@@ -192,6 +192,7 @@ static void init_once(void *foo)
 	init_rwsem(&ei->xattr_sem);
 #endif
 	mutex_init(&ei->truncate_mutex);
+	init_rwsem(&ei->dax_sem);
 	inode_init_once(&ei->vfs_inode);
 }
 
-- 
2.1.0

Re: [PATCH 2/2] ext2: Add locking for DAX faults

[Dan Williams]

On Fri, Oct 9, 2015 at 3:02 PM, Ross Zwisler
<ross.zwisler@linux.intel.com> wrote:
> Add locking to ensure that DAX faults are isolated from ext2 operations
> that modify the data blocks allocation for an inode.  This is intended to
> be analogous to the work being done in XFS by Dave Chinner:
>
> http://www.spinics.net/lists/linux-fsdevel/msg90260.html
>
> Compared with XFS the ext2 case is greatly simplified by the fact that ext2
> already allocates and zeros new blocks before they are returned as part of
> ext2_get_block(), so DAX doesn't need to worry about getting unmapped or
> unwritten buffer heads.
>
> This means that the only work we need to do in ext2 is to isolate the DAX
> faults from inode block allocation changes.  I believe this just means that
> we need to isolate the DAX faults from truncate operations.
>
> The newly introduced dax_sem is intended to replicate the protection
> offered by i_mmaplock in XFS.  In addition to truncate the i_mmaplock also
> protects XFS operations like hole punching, fallocate down, extent
> manipulation IOCTLS like xfs_ioc_space() and extent swapping.  Truncate is
> the only one of these operations supported by ext2.
>
> Signed-off-by: Ross Zwisler <ross.zwisler@linux.intel.com>
[..]

...not a review of the ext2 changes.

> diff --git a/fs/ext2/inode.c b/fs/ext2/inode.c
> index c60a248..2b974fc 100644
> --- a/fs/ext2/inode.c
> +++ b/fs/ext2/inode.c
> @@ -1085,6 +1085,7 @@ static void ext2_free_branches(struct inode *inode, __le32 *p, __le32 *q, int de
>                 ext2_free_data(inode, p, q);
>  }
>
> +/* dax_sem must be held when calling this function */
>  static void __ext2_truncate_blocks(struct inode *inode, loff_t offset)
>  {

How about  a "WARN_ON(!rwsem_is_locked(&ei->dax_sem));" to backstop
this assumption?

Re: [PATCH 2/2] ext2: Add locking for DAX faults

[Dan Williams]

On Fri, Oct 9, 2015 at 3:02 PM, Ross Zwisler
<ross.zwisler@linux.intel.com> wrote:
> Add locking to ensure that DAX faults are isolated from ext2 operations
> that modify the data blocks allocation for an inode.  This is intended to
> be analogous to the work being done in XFS by Dave Chinner:
>
> http://www.spinics.net/lists/linux-fsdevel/msg90260.html
>
> Compared with XFS the ext2 case is greatly simplified by the fact that ext2
> already allocates and zeros new blocks before they are returned as part of
> ext2_get_block(), so DAX doesn't need to worry about getting unmapped or
> unwritten buffer heads.
>
> This means that the only work we need to do in ext2 is to isolate the DAX
> faults from inode block allocation changes.  I believe this just means that
> we need to isolate the DAX faults from truncate operations.
>
> The newly introduced dax_sem is intended to replicate the protection
> offered by i_mmaplock in XFS.  In addition to truncate the i_mmaplock also
> protects XFS operations like hole punching, fallocate down, extent
> manipulation IOCTLS like xfs_ioc_space() and extent swapping.  Truncate is
> the only one of these operations supported by ext2.
>
> Signed-off-by: Ross Zwisler <ross.zwisler@linux.intel.com>
[..]

...not a review of the ext2 changes.

> diff --git a/fs/ext2/inode.c b/fs/ext2/inode.c
> index c60a248..2b974fc 100644
> --- a/fs/ext2/inode.c
> +++ b/fs/ext2/inode.c
> @@ -1085,6 +1085,7 @@ static void ext2_free_branches(struct inode *inode, __le32 *p, __le32 *q, int de
>                 ext2_free_data(inode, p, q);
>  }
>
> +/* dax_sem must be held when calling this function */
>  static void __ext2_truncate_blocks(struct inode *inode, loff_t offset)
>  {

How about  a "WARN_ON(!rwsem_is_locked(&ei->dax_sem));" to backstop
this assumption?

Re: [PATCH 2/2] ext2: Add locking for DAX faults

[Ross Zwisler]

On Fri, Oct 09, 2015 at 03:18:11PM -0700, Dan Williams wrote:
> On Fri, Oct 9, 2015 at 3:02 PM, Ross Zwisler
> <ross.zwisler@linux.intel.com> wrote:
> > Add locking to ensure that DAX faults are isolated from ext2 operations
> > that modify the data blocks allocation for an inode.  This is intended to
> > be analogous to the work being done in XFS by Dave Chinner:
> >
> > http://www.spinics.net/lists/linux-fsdevel/msg90260.html
> >
> > Compared with XFS the ext2 case is greatly simplified by the fact that ext2
> > already allocates and zeros new blocks before they are returned as part of
> > ext2_get_block(), so DAX doesn't need to worry about getting unmapped or
> > unwritten buffer heads.
> >
> > This means that the only work we need to do in ext2 is to isolate the DAX
> > faults from inode block allocation changes.  I believe this just means that
> > we need to isolate the DAX faults from truncate operations.
> >
> > The newly introduced dax_sem is intended to replicate the protection
> > offered by i_mmaplock in XFS.  In addition to truncate the i_mmaplock also
> > protects XFS operations like hole punching, fallocate down, extent
> > manipulation IOCTLS like xfs_ioc_space() and extent swapping.  Truncate is
> > the only one of these operations supported by ext2.
> >
> > Signed-off-by: Ross Zwisler <ross.zwisler@linux.intel.com>
> [..]
> 
> ...not a review of the ext2 changes.
> 
> > diff --git a/fs/ext2/inode.c b/fs/ext2/inode.c
> > index c60a248..2b974fc 100644
> > --- a/fs/ext2/inode.c
> > +++ b/fs/ext2/inode.c
> > @@ -1085,6 +1085,7 @@ static void ext2_free_branches(struct inode *inode, __le32 *p, __le32 *q, int de
> >                 ext2_free_data(inode, p, q);
> >  }
> >
> > +/* dax_sem must be held when calling this function */
> >  static void __ext2_truncate_blocks(struct inode *inode, loff_t offset)
> >  {
> 
> How about  a "WARN_ON(!rwsem_is_locked(&ei->dax_sem));" to backstop
> this assumption?

Yep, sounds like a good idea.  Thanks.

Re: [PATCH 2/2] ext2: Add locking for DAX faults

[Ross Zwisler]

On Fri, Oct 09, 2015 at 03:18:11PM -0700, Dan Williams wrote:
> On Fri, Oct 9, 2015 at 3:02 PM, Ross Zwisler
> <ross.zwisler@linux.intel.com> wrote:
> > Add locking to ensure that DAX faults are isolated from ext2 operations
> > that modify the data blocks allocation for an inode.  This is intended to
> > be analogous to the work being done in XFS by Dave Chinner:
> >
> > http://www.spinics.net/lists/linux-fsdevel/msg90260.html
> >
> > Compared with XFS the ext2 case is greatly simplified by the fact that ext2
> > already allocates and zeros new blocks before they are returned as part of
> > ext2_get_block(), so DAX doesn't need to worry about getting unmapped or
> > unwritten buffer heads.
> >
> > This means that the only work we need to do in ext2 is to isolate the DAX
> > faults from inode block allocation changes.  I believe this just means that
> > we need to isolate the DAX faults from truncate operations.
> >
> > The newly introduced dax_sem is intended to replicate the protection
> > offered by i_mmaplock in XFS.  In addition to truncate the i_mmaplock also
> > protects XFS operations like hole punching, fallocate down, extent
> > manipulation IOCTLS like xfs_ioc_space() and extent swapping.  Truncate is
> > the only one of these operations supported by ext2.
> >
> > Signed-off-by: Ross Zwisler <ross.zwisler@linux.intel.com>
> [..]
> 
> ...not a review of the ext2 changes.
> 
> > diff --git a/fs/ext2/inode.c b/fs/ext2/inode.c
> > index c60a248..2b974fc 100644
> > --- a/fs/ext2/inode.c
> > +++ b/fs/ext2/inode.c
> > @@ -1085,6 +1085,7 @@ static void ext2_free_branches(struct inode *inode, __le32 *p, __le32 *q, int de
> >                 ext2_free_data(inode, p, q);
> >  }
> >
> > +/* dax_sem must be held when calling this function */
> >  static void __ext2_truncate_blocks(struct inode *inode, loff_t offset)
> >  {
> 
> How about  a "WARN_ON(!rwsem_is_locked(&ei->dax_sem));" to backstop
> this assumption?

Yep, sounds like a good idea.  Thanks.

Re: [PATCH 2/2] ext2: Add locking for DAX faults

[Dave Chinner]

[Nit: please send all patches of the series to the same list of
recipients. Otherwise people with list based filters end up with the
series spread across different mailboxes and people not subscribed
to all lists don't get the full series and so are lacking in context
for proper review. ]

On Fri, Oct 09, 2015 at 04:02:08PM -0600, Ross Zwisler wrote:
> Add locking to ensure that DAX faults are isolated from ext2 operations
> that modify the data blocks allocation for an inode.  This is intended to
> be analogous to the work being done in XFS by Dave Chinner:
> 
> http://www.spinics.net/lists/linux-fsdevel/msg90260.html
> 
> Compared with XFS the ext2 case is greatly simplified by the fact that ext2
> already allocates and zeros new blocks before they are returned as part of
> ext2_get_block(), so DAX doesn't need to worry about getting unmapped or
> unwritten buffer heads.
> 
> This means that the only work we need to do in ext2 is to isolate the DAX
> faults from inode block allocation changes.  I believe this just means that
> we need to isolate the DAX faults from truncate operations.

Why limit this just to DAX page faults?

> The newly introduced dax_sem is intended to replicate the protection
> offered by i_mmaplock in XFS.  In addition to truncate the i_mmaplock also
> protects XFS operations like hole punching, fallocate down, extent
> manipulation IOCTLS like xfs_ioc_space() and extent swapping.  Truncate is
> the only one of these operations supported by ext2.
> 
> Signed-off-by: Ross Zwisler <ross.zwisler@linux.intel.com>
> ---
>  fs/ext2/ext2.h  |  1 +
>  fs/ext2/file.c  | 51 +++++++++++++++++++++++++++++++++++++++++++++++----
>  fs/ext2/inode.c |  9 +++++++++
>  fs/ext2/super.c |  1 +
>  4 files changed, 58 insertions(+), 4 deletions(-)
> 
> diff --git a/fs/ext2/ext2.h b/fs/ext2/ext2.h
> index 8d15feb..ec3cd02 100644
> --- a/fs/ext2/ext2.h
> +++ b/fs/ext2/ext2.h
> @@ -684,6 +684,7 @@ struct ext2_inode_info {
>  	struct rw_semaphore xattr_sem;
>  #endif
>  	rwlock_t i_meta_lock;
> +	struct rw_semaphore dax_sem;
>  
>  	/*
>  	 * truncate_mutex is for serialising ext2_truncate() against
> diff --git a/fs/ext2/file.c b/fs/ext2/file.c
> index 1982c3f..389c5d5 100644
> --- a/fs/ext2/file.c
> +++ b/fs/ext2/file.c
> @@ -27,27 +27,70 @@
>  #include "acl.h"
>  
>  #ifdef CONFIG_FS_DAX
> +/*
> + * The lock ordering for ext2 DAX fault paths is:
> + *
> + * mmap_sem (MM)
> + *   ext2_inode_info->dax_sem
> + *     sb_start_pagefault (vfs, freeze - taken in DAX)
> + *       address_space->i_mmap_rwsem or page_lock (mutually exclusive in DAX)
> + *         ext2_inode_info->truncate_mutex

This is a different lock order to XFS - it puts the i_mmaplock
inside sb_start_pagefault(), not outside it. This ordering means the
timestamp updates during the page fault are also under
ext2_inode_info->dax_sem...

Other than that, I can't really comment on the rest of the changes.

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

Re: [PATCH 2/2] ext2: Add locking for DAX faults

[Dave Chinner]

[Nit: please send all patches of the series to the same list of
recipients. Otherwise people with list based filters end up with the
series spread across different mailboxes and people not subscribed
to all lists don't get the full series and so are lacking in context
for proper review. ]

On Fri, Oct 09, 2015 at 04:02:08PM -0600, Ross Zwisler wrote:
> Add locking to ensure that DAX faults are isolated from ext2 operations
> that modify the data blocks allocation for an inode.  This is intended to
> be analogous to the work being done in XFS by Dave Chinner:
> 
> http://www.spinics.net/lists/linux-fsdevel/msg90260.html
> 
> Compared with XFS the ext2 case is greatly simplified by the fact that ext2
> already allocates and zeros new blocks before they are returned as part of
> ext2_get_block(), so DAX doesn't need to worry about getting unmapped or
> unwritten buffer heads.
> 
> This means that the only work we need to do in ext2 is to isolate the DAX
> faults from inode block allocation changes.  I believe this just means that
> we need to isolate the DAX faults from truncate operations.

Why limit this just to DAX page faults?

> The newly introduced dax_sem is intended to replicate the protection
> offered by i_mmaplock in XFS.  In addition to truncate the i_mmaplock also
> protects XFS operations like hole punching, fallocate down, extent
> manipulation IOCTLS like xfs_ioc_space() and extent swapping.  Truncate is
> the only one of these operations supported by ext2.
> 
> Signed-off-by: Ross Zwisler <ross.zwisler@linux.intel.com>
> ---
>  fs/ext2/ext2.h  |  1 +
>  fs/ext2/file.c  | 51 +++++++++++++++++++++++++++++++++++++++++++++++----
>  fs/ext2/inode.c |  9 +++++++++
>  fs/ext2/super.c |  1 +
>  4 files changed, 58 insertions(+), 4 deletions(-)
> 
> diff --git a/fs/ext2/ext2.h b/fs/ext2/ext2.h
> index 8d15feb..ec3cd02 100644
> --- a/fs/ext2/ext2.h
> +++ b/fs/ext2/ext2.h
> @@ -684,6 +684,7 @@ struct ext2_inode_info {
>  	struct rw_semaphore xattr_sem;
>  #endif
>  	rwlock_t i_meta_lock;
> +	struct rw_semaphore dax_sem;
>  
>  	/*
>  	 * truncate_mutex is for serialising ext2_truncate() against
> diff --git a/fs/ext2/file.c b/fs/ext2/file.c
> index 1982c3f..389c5d5 100644
> --- a/fs/ext2/file.c
> +++ b/fs/ext2/file.c
> @@ -27,27 +27,70 @@
>  #include "acl.h"
>  
>  #ifdef CONFIG_FS_DAX
> +/*
> + * The lock ordering for ext2 DAX fault paths is:
> + *
> + * mmap_sem (MM)
> + *   ext2_inode_info->dax_sem
> + *     sb_start_pagefault (vfs, freeze - taken in DAX)
> + *       address_space->i_mmap_rwsem or page_lock (mutually exclusive in DAX)
> + *         ext2_inode_info->truncate_mutex

This is a different lock order to XFS - it puts the i_mmaplock
inside sb_start_pagefault(), not outside it. This ordering means the
timestamp updates during the page fault are also under
ext2_inode_info->dax_sem...

Other than that, I can't really comment on the rest of the changes.

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

Re: [PATCH 2/2] ext2: Add locking for DAX faults

[Ross Zwisler]

On Mon, Oct 12, 2015 at 10:14:43AM +1100, Dave Chinner wrote:
> [Nit: please send all patches of the series to the same list of
> recipients. Otherwise people with list based filters end up with the
> series spread across different mailboxes and people not subscribed
> to all lists don't get the full series and so are lacking in context
> for proper review. ]

Okay, will do.

> On Fri, Oct 09, 2015 at 04:02:08PM -0600, Ross Zwisler wrote:
<>
> > +/*
> > + * The lock ordering for ext2 DAX fault paths is:
> > + *
> > + * mmap_sem (MM)
> > + *   ext2_inode_info->dax_sem
> > + *     sb_start_pagefault (vfs, freeze - taken in DAX)
> > + *       address_space->i_mmap_rwsem or page_lock (mutually exclusive in DAX)
> > + *         ext2_inode_info->truncate_mutex
> 
> This is a different lock order to XFS - it puts the i_mmaplock
> inside sb_start_pagefault(), not outside it. This ordering means the
> timestamp updates during the page fault are also under
> ext2_inode_info->dax_sem...

Yep - I was trying not not open code dax_fault() yet again, but it looks like
both XFS and ext4 both open-code dax_fault() and call __dax_fault() directly.

I assume that when we get an analogous lock to i_mmaplock in the ext4 fault
path we'll end up with the same locking order that is found in XFS.

I'll do the same for v2 of this patch and I'll kill the then-unused
dax_fault().

Re: [PATCH 2/2] ext2: Add locking for DAX faults

[Ross Zwisler]

On Mon, Oct 12, 2015 at 10:14:43AM +1100, Dave Chinner wrote:
> [Nit: please send all patches of the series to the same list of
> recipients. Otherwise people with list based filters end up with the
> series spread across different mailboxes and people not subscribed
> to all lists don't get the full series and so are lacking in context
> for proper review. ]

Okay, will do.

> On Fri, Oct 09, 2015 at 04:02:08PM -0600, Ross Zwisler wrote:
<>
> > +/*
> > + * The lock ordering for ext2 DAX fault paths is:
> > + *
> > + * mmap_sem (MM)
> > + *   ext2_inode_info->dax_sem
> > + *     sb_start_pagefault (vfs, freeze - taken in DAX)
> > + *       address_space->i_mmap_rwsem or page_lock (mutually exclusive in DAX)
> > + *         ext2_inode_info->truncate_mutex
> 
> This is a different lock order to XFS - it puts the i_mmaplock
> inside sb_start_pagefault(), not outside it. This ordering means the
> timestamp updates during the page fault are also under
> ext2_inode_info->dax_sem...

Yep - I was trying not not open code dax_fault() yet again, but it looks like
both XFS and ext4 both open-code dax_fault() and call __dax_fault() directly.

I assume that when we get an analogous lock to i_mmaplock in the ext4 fault
path we'll end up with the same locking order that is found in XFS.

I'll do the same for v2 of this patch and I'll kill the then-unused
dax_fault().

Re: [PATCH 2/2] ext2: Add locking for DAX faults

[Ross Zwisler]

On Mon, Oct 12, 2015 at 10:14:43AM +1100, Dave Chinner wrote:
> On Fri, Oct 09, 2015 at 04:02:08PM -0600, Ross Zwisler wrote:
> > Add locking to ensure that DAX faults are isolated from ext2 operations
> > that modify the data blocks allocation for an inode.  This is intended to
> > be analogous to the work being done in XFS by Dave Chinner:
> > 
> > http://www.spinics.net/lists/linux-fsdevel/msg90260.html
> > 
> > Compared with XFS the ext2 case is greatly simplified by the fact that ext2
> > already allocates and zeros new blocks before they are returned as part of
> > ext2_get_block(), so DAX doesn't need to worry about getting unmapped or
> > unwritten buffer heads.
> > 
> > This means that the only work we need to do in ext2 is to isolate the DAX
> > faults from inode block allocation changes.  I believe this just means that
> > we need to isolate the DAX faults from truncate operations.
> 
> Why limit this just to DAX page faults?

Yep, I see that XFS uses the same locking to protect both DAX and non-DAX
faults.  I'll add this protection to non-DAX ext2 faults as well.

One quick question - it looks like that dax_pmd_fault() only grabs the
pagefault lock and updates the file_update_time() if the FAULT_WRITE_FLAG is
set. In xfs_filemap_pfn_mkwrite(), though, these two steps are taken for read
faults as well.  Is this intentional?

Re: [PATCH 2/2] ext2: Add locking for DAX faults

[Ross Zwisler]

On Mon, Oct 12, 2015 at 10:14:43AM +1100, Dave Chinner wrote:
> On Fri, Oct 09, 2015 at 04:02:08PM -0600, Ross Zwisler wrote:
> > Add locking to ensure that DAX faults are isolated from ext2 operations
> > that modify the data blocks allocation for an inode.  This is intended to
> > be analogous to the work being done in XFS by Dave Chinner:
> > 
> > http://www.spinics.net/lists/linux-fsdevel/msg90260.html
> > 
> > Compared with XFS the ext2 case is greatly simplified by the fact that ext2
> > already allocates and zeros new blocks before they are returned as part of
> > ext2_get_block(), so DAX doesn't need to worry about getting unmapped or
> > unwritten buffer heads.
> > 
> > This means that the only work we need to do in ext2 is to isolate the DAX
> > faults from inode block allocation changes.  I believe this just means that
> > we need to isolate the DAX faults from truncate operations.
> 
> Why limit this just to DAX page faults?

Yep, I see that XFS uses the same locking to protect both DAX and non-DAX
faults.  I'll add this protection to non-DAX ext2 faults as well.

One quick question - it looks like that dax_pmd_fault() only grabs the
pagefault lock and updates the file_update_time() if the FAULT_WRITE_FLAG is
set. In xfs_filemap_pfn_mkwrite(), though, these two steps are taken for read
faults as well.  Is this intentional?

Re: [PATCH 2/2] ext2: Add locking for DAX faults

[Dave Chinner]

On Mon, Oct 12, 2015 at 11:21:56AM -0600, Ross Zwisler wrote:
> On Mon, Oct 12, 2015 at 10:14:43AM +1100, Dave Chinner wrote:
> > On Fri, Oct 09, 2015 at 04:02:08PM -0600, Ross Zwisler wrote:
> <>
> > > +/*
> > > + * The lock ordering for ext2 DAX fault paths is:
> > > + *
> > > + * mmap_sem (MM)
> > > + *   ext2_inode_info->dax_sem
> > > + *     sb_start_pagefault (vfs, freeze - taken in DAX)
> > > + *       address_space->i_mmap_rwsem or page_lock (mutually exclusive in DAX)
> > > + *         ext2_inode_info->truncate_mutex
> > 
> > This is a different lock order to XFS - it puts the i_mmaplock
> > inside sb_start_pagefault(), not outside it. This ordering means the
> > timestamp updates during the page fault are also under
> > ext2_inode_info->dax_sem...
> 
> Yep - I was trying not not open code dax_fault() yet again, but it looks like
> both XFS and ext4 both open-code dax_fault() and call __dax_fault() directly.
> 
> I assume that when we get an analogous lock to i_mmaplock in the ext4 fault
> path we'll end up with the same locking order that is found in XFS.
> 
> I'll do the same for v2 of this patch and I'll kill the then-unused
> dax_fault().

Great!

I'd suggest that you also then rename __dax_fault() to dax_fault()
in that case, and also document the requirements for callers in
terms of sb_start_pagefault() and timestamp updates...

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

Re: [PATCH 2/2] ext2: Add locking for DAX faults

[Dave Chinner]

On Mon, Oct 12, 2015 at 11:21:56AM -0600, Ross Zwisler wrote:
> On Mon, Oct 12, 2015 at 10:14:43AM +1100, Dave Chinner wrote:
> > On Fri, Oct 09, 2015 at 04:02:08PM -0600, Ross Zwisler wrote:
> <>
> > > +/*
> > > + * The lock ordering for ext2 DAX fault paths is:
> > > + *
> > > + * mmap_sem (MM)
> > > + *   ext2_inode_info->dax_sem
> > > + *     sb_start_pagefault (vfs, freeze - taken in DAX)
> > > + *       address_space->i_mmap_rwsem or page_lock (mutually exclusive in DAX)
> > > + *         ext2_inode_info->truncate_mutex
> > 
> > This is a different lock order to XFS - it puts the i_mmaplock
> > inside sb_start_pagefault(), not outside it. This ordering means the
> > timestamp updates during the page fault are also under
> > ext2_inode_info->dax_sem...
> 
> Yep - I was trying not not open code dax_fault() yet again, but it looks like
> both XFS and ext4 both open-code dax_fault() and call __dax_fault() directly.
> 
> I assume that when we get an analogous lock to i_mmaplock in the ext4 fault
> path we'll end up with the same locking order that is found in XFS.
> 
> I'll do the same for v2 of this patch and I'll kill the then-unused
> dax_fault().

Great!

I'd suggest that you also then rename __dax_fault() to dax_fault()
in that case, and also document the requirements for callers in
terms of sb_start_pagefault() and timestamp updates...

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

Re: [PATCH 2/2] ext2: Add locking for DAX faults

[Dave Chinner]

On Mon, Oct 12, 2015 at 03:41:35PM -0600, Ross Zwisler wrote:
> On Mon, Oct 12, 2015 at 10:14:43AM +1100, Dave Chinner wrote:
> > On Fri, Oct 09, 2015 at 04:02:08PM -0600, Ross Zwisler wrote:
> > > Add locking to ensure that DAX faults are isolated from ext2 operations
> > > that modify the data blocks allocation for an inode.  This is intended to
> > > be analogous to the work being done in XFS by Dave Chinner:
> > > 
> > > http://www.spinics.net/lists/linux-fsdevel/msg90260.html
> > > 
> > > Compared with XFS the ext2 case is greatly simplified by the fact that ext2
> > > already allocates and zeros new blocks before they are returned as part of
> > > ext2_get_block(), so DAX doesn't need to worry about getting unmapped or
> > > unwritten buffer heads.
> > > 
> > > This means that the only work we need to do in ext2 is to isolate the DAX
> > > faults from inode block allocation changes.  I believe this just means that
> > > we need to isolate the DAX faults from truncate operations.
> > 
> > Why limit this just to DAX page faults?
> 
> Yep, I see that XFS uses the same locking to protect both DAX and non-DAX
> faults.  I'll add this protection to non-DAX ext2 faults as well.
> 
> One quick question - it looks like that dax_pmd_fault() only grabs the
> pagefault lock and updates the file_update_time() if the FAULT_WRITE_FLAG is
> set. In xfs_filemap_pfn_mkwrite(), though, these two steps are taken for read
> faults as well.  Is this intentional?

xfs_filemap_pfn_mkwrite() should not be called for read faults.
We've already had to have a fault that maps the page to pfn for us
to get a pfn based fault, and hence that code is correct.

Or are you talking about xfs_filemap_pmd_fault()? In which case, I
refer you to the commit log and it should be obvious that it was
committed without me even looking at it.  I have another patch in my
current series for 4.4 that will fix this.

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

Re: [PATCH 2/2] ext2: Add locking for DAX faults

[Dave Chinner]

On Mon, Oct 12, 2015 at 03:41:35PM -0600, Ross Zwisler wrote:
> On Mon, Oct 12, 2015 at 10:14:43AM +1100, Dave Chinner wrote:
> > On Fri, Oct 09, 2015 at 04:02:08PM -0600, Ross Zwisler wrote:
> > > Add locking to ensure that DAX faults are isolated from ext2 operations
> > > that modify the data blocks allocation for an inode.  This is intended to
> > > be analogous to the work being done in XFS by Dave Chinner:
> > > 
> > > http://www.spinics.net/lists/linux-fsdevel/msg90260.html
> > > 
> > > Compared with XFS the ext2 case is greatly simplified by the fact that ext2
> > > already allocates and zeros new blocks before they are returned as part of
> > > ext2_get_block(), so DAX doesn't need to worry about getting unmapped or
> > > unwritten buffer heads.
> > > 
> > > This means that the only work we need to do in ext2 is to isolate the DAX
> > > faults from inode block allocation changes.  I believe this just means that
> > > we need to isolate the DAX faults from truncate operations.
> > 
> > Why limit this just to DAX page faults?
> 
> Yep, I see that XFS uses the same locking to protect both DAX and non-DAX
> faults.  I'll add this protection to non-DAX ext2 faults as well.
> 
> One quick question - it looks like that dax_pmd_fault() only grabs the
> pagefault lock and updates the file_update_time() if the FAULT_WRITE_FLAG is
> set. In xfs_filemap_pfn_mkwrite(), though, these two steps are taken for read
> faults as well.  Is this intentional?

xfs_filemap_pfn_mkwrite() should not be called for read faults.
We've already had to have a fault that maps the page to pfn for us
to get a pfn based fault, and hence that code is correct.

Or are you talking about xfs_filemap_pmd_fault()? In which case, I
refer you to the commit log and it should be obvious that it was
committed without me even looking at it.  I have another patch in my
current series for 4.4 that will fix this.

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

Re: [PATCH 2/2] ext2: Add locking for DAX faults

[Eric Curtin]

On 13 October 2015 at 00:24, Dave Chinner <david@fromorbit.com> wrote:
>
> On Mon, Oct 12, 2015 at 03:41:35PM -0600, Ross Zwisler wrote:
> > On Mon, Oct 12, 2015 at 10:14:43AM +1100, Dave Chinner wrote:
> > > On Fri, Oct 09, 2015 at 04:02:08PM -0600, Ross Zwisler wrote:
> > > > Add locking to ensure that DAX faults are isolated from ext2 operations
> > > > that modify the data blocks allocation for an inode.  This is intended to
> > > > be analogous to the work being done in XFS by Dave Chinner:
> > > >
> > > > http://www.spinics.net/lists/linux-fsdevel/msg90260.html
> > > >
> > > > Compared with XFS the ext2 case is greatly simplified by the fact that ext2
> > > > already allocates and zeros new blocks before they are returned as part of
> > > > ext2_get_block(), so DAX doesn't need to worry about getting unmapped or
> > > > unwritten buffer heads.
> > > >
> > > > This means that the only work we need to do in ext2 is to isolate the DAX
> > > > faults from inode block allocation changes.  I believe this just means that
> > > > we need to isolate the DAX faults from truncate operations.
> > >
> > > Why limit this just to DAX page faults?
> >
> > Yep, I see that XFS uses the same locking to protect both DAX and non-DAX
> > faults.  I'll add this protection to non-DAX ext2 faults as well.
> >
> > One quick question - it looks like that dax_pmd_fault() only grabs the
> > pagefault lock and updates the file_update_time() if the FAULT_WRITE_FLAG is
> > set. In xfs_filemap_pfn_mkwrite(), though, these two steps are taken for read
> > faults as well.  Is this intentional?
>
> xfs_filemap_pfn_mkwrite() should not be called for read faults.
> We've already had to have a fault that maps the page to pfn for us
> to get a pfn based fault, and hence that code is correct.
>
> Or are you talking about xfs_filemap_pmd_fault()? In which case, I
> refer you to the commit log and it should be obvious that it was
> committed without me even looking at it.  I have another patch in my
> current series for 4.4 that will fix this.
>
> Cheers,
>
> Dave.
> --
> Dave Chinner
> david@fromorbit.com
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/

Hi Ross,

For all those int ret declarations. Why not declare and initialize all
on the same line?

Regards,

Eric

Re: [PATCH 2/2] ext2: Add locking for DAX faults

[Eric Curtin]

On 13 October 2015 at 00:24, Dave Chinner <david@fromorbit.com> wrote:
>
> On Mon, Oct 12, 2015 at 03:41:35PM -0600, Ross Zwisler wrote:
> > On Mon, Oct 12, 2015 at 10:14:43AM +1100, Dave Chinner wrote:
> > > On Fri, Oct 09, 2015 at 04:02:08PM -0600, Ross Zwisler wrote:
> > > > Add locking to ensure that DAX faults are isolated from ext2 operations
> > > > that modify the data blocks allocation for an inode.  This is intended to
> > > > be analogous to the work being done in XFS by Dave Chinner:
> > > >
> > > > http://www.spinics.net/lists/linux-fsdevel/msg90260.html
> > > >
> > > > Compared with XFS the ext2 case is greatly simplified by the fact that ext2
> > > > already allocates and zeros new blocks before they are returned as part of
> > > > ext2_get_block(), so DAX doesn't need to worry about getting unmapped or
> > > > unwritten buffer heads.
> > > >
> > > > This means that the only work we need to do in ext2 is to isolate the DAX
> > > > faults from inode block allocation changes.  I believe this just means that
> > > > we need to isolate the DAX faults from truncate operations.
> > >
> > > Why limit this just to DAX page faults?
> >
> > Yep, I see that XFS uses the same locking to protect both DAX and non-DAX
> > faults.  I'll add this protection to non-DAX ext2 faults as well.
> >
> > One quick question - it looks like that dax_pmd_fault() only grabs the
> > pagefault lock and updates the file_update_time() if the FAULT_WRITE_FLAG is
> > set. In xfs_filemap_pfn_mkwrite(), though, these two steps are taken for read
> > faults as well.  Is this intentional?
>
> xfs_filemap_pfn_mkwrite() should not be called for read faults.
> We've already had to have a fault that maps the page to pfn for us
> to get a pfn based fault, and hence that code is correct.
>
> Or are you talking about xfs_filemap_pmd_fault()? In which case, I
> refer you to the commit log and it should be obvious that it was
> committed without me even looking at it.  I have another patch in my
> current series for 4.4 that will fix this.
>
> Cheers,
>
> Dave.
> --
> Dave Chinner
> david@fromorbit.com
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/

Hi Ross,

For all those int ret declarations. Why not declare and initialize all
on the same line?

Regards,

Eric

Re: [PATCH 2/2] ext2: Add locking for DAX faults

[Jan Kara]

On Mon 12-10-15 15:41:35, Ross Zwisler wrote:
> On Mon, Oct 12, 2015 at 10:14:43AM +1100, Dave Chinner wrote:
> > On Fri, Oct 09, 2015 at 04:02:08PM -0600, Ross Zwisler wrote:
> > > Add locking to ensure that DAX faults are isolated from ext2 operations
> > > that modify the data blocks allocation for an inode.  This is intended to
> > > be analogous to the work being done in XFS by Dave Chinner:
> > > 
> > > http://www.spinics.net/lists/linux-fsdevel/msg90260.html
> > > 
> > > Compared with XFS the ext2 case is greatly simplified by the fact that ext2
> > > already allocates and zeros new blocks before they are returned as part of
> > > ext2_get_block(), so DAX doesn't need to worry about getting unmapped or
> > > unwritten buffer heads.
> > > 
> > > This means that the only work we need to do in ext2 is to isolate the DAX
> > > faults from inode block allocation changes.  I believe this just means that
> > > we need to isolate the DAX faults from truncate operations.
> > 
> > Why limit this just to DAX page faults?
> 
> Yep, I see that XFS uses the same locking to protect both DAX and non-DAX
> faults.  I'll add this protection to non-DAX ext2 faults as well.

Actually, since ext2 driver doesn't support punch hole, there is no need
for additional locking in non-DAX paths. So we can save some space in inode
and locking for that common case. So I'd prefer if we didn't add
unnecessary locking in those paths and just document that for non-DAX
faults using page lock and i_size check is enough. After all the main
usecase of ext2 driver these days is for people with tiny devices...

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

Re: [PATCH 2/2] ext2: Add locking for DAX faults

[Jan Kara]

On Mon 12-10-15 15:41:35, Ross Zwisler wrote:
> On Mon, Oct 12, 2015 at 10:14:43AM +1100, Dave Chinner wrote:
> > On Fri, Oct 09, 2015 at 04:02:08PM -0600, Ross Zwisler wrote:
> > > Add locking to ensure that DAX faults are isolated from ext2 operations
> > > that modify the data blocks allocation for an inode.  This is intended to
> > > be analogous to the work being done in XFS by Dave Chinner:
> > > 
> > > http://www.spinics.net/lists/linux-fsdevel/msg90260.html
> > > 
> > > Compared with XFS the ext2 case is greatly simplified by the fact that ext2
> > > already allocates and zeros new blocks before they are returned as part of
> > > ext2_get_block(), so DAX doesn't need to worry about getting unmapped or
> > > unwritten buffer heads.
> > > 
> > > This means that the only work we need to do in ext2 is to isolate the DAX
> > > faults from inode block allocation changes.  I believe this just means that
> > > we need to isolate the DAX faults from truncate operations.
> > 
> > Why limit this just to DAX page faults?
> 
> Yep, I see that XFS uses the same locking to protect both DAX and non-DAX
> faults.  I'll add this protection to non-DAX ext2 faults as well.

Actually, since ext2 driver doesn't support punch hole, there is no need
for additional locking in non-DAX paths. So we can save some space in inode
and locking for that common case. So I'd prefer if we didn't add
unnecessary locking in those paths and just document that for non-DAX
faults using page lock and i_size check is enough. After all the main
usecase of ext2 driver these days is for people with tiny devices...

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

Re: [PATCH 2/2] ext2: Add locking for DAX faults

[Ross Zwisler]

On Tue, Oct 13, 2015 at 10:24:28AM +1100, Dave Chinner wrote:
> On Mon, Oct 12, 2015 at 03:41:35PM -0600, Ross Zwisler wrote:
> > On Mon, Oct 12, 2015 at 10:14:43AM +1100, Dave Chinner wrote:
> > > On Fri, Oct 09, 2015 at 04:02:08PM -0600, Ross Zwisler wrote:
> > > > Add locking to ensure that DAX faults are isolated from ext2 operations
> > > > that modify the data blocks allocation for an inode.  This is intended to
> > > > be analogous to the work being done in XFS by Dave Chinner:
> > > > 
> > > > http://www.spinics.net/lists/linux-fsdevel/msg90260.html
> > > > 
> > > > Compared with XFS the ext2 case is greatly simplified by the fact that ext2
> > > > already allocates and zeros new blocks before they are returned as part of
> > > > ext2_get_block(), so DAX doesn't need to worry about getting unmapped or
> > > > unwritten buffer heads.
> > > > 
> > > > This means that the only work we need to do in ext2 is to isolate the DAX
> > > > faults from inode block allocation changes.  I believe this just means that
> > > > we need to isolate the DAX faults from truncate operations.
> > > 
> > > Why limit this just to DAX page faults?
> > 
> > Yep, I see that XFS uses the same locking to protect both DAX and non-DAX
> > faults.  I'll add this protection to non-DAX ext2 faults as well.
> > 
> > One quick question - it looks like that dax_pmd_fault() only grabs the
> > pagefault lock and updates the file_update_time() if the FAULT_WRITE_FLAG is
> > set. In xfs_filemap_pfn_mkwrite(), though, these two steps are taken for read
> > faults as well.  Is this intentional?
> 
> xfs_filemap_pfn_mkwrite() should not be called for read faults.
> We've already had to have a fault that maps the page to pfn for us
> to get a pfn based fault, and hence that code is correct.
> 
> Or are you talking about xfs_filemap_pmd_fault()? In which case, I
> refer you to the commit log and it should be obvious that it was
> committed without me even looking at it.  I have another patch in my
> current series for 4.4 that will fix this.

Yep, sorry, I meant xfs_filemap_pmd_fault().  Thanks, I was just making sure the
pattern I'm following in ext2 where I'm only locking the pagefault lock &
updating the file_update_time() on writes was correct.

Re: [PATCH 2/2] ext2: Add locking for DAX faults

[Ross Zwisler]

On Tue, Oct 13, 2015 at 10:24:28AM +1100, Dave Chinner wrote:
> On Mon, Oct 12, 2015 at 03:41:35PM -0600, Ross Zwisler wrote:
> > On Mon, Oct 12, 2015 at 10:14:43AM +1100, Dave Chinner wrote:
> > > On Fri, Oct 09, 2015 at 04:02:08PM -0600, Ross Zwisler wrote:
> > > > Add locking to ensure that DAX faults are isolated from ext2 operations
> > > > that modify the data blocks allocation for an inode.  This is intended to
> > > > be analogous to the work being done in XFS by Dave Chinner:
> > > > 
> > > > http://www.spinics.net/lists/linux-fsdevel/msg90260.html
> > > > 
> > > > Compared with XFS the ext2 case is greatly simplified by the fact that ext2
> > > > already allocates and zeros new blocks before they are returned as part of
> > > > ext2_get_block(), so DAX doesn't need to worry about getting unmapped or
> > > > unwritten buffer heads.
> > > > 
> > > > This means that the only work we need to do in ext2 is to isolate the DAX
> > > > faults from inode block allocation changes.  I believe this just means that
> > > > we need to isolate the DAX faults from truncate operations.
> > > 
> > > Why limit this just to DAX page faults?
> > 
> > Yep, I see that XFS uses the same locking to protect both DAX and non-DAX
> > faults.  I'll add this protection to non-DAX ext2 faults as well.
> > 
> > One quick question - it looks like that dax_pmd_fault() only grabs the
> > pagefault lock and updates the file_update_time() if the FAULT_WRITE_FLAG is
> > set. In xfs_filemap_pfn_mkwrite(), though, these two steps are taken for read
> > faults as well.  Is this intentional?
> 
> xfs_filemap_pfn_mkwrite() should not be called for read faults.
> We've already had to have a fault that maps the page to pfn for us
> to get a pfn based fault, and hence that code is correct.
> 
> Or are you talking about xfs_filemap_pmd_fault()? In which case, I
> refer you to the commit log and it should be obvious that it was
> committed without me even looking at it.  I have another patch in my
> current series for 4.4 that will fix this.

Yep, sorry, I meant xfs_filemap_pmd_fault().  Thanks, I was just making sure the
pattern I'm following in ext2 where I'm only locking the pagefault lock &
updating the file_update_time() on writes was correct.

Re: [PATCH 2/2] ext2: Add locking for DAX faults

[Ross Zwisler]

On Tue, Oct 13, 2015 at 10:07:12AM +0200, Jan Kara wrote:
> On Mon 12-10-15 15:41:35, Ross Zwisler wrote:
> > On Mon, Oct 12, 2015 at 10:14:43AM +1100, Dave Chinner wrote:
> > > On Fri, Oct 09, 2015 at 04:02:08PM -0600, Ross Zwisler wrote:
> > > > Add locking to ensure that DAX faults are isolated from ext2 operations
> > > > that modify the data blocks allocation for an inode.  This is intended to
> > > > be analogous to the work being done in XFS by Dave Chinner:
> > > > 
> > > > http://www.spinics.net/lists/linux-fsdevel/msg90260.html
> > > > 
> > > > Compared with XFS the ext2 case is greatly simplified by the fact that ext2
> > > > already allocates and zeros new blocks before they are returned as part of
> > > > ext2_get_block(), so DAX doesn't need to worry about getting unmapped or
> > > > unwritten buffer heads.
> > > > 
> > > > This means that the only work we need to do in ext2 is to isolate the DAX
> > > > faults from inode block allocation changes.  I believe this just means that
> > > > we need to isolate the DAX faults from truncate operations.
> > > 
> > > Why limit this just to DAX page faults?
> > 
> > Yep, I see that XFS uses the same locking to protect both DAX and non-DAX
> > faults.  I'll add this protection to non-DAX ext2 faults as well.
> 
> Actually, since ext2 driver doesn't support punch hole, there is no need
> for additional locking in non-DAX paths. So we can save some space in inode
> and locking for that common case. So I'd prefer if we didn't add
> unnecessary locking in those paths and just document that for non-DAX
> faults using page lock and i_size check is enough. After all the main
> usecase of ext2 driver these days is for people with tiny devices...

Based on this comment I'm assuming you'd like the definition of dax_sem in
struct ext2_inode_info to be conditional like ext2_inode_info->xattr_sem,
correct?

Re: [PATCH 2/2] ext2: Add locking for DAX faults

[Ross Zwisler]

On Tue, Oct 13, 2015 at 10:07:12AM +0200, Jan Kara wrote:
> On Mon 12-10-15 15:41:35, Ross Zwisler wrote:
> > On Mon, Oct 12, 2015 at 10:14:43AM +1100, Dave Chinner wrote:
> > > On Fri, Oct 09, 2015 at 04:02:08PM -0600, Ross Zwisler wrote:
> > > > Add locking to ensure that DAX faults are isolated from ext2 operations
> > > > that modify the data blocks allocation for an inode.  This is intended to
> > > > be analogous to the work being done in XFS by Dave Chinner:
> > > > 
> > > > http://www.spinics.net/lists/linux-fsdevel/msg90260.html
> > > > 
> > > > Compared with XFS the ext2 case is greatly simplified by the fact that ext2
> > > > already allocates and zeros new blocks before they are returned as part of
> > > > ext2_get_block(), so DAX doesn't need to worry about getting unmapped or
> > > > unwritten buffer heads.
> > > > 
> > > > This means that the only work we need to do in ext2 is to isolate the DAX
> > > > faults from inode block allocation changes.  I believe this just means that
> > > > we need to isolate the DAX faults from truncate operations.
> > > 
> > > Why limit this just to DAX page faults?
> > 
> > Yep, I see that XFS uses the same locking to protect both DAX and non-DAX
> > faults.  I'll add this protection to non-DAX ext2 faults as well.
> 
> Actually, since ext2 driver doesn't support punch hole, there is no need
> for additional locking in non-DAX paths. So we can save some space in inode
> and locking for that common case. So I'd prefer if we didn't add
> unnecessary locking in those paths and just document that for non-DAX
> faults using page lock and i_size check is enough. After all the main
> usecase of ext2 driver these days is for people with tiny devices...

Based on this comment I'm assuming you'd like the definition of dax_sem in
struct ext2_inode_info to be conditional like ext2_inode_info->xattr_sem,
correct?

Re: [PATCH 2/2] ext2: Add locking for DAX faults

[Jan Kara]

On Tue 13-10-15 11:33:20, Ross Zwisler wrote:
> On Tue, Oct 13, 2015 at 10:07:12AM +0200, Jan Kara wrote:
> > On Mon 12-10-15 15:41:35, Ross Zwisler wrote:
> > > On Mon, Oct 12, 2015 at 10:14:43AM +1100, Dave Chinner wrote:
> > > > On Fri, Oct 09, 2015 at 04:02:08PM -0600, Ross Zwisler wrote:
> > > > > Add locking to ensure that DAX faults are isolated from ext2 operations
> > > > > that modify the data blocks allocation for an inode.  This is intended to
> > > > > be analogous to the work being done in XFS by Dave Chinner:
> > > > > 
> > > > > http://www.spinics.net/lists/linux-fsdevel/msg90260.html
> > > > > 
> > > > > Compared with XFS the ext2 case is greatly simplified by the fact that ext2
> > > > > already allocates and zeros new blocks before they are returned as part of
> > > > > ext2_get_block(), so DAX doesn't need to worry about getting unmapped or
> > > > > unwritten buffer heads.
> > > > > 
> > > > > This means that the only work we need to do in ext2 is to isolate the DAX
> > > > > faults from inode block allocation changes.  I believe this just means that
> > > > > we need to isolate the DAX faults from truncate operations.
> > > > 
> > > > Why limit this just to DAX page faults?
> > > 
> > > Yep, I see that XFS uses the same locking to protect both DAX and non-DAX
> > > faults.  I'll add this protection to non-DAX ext2 faults as well.
> > 
> > Actually, since ext2 driver doesn't support punch hole, there is no need
> > for additional locking in non-DAX paths. So we can save some space in inode
> > and locking for that common case. So I'd prefer if we didn't add
> > unnecessary locking in those paths and just document that for non-DAX
> > faults using page lock and i_size check is enough. After all the main
> > usecase of ext2 driver these days is for people with tiny devices...
> 
> Based on this comment I'm assuming you'd like the definition of dax_sem in
> struct ext2_inode_info to be conditional like ext2_inode_info->xattr_sem,
> correct?

Yes, please. Thanks!

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

Re: [PATCH 2/2] ext2: Add locking for DAX faults

[Jan Kara]

On Tue 13-10-15 11:33:20, Ross Zwisler wrote:
> On Tue, Oct 13, 2015 at 10:07:12AM +0200, Jan Kara wrote:
> > On Mon 12-10-15 15:41:35, Ross Zwisler wrote:
> > > On Mon, Oct 12, 2015 at 10:14:43AM +1100, Dave Chinner wrote:
> > > > On Fri, Oct 09, 2015 at 04:02:08PM -0600, Ross Zwisler wrote:
> > > > > Add locking to ensure that DAX faults are isolated from ext2 operations
> > > > > that modify the data blocks allocation for an inode.  This is intended to
> > > > > be analogous to the work being done in XFS by Dave Chinner:
> > > > > 
> > > > > http://www.spinics.net/lists/linux-fsdevel/msg90260.html
> > > > > 
> > > > > Compared with XFS the ext2 case is greatly simplified by the fact that ext2
> > > > > already allocates and zeros new blocks before they are returned as part of
> > > > > ext2_get_block(), so DAX doesn't need to worry about getting unmapped or
> > > > > unwritten buffer heads.
> > > > > 
> > > > > This means that the only work we need to do in ext2 is to isolate the DAX
> > > > > faults from inode block allocation changes.  I believe this just means that
> > > > > we need to isolate the DAX faults from truncate operations.
> > > > 
> > > > Why limit this just to DAX page faults?
> > > 
> > > Yep, I see that XFS uses the same locking to protect both DAX and non-DAX
> > > faults.  I'll add this protection to non-DAX ext2 faults as well.
> > 
> > Actually, since ext2 driver doesn't support punch hole, there is no need
> > for additional locking in non-DAX paths. So we can save some space in inode
> > and locking for that common case. So I'd prefer if we didn't add
> > unnecessary locking in those paths and just document that for non-DAX
> > faults using page lock and i_size check is enough. After all the main
> > usecase of ext2 driver these days is for people with tiny devices...
> 
> Based on this comment I'm assuming you'd like the definition of dax_sem in
> struct ext2_inode_info to be conditional like ext2_inode_info->xattr_sem,
> correct?

Yes, please. Thanks!

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

Re: [PATCH 2/2] ext2: Add locking for DAX faults

[Ross Zwisler]

On Tue, Oct 13, 2015 at 12:35:44AM +0100, Eric Curtin wrote:
> Hi Ross,
> 
> For all those int ret declarations. Why not declare and initialize all
> on the same line?
> 
> Regards,
> Eric

Because the return value 'ret' is set unconditionally later in the function as
part of a call into the DAX code.  This call needs to be made after we've done
appropriate locking, though, which really is the point of the ext2 wrapper for
the DAX function.  Setting it to some random value and then unconditionally
overwriting it later in the function is a bad pattern because a) the initial
value is meaningless and b) it can hide coding errors where you are no longer
setting the variable to something meaningful.  The "warning: ‘ret’ is used
uninitialized in this function" warnings would otherwise warn you that you've
made a mistake.

Re: [PATCH 2/2] ext2: Add locking for DAX faults

[Ross Zwisler]

On Tue, Oct 13, 2015 at 12:35:44AM +0100, Eric Curtin wrote:
> Hi Ross,
> 
> For all those int ret declarations. Why not declare and initialize all
> on the same line?
> 
> Regards,
> Eric

Because the return value 'ret' is set unconditionally later in the function as
part of a call into the DAX code.  This call needs to be made after we've done
appropriate locking, though, which really is the point of the ext2 wrapper for
the DAX function.  Setting it to some random value and then unconditionally
overwriting it later in the function is a bad pattern because a) the initial
value is meaningless and b) it can hide coding errors where you are no longer
setting the variable to something meaningful.  The "warning: ‘ret’ is used
uninitialized in this function" warnings would otherwise warn you that you've
made a mistake.