lkml.org 
[lkml]   [2017]   [Nov]   [14]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    From
    Date
    SubjectRe: [GIT PULL] ext4 updates for 4.15
    On Mon, Nov 13, 2017 at 8:25 AM, Theodore Ts'o <tytso@mit.edu> wrote:
    > I forgot to mention, there's a merge conflict when pulling the ext4
    > and fscrypt trees. The fixup is relatively straightforward:

    It doesn't actually look all that straightforward, and in particular,
    the resolution you sent me doesn't actually seem correct:

    > new_fl |= S_NOATIME;
    > if (flags & EXT4_DIRSYNC_FL)
    > new_fl |= S_DIRSYNC;
    > - if (test_opt(inode->i_sb, DAX) && S_ISREG(inode->i_mode) &&
    > - !ext4_should_journal_data(inode) && !ext4_has_inline_data(inode) &&
    > - !(flags & EXT4_ENCRYPT_FL))
    > + if (ext4_should_use_dax(inode))
    > new_fl |= S_DAX;

    This now loses the "!(flags & EXT4_ENCRYPT_FL)" test when it sets S_DAX.

    Yes, in ext4_should_use_dax(), it has this code

    if (ext4_encrypted_inode(inode))
    return false;

    but that test was what commit 2ee6a576be56 changed in favor of just
    checking !(flags & EXT4_ENCRYPT_FL).

    So that suggested merge resolkution actually undoes some of that
    commit 2ee6a576be56.

    Of course,

    (flags & EXT4_ENCRYPT_FL)

    _should_ be the same as

    ext4_test_inode_flag(inode, EXT4_INODE_ENCRYPT);

    so It does seem to be harmless, but it's a bit dodgy.

    I'll do that suggested resolution, but I have to say that the ext4 bit
    testing is incredibly broken and non-obvious. Just as an example:

    fs/ext4/ext4.h:#define EXT4_ENCRYPT_FL 0x00000800
    /* encrypted file */
    fs/ext4/ext4.h: EXT4_INODE_ENCRYPT = 11, /* Encrypted file */

    yeah, it's the same bit, but it sure as hell isn't obvious. Why the
    two totally different ways to define that data?

    Linus

    \
     
     \ /
      Last update: 2017-11-14 22:00    [W:4.113 / U:0.568 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site