Re: Ask help for code review (was Re: [PATCH 03/14] bcache: add initial data structures for nvm pages)

From: Coly Li <colyli@suse.de>
To: Christoph Hellwig <hch@lst.de>
Cc: "Huang, Ying" <ying.huang@intel.com>,
	Dan Williams <dan.j.williams@intel.com>, Jan Kara <jack@suse.com>,
	Hannes Reinecke <hare@suse.com>,
	linux-bcache@vger.kernel.org, linux-block@vger.kernel.org,
	Jianpeng Ma <jianpeng.ma@intel.com>,
	Qiaowei Ren <qiaowei.ren@intel.com>,
	axboe@kernel.dk
Subject: Re: Ask help for code review (was Re: [PATCH 03/14] bcache: add initial data structures for nvm pages)
Date: Wed, 23 Jun 2021 18:05:51 +0800	[thread overview]
Message-ID: <466c1678-8cdc-7240-1422-b435a599cad3@suse.de> (raw)
In-Reply-To: <20210623072140.GA837@lst.de>

On 6/23/21 3:21 PM, Christoph Hellwig wrote:
> On Wed, Jun 23, 2021 at 03:19:11PM +0800, Coly Li wrote:
>> Bcache does not support endian clean indeed,
> Then we need to fix that eventually rather than making it worse.  Which
> means any _new_ data structure should start that way.

The cache device (typically SSD) of bcache is designed to dedicate to a
single local machine. Any
storage migration between machines with different endians should firstly
flush the dirty data to
backing hard drive. For bcache meta-data on cache device (typically
SSD), it is designed to NOT
move dirty cache between machines with different endians, and in
practice there is no such use
case indeed and not supported by any Linux Distribution.

Not supporting different endian is as design, why we should fix it for
no real use case ?

BTW, the discussion is only for cache device because the bcache meta
data stored on it. For
backing hard drive, its endian is transparent to bcache and decided by
upper layer code like
file system or user space application, it is fully endian clean.

>> and libnvdimm only works with
>> 64bit physical address width.
> Maybe it does right now.  But ther is nothing fundamental in that, so
> please don't design stupid on-disk formats to encode that are going to
> come back to bite us sooner or later.  Be that by adding 32-bit support
> for any Linux DAX device, or by new 96 or 128bit CPUs.

This is unfair restriction :-)
The nvdimm support for bcache heavily depends on libnvdimm, that is, for
all conditions that libnvdimm
supports we should follow up. But requiring us to support the condition
that even libnvdimm does not
support yet, it is too early at this stage.

And, if libnvdimm (not DAX) supports 32-bit or new 96 or 128bit CPUs,
considering the data sturectures
are arrays and single lists,  it won't be too complicated to follow up.

>> The only restriction here by using pointer is
>> the CPU register word should be 64bits, because we use the NVDIMM as memory.
>>
>> Is it one of the way how NVDIMM (especially Intel AEP) designed to use ?
>> As a non-volatiled memory.
> Not for on-disk data structures.

This is not on-disk data structure. We use the NVDIMM as memory, and
access the internal data
structures as current existing code does onto DRAM.

Please encourage us to have a series try with this might-be-different idea.

>> Does the already mapped DAX base address change in runtime during memory
>> hot plugable ?
>> If not, it won't be a problem here for this specific use case.
> It could change between one use and another.

Hmm, I don't understand the implicit meaning of the above line.
Could you please offer a detail example ?

Thank you for looking at this and provide value comment. All the above
response is not argument or
stubbornness, I do want to have a clear understand by the discussion
with you, that we won't regret
in future for current design.

Coly Li