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

[NeilBrown <neilb@suse.de>]

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

On Thu, 24 Apr 2014 00:38:01 -0700 Dan Williams <dan.j.williams@intel.com>
wrote:

> On Thu, Apr 24, 2014 at 12:24 AM, NeilBrown <neilb@suse.de> wrote:
> > 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.
> 
> No, they don't.  Let me throw together a proper cheat sheet.

Thanks.


> 
> > You've chosen '8' for the 'level' number.
> 
> Hmm, ok.  I use -12 in the mdadm bits, I neglected to go back and fix
> up the kernel.
> 
> > 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.
> 
> Doesn't kref == 0 imply object destroyed?  It's a count of pending
> metadata events.

kref means that in a kobject.  Elsewhere it means whatever you want.

mpb_read_endio would kref_put(&conf->ref, release)

where release would get the conf and wake_up(&conf->eventq);

It probably isn't a big win..

NeilBrown

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