On Tue 11-09-18 11:15:18, Dan Williams wrote:

On Tue, Sep 11, 2018 at 8:42 AM, Toshi Kani [off-list ref] wrote:

quoted

Sync syscall to an existing DAX file needs to flush processor cache,
but it does not currently.  This is because 'ext4_da_aops' is set to
address_space_operations of existing DAX files, instead of 'ext4_dax_aops',
since S_DAX flag is set after ext4_set_aops() in the open path.

  New file
  --------
  lookup_open
    ext4_create
      __ext4_new_inode
        ext4_set_inode_flags   // Set S_DAX flag
      ext4_set_aops            // Set aops to ext4_dax_aops

  Existing file
  -------------
  lookup_open
    ext4_lookup
      ext4_iget
        ext4_set_aops          // Set aops to ext4_da_aops
        ext4_set_inode_flags   // Set S_DAX flag

Change ext4_iget() to call ext4_set_inode_flags() before ext4_set_aops().

Fixes: 5f0663bb4a64f588f0a2dd6d1be68d40f9af0086

Same format nit:

Fixes: 5f0663bb4a64 ("ext4, dax: introduce ext4_dax_aops")
Cc: <redacted>

quoted

Signed-off-by: Toshi Kani <redacted>
Cc: Jan Kara <redacted>
Cc: Dan Williams <redacted>
Cc: "Theodore Ts'o" <redacted>
Cc: Andreas Dilger <redacted>
---
 fs/ext4/inode.c |    3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/fs/ext4/inode.c b/fs/ext4/inode.c
index 775cd9b4af55..93cbbb859c40 100644
--- a/fs/ext4/inode.c
+++ b/fs/ext4/inode.c

@@ -4998,6 +4998,8 @@ struct inode *ext4_iget(struct super_block *sb, unsigned long ino)
        if (ret)
                goto bad_inode;

+       ext4_set_inode_flags(inode);
+

Hmm, does this have unintended behavior changes?

I notice that there are some checks for flags "IS_APPEND(inode) ||
IS_IMMUTABLE(inode)" *before* the call to ext4_set_inode_flags(). I
didn't look too much deeper at whether those checks are bogus, but it
would seem safer to do something like this for a lower risk fix.

Thoughts?

Well, safer but it would leave the landmine around for others to hit.
Toshi, please move the ext4_set_inode_flags() call to be just after the
assignment:

	ei->i_flags = le32_to_cpu(raw_inode->i_flags);

in ext4_iget(). That way people won't introduce checks for i_flags that can
never hit... And yes, it fixes also other bugs (mostly in sanity checks
AFAICS) than the DAX issue.

								Honza

-- 
Jan Kara [off-list ref]
SUSE Labs, CR