lkml.org 
[lkml]   [2018]   [Mar]   [2]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [PATCH v5 02/12] dax: introduce IS_DEVDAX() and IS_FSDAX()
    On Thu, Mar 01, 2018 at 07:53:44PM -0800, Dan Williams wrote:
    > The current IS_DAX() helper that checks if a file is in DAX mode serves
    > two purposes. It is a control flow branch condition for DAX vs
    > non-DAX paths and it is a mechanism to perform dead code elimination. The
    > dead code elimination is required in the CONFIG_FS_DAX=n case since
    > there are symbols in fs/dax.c that will be elided. While the
    > dead code elimination can be addressed with nop stubs for the fs/dax.c
    > symbols that does not address the need for a DAX control flow helper
    > where fs/dax.c symbols are not involved.
    >
    > Moreover, the control flow changes, in some cases, need to be cognizant
    > of whether the DAX file is a typical file or a Device-DAX special file.
    > Introduce IS_DEVDAX() and IS_FSDAX() to simultaneously address the
    > file-type control flow and dead-code elimination use cases. IS_DAX()
    > will be deleted after all sites are converted to use the file-type
    > specific helper.
    >
    > Note, this change is also a pre-requisite for fixing the definition of
    > the S_DAX inode flag in the CONFIG_FS_DAX=n + CONFIG_DEV_DAX=y case.
    > The flag needs to be defined, non-zero, if either DAX facility is
    > enabled.
    >
    > Cc: "Theodore Ts'o" <tytso@mit.edu>
    > Cc: Andreas Dilger <adilger.kernel@dilger.ca>
    > Cc: Alexander Viro <viro@zeniv.linux.org.uk>
    > Cc: "Darrick J. Wong" <darrick.wong@oracle.com>
    > Cc: linux-xfs@vger.kernel.org
    > Cc: Matthew Wilcox <mawilcox@microsoft.com>
    > Cc: Ross Zwisler <ross.zwisler@linux.intel.com>
    > Cc: <stable@vger.kernel.org>
    > Fixes: dee410792419 ("/dev/dax, core: file operations and dax-mmap")
    > Reported-by: Jan Kara <jack@suse.cz>
    > Reviewed-by: Jan Kara <jack@suse.cz>
    > Signed-off-by: Dan Williams <dan.j.williams@intel.com>
    > ---
    > include/linux/fs.h | 22 ++++++++++++++++++++++
    > 1 file changed, 22 insertions(+)
    >
    > diff --git a/include/linux/fs.h b/include/linux/fs.h
    > index 79c413985305..bd0c46880572 100644
    > --- a/include/linux/fs.h
    > +++ b/include/linux/fs.h
    > @@ -1909,6 +1909,28 @@ static inline bool sb_rdonly(const struct super_block *sb) { return sb->s_flags
    > #define IS_WHITEOUT(inode) (S_ISCHR(inode->i_mode) && \
    > (inode)->i_rdev == WHITEOUT_DEV)
    >
    > +static inline bool IS_DEVDAX(struct inode *inode)
    > +{
    > + if (!IS_ENABLED(CONFIG_DEV_DAX))
    > + return false;
    > + if ((inode->i_flags & S_DAX) == 0)
    > + return false;
    > + if (!S_ISCHR(inode->i_mode))
    > + return false;
    > + return true;
    > +}
    > +
    > +static inline bool IS_FSDAX(struct inode *inode)
    > +{
    > + if (!IS_ENABLED(CONFIG_FS_DAX))
    > + return false;

    I echo Jan's complaint from the last round that the dead code
    elimination here is subtle, as compared to:

    #if IS_ENABLED(CONFIG_FS_DAX)
    static inline bool IS_FSDAX(struct inode *inode) { ... }
    #else
    # define IS_FSDAX(inode) (false)
    #endif

    But I guess even with that we're relying on dead code elimination higher
    up in the call stack...

    > + if ((inode->i_flags & S_DAX) == 0)
    > + return false;
    > + if (S_ISCHR(inode->i_mode))
    > + return false;

    I'm curious, do we have character devices with S_DAX set?

    I /think/ we're expecting that only block/char devices and files will
    ever have S_DAX set, so IS_FSDAX is only true for block devices and
    files. Right?

    (A comment here about why S_ISCHR->false here would be helpful.)

    --D

    > + return true;
    > +}
    > +
    > static inline bool HAS_UNMAPPED_ID(struct inode *inode)
    > {
    > return !uid_valid(inode->i_uid) || !gid_valid(inode->i_gid);
    >
    > --
    > To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
    > the body of a message to majordomo@vger.kernel.org
    > More majordomo info at http://vger.kernel.org/majordomo-info.html

    \
     
     \ /
      Last update: 2018-03-02 18:46    [W:2.659 / U:0.492 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site