[PATCH 2/9] xfs: always use DAX if mount option is used

[PATCH 2/9] xfs: always use DAX if mount option is used

[Ross Zwisler]

The current code has an issue where the user can't reliably tell whether or
not DAX is being used to service page faults and I/O when the DAX mount
option is used.  In this case each inode within the mounted filesystem
starts with S_DAX set due to the mount option, but it can be cleared if
someone touches the individual inode flag.

For example:

  # mount | grep dax
  /dev/pmem0 on /mnt type xfs
  (rw,relatime,seclabel,attr2,dax,inode64,sunit=4096,swidth=4096,noquota)

  # touch /mnt/a /mnt/b   # both files currently use DAX

  # xfs_io -c "lsattr" /mnt/*  # neither has the DAX inode option set
  ----------e----- /mnt/a
  ----------e----- /mnt/b

  # xfs_io -c "chattr -x" /mnt/a  # this clears S_DAX for /mnt/a

  # xfs_io -c "lsattr" /mnt/*
  ----------e----- /mnt/a
  ----------e----- /mnt/b

We end up with both /mnt/a and /mnt/b looking identical from the point of
view of the mount option and from lsattr, but one is using DAX and the
other is not.

Fix this by always doing DAX I/O when either the mount option is set or
when the DAX inode flag is set.  This means that DAX will always be used
for all inodes on a filesystem mounted with -o dax, making the usage
reliable and detectable.

Signed-off-by: Ross Zwisler <ross.zwisler@linux.intel.com>
CC: stable@vger.kernel.org
---
 fs/xfs/xfs_ioctl.c | 11 +++++++++--
 1 file changed, 9 insertions(+), 2 deletions(-)

