Re: [patch 4/4] dm-writecache: use new API for flushing

[Mike Snitzer <snitzer-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>]

On Thu, May 31 2018 at  4:16am -0400,
Mikulas Patocka <mpatocka-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> wrote:

> 
> 
> On Wed, 30 May 2018, Mike Snitzer wrote:
> 
> > On Wed, May 30 2018 at 10:09P -0400,
> > Mikulas Patocka <mpatocka-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> wrote:
> >  
> > > And what about this?
> > > #define WC_MODE_PMEM(wc)                        ((wc)->pmem_mode)
> > > 
> > > The code that I had just allowed the compiler to optimize out 
> > > persistent-memory code if we have DM_WRITECACHE_ONLY_SSD defined - and you 
> > > deleted it.
> > > 
> > > Most architectures don't have persistent memory and the dm-writecache 
> > > driver could work in ssd-only mode on them. On these architectures, I 
> > > define
> > > #define WC_MODE_PMEM(wc)                        false
> > > - and the compiler will just automatically remove the tests for that 
> > > condition and the unused branch. It does also eliminate unused static 
> > > functions.
> > 
> > Here is the patch that I just folded into the rebased version of
> > dm-writecache now in the dm-4.18 branch.
> > 
> > (I rebased ontop of Jens' latest block tree for 4.18 that now includes
> > the mempool_init changes, etc.)
> > 
> > ---
> >  drivers/md/dm-writecache.c | 17 +++++++++++++++--
> >  1 file changed, 15 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/md/dm-writecache.c b/drivers/md/dm-writecache.c
> > index fcbfaf7c27ec..691b5ffb799f 100644
> > --- a/drivers/md/dm-writecache.c
> > +++ b/drivers/md/dm-writecache.c
> > @@ -34,13 +34,21 @@
> >  #define BITMAP_GRANULARITY	PAGE_SIZE
> >  #endif
> >  
> > +#if defined(CONFIG_ARCH_HAS_PMEM_API) && IS_ENABLED(CONFIG_DAX_DRIVER)
> > +#define DM_WRITECACHE_HAS_PMEM
> > +#endif
> > +
> > +#ifdef DM_WRITECACHE_HAS_PMEM
> >  #define pmem_assign(dest, src)					\
> >  do {								\
> >  	typeof(dest) uniq = (src);				\
> >  	memcpy_flushcache(&(dest), &uniq, sizeof(dest));	\
> >  } while (0)
> > +#else
> > +#define pmem_assign(dest, src)	((dest)) = (src))
> > +#endif
> >  
> > -#if defined(__HAVE_ARCH_MEMCPY_MCSAFE) && defined(CONFIG_ARCH_HAS_PMEM_API)
> > +#if defined(__HAVE_ARCH_MEMCPY_MCSAFE) && defined(DM_WRITECACHE_HAS_PMEM)
> >  #define DM_WRITECACHE_HANDLE_HARDWARE_ERRORS
> >  #endif
> >  
> > @@ -87,8 +95,13 @@ struct wc_entry {
> >  #endif
> >  };
> >  
> > +#ifdef DM_WRITECACHE_HAS_PMEM
> >  #define WC_MODE_PMEM(wc)			((wc)->pmem_mode)
> >  #define WC_MODE_FUA(wc)				((wc)->writeback_fua)
> > +#else
> > +#define WC_MODE_PMEM(wc)			false
> > +#define WC_MODE_FUA(wc)				false
> > +#endif
> >  #define WC_MODE_SORT_FREELIST(wc)		(!WC_MODE_PMEM(wc))
> >  
> >  struct dm_writecache {
> > @@ -1857,7 +1870,7 @@ static int writecache_ctr(struct dm_target *ti, unsigned argc, char **argv)
> >  	if (!strcasecmp(string, "s")) {
> >  		wc->pmem_mode = false;
> >  	} else if (!strcasecmp(string, "p")) {
> > -#if defined(CONFIG_ARCH_HAS_PMEM_API) && IS_ENABLED(CONFIG_DAX_DRIVER)
> > +#ifdef DM_WRITECACHE_HAS_PMEM
> >  		wc->pmem_mode = true;
> >  		wc->writeback_fua = true;
> >  #else
> > -- 
> > 2.15.0
> 
> OK.
> 
> I think that persistent_memory_claim should also be conditioned based on 
> DM_WRITECACHE_HAS_PMEM - i.e. if we have DM_WRITECACHE_HAS_PMEM, we don't 
> need to use other ifdefs.
> 
> Is there some difference between "#if IS_ENABLED(CONFIG_OPTION)" and
> "#if defined(CONFIG_OPTION)"?

Not that I'm aware of:
  #define IS_ENABLED(option) __or(IS_BUILTIN(option), IS_MODULE(option))

Here is the incremental patch I just folded in:

diff --git a/drivers/md/dm-writecache.c b/drivers/md/dm-writecache.c
index fd3bc232b7d6..f2ae02f22c43 100644
--- a/drivers/md/dm-writecache.c
+++ b/drivers/md/dm-writecache.c
@@ -34,7 +34,7 @@
 #define BITMAP_GRANULARITY     PAGE_SIZE
 #endif

-#if defined(CONFIG_ARCH_HAS_PMEM_API) && IS_ENABLED(CONFIG_DAX_DRIVER)
+#if IS_ENABLED(CONFIG_ARCH_HAS_PMEM_API) && IS_ENABLED(CONFIG_DAX_DRIVER)
 #define DM_WRITECACHE_HAS_PMEM
 #endif

@@ -218,7 +218,7 @@ static void wc_unlock(struct dm_writecache *wc)
        mutex_unlock(&wc->lock);
 }

-#if IS_ENABLED(CONFIG_DAX_DRIVER)
+#ifdef DM_WRITECACHE_HAS_PMEM
 static int persistent_memory_claim(struct dm_writecache *wc)
 {
        int r;