Re: [PATCH] dm writecache: skip writecache_wait for pmem mode

Re: [PATCH] dm writecache: skip writecache_wait for pmem mode

[Huaisheng HS1 Ye]

> -----Original Message-----
> From: Mikulas Patocka <mpatocka@redhat.com>
> Sent: Wednesday, September 4, 2019 11:36 PM
> On Wed, 4 Sep 2019, Huaisheng HS1 Ye wrote:
> 
> >
> > Hi Mikulas,
> >
> > Thanks for your reply, I see what you mean, but I can't agree with you.
> >
> > For pmem mode, this code path (writecache_flush) is much more hot than
> > SSD mode. Because in the code, the AUTOCOMMIT_BLOCKS_PMEM has been
> > defined to 64, which means if more than 64 blocks have been inserted
> > to cache device, also called uncommitted, writecache_flush would be called.
> > Otherwise, there is a timer callback function will be called every
> > 1000 milliseconds.
> >
> > #define AUTOCOMMIT_BLOCKS_SSD		65536
> > #define AUTOCOMMIT_BLOCKS_PMEM		64
> > #define AUTOCOMMIT_MSEC			1000
> >
> > So when dm-writecache running in working mode, there are continuous
> > WRITE operations has been mapped to writecache_map, writecache_flush
> > will be used much more often than SSD mode.
> >
> > Cheers,
> > Huaisheng Ye
> 
> So, you save one instruction cache line for every 64*4096 bytes written to
> persistent memory.
> 
> If you insist on it, I can acknowledge it, but I think it is really an
> over-optimization.
> 
> Acked-By: Mikulas Patocka <mpatocka@redhat.com>
> 
> Mikulas

Thanks for your Acked-by, I have learned so much from your code.

And I have another question about the LRU.
Current code only put the last written blocks into the front of list wc->lru, READ hit doesn't affect the position of block in wc->lru.
That is to say, if a block has been written to cache device, even there would be a lot of READ operation for that block next but without WRITE hit, which still would flow to the end of wc->lru, and eventually it would be written back.

I am not sure whether this behavior disobeys LRU principle or not. But if this situation above appears, that would lead to some HOT blocks (without WRITE hit) had been written back, even READ hit many times.
Is it worth submitting patch to adjust the position of blocks when READ hit?
Just a discussion, I want to know your design idea.

Cheers,
Huaisheng Ye

Re: [PATCH] dm writecache: skip writecache_wait for pmem mode

[Mikulas Patocka]

On Thu, 5 Sep 2019, Huaisheng HS1 Ye wrote:

> > -----Original Message-----
> > From: Mikulas Patocka <mpatocka@redhat.com>
> > Sent: Wednesday, September 4, 2019 11:36 PM
> > On Wed, 4 Sep 2019, Huaisheng HS1 Ye wrote:
> > 
> > >
> > > Hi Mikulas,
> > >
> > > Thanks for your reply, I see what you mean, but I can't agree with you.
> > >
> > > For pmem mode, this code path (writecache_flush) is much more hot than
> > > SSD mode. Because in the code, the AUTOCOMMIT_BLOCKS_PMEM has been
> > > defined to 64, which means if more than 64 blocks have been inserted
> > > to cache device, also called uncommitted, writecache_flush would be called.
> > > Otherwise, there is a timer callback function will be called every
> > > 1000 milliseconds.
> > >
> > > #define AUTOCOMMIT_BLOCKS_SSD		65536
> > > #define AUTOCOMMIT_BLOCKS_PMEM		64
> > > #define AUTOCOMMIT_MSEC			1000
> > >
> > > So when dm-writecache running in working mode, there are continuous
> > > WRITE operations has been mapped to writecache_map, writecache_flush
> > > will be used much more often than SSD mode.
> > >
> > > Cheers,
> > > Huaisheng Ye
> > 
> > So, you save one instruction cache line for every 64*4096 bytes written to
> > persistent memory.
> > 
> > If you insist on it, I can acknowledge it, but I think it is really an
> > over-optimization.
> > 
> > Acked-By: Mikulas Patocka <mpatocka@redhat.com>
> > 
> > Mikulas
> 
> Thanks for your Acked-by, I have learned so much from your code.
> 
> And I have another question about the LRU.
> 
> Current code only put the last written blocks into the front of list 
> wc->lru, READ hit doesn't affect the position of block in wc->lru. That 
> is to say, if a block has been written to cache device, even there would 
> be a lot of READ operation for that block next but without WRITE hit, 
> which still would flow to the end of wc->lru, and eventually it would be 
> written back.
> 
> I am not sure whether this behavior disobeys LRU principle or not. But 
> if this situation above appears, that would lead to some HOT blocks 
> (without WRITE hit) had been written back, even READ hit many times. Is 
> it worth submitting patch to adjust the position of blocks when READ 
> hit? Just a discussion, I want to know your design idea.
> 
> Cheers,
> Huaisheng Ye