diff --git a/fs/xfs/xfs_ioctl.c b/fs/xfs/xfs_ioctl.c
index 9c0c7a9..8155ddc 100644
--- a/fs/xfs/xfs_ioctl.c
+++ b/fs/xfs/xfs_ioctl.c
@@ -1008,7 +1008,7 @@ xfs_diflags_to_linux(
 		inode->i_flags |= S_NOATIME;
 	else
 		inode->i_flags &= ~S_NOATIME;
-	if (xflags & FS_XFLAG_DAX)
+	if ((xflags & FS_XFLAG_DAX) || (ip->i_mount->m_flags & XFS_MOUNT_DAX))
 		inode->i_flags |= S_DAX;
 	else
 		inode->i_flags &= ~S_DAX;
@@ -1091,7 +1091,14 @@ xfs_ioctl_setattr_dax_invalidate(
 			return -EINVAL;
 	}
 
-	/* If the DAX state is not changing, we have nothing to do here. */
+	/*
+	 * If the DAX state is not changing, we have nothing to do here.  If
+	 * the DAX mount option was used we will update the DAX inode flag as
+	 * the user requested but we will continue to use DAX for I/O and page
+	 * faults regardless of how the inode flag is set.
+	 */
+	if (ip->i_mount->m_flags & XFS_MOUNT_DAX)
+		return 0;
 	if ((fa->fsx_xflags & FS_XFLAG_DAX) && IS_DAX(inode))
 		return 0;
 	if (!(fa->fsx_xflags & FS_XFLAG_DAX) && !IS_DAX(inode))
-- 
2.9.5

[PATCH 3/9] xfs: validate bdev support for DAX inode flag

[Ross Zwisler]

Currently only the blocksize is checked, but we should really be calling
bdev_dax_supported() which also tests to make sure we can get a
struct dax_device and that the dax_direct_access() path is working.

This is the same check that we do for the "-o dax" mount option in
xfs_fs_fill_super().

Signed-off-by: Ross Zwisler <ross.zwisler@linux.intel.com>
CC: stable@vger.kernel.org
---
 fs/xfs/xfs_ioctl.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/fs/xfs/xfs_ioctl.c b/fs/xfs/xfs_ioctl.c
index 8155ddc..dac195d 100644
--- a/fs/xfs/xfs_ioctl.c
+++ b/fs/xfs/xfs_ioctl.c
@@ -1075,6 +1075,7 @@ xfs_ioctl_setattr_dax_invalidate(
 	int			*join_flags)
 {
 	struct inode		*inode = VFS_I(ip);
+	struct super_block	*sb = inode->i_sb;
 	int			error;
 
 	*join_flags = 0;
@@ -1087,7 +1088,7 @@ xfs_ioctl_setattr_dax_invalidate(
 	if (fa->fsx_xflags & FS_XFLAG_DAX) {
 		if (!(S_ISREG(inode->i_mode) || S_ISDIR(inode->i_mode)))
 			return -EINVAL;
-		if (ip->i_mount->m_sb.sb_blocksize != PAGE_SIZE)
+		if (bdev_dax_supported(sb, sb->s_blocksize) < 0)
 			return -EINVAL;
 	}
 
-- 
2.9.5

[PATCH 5/9] ext4: ext4_change_inode_journal_flag error handling

[Ross Zwisler]

Rework the error handling in ext4_change_inode_journal_flag() so that
multiple paths can re-use portions of the same cleanup code via gotos
instead of each path doing their own cleanup.  This will benefit later
patches that add more paths to this function that must be unwound on error.

Signed-off-by: Ross Zwisler <ross.zwisler@linux.intel.com>
CC: stable@vger.kernel.org
---
 fs/ext4/inode.c | 24 +++++++++++++-----------
 1 file changed, 13 insertions(+), 11 deletions(-)

diff --git a/fs/ext4/inode.c b/fs/ext4/inode.c
index 864fb94..d218991 100644
--- a/fs/ext4/inode.c
+++ b/fs/ext4/inode.c
@@ -5953,11 +5953,8 @@ int ext4_change_inode_journal_flag(struct inode *inode, int val)
 	if (val) {
 		down_write(&EXT4_I(inode)->i_mmap_sem);
 		err = filemap_write_and_wait(inode->i_mapping);
-		if (err < 0) {
-			up_write(&EXT4_I(inode)->i_mmap_sem);
-			ext4_inode_resume_unlocked_dio(inode);
-			return err;
-		}
+		if (err < 0)
+			goto out_unlock;
 	}
 
 	percpu_down_write(&sbi->s_journal_flag_rwsem);
@@ -5975,12 +5972,8 @@ int ext4_change_inode_journal_flag(struct inode *inode, int val)
 		ext4_set_inode_flag(inode, EXT4_INODE_JOURNAL_DATA);
 	else {
 		err = jbd2_journal_flush(journal);
-		if (err < 0) {
-			jbd2_journal_unlock_updates(journal);
-			percpu_up_write(&sbi->s_journal_flag_rwsem);
-			ext4_inode_resume_unlocked_dio(inode);
-			return err;
-		}
+		if (err < 0)
+			goto out_journal_unlock;
 		ext4_clear_inode_flag(inode, EXT4_INODE_JOURNAL_DATA);
 	}
 	ext4_set_aops(inode);
@@ -6009,6 +6002,15 @@ int ext4_change_inode_journal_flag(struct inode *inode, int val)
 	ext4_std_error(inode->i_sb, err);
 
 	return err;
+
+out_journal_unlock:
+	jbd2_journal_unlock_updates(journal);
+	percpu_up_write(&sbi->s_journal_flag_rwsem);
+out_unlock:
+	if (val)
+		up_write(&EXT4_I(inode)->i_mmap_sem);
+	ext4_inode_resume_unlocked_dio(inode);
+	return err;
 }
 
 static int ext4_bh_unmapped(handle_t *handle, struct buffer_head *bh)
-- 
2.9.5

[PATCH 6/9] ext4: safely transition S_DAX on journaling changes

[Ross Zwisler]

The IOCTL path which switches the journaling mode for an inode is currently
unsafe because it doesn't properly do a writeback and invalidation on the
inode.  In XFS, for example, safe transitions of S_DAX are handled by
xfs_ioctl_setattr_dax_invalidate() which locks out page faults and I/O,
does a writeback via filemap_write_and_wait() and an invalidation via
invalidate_inode_pages2().

Without this in place we can see the following kernel warning when we try
and insert a DAX exceptional entry but find that a dirty page cache page is
still in the mapping->radix_tree:

 WARNING: CPU: 4 PID: 1052 at mm/filemap.c:262 __delete_from_page_cache+0x375/0x550
 Modules linked in: dax_pmem nd_pmem device_dax nd_btt nfit libnvdimm
 CPU: 4 PID: 1052 Comm: small Not tainted 4.13.0-rc6-00055-gac26931 #3
 Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 1.9.3-1.fc25 04/01/2014
 task: ffff88020ccd0000 task.stack: ffffc900021d4000
 RIP: 0010:__delete_from_page_cache+0x375/0x550
 RSP: 0000:ffffc900021d7b90 EFLAGS: 00010002
 RAX: 002fffc00001123d RBX: ffffffffffffffff RCX: ffff8801d9440d68
 RDX: 0000000000000000 RSI: ffffffff81fd5b84 RDI: ffffffff81f6f0e5
 RBP: ffffc900021d7be0 R08: 0000000000000000 R09: ffff8801f9938c70
 R10: 0000000000000021 R11: ffff8801f9938c91 R12: ffff8801d9440d70
 R13: ffffea0007fdda80 R14: 0000000000000001 R15: ffff8801d9440d68
 FS:  00007feacc041700(0000) GS:ffff880211800000(0000) knlGS:0000000000000000
 CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
 CR2: 0000000010420000 CR3: 000000020cfd8000 CR4: 00000000000006e0
 Call Trace:
  dax_insert_mapping_entry+0x158/0x2c0
  dax_iomap_fault+0x1020/0x1bb0
  ext4_dax_huge_fault+0xc8/0x160
  ext4_dax_fault+0x10/0x20
  __do_fault+0x20/0x110
  __handle_mm_fault+0x97d/0x1120
  handle_mm_fault+0x188/0x2f0
  __do_page_fault+0x28f/0x590
  trace_do_page_fault+0x58/0x2c0
  do_async_page_fault+0x2c/0x90
  async_page_fault+0x28/0x30

I'm pretty sure we could make a test that shows userspace visible data
corruption as well in this scenario.

Make it safe to change the journaling mode and turn on or off S_DAX by
adding locking to properly lock out page faults (i_mmap_sem) and then doing
the writeback and invalidate.  I/O is already held off because all callers
of ext4_ioctl_setflags() hold the inode lock.

The locking for this new code is complex because of the following:

1) filemap_write_and_wait() eventually calls ext4_writepages(), which
acquires the sbi->s_journal_flag_rwsem.  This lock ranks above the
jbdw_handle which is eventually taken by ext4_journal_start().  This
essentially means that the writeback has to happen outside of the context
of an active journal handle (outside of ext4_journal_start() to
ext4_journal_stop().)

2) To lock out page faults we take a write lock on the ei->i_mmap_sem, and
this lock again ranks above the jbd2_handle taken by ext4_journal_start().
So, as with the writeback code in 1) above we have to take ei->i_mmap_sem
outside of the context of an active journal handle.

