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

[Christoph Hellwig]

> +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;
> + if ((inode->i_flags & S_DAX) == 0)
> + return false;
> + if (S_ISCHR(inode->i_mode))
> + return false;
> + return true;

Encoding the is char device or not thing here is just nasty.  I think
this is going entirely in the wrong direction.
___
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm

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  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" 
>> Cc: Andreas Dilger 
>> Cc: Alexander Viro 
>> Cc: "Darrick J. Wong" 
>> Cc: linux-...@vger.kernel.org
>> Cc: Matthew Wilcox 
>> Cc: Ross Zwisler 
>> Cc: 
>> Fixes: dee410792419 ("/dev/dax, core: file operations and dax-mmap")
>> Reported-by: Jan Kara 
>> Reviewed-by: Jan Kara 
>> Signed-off-by: Dan Williams 
>> ---
>>  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.
___
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm

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

[Darrick J. Wong]

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" 
> Cc: Andreas Dilger 
> Cc: Alexander Viro 
> Cc: "Darrick J. Wong" 
> Cc: linux-...@vger.kernel.org
> Cc: Matthew Wilcox 
> Cc: Ross Zwisler 
> Cc: 
> Fixes: dee410792419 ("/dev/dax, core: file operations and dax-mmap")
> Reported-by: Jan Kara 
> Reviewed-by: Jan Kara 
> Signed-off-by: Dan Williams 
> ---
>  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 majord...@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
___
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm