On Sep 5, 2017, at 4:35 PM, Ross Zwisler [off-list ref] 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.

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().

Same?

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