[PATCH v3 0/5] Optimize writecache when using pmem as cache

[PATCH v3 0/5] Optimize writecache when using pmem as cache

[Huaisheng Ye]

From: Huaisheng Ye <yehs1@lenovo.com>

This patch set could be used for dm-writecache when use persistent
memory as cache data device.

Patch 1 and 2 go towards removing unused parameter and codes which
actually doesn't really work.

Patch 3 and 4 are targeted at solving problem fn ctr failed to work
due to invalid magic or version, which is caused by the super block
of pmem has messy data stored.

Patch 5 is used for getting the status of seq_count.

Changes Since v2: 
        - seq_count is important for flush operations, output it within status
          for debugging and analyzing code behavior.
	[1]: https://lkml.org/lkml/2019/1/3/43
        [2]: https://lkml.org/lkml/2019/1/9/6

Huaisheng Ye (5):
  dm-writecache: remove unused size to writecache_flush_region
  dm-writecache: get rid of memory_data flush to writecache_flush_entry
  dm-writecache: expand pmem_reinit for struct dm_writecache
  Documentation/device-mapper: add optional parameter reinit
  dm-writecache: output seq_count within status

 Documentation/device-mapper/writecache.txt |  4 ++++
 drivers/md/dm-writecache.c                 | 23 +++++++++++++----------
 2 files changed, 17 insertions(+), 10 deletions(-)

-- 
1.8.3.1

[PATCH v3 1/5] dm-writecache: remove unused size to writecache_flush_region

[Huaisheng Ye]

From: Huaisheng Ye <yehs1@lenovo.com>

writecache_flush_region doesn't use size to calculate flush region.
That uses _set_bits to mark the region in dirty_bitmap directly.

Signed-off-by: Huaisheng Ye <yehs1@lenovo.com>
---
 drivers/md/dm-writecache.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/drivers/md/dm-writecache.c b/drivers/md/dm-writecache.c
index 2d50eec..2d8e0c0 100644
--- a/drivers/md/dm-writecache.c
+++ b/drivers/md/dm-writecache.c
@@ -418,7 +418,7 @@ static void writecache_flush_all_metadata(struct dm_writecache *wc)
 		memset(wc->dirty_bitmap, -1, wc->dirty_bitmap_size);
 }
 