The dm-writecache target is supposed to optimize writes, not reads. 
Normally, there won't be any reads following a write, because the data 
would be stored in the cache in RAM.

Mikulas

RE: [External] Re: [PATCH] dm writecache: skip writecache_wait for pmem mode

[Huaisheng HS1 Ye]

> -----Original Message-----
> From: Mikulas Patocka <mpatocka@redhat.com>
> Sent: Wednesday, September 18, 2019 6:24 PM
> The dm-writecache target is supposed to optimize writes, not reads.
> Normally, there won't be any reads following a write, because the data would be
> stored in the cache in RAM.
> 
> Mikulas

Got it, Thanks for pointing that out.

Cheers,
Huaisheng Ye | 叶怀胜
Linux kernel | Lenovo DCG

Re: [PATCH] dm writecache: skip writecache_wait for pmem mode

[Mikulas Patocka]

On Mon, 2 Sep 2019, Huaisheng Ye wrote:

> From: Huaisheng Ye <yehs1@lenovo.com>
> 
> The array bio_in_progress[2] only have chance to be increased and
> decreased with ssd mode. For pmem mode, they are not involved at all.
> So skip writecache_wait_for_ios in writecache_flush for pmem.
> 
> Suggested-by: Doris Yu <tyu1@lenovo.com>
> Signed-off-by: Huaisheng Ye <yehs1@lenovo.com>
> ---
>  drivers/md/dm-writecache.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/md/dm-writecache.c b/drivers/md/dm-writecache.c
> index c481947..d06b8aa 100644
> --- a/drivers/md/dm-writecache.c
> +++ b/drivers/md/dm-writecache.c
> @@ -726,7 +726,8 @@ static void writecache_flush(struct dm_writecache *wc)
>  	}
>  	writecache_commit_flushed(wc);
>  
> -	writecache_wait_for_ios(wc, WRITE);
> +	if (!WC_MODE_PMEM(wc))
> +		writecache_wait_for_ios(wc, WRITE);
>  
>  	wc->seq_count++;
>  	pmem_assign(sb(wc)->seq_count, cpu_to_le64(wc->seq_count));
> -- 
> 1.8.3.1

I think this is not needed - wait_event in writecache_wait_for_ios exits 
immediatelly if the condition is true.

This code path is not so hot that we would need microoptimizations like 
this to avoid function calls.

Mikulas

[PATCH] dm writecache: skip writecache_wait for pmem mode

[Huaisheng Ye]

From: Huaisheng Ye <yehs1@lenovo.com>

The array bio_in_progress[2] only have chance to be increased and
decreased with ssd mode. For pmem mode, they are not involved at all.
So skip writecache_wait_for_ios in writecache_flush for pmem.

Suggested-by: Doris Yu <tyu1@lenovo.com>
Signed-off-by: Huaisheng Ye <yehs1@lenovo.com>
---
 drivers/md/dm-writecache.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/md/dm-writecache.c b/drivers/md/dm-writecache.c
index c481947..d06b8aa 100644
--- a/drivers/md/dm-writecache.c
+++ b/drivers/md/dm-writecache.c
@@ -726,7 +726,8 @@ static void writecache_flush(struct dm_writecache *wc)
 	}
 	writecache_commit_flushed(wc);
 
-	writecache_wait_for_ios(wc, WRITE);
+	if (!WC_MODE_PMEM(wc))
+		writecache_wait_for_ios(wc, WRITE);
 
 	wc->seq_count++;
 	pmem_assign(sb(wc)->seq_count, cpu_to_le64(wc->seq_count));
-- 
1.8.3.1