Re: [RFC PATCH 1/3] md/isrt: base infrastructure and metadata loading

[NeilBrown <neilb@suse.de>]

[-- Attachment #1: Type: text/plain, Size: 1693 bytes --]

On Wed, 23 Apr 2014 23:18:49 -0700 Dan Williams <dan.j.williams@intel.com>
wrote:

> Initial md / block boilerplate for the Intel (R) Smart Response
> Technology compatibility driver.  Supports reading the packed  metadata
> and parsing it into a cache lookup tree.
> 
> Cc: Dave Jiang <dave.jiang@intel.com>
> Cc: Artur Paszkiewicz <artur.paszkiewicz@intel.com>
> Signed-off-by: Dan Williams <dan.j.williams@intel.com>

It would really help in reviewing this to have a glossary.

There are frames and segments and sectors and pages.

I hope sectors are 512 bytes and pages are PAGE_SIZE, which may or may not be
4096.

And there are 16 sectors per frame, so I guess space is allocated in the
cache in 8K aligned frames ??

There are 64 segments per page so if pages did happen to be 4096 bytes, that
makes 64 bytes per segment.  What are they?

There is a list somewhere of 32byte frame descriptors which is read into a
single vmalloced region (why? you keep page pointers, so why not read it into
separate pages?)
How is this organised?  I might be able to work that out from the code, but
I'd rather not.

Please don't make me guess, I'm not good at it.

I guess it didn't help that diff out the header after the code.  I got bored
before I got there and didn't read all to words, so maybe some answers are in
there.  They don't really stand out though.


You've chosen '8' for the 'level' number.
As this is an array which doesn't have redundancy, I'd rather a number <= 0.
I think there are places where I assume >=1 has redundancy and understands
spares etc.

Should conf->count be a kref??? Just a thought, not a requirement.


NeilBrown


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 828 bytes --]