Signed-off-by: Ross Zwisler <ross.zwisler@linux.intel.com>
CC: stable@vger.kernel.org
---
Please let me know if my "context of an active journal handle" terminology
makes sense, or if some other phrasing is more common.

I'll work on an xfstest which shows this data corruption.
---
 fs/ext4/inode.c | 25 +++++++++++++++----------
 1 file changed, 15 insertions(+), 10 deletions(-)

diff --git a/fs/ext4/inode.c b/fs/ext4/inode.c
index d218991..facb5ae 100644
--- a/fs/ext4/inode.c
+++ b/fs/ext4/inode.c
@@ -5949,13 +5949,15 @@ int ext4_change_inode_journal_flag(struct inode *inode, int val)
 	 * fallocate or buffered writes in dioread_nolock mode covered by
 	 * dirty data which can be converted only after flushing the dirty
 	 * data (and journalled aops don't know how to handle these cases).
+	 *
+	 * Because the changes to our journaling mode may also modify S_DAX we
+	 * need to hold off I/O and page faults so we can write back any dirty
+	 * data and invalidate all mappings.
 	 */
-	if (val) {
-		down_write(&EXT4_I(inode)->i_mmap_sem);
-		err = filemap_write_and_wait(inode->i_mapping);
-		if (err < 0)
-			goto out_unlock;
-	}
+	down_write(&EXT4_I(inode)->i_mmap_sem);
+	err = filemap_write_and_wait(inode->i_mapping);
+	if (err < 0)
+		goto out_unlock;
 
 	percpu_down_write(&sbi->s_journal_flag_rwsem);
 	jbd2_journal_lock_updates(journal);
@@ -5976,6 +5978,11 @@ int ext4_change_inode_journal_flag(struct inode *inode, int val)
 			goto out_journal_unlock;
 		ext4_clear_inode_flag(inode, EXT4_INODE_JOURNAL_DATA);
 	}
+
+	err = invalidate_inode_pages2(inode->i_mapping);
+	if (err < 0)
+		goto out_journal_unlock;
+
 	ext4_set_aops(inode);
 	/*
 	 * Update inode->i_flags after EXT4_INODE_JOURNAL_DATA was updated.
@@ -5986,8 +5993,7 @@ int ext4_change_inode_journal_flag(struct inode *inode, int val)
 	jbd2_journal_unlock_updates(journal);
 	percpu_up_write(&sbi->s_journal_flag_rwsem);
 
-	if (val)
-		up_write(&EXT4_I(inode)->i_mmap_sem);
+	up_write(&EXT4_I(inode)->i_mmap_sem);
 	ext4_inode_resume_unlocked_dio(inode);
 
 	/* Finally we can mark the inode as dirty. */
@@ -6007,8 +6013,7 @@ int ext4_change_inode_journal_flag(struct inode *inode, int val)
 	jbd2_journal_unlock_updates(journal);
 	percpu_up_write(&sbi->s_journal_flag_rwsem);
 out_unlock:
-	if (val)
-		up_write(&EXT4_I(inode)->i_mmap_sem);
+	up_write(&EXT4_I(inode)->i_mmap_sem);
 	ext4_inode_resume_unlocked_dio(inode);
 	return err;
 }
-- 
2.9.5

Re: [PATCH 6/9] ext4: safely transition S_DAX on journaling changes

[Jan Kara]

On Tue 05-09-17 16:35:38, Ross Zwisler wrote:
> The IOCTL path which switches the journaling mode for an inode is currently
> unsafe because it doesn't properly do a writeback and invalidation on the
> inode.  In XFS, for example, safe transitions of S_DAX are handled by
> xfs_ioctl_setattr_dax_invalidate() which locks out page faults and I/O,
> does a writeback via filemap_write_and_wait() and an invalidation via
> invalidate_inode_pages2().
> 
> Without this in place we can see the following kernel warning when we try
> and insert a DAX exceptional entry but find that a dirty page cache page is
> still in the mapping->radix_tree:
> 
>  WARNING: CPU: 4 PID: 1052 at mm/filemap.c:262 __delete_from_page_cache+0x375/0x550
>  Modules linked in: dax_pmem nd_pmem device_dax nd_btt nfit libnvdimm
>  CPU: 4 PID: 1052 Comm: small Not tainted 4.13.0-rc6-00055-gac26931 #3
>  Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 1.9.3-1.fc25 04/01/2014
>  task: ffff88020ccd0000 task.stack: ffffc900021d4000
>  RIP: 0010:__delete_from_page_cache+0x375/0x550
>  RSP: 0000:ffffc900021d7b90 EFLAGS: 00010002
>  RAX: 002fffc00001123d RBX: ffffffffffffffff RCX: ffff8801d9440d68
>  RDX: 0000000000000000 RSI: ffffffff81fd5b84 RDI: ffffffff81f6f0e5
>  RBP: ffffc900021d7be0 R08: 0000000000000000 R09: ffff8801f9938c70
>  R10: 0000000000000021 R11: ffff8801f9938c91 R12: ffff8801d9440d70
>  R13: ffffea0007fdda80 R14: 0000000000000001 R15: ffff8801d9440d68
>  FS:  00007feacc041700(0000) GS:ffff880211800000(0000) knlGS:0000000000000000
>  CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
>  CR2: 0000000010420000 CR3: 000000020cfd8000 CR4: 00000000000006e0
>  Call Trace:
>   dax_insert_mapping_entry+0x158/0x2c0
>   dax_iomap_fault+0x1020/0x1bb0
>   ext4_dax_huge_fault+0xc8/0x160
>   ext4_dax_fault+0x10/0x20
>   __do_fault+0x20/0x110
>   __handle_mm_fault+0x97d/0x1120
>   handle_mm_fault+0x188/0x2f0
>   __do_page_fault+0x28f/0x590
>   trace_do_page_fault+0x58/0x2c0
>   do_async_page_fault+0x2c/0x90
>   async_page_fault+0x28/0x30
> 
> I'm pretty sure we could make a test that shows userspace visible data
> corruption as well in this scenario.
> 
> Make it safe to change the journaling mode and turn on or off S_DAX by
> adding locking to properly lock out page faults (i_mmap_sem) and then doing
> the writeback and invalidate.  I/O is already held off because all callers
> of ext4_ioctl_setflags() hold the inode lock.

