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" <matthew.r.wilcox@xxxxxxxxx>, linux-fsdevel@xxxxxxxxxxxxxxx, linux-mm@xxxxxxxxx,
> linux-kernel@xxxxxxxxxxxxxxx, "Ross Zwisler" <ross.zwisler@xxxxxxxxxxxxxxx>
> Sent: Friday, October 17, 2014 12:01:26 AM
> Subject: Re: [PATCH v11 19/21] dax: Add dax_zero_page_range
> 
> On Thu, Oct 16, 2014 at 02:38:24PM +0200, Mathieu Desnoyers wrote:
> > > +int dax_zero_page_range(struct inode *inode, loff_t from, unsigned
> > > length,
> > 
> > nit: unsigned -> unsigned int ?
> > 
> > Do we want a unsigned int or unsigned long here ?
> 
> It's supposed to be for a fragment of a page, so until we see a machine
> with PAGE_SIZE > 4GB, we're good to use an unsigned int.

OK

> 
> > >  	if (!length)
> > >  		return 0;
> > > +	BUG_ON((offset + length) > PAGE_CACHE_SIZE);
> > 
> > Isn't it a bit extreme to BUG_ON this condition ? We could return an
> > error to the caller, and perhaps WARN_ON_ONCE(), but BUG_ON() appears to
> > be slightly too strong here.
> 
> Dave Chinner asked for it :-)  The filesystem is supposed to be doing
> this clamping (until the last version, I had this function doing the
> clamping, and I was told off for "leaving landmines lying around".

Makes sense,

> 
> > > +static inline int dax_zero_page_range(struct inode *i, loff_t frm,
> > > +						unsigned len, get_block_t gb)
> > > +{
> > > +	return 0;
> > 
> > Should we return 0 or -ENOSYS here ?
> 
> 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.

The only reason why I have not pointed this out for some of your other
patches was because it was clear that the local style of those files was
to define stubbed functions as 0. But I still dislike it.

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/