[dm-devel] [PATCH] dm-writecache

[Christoph Hellwig]

On Thu, Mar 08, 2018 at 10:26:17PM -0500, Mikulas Patocka wrote:
> > no business having this around.
> 
> It's the default setting of the flag wc->writeback_fua (it can be changed 
> with target parameters). The flag selects whether the target uses FUA 
> requests when doing writeback or whether it uses non-FUA requests and 
> FLUSH afterwards. For some block devices, FUA is faster, for some 
> nonFUA+FLUSH is faster.

So just use true as the default flag, adding a name for it in addition
to the field it is assigned to makes no sense at all.

> > > +#ifndef bio_set_dev
> > > +#define	bio_set_dev(bio, dev)	((bio)->bi_bdev = (dev))
> > > +#endif
> > > +#ifndef timer_setup
> > > +#define timer_setup(t, c, f)	setup_timer(t, c, (unsigned long)(t))
> > > +#endif
> > 
> > no business in mainline.
> 
> People removed dax support for ramdisk in 4.15.
> 
> If I need to test it on non-x86 architecture, I need ramdisk as a fake dax 
> device - and that only works up to 4.14. These defines are for 4.14 
> compatibility.

So add them when you backport, or use the existing automated backport
frameworks.  But do not add dead code to an upstream submission.

> > > +#if defined(CONFIG_X86_64)
> > > +#define NT_STORE(dest, src)				\
> > > +do {							\
> > > +	typeof(src) val = (src);			\
> > > +	memcpy_flushcache(&(dest), &val, sizeof(src));	\
> > > +} while (0)
> > > +#define COMMIT_FLUSHED()	wmb()
> > > +#else
> > > +#define NT_STORE(dest, src)	WRITE_ONCE(dest, src)
> > > +#define FLUSH_RANGE		dax_flush
> > > +#define COMMIT_FLUSHED()	do { } while (0)
> > > +#endif
> > 
> > Please use proper APIs for this, this has no business in a driver.
> > 
> > And that's it for now.  This is clearly not submission ready, and I
> > should got back to my backlog of other things.
> 
> Why is memcpy_flushcache and dax_flush "improper"? What should I use 
> instead of them?

They are proper and should be used directly instead of through your
hacky macros.