Yeah, this is a good point. It is just that this is not enough as I
discovered in [1]. You also need to tear down & recreate VMAs when changing
DAX flag which is a bit tricky. So for now I think returning EBUSY when
file is mmaped and we'd like to flip DAX flag is the best solution. Hmm?

[1] https://www.spinics.net/lists/linux-xfs/msg09859.html

> The locking for this new code is complex because of the following:
> 
> 1) filemap_write_and_wait() eventually calls ext4_writepages(), which
> acquires the sbi->s_journal_flag_rwsem.  This lock ranks above the
> jbdw_handle which is eventually taken by ext4_journal_start().  This
> essentially means that the writeback has to happen outside of the context
> of an active journal handle (outside of ext4_journal_start() to
> ext4_journal_stop().)
> 
> 2) To lock out page faults we take a write lock on the ei->i_mmap_sem, and
> this lock again ranks above the jbd2_handle taken by ext4_journal_start().
> So, as with the writeback code in 1) above we have to take ei->i_mmap_sem
> outside of the context of an active journal handle.

Welcome to the joy of fs locking ;)

> Signed-off-by: Ross Zwisler <ross.zwisler@linux.intel.com>
> CC: stable@vger.kernel.org

								Honza

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

Re: [PATCH 6/9] ext4: safely transition S_DAX on journaling changes

[Ross Zwisler]

On Wed, Sep 06, 2017 at 11:47:00AM +0200, Jan Kara wrote:
> On Tue 05-09-17 16:35:38, Ross Zwisler wrote:
> > The IOCTL path which switches the journaling mode for an inode is currently
> > unsafe because it doesn't properly do a writeback and invalidation on the
> > inode.  In XFS, for example, safe transitions of S_DAX are handled by
> > xfs_ioctl_setattr_dax_invalidate() which locks out page faults and I/O,
> > does a writeback via filemap_write_and_wait() and an invalidation via
> > invalidate_inode_pages2().
> > 
> > Without this in place we can see the following kernel warning when we try
> > and insert a DAX exceptional entry but find that a dirty page cache page is
> > still in the mapping->radix_tree:
> > 
> >  WARNING: CPU: 4 PID: 1052 at mm/filemap.c:262 __delete_from_page_cache+0x375/0x550
> >  Modules linked in: dax_pmem nd_pmem device_dax nd_btt nfit libnvdimm
> >  CPU: 4 PID: 1052 Comm: small Not tainted 4.13.0-rc6-00055-gac26931 #3
> >  Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 1.9.3-1.fc25 04/01/2014
> >  task: ffff88020ccd0000 task.stack: ffffc900021d4000
> >  RIP: 0010:__delete_from_page_cache+0x375/0x550
> >  RSP: 0000:ffffc900021d7b90 EFLAGS: 00010002
> >  RAX: 002fffc00001123d RBX: ffffffffffffffff RCX: ffff8801d9440d68
> >  RDX: 0000000000000000 RSI: ffffffff81fd5b84 RDI: ffffffff81f6f0e5
> >  RBP: ffffc900021d7be0 R08: 0000000000000000 R09: ffff8801f9938c70
> >  R10: 0000000000000021 R11: ffff8801f9938c91 R12: ffff8801d9440d70
> >  R13: ffffea0007fdda80 R14: 0000000000000001 R15: ffff8801d9440d68
> >  FS:  00007feacc041700(0000) GS:ffff880211800000(0000) knlGS:0000000000000000
> >  CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> >  CR2: 0000000010420000 CR3: 000000020cfd8000 CR4: 00000000000006e0
> >  Call Trace:
> >   dax_insert_mapping_entry+0x158/0x2c0
> >   dax_iomap_fault+0x1020/0x1bb0
> >   ext4_dax_huge_fault+0xc8/0x160
> >   ext4_dax_fault+0x10/0x20
> >   __do_fault+0x20/0x110
> >   __handle_mm_fault+0x97d/0x1120
> >   handle_mm_fault+0x188/0x2f0
> >   __do_page_fault+0x28f/0x590
> >   trace_do_page_fault+0x58/0x2c0
> >   do_async_page_fault+0x2c/0x90
> >   async_page_fault+0x28/0x30
> > 
> > I'm pretty sure we could make a test that shows userspace visible data
> > corruption as well in this scenario.
> > 
> > Make it safe to change the journaling mode and turn on or off S_DAX by
> > adding locking to properly lock out page faults (i_mmap_sem) and then doing
> > the writeback and invalidate.  I/O is already held off because all callers
> > of ext4_ioctl_setflags() hold the inode lock.
> 
> Yeah, this is a good point. It is just that this is not enough as I
> discovered in [1]. You also need to tear down & recreate VMAs when changing
> DAX flag which is a bit tricky. So for now I think returning EBUSY when
> file is mmaped and we'd like to flip DAX flag is the best solution. Hmm?
> 
> [1] https://www.spinics.net/lists/linux-xfs/msg09859.html

