From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mx1.redhat.com (mx3-rdu2.redhat.com [66.187.233.73]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by ml01.01.org (Postfix) with ESMTPS id EB569203B8CF9 for ; Thu, 31 May 2018 05:09:08 -0700 (PDT) Date: Thu, 31 May 2018 08:09:06 -0400 From: Mike Snitzer Subject: Re: [patch 4/4] dm-writecache: use new API for flushing Message-ID: <20180531120906.GA8456@redhat.com> References: <20180530131623.GB2106@redhat.com> <20180530132647.GB5157@redhat.com> <20180530135421.GA81788@redhat.com> <20180531033919.GA88973@redhat.com> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Errors-To: linux-nvdimm-bounces@lists.01.org Sender: "Linux-nvdimm" To: Mikulas Patocka Cc: device-mapper development , linux-nvdimm List-ID: On Thu, May 31 2018 at 4:16am -0400, Mikulas Patocka wrote: > > > On Wed, 30 May 2018, Mike Snitzer wrote: > > > On Wed, May 30 2018 at 10:09P -0400, > > Mikulas Patocka 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; _______________________________________________ Linux-nvdimm mailing list Linux-nvdimm@lists.01.org https://lists.01.org/mailman/listinfo/linux-nvdimm