-static void writecache_flush_region(struct dm_writecache *wc, void *ptr, size_t size)
+static void writecache_flush_region(struct dm_writecache *wc, void *ptr)
 {
 	if (!WC_MODE_PMEM(wc))
 		__set_bit(((char *)ptr - (char *)wc->memory_map) / BITMAP_GRANULARITY,
@@ -657,7 +657,7 @@ static void writecache_free_entry(struct dm_writecache *wc, struct wc_entry *e)
 	writecache_unlink(wc, e);
 	writecache_add_to_freelist(wc, e);
 	clear_seq_count(wc, e);
-	writecache_flush_region(wc, memory_entry(wc, e), sizeof(struct wc_memory_entry));
+	writecache_flush_region(wc, memory_entry(wc, e));
 	if (unlikely(waitqueue_active(&wc->freelist_wait)))
 		wake_up(&wc->freelist_wait);
 }
@@ -687,9 +687,9 @@ static void writecache_poison_lists(struct dm_writecache *wc)
 
 static void writecache_flush_entry(struct dm_writecache *wc, struct wc_entry *e)
 {
-	writecache_flush_region(wc, memory_entry(wc, e), sizeof(struct wc_memory_entry));
+	writecache_flush_region(wc, memory_entry(wc, e));
 	if (WC_MODE_PMEM(wc))
-		writecache_flush_region(wc, memory_data(wc, e), wc->block_size);
+		writecache_flush_region(wc, memory_data(wc, e));
 }
 
 static bool writecache_entry_is_committed(struct dm_writecache *wc, struct wc_entry *e)
@@ -733,7 +733,7 @@ static void writecache_flush(struct dm_writecache *wc)
 
 	wc->seq_count++;
 	pmem_assign(sb(wc)->seq_count, cpu_to_le64(wc->seq_count));
-	writecache_flush_region(wc, &sb(wc)->seq_count, sizeof sb(wc)->seq_count);
+	writecache_flush_region(wc, &sb(wc)->seq_count);
 	writecache_commit_flushed(wc);
 
 	wc->overwrote_committed = false;
@@ -1757,7 +1757,7 @@ static int init_memory(struct dm_writecache *wc)
 	writecache_flush_all_metadata(wc);
 	writecache_commit_flushed(wc);
 	pmem_assign(sb(wc)->magic, cpu_to_le32(MEMORY_SUPERBLOCK_MAGIC));
-	writecache_flush_region(wc, &sb(wc)->magic, sizeof sb(wc)->magic);
+	writecache_flush_region(wc, &sb(wc)->magic);
 	writecache_commit_flushed(wc);
 
 	return 0;
-- 
1.8.3.1

[PATCH v3 2/5] dm-writecache: get rid of memory_data flush to writecache_flush_entry

[Huaisheng Ye]

From: Huaisheng Ye <yehs1@lenovo.com>

writecache_flush_region only works when SSD mode.
If wc->pmem_mode sets, writecache_flush_region doesn't need to be called in
writecache_flush_entry.

Signed-off-by: Huaisheng Ye <yehs1@lenovo.com>
---
 drivers/md/dm-writecache.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/drivers/md/dm-writecache.c b/drivers/md/dm-writecache.c
index 2d8e0c0..c69317c 100644
--- a/drivers/md/dm-writecache.c
+++ b/drivers/md/dm-writecache.c
@@ -688,8 +688,6 @@ static void writecache_poison_lists(struct dm_writecache *wc)
 static void writecache_flush_entry(struct dm_writecache *wc, struct wc_entry *e)
 {
 	writecache_flush_region(wc, memory_entry(wc, e));
-	if (WC_MODE_PMEM(wc))
-		writecache_flush_region(wc, memory_data(wc, e));
 }
 
 static bool writecache_entry_is_committed(struct dm_writecache *wc, struct wc_entry *e)
-- 
1.8.3.1

[PATCH v3 3/5] dm-writecache: expand pmem_reinit for struct dm_writecache

[Huaisheng Ye]

From: Huaisheng Ye <yehs1@lenovo.com>

When use persistent memory as cache data device, sometimes
the super block of pmem has messy data stored in it. That would
have risk to lead fn ctr failed to work due to invalid magic or
version.

Here we expand pmem_reinit to optional parameters in order to solve
this issue. When user gets pmem device, which has unrelated data,
as cache device, he should use paramenter 'reinit' to avoid s.magic
and s.version don't equal to NULL or correct
MEMORY_SUPERBLOCK_MAGIC/VERSION.

Signed-off-by: Huaisheng Ye <yehs1@lenovo.com>
---
 drivers/md/dm-writecache.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/drivers/md/dm-writecache.c b/drivers/md/dm-writecache.c
index c69317c..2c1e825 100644
--- a/drivers/md/dm-writecache.c
+++ b/drivers/md/dm-writecache.c
@@ -149,6 +149,7 @@ struct dm_writecache {
 
 	bool pmem_mode:1;
 	bool writeback_fua:1;
+	bool pmem_reinit:1;
 
 	bool overwrote_committed:1;
 	bool memory_vmapped:1;
@@ -2026,6 +2027,10 @@ static int writecache_ctr(struct dm_target *ti, unsigned argc, char **argv)
 				wc->writeback_fua = false;
 				wc->writeback_fua_set = true;
 			} else goto invalid_optional;
+		} else if (!strcasecmp(string, "reinit")) {
+			if (WC_MODE_PMEM(wc))
+				wc->pmem_reinit = true;
+			else goto invalid_optional;
 		} else {
 invalid_optional:
 			r = -EINVAL;
@@ -2127,7 +2132,7 @@ static int writecache_ctr(struct dm_target *ti, unsigned argc, char **argv)
 		ti->error = "Hardware memory error when reading superblock";
 		goto bad;
 	}
