linux-kernel - RE: [External] Re: [PATCH] dm writecache: skip writecache

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Wed, 4 Sep 2019 15:18:42 +0000
From:   Huaisheng HS1 Ye <yehs1@...ovo.com>
To:     Mikulas Patocka <mpatocka@...hat.com>,
        "snitzer@...hat.com" <snitzer@...hat.com>
CC:     "agk@...hat.com" <agk@...hat.com>,
        "prarit@...hat.com" <prarit@...hat.com>,
        Tzu ting Yu1 <tyu1@...ovo.com>,
        "dm-devel@...hat.com" <dm-devel@...hat.com>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        Huaisheng Ye <yehs2007@...o.com>
Subject: RE: [External]  Re: [PATCH] dm writecache: skip writecache_wait for
 pmem mode

> -----Original Message-----
> From: Mikulas Patocka <mpatocka@...hat.com>
> Sent: Wednesday, September 4, 2019 4:49 PM
> On Mon, 2 Sep 2019, Huaisheng Ye wrote:
> 
> > From: Huaisheng Ye <yehs1@...ovo.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@...ovo.com>
> > Signed-off-by: Huaisheng Ye <yehs1@...ovo.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.

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