Yea, thanks for the link, I totally missed this discussion (obviously). 

Cool, I'll rework this for v2.

> > The locking for this new code is complex because of the following:
> > 
> > 1) filemap_write_and_wait() eventually calls ext4_writepages(), which
> > acquires the sbi->s_journal_flag_rwsem.  This lock ranks above the
> > jbdw_handle which is eventually taken by ext4_journal_start().  This
> > essentially means that the writeback has to happen outside of the context
> > of an active journal handle (outside of ext4_journal_start() to
> > ext4_journal_stop().)
> > 
> > 2) To lock out page faults we take a write lock on the ei->i_mmap_sem, and
> > this lock again ranks above the jbd2_handle taken by ext4_journal_start().
> > So, as with the writeback code in 1) above we have to take ei->i_mmap_sem
> > outside of the context of an active journal handle.
> 
> Welcome to the joy of fs locking ;)

:)  Well, I feel like I learned a lot more about ext4 during this patch set!

> > Signed-off-by: Ross Zwisler <ross.zwisler@linux.intel.com>
> > CC: stable@vger.kernel.org
> 
> 								Honza
> 
> -- 
> Jan Kara <jack@suse.com>
> SUSE Labs, CR

[PATCH 7/9] ext4: prevent data corruption with inline data + DAX

[Ross Zwisler]

If an inode has inline data it is currently prevented from using DAX by a
check in ext4_should_use_dax().  When the inode grows inline data via
ext4_create_inline_data() or removes its inline data via
ext4_destroy_inline_data_nolock(), the value of S_DAX can change.

Currently these changes are unsafe because we don't hold off page faults
and I/O, write back dirty radix tree entries and invalidate all mappings.
This work is done in XFS via xfs_ioctl_setattr_dax_invalidate(), for
example.  This unsafe transitioning of S_DAX could potentially lead to data
corruption.

Fix this issue by preventing the DAX mount option from being used on
filesystems that were created to support inline data.  Inline data is an
option given to mkfs.ext4.

We prevent DAX from being used with inline data as opposed to trying to
safely manage the transition of S_DAX because of the locking complexities:

1) filemap_write_and_wait() eventually calls ext4_writepages(), which
acquires the sbi->s_journal_flag_rwsem.  This lock ranks above the
jbdw_handle which is eventually taken by ext4_journal_start().  This
essentially means that the writeback has to happen outside of the context
of an active journal handle (outside of ext4_journal_start() to
ext4_journal_stop().)

2) To lock out page faults we take a write lock on the ei->i_mmap_sem, and
this lock again ranks above the jbd2_handle taken by ext4_journal_start().
So, as with the writeback code in 1) above we have to take ei->i_mmap_sem
outside of the context of an active journal handle.

We are able to work around both of these restrictions and safely transition
S_DAX when we change the journaling mode, but for inline data it's much
harder because each of ext4_create_inline_data() and
ext4_destroy_inline_data_nolock() are passed in journal handles that have
already been started.

To be able to safely writeback and invalidate our dirty inode data we'd
either need to uplevel the locking, writeback and invalidate into all the
callers of those two functions, or we'd need to stop our current journal
handle, do the appropriate locking, writeback and invalidate, unlock and
restart the journal handle.

These both seem too complex, and I don't know if we have a valid use case
where we need to combine a filesystem with inline data and DAX, so just
prevent them from being used together.

Signed-off-by: Ross Zwisler <ross.zwisler@linux.intel.com>
CC: stable@vger.kernel.org
---
 fs/ext4/inline.c | 10 ----------
 fs/ext4/super.c  |  5 +++++
 2 files changed, 5 insertions(+), 10 deletions(-)

diff --git a/fs/ext4/inline.c b/fs/ext4/inline.c
index 28c5c3a..fd95019 100644
--- a/fs/ext4/inline.c
+++ b/fs/ext4/inline.c
@@ -302,11 +302,6 @@ static int ext4_create_inline_data(handle_t *handle,
 	EXT4_I(inode)->i_inline_size = len + EXT4_MIN_INLINE_DATA_SIZE;
 	ext4_clear_inode_flag(inode, EXT4_INODE_EXTENTS);
 	ext4_set_inode_flag(inode, EXT4_INODE_INLINE_DATA);
-	/*
-	 * Propagate changes to inode->i_flags as well - e.g. S_DAX may
-	 * get cleared
-	 */
-	ext4_set_inode_flags(inode);
 	get_bh(is.iloc.bh);
 	error = ext4_mark_iloc_dirty(handle, inode, &is.iloc);
 
@@ -451,11 +446,6 @@ static int ext4_destroy_inline_data_nolock(handle_t *handle,
 		}
 	}
 	ext4_clear_inode_flag(inode, EXT4_INODE_INLINE_DATA);
-	/*
-	 * Propagate changes to inode->i_flags as well - e.g. S_DAX may
-	 * get set.
-	 */
-	ext4_set_inode_flags(inode);
 
 	get_bh(is.iloc.bh);
 	error = ext4_mark_iloc_dirty(handle, inode, &is.iloc);
