Re: [PATCH v5 02/12] dax: introduce IS_DEVDAX() and IS_FSDAX()

[Dan Williams]

On Fri, Mar 2, 2018 at 9:45 AM, Darrick J. Wong <darrick.wong@xxxxxxxxxx> wrote:
> 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@xxxxxxx>
>> Cc: Andreas Dilger <adilger.kernel@xxxxxxxxx>
>> Cc: Alexander Viro <viro@xxxxxxxxxxxxxxxxxx>
>> Cc: "Darrick J. Wong" <darrick.wong@xxxxxxxxxx>
>> Cc: linux-xfs@xxxxxxxxxxxxxxx
>> Cc: Matthew Wilcox <mawilcox@xxxxxxxxxxxxx>
>> Cc: Ross Zwisler <ross.zwisler@xxxxxxxxxxxxxxx>
>> Cc: <stable@xxxxxxxxxxxxxxx>
>> Fixes: dee410792419 ("/dev/dax, core: file operations and dax-mmap")
>> Reported-by: Jan Kara <jack@xxxxxxx>
>> Reviewed-by: Jan Kara <jack@xxxxxxx>
>> Signed-off-by: Dan Williams <dan.j.williams@xxxxxxxxx>
>> ---
>>  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 IS_FSDAX() was only a dead-code elimination mechanism rather than a
runtime branch condition then I agree. Otherwise I think IS_ENABLED()
is suitable and not subtle, especially when used in a header file.

>> +     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?

Yes, Device-DAX, see:

    ab68f2622136 /dev/dax, pmem: direct access to persistent memory

> 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?

We had S_DAX on block-devices for a short while, but deleted it and
went with the Device-DAX interface instead. So it's only regular files
and /dev/daxX.Y nodes these days.

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

Ok.