Re: [PATCH v11 19/21] dax: Add dax_zero_page_range

[Mathieu Desnoyers]

----- Original Message -----
> From: "Matthew Wilcox" <willy@xxxxxxxxxxxxxxx>
> To: "Mathieu Desnoyers" <mathieu.desnoyers@xxxxxxxxxxxx>
> Cc: "Matthew Wilcox" <willy@xxxxxxxxxxxxxxx>, "Matthew Wilcox" <matthew.r.wilcox@xxxxxxxxx>,
> linux-fsdevel@xxxxxxxxxxxxxxx, linux-mm@xxxxxxxxx, linux-kernel@xxxxxxxxxxxxxxx, "Ross Zwisler"
> <ross.zwisler@xxxxxxxxxxxxxxx>
> Sent: Saturday, October 18, 2014 7:41:00 PM
> Subject: Re: [PATCH v11 19/21] dax: Add dax_zero_page_range
> 
> On Fri, Oct 17, 2014 at 03:49:39PM +0000, Mathieu Desnoyers wrote:
> > > I kind of wonder if we shouldn't just declare the function.  It's called
> > > like this:
> > > 
> > >         if (IS_DAX(inode))
> > >                 return dax_zero_page_range(inode, from, length,
> > >                 ext4_get_block);
> > >         return __ext4_block_zero_page_range(handle, mapping, from,
> > >         length);
> > > 
> > > and if CONFIG_DAX is not set, IS_DAX evaluates to 0 at compile time, so
> > > the compiler will optimise out the call to dax_zero_page_range() anyway.
> > 
> > I strongly prefer to implement "unimplemented stub" as static inlines
> > rather than defining to 0, because the compiler can check that the types
> > passed to the function are valid, even in the #else configuration which
> > uses the stubs.
> 
> I think my explanation was unclear.  This is what I meant:
> 
> +++ b/include/linux/fs.h
> @@ -2473,7 +2473,6 @@ extern loff_t fixed_size_llseek(struct file *file,
> loff_t
> offset,
>  extern int generic_file_open(struct inode * inode, struct file * filp);
>  extern int nonseekable_open(struct inode * inode, struct file * filp);
>  
> -#ifdef CONFIG_FS_DAX
>  int dax_clear_blocks(struct inode *, sector_t block, long size);
>  int dax_zero_page_range(struct inode *, loff_t from, unsigned len,
>  get_block_t)
> ;
>  int dax_truncate_page(struct inode *, loff_t from, get_block_t);
>  #define dax_mkwrite(vma, vmf, gb)      dax_fault(vma, vmf, gb)
> -#else
> -static inline int dax_clear_blocks(struct inode *i, sector_t blk, long sz)
> -{
> -       return 0;
> -}
> -
> -static inline int dax_truncate_page(struct inode *i, loff_t frm, get_block_t
> gb)
> -{
> -       return 0;
> -}
> -
> -static inline int dax_zero_page_range(struct inode *i, loff_t frm,
> -                                               unsigned len, get_block_t gb)
> -{
> -       return 0;
> -}
> -
> -static inline ssize_t dax_do_io(int rw, struct kiocb *iocb,
> -               struct inode *inode, struct iov_iter *iter, loff_t pos,
> -               get_block_t get_block, dio_iodone_t end_io, int flags)
> -{
> -       return -ENOTTY;
> -}
> -#endif
>  
>  #ifdef CONFIG_BLOCK
>  typedef void (dio_submit_t)(int rw, struct bio *bio, struct inode *inode,
> 
> 
> So after the preprocessor has run, the compiler will see:
> 
> 	if (0)
> 		return dax_zero_page_range(inode, from, length, ext4_get_block);
> 
> and it will still do type checking on the call, even though it will eliminate
> the call.
> 

Indeed, since Linux is always compiled in O2 or Os, it will work.

> I think what you're really complaining about is that the argument to
> IS_DAX() is not checked for being an inode.
> 
> We could solve that this way:
> 
> #ifdef CONFIG_FS_DAX
> #define S_DAX		8192
> #else
> #define S_DAX		0
> #endif
> ...
> #define IS_DAX(inode)           ((inode)->i_flags & S_DAX)
> 
> After preprocessing, the compiler than sees:
> 
> 	if (((inode)->i_flags & 0))
> 		return dax_zero_page_range(inode, from, length, ext4_get_block);
> 
> and successfully deduces that the condition evaluates to 0, and still
> elide the reference to dax_zero_page_range (checked with 'nm').

Sounds good,

Thanks,

Mathieu


-- 
Mathieu Desnoyers
EfficiOS Inc.
http://www.efficios.com
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/