diff --git a/fs/ext4/super.c b/fs/ext4/super.c
index d61a70e2..d549dfb 100644
--- a/fs/ext4/super.c
+++ b/fs/ext4/super.c
@@ -3686,6 +3686,11 @@ static int ext4_fill_super(struct super_block *sb, void *data, int silent)
 	}
 
 	if (sbi->s_mount_opt & EXT4_MOUNT_DAX) {
+		if (ext4_has_feature_inline_data(sb)) {
+			ext4_msg(sb, KERN_ERR, "Cannot use DAX on a filesystem"
+					" that may contain inline data");
+			goto failed_mount;
+		}
 		err = bdev_dax_supported(sb, blocksize);
 		if (err)
 			goto failed_mount;
-- 
2.9.5

Re: [PATCH 7/9] ext4: prevent data corruption with inline data + DAX

[Andreas Dilger]

[-- Attachment #1: Type: text/plain, Size: 5330 bytes --]

On Sep 5, 2017, at 4:35 PM, Ross Zwisler <ross.zwisler@linux.intel.com> wrote:
> 
> If an inode has inline data it is currently prevented from using DAX by a
> check in ext4_should_use_dax().  When the inode grows inline data via
> ext4_create_inline_data() or removes its inline data via
> ext4_destroy_inline_data_nolock(), the value of S_DAX can change.
> 
> Currently these changes are unsafe because we don't hold off page faults
> and I/O, write back dirty radix tree entries and invalidate all mappings.
> This work is done in XFS via xfs_ioctl_setattr_dax_invalidate(), for
> example.  This unsafe transitioning of S_DAX could potentially lead to data
> corruption.
> 
> Fix this issue by preventing the DAX mount option from being used on
> filesystems that were created to support inline data.  Inline data is an
> option given to mkfs.ext4.
> 
> We prevent DAX from being used with inline data as opposed to trying to
> safely manage the transition of S_DAX because of the locking complexities:
> 
> 1) filemap_write_and_wait() eventually calls ext4_writepages(), which
> acquires the sbi->s_journal_flag_rwsem.  This lock ranks above the
> jbdw_handle which is eventually taken by ext4_journal_start().  This
> essentially means that the writeback has to happen outside of the context
> of an active journal handle (outside of ext4_journal_start() to
> ext4_journal_stop().)
> 
> 2) To lock out page faults we take a write lock on the ei->i_mmap_sem, and
> this lock again ranks above the jbd2_handle taken by ext4_journal_start().
> So, as with the writeback code in 1) above we have to take ei->i_mmap_sem
> outside of the context of an active journal handle.
> 
> We are able to work around both of these restrictions and safely transition
> S_DAX when we change the journaling mode, but for inline data it's much
> harder because each of ext4_create_inline_data() and
> ext4_destroy_inline_data_nolock() are passed in journal handles that have
> already been started.
> 
> To be able to safely writeback and invalidate our dirty inode data we'd
> either need to uplevel the locking, writeback and invalidate into all the
> callers of those two functions, or we'd need to stop our current journal
> handle, do the appropriate locking, writeback and invalidate, unlock and
> restart the journal handle.
> 
> These both seem too complex, and I don't know if we have a valid use case
> where we need to combine a filesystem with inline data and DAX, so just
> prevent them from being used together.

The one reason I can see to use inline data + DAX is that inline data saves
space for very small files, even if the performance improvement is minimal.
Since NVDIMMs are still relatively expensive, storing very small files and
directories directly in the inode is probably worthwhile.

That said, there are still occasional bugs in the inline data code, so it
makes sense to ensure these two features are not enabled at the same time
if they don't play well together.

> Signed-off-by: Ross Zwisler <ross.zwisler@linux.intel.com>
> CC: stable@vger.kernel.org
> ---
> fs/ext4/inline.c | 10 ----------
> fs/ext4/super.c  |  5 +++++
> 2 files changed, 5 insertions(+), 10 deletions(-)
> 
> diff --git a/fs/ext4/inline.c b/fs/ext4/inline.c
> index 28c5c3a..fd95019 100644
> --- a/fs/ext4/inline.c
> +++ b/fs/ext4/inline.c
> @@ -302,11 +302,6 @@ static int ext4_create_inline_data(handle_t *handle,
> 	EXT4_I(inode)->i_inline_size = len + EXT4_MIN_INLINE_DATA_SIZE;
> 	ext4_clear_inode_flag(inode, EXT4_INODE_EXTENTS);
> 	ext4_set_inode_flag(inode, EXT4_INODE_INLINE_DATA);
> -	/*
> -	 * Propagate changes to inode->i_flags as well - e.g. S_DAX may
> -	 * get cleared
> -	 */
> -	ext4_set_inode_flags(inode);

What about other flags in the inode?  It doesn't make sense to drop this
entirely.  The S_DAX flag shouldn't be set if the inode has the inline
data flag set, according to ext4_set_inode_flags().

> 	get_bh(is.iloc.bh);
> 	error = ext4_mark_iloc_dirty(handle, inode, &is.iloc);
> 
> @@ -451,11 +446,6 @@ static int ext4_destroy_inline_data_nolock(handle_t *handle,
> 		}
> 	}
> 	ext4_clear_inode_flag(inode, EXT4_INODE_INLINE_DATA);
> -	/*
> -	 * Propagate changes to inode->i_flags as well - e.g. S_DAX may
> -	 * get set.
> -	 */
> -	ext4_set_inode_flags(inode);

