----- 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, send a message with 'unsubscribe linux-mm' in
the body to majordomo@xxxxxxxxx. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@xxxxxxxxx";> email@xxxxxxxxx </a>