linux-kernel - Re: [PATCH v11 19/21] dax: Add dax_zero_page

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:	Sat, 18 Oct 2014 21:16:23 +0000 (UTC)
From:	Mathieu Desnoyers <mathieu.desnoyers@...icios.com>
To:	Matthew Wilcox <willy@...ux.intel.com>
Cc:	Matthew Wilcox <matthew.r.wilcox@...el.com>,
	linux-fsdevel@...r.kernel.org, linux-mm@...ck.org,
	linux-kernel@...r.kernel.org,
	Ross Zwisler <ross.zwisler@...ux.intel.com>
Subject: Re: [PATCH v11 19/21] dax: Add dax_zero_page_range

----- Original Message -----
> From: "Matthew Wilcox" <willy@...ux.intel.com>
> To: "Mathieu Desnoyers" <mathieu.desnoyers@...icios.com>
> Cc: "Matthew Wilcox" <willy@...ux.intel.com>, "Matthew Wilcox" <matthew.r.wilcox@...el.com>,
> linux-fsdevel@...r.kernel.org, linux-mm@...ck.org, linux-kernel@...r.kernel.org, "Ross Zwisler"
> <ross.zwisler@...ux.intel.com>
> 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@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/