Same?

> 	get_bh(is.iloc.bh);
> 	error = ext4_mark_iloc_dirty(handle, inode, &is.iloc);
> diff --git a/fs/ext4/super.c b/fs/ext4/super.c
> index d61a70e2..d549dfb 100644
> --- a/fs/ext4/super.c
> +++ b/fs/ext4/super.c
> @@ -3686,6 +3686,11 @@ static int ext4_fill_super(struct super_block *sb, void *data, int silent)
> 	}
> 
> 	if (sbi->s_mount_opt & EXT4_MOUNT_DAX) {
> +		if (ext4_has_feature_inline_data(sb)) {
> +			ext4_msg(sb, KERN_ERR, "Cannot use DAX on a filesystem"
> +					" that may contain inline data");
> +			goto failed_mount;
> +		}

Wouldn't it be enough to just prevent modification of inodes that are stored
in the inode?  It should be OK to read such files.  At a minimum that means
there should not be an error in case of read-only mounting.  A better choice
would be to return an error only at runtime in case of open-for-write, or
only if the file is actually being written.

Cheers, Andreas






[-- Attachment #2: Message signed with OpenPGP --]
[-- Type: application/pgp-signature, Size: 195 bytes --]

Re: [PATCH 7/9] ext4: prevent data corruption with inline data + DAX

[Ross Zwisler]

On Wed, Sep 06, 2017 at 02:55:31PM -0600, Andreas Dilger wrote:
> On Sep 5, 2017, at 4:35 PM, Ross Zwisler <ross.zwisler@linux.intel.com> wrote:
> > 
> > If an inode has inline data it is currently prevented from using DAX by a
> > check in ext4_should_use_dax().  When the inode grows inline data via
> > ext4_create_inline_data() or removes its inline data via
> > ext4_destroy_inline_data_nolock(), the value of S_DAX can change.
> > 
> > Currently these changes are unsafe because we don't hold off page faults
> > and I/O, write back dirty radix tree entries and invalidate all mappings.
> > This work is done in XFS via xfs_ioctl_setattr_dax_invalidate(), for
> > example.  This unsafe transitioning of S_DAX could potentially lead to data
> > corruption.
> > 
> > Fix this issue by preventing the DAX mount option from being used on
> > filesystems that were created to support inline data.  Inline data is an
> > option given to mkfs.ext4.
> > 
> > We prevent DAX from being used with inline data as opposed to trying to
> > safely manage the transition of S_DAX because of the locking complexities:
> > 
> > 1) filemap_write_and_wait() eventually calls ext4_writepages(), which
> > acquires the sbi->s_journal_flag_rwsem.  This lock ranks above the
> > jbdw_handle which is eventually taken by ext4_journal_start().  This
> > essentially means that the writeback has to happen outside of the context
> > of an active journal handle (outside of ext4_journal_start() to
> > ext4_journal_stop().)
> > 
> > 2) To lock out page faults we take a write lock on the ei->i_mmap_sem, and
> > this lock again ranks above the jbd2_handle taken by ext4_journal_start().
> > So, as with the writeback code in 1) above we have to take ei->i_mmap_sem
> > outside of the context of an active journal handle.
> > 
> > We are able to work around both of these restrictions and safely transition
> > S_DAX when we change the journaling mode, but for inline data it's much
> > harder because each of ext4_create_inline_data() and
> > ext4_destroy_inline_data_nolock() are passed in journal handles that have
> > already been started.
> > 
> > To be able to safely writeback and invalidate our dirty inode data we'd
> > either need to uplevel the locking, writeback and invalidate into all the
> > callers of those two functions, or we'd need to stop our current journal
> > handle, do the appropriate locking, writeback and invalidate, unlock and
> > restart the journal handle.
> > 
> > These both seem too complex, and I don't know if we have a valid use case
> > where we need to combine a filesystem with inline data and DAX, so just
> > prevent them from being used together.
> 
> The one reason I can see to use inline data + DAX is that inline data saves
> space for very small files, even if the performance improvement is minimal.
> Since NVDIMMs are still relatively expensive, storing very small files and
> directories directly in the inode is probably worthwhile.
> 
> That said, there are still occasional bugs in the inline data code, so it
> makes sense to ensure these two features are not enabled at the same time
> if they don't play well together.

Yep, unfortunately it's increasingly looking like we just can't ever safely
transition S_DAX at runtime:

https://www.spinics.net/lists/linux-xfs/msg09859.html

I think the current thought is to just avoid it completely until/unless we can
figure out a way to make it safe that is worthwhile.