-	if (!le32_to_cpu(s.magic) && !le32_to_cpu(s.version)) {
+	if (wc->pmem_reinit || (!le32_to_cpu(s.magic) && !le32_to_cpu(s.version))) {
 		r = init_memory(wc);
 		if (r) {
 			ti->error = "Unable to initialize device";
-- 
1.8.3.1

[PATCH v3 4/5] Documentation/device-mapper: add optional parameter reinit

[Huaisheng Ye]

From: Huaisheng Ye <yehs1@lenovo.com>

Add intro and usage guide for reinit.

Signed-off-by: Huaisheng Ye <yehs1@lenovo.com>
---
 Documentation/device-mapper/writecache.txt | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/Documentation/device-mapper/writecache.txt b/Documentation/device-mapper/writecache.txt
index 01532b3..255c68c 100644
--- a/Documentation/device-mapper/writecache.txt
+++ b/Documentation/device-mapper/writecache.txt
@@ -45,6 +45,10 @@ Constructor parameters:
 		afterwards
 		- some underlying devices perform better with fua, some
 		  with nofua. The user should test it
+	reinit		(by default off)
+		applicable only to persistent memory - use the REINIT flag
+		when the surper block has messy data, that would cause fn ctr
+		failed to work with invalid magic or version in the superblock
 
 Status:
 1. error indicator - 0 if there was no error, otherwise error number
-- 
1.8.3.1

[PATCH v3 5/5] dm-writecache: output seq_count within status

[Huaisheng Ye]

From: Huaisheng Ye <yehs1@lenovo.com>

seq_count is important for flush operations, output it within status
for debugging and analyzing code behavior.

Signed-off-by: Huaisheng Ye <yehs1@lenovo.com>
---
 drivers/md/dm-writecache.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/md/dm-writecache.c b/drivers/md/dm-writecache.c
index 2c1e825..3396710 100644
--- a/drivers/md/dm-writecache.c
+++ b/drivers/md/dm-writecache.c
@@ -2231,9 +2231,9 @@ static void writecache_status(struct dm_target *ti, status_type_t type,
 
 	switch (type) {
 	case STATUSTYPE_INFO:
-		DMEMIT("%ld %llu %llu %llu", writecache_has_error(wc),
+		DMEMIT("%ld %llu %llu %llu %llu", writecache_has_error(wc),
 		       (unsigned long long)wc->n_blocks, (unsigned long long)wc->freelist_size,
-		       (unsigned long long)wc->writeback_size);
+		       (unsigned long long)wc->writeback_size, wc->seq_count);
 		break;
 	case STATUSTYPE_TABLE:
 		DMEMIT("%c %s %s %u ", WC_MODE_PMEM(wc) ? 'p' : 's',
-- 
1.8.3.1

Re: [PATCH v3 0/5] Optimize writecache when using pmem as cache

[Mikulas Patocka]

On Thu, 31 Jan 2019, Huaisheng Ye wrote:

> From: Huaisheng Ye <yehs1@lenovo.com>
> 
> This patch set could be used for dm-writecache when use persistent
> memory as cache data device.
> 
> Patch 1 and 2 go towards removing unused parameter and codes which
> actually doesn't really work.

I agree that there is some unused variables and code, but I would let it 
be as it is. The processors can write data to persistent memory either by 
using non-temporal stores (the movnti instruction) or by normal stores 
followed by the clwb instruction.

Currently, the movnti instruction is faster - however, it may be possible 
that with some newer processors, the clwb instruction could be faster - 
and in that case, we need the code that you have removed.

I would like to keep both flush strategies around (movnti and clwb), so 
that we can test how do they perform on various processors. 
Unfortunatelly, some upstream developers hate code with #ifdefs :-(

Note that compiler optimizations already remove the unused parameter and 
the impossible code behind "if (WC_MODE_PMEM(wc)) if (!WC_MODE_PMEM(wc))".

> Patch 3 and 4 are targeted at solving problem fn ctr failed to work
> due to invalid magic or version, which is caused by the super block
> of pmem has messy data stored.

LVM zeros the beginning of new logical volumes, so there should be no 
problem with it. If the user wants to use the writecache target without 
LVM, he should zero the superblock with dd if=/dev/zero of=/dev/pmem0 
bs=4k count=1

Note that other device mapper targets also follow this policy - for 
example see drivers/md/dm-snap-persistent.c:
        if (le32_to_cpu(dh->magic) == 0) {
                *new_snapshot = 1;
                return 0;
        }

        if (le32_to_cpu(dh->magic) != SNAP_MAGIC) {
                DMWARN("Invalid or corrupt snapshot");
                r = -ENXIO;
                goto bad;
        }

So, I think there is no need for these patches - dm-writecache just does 
what others targets do.

> Patch 5 is used for getting the status of seq_count.

It may be accepted if other LVM team members find some use for this value.

Mikulas

> Changes Since v2: 
>         - seq_count is important for flush operations, output it within status
>           for debugging and analyzing code behavior.
> 	[1]: https://lkml.org/lkml/2019/1/3/43
>         [2]: https://lkml.org/lkml/2019/1/9/6
> 
> Huaisheng Ye (5):
>   dm-writecache: remove unused size to writecache_flush_region
>   dm-writecache: get rid of memory_data flush to writecache_flush_entry
>   dm-writecache: expand pmem_reinit for struct dm_writecache
>   Documentation/device-mapper: add optional parameter reinit
>   dm-writecache: output seq_count within status
> 
>  Documentation/device-mapper/writecache.txt |  4 ++++
>  drivers/md/dm-writecache.c                 | 23 +++++++++++++----------
>  2 files changed, 17 insertions(+), 10 deletions(-)
> 
> -- 
> 1.8.3.1
> 
> 

RE: [External] Re: [PATCH v3 0/5] Optimize writecache when using pmem as cache

[Huaisheng HS1 Ye]

> From: Mikulas Patocka <mpatocka@redhat.com>
> Sent: Monday, February 04, 2019 7:28 PM
> On Thu, 31 Jan 2019, Huaisheng Ye wrote:
> 
> > From: Huaisheng Ye <yehs1@lenovo.com>
> >
> > This patch set could be used for dm-writecache when use persistent
> > memory as cache data device.
> >
> > Patch 1 and 2 go towards removing unused parameter and codes which
> > actually doesn't really work.
> 
> I agree that there is some unused variables and code, but I would let it
> be as it is. The processors can write data to persistent memory either by
> using non-temporal stores (the movnti instruction) or by normal stores
> followed by the clwb instruction.
> 
> Currently, the movnti instruction is faster - however, it may be possible
> that with some newer processors, the clwb instruction could be faster -
> and in that case, we need the code that you have removed.
> 
> I would like to keep both flush strategies around (movnti and clwb), so
> that we can test how do they perform on various processors.
> Unfortunatelly, some upstream developers hate code with #ifdefs :-(
> 
> Note that compiler optimizations already remove the unused parameter and
> the impossible code behind "if (WC_MODE_PMEM(wc)) if (!WC_MODE_PMEM(wc))".

Hi Mikulas,

Thanks for your reply, now I could understand the code flow of dm-writecache better
than before.

In the process of playing around the code, I found that writecache_flush would try
to free earlier committed entry with lower seq-count. More seriously is that, writecache_flush
must check it for all entries which hasn't been committed. That's a lot of work to do when
there are many entries need to be flushed.

I have a plan for writecache_map to avoid using free entry when the committed writecache
has been hit. Does it make sense to simple the code flow, especially for saving additional rb-tree
node insertion and free steps?

Cheers,
Huaisheng Ye

> > Patch 3 and 4 are targeted at solving problem fn ctr failed to work
> > due to invalid magic or version, which is caused by the super block
> > of pmem has messy data stored.
> 
> LVM zeros the beginning of new logical volumes, so there should be no
> problem with it. If the user wants to use the writecache target without
> LVM, he should zero the superblock with dd if=/dev/zero of=/dev/pmem0
> bs=4k count=1
> 
> Note that other device mapper targets also follow this policy - for
> example see drivers/md/dm-snap-persistent.c:
>         if (le32_to_cpu(dh->magic) == 0) {
>                 *new_snapshot = 1;
>                 return 0;
>         }
> 
>         if (le32_to_cpu(dh->magic) != SNAP_MAGIC) {
>                 DMWARN("Invalid or corrupt snapshot");
>                 r = -ENXIO;
>                 goto bad;
>         }
> 
> So, I think there is no need for these patches - dm-writecache just does
> what others targets do.
> 
> > Patch 5 is used for getting the status of seq_count.
> 
> It may be accepted if other LVM team members find some use for this value.
> 
> Mikulas
> 
> > Changes Since v2:
> >         - seq_count is important for flush operations, output it within status
> >           for debugging and analyzing code behavior.
> > 	[1]: https://lkml.org/lkml/2019/1/3/43
> >         [2]: https://lkml.org/lkml/2019/1/9/6
> >
> > Huaisheng Ye (5):
> >   dm-writecache: remove unused size to writecache_flush_region
> >   dm-writecache: get rid of memory_data flush to writecache_flush_entry
> >   dm-writecache: expand pmem_reinit for struct dm_writecache
> >   Documentation/device-mapper: add optional parameter reinit
> >   dm-writecache: output seq_count within status
> >
> >  Documentation/device-mapper/writecache.txt |  4 ++++
> >  drivers/md/dm-writecache.c                 | 23 +++++++++++++----------
> >  2 files changed, 17 insertions(+), 10 deletions(-)
> >
> > --
> > 1.8.3.1
> >
> >