> > Signed-off-by: Ross Zwisler <ross.zwisler@linux.intel.com>
> > CC: stable@vger.kernel.org
> > ---
> > fs/ext4/inline.c | 10 ----------
> > fs/ext4/super.c  |  5 +++++
> > 2 files changed, 5 insertions(+), 10 deletions(-)
> > 
> > diff --git a/fs/ext4/inline.c b/fs/ext4/inline.c
> > index 28c5c3a..fd95019 100644
> > --- a/fs/ext4/inline.c
> > +++ b/fs/ext4/inline.c
> > @@ -302,11 +302,6 @@ static int ext4_create_inline_data(handle_t *handle,
> > 	EXT4_I(inode)->i_inline_size = len + EXT4_MIN_INLINE_DATA_SIZE;
> > 	ext4_clear_inode_flag(inode, EXT4_INODE_EXTENTS);
> > 	ext4_set_inode_flag(inode, EXT4_INODE_INLINE_DATA);
> > -	/*
> > -	 * Propagate changes to inode->i_flags as well - e.g. S_DAX may
> > -	 * get cleared
> > -	 */
> > -	ext4_set_inode_flags(inode);
> 
> What about other flags in the inode?  It doesn't make sense to drop this
> entirely.  The S_DAX flag shouldn't be set if the inode has the inline
> data flag set, according to ext4_set_inode_flags().

AFAICT all the other flags in the inode are independent of
EXT4_INODE_INLINE_DATA.  The only one that could switch as a side-effect of
EXT4_INODE_INLINE_DATA is S_DAX.

The lines I'm deleting were added explicitly for the side-effect transition of
S_DAX (commit a3caa24b703794), so I don't think we still need them at all if
we prevent S_DAX from transitioning.

Ditto for the other deletion.

> 
> > 	get_bh(is.iloc.bh);
> > 	error = ext4_mark_iloc_dirty(handle, inode, &is.iloc);
> > 
> > @@ -451,11 +446,6 @@ static int ext4_destroy_inline_data_nolock(handle_t *handle,
> > 		}
> > 	}
> > 	ext4_clear_inode_flag(inode, EXT4_INODE_INLINE_DATA);
> > -	/*
> > -	 * Propagate changes to inode->i_flags as well - e.g. S_DAX may
> > -	 * get set.
> > -	 */
> > -	ext4_set_inode_flags(inode);
> 
> Same?
> 
> > 	get_bh(is.iloc.bh);
> > 	error = ext4_mark_iloc_dirty(handle, inode, &is.iloc);
> > diff --git a/fs/ext4/super.c b/fs/ext4/super.c
> > index d61a70e2..d549dfb 100644
> > --- a/fs/ext4/super.c
> > +++ b/fs/ext4/super.c
> > @@ -3686,6 +3686,11 @@ static int ext4_fill_super(struct super_block *sb, void *data, int silent)
> > 	}
> > 
> > 	if (sbi->s_mount_opt & EXT4_MOUNT_DAX) {
> > +		if (ext4_has_feature_inline_data(sb)) {
> > +			ext4_msg(sb, KERN_ERR, "Cannot use DAX on a filesystem"
> > +					" that may contain inline data");
> > +			goto failed_mount;
> > +		}
> 
> Wouldn't it be enough to just prevent modification of inodes that are stored
> in the inode?  It should be OK to read such files.  At a minimum that means
> there should not be an error in case of read-only mounting.  A better choice
> would be to return an error only at runtime in case of open-for-write, or
> only if the file is actually being written.

Hmm...yes?  I think this *could* work - we'd have to prevent anything that
would make the inode switch from using inline data to using extents.  This
includes writes (all writes?  Or just ones that would trigger the
transition?), fallocate, etc.

I think we could make this work, but the consequence would be that
applications would receive I/O errors or errors for seemingly random syscalls,
and they might have a hard time understanding why.  

It seems cleaner and simpler to me to just disallow the combination of inline
data + DAX, and to print a descriptive message in dmesg?  The combination of
the two features would end up being pretty unfriendly anyway, and the
implementation to make it safe would probably be more complex.

[PATCH 8/9] ext4: add sanity check for encryption + DAX

[Ross Zwisler]

We prevent DAX from being used on inodes which are using ext4's built in
encryption via a check in ext4_should_use_dax().  We do have what appears
to be an unsafe transition of S_DAX in ext4_set_context(), though, where
S_DAX can get disabled without us doing a proper writeback + invalidate.

I actually think we are safe in this case because of the following:

1) You can't encrypt an existing file.  Encryption can only be set on an
empty directory, with new inodes in that directory being created with
encryption turned on, so I don't think it's possible to turn encryption on
for a file that has open DAX mmaps or outstanding I/Os.

2) There is no way to turn encryption off on a given file.  Once an inode
is encrypted, it stays encrypted for the life of that inode, so we don't
have to worry about the case where we turn encryption off and S_DAX
suddenly turns on.

3) The only way we end up in ext4_set_context() to turn on encryption is
when we are creating a new file in the encrypted directory.  This happens
as part of ext4_create() before the inode has been allowed to do any I/O.
Here's the call tree:

 ext4_create()
   __ext4_new_inode()
	 ext4_set_inode_flags() // sets S_DAX
	 fscrypt_inherit_context()
		fscrypt_get_encryption_info();
		ext4_set_context() // sets EXT4_INODE_ENCRYPT, clears S_DAX

So, I actually think it's safe to transition S_DAX in ext4_set_context()
without any locking, writebacks or invalidations.  I've added a
WARN_ON_ONCE() sanity check to make sure that we are notified if we ever
encounter a case where we are encrypting an inode that already has data,
in which case we need to add code to safely transition S_DAX.

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

diff --git a/fs/ext4/super.c b/fs/ext4/super.c
index d549dfb..6604a18 100644
--- a/fs/ext4/super.c
+++ b/fs/ext4/super.c
@@ -1159,6 +1159,9 @@ static int ext4_set_context(struct inode *inode, const void *ctx, size_t len,
 	if (inode->i_ino == EXT4_ROOT_INO)
 		return -EPERM;
 
+	if (WARN_ON_ONCE(IS_DAX(inode) && i_size_read(inode)))
+		return -EINVAL;
+
 	res = ext4_convert_inline_data(inode);
 	if (res)
 		return res;
-- 
2.9.5