On Sat, 5 Nov 2011 14:17:50 +0200 Alexander Lyakas <alex.bolshoy@xxxxxxxxx>
wrote:
> Hi Neil,
> I wanted to ask how the mentoring you're willing to offer would work,
> on your opinion?
Primarily you asking questions and me answering them as helpfully as I can.
Also you proposing patches and me giving useful review, and/or queuing them
for upstream.
It could also involve me suggesting things you could work on, but I think
people are more motivated to work on things they have chosen themselves, so I
don't expect that would be a large part of it.
> I had much fun developing for that *other* operating system kernel,
> and I am looking forward to do things for Linux kernel too some day.
Great!
>
> My pain at present, is the lack of kernel printouts during various
> config sequences. I have been trying to follow, for example, the
> ADD_NEW_DISK sequence and the slot_store() sequence, to understand how
> it detects whether a full-sync or not will be required. And there are
> many possibilities before you figure out the right "if"
> (non-persistent arrays, containers, external metadata....).
>
> I am used to heavily print-around all config (non-IO-path) sequences
> (such as ADD_NEW_DISK). This also helps if a particular step in the
> sequence takes a long time, due to a slow IO or something like that.
> What will be the pros/cons of doing that in md on your opinion?
I think you are suggesting adding more pr_debug() calls in md.c. Is that
correct? I don't object to that - they are fairly cheap and can easily be
compiled out completely for those who don't want them. So feel free to send
me a patch adding some pr_debug's that you find useful, and I'll let you know
what I think of them.
NeilBrown
>
> Thanks!
> Alex.
>
>
>
> On Sun, Sep 25, 2011 at 12:10 PM, NeilBrown <neilb@xxxxxxx> wrote:
> > On Sat, 24 Sep 2011 23:57:58 +0200 Aapo Laine <aapo.laine@xxxxxxxxxxxxx>
> > wrote:
> >
> >> On 09/23/11 11:15, NeilBrown wrote:
> >> > On Fri, 23 Sep 2011 02:09:36 -0600 Thomas Fjellstrom<thomas@xxxxxxxxxxxxx>
> >> > wrote:
> >> >> I forgot to say, but: Thank you very much :) for the help, and your tireless
> >> >> work on md.
> >> >>
> >> >
> >> > You've very welcome .... but I felt I needed to respond to that word
> >> > "tireless".
> >> > The truth is that I am getting rather tired of md .... if anyone knows anyone
> >> > who wants to get into kernel development and is wondering where to start -
> >> > please consider whispering 'the md driver' in their ear. Plenty to do, great
> >> > mentoring possibilities, and competent linux kernel engineers with good
> >> > experience are unlikely to have much trouble finding a job ;-)
> >> >
> >> > NeilBrown
> >>
> >> Whoa this is shocking news!
> >
> > Hopefully not too shocking... I'm not planning on leaving md any time soon.
> > I do still enjoy working on it.
> > But it certainly isn't as fresh and new as it was 10 years again. It would
> > probably do both me and md a lot of good to have someone with new enthusiasm
> > and new perspectives...
> >
> >
> >>
> >> Firstly then, thank you so much for your excellent work up to now. Linux
> >> has what I believe to be the best software raid of all operating systems
> >> thanks to you. Excellent in both features, and reliability i.e. quality
> >> of code.
> >>
> >> And also the support through the list was great. I found so many
> >> problems solved already just by looking at the archives... so many
> >> people with their arse saved by you.
> >>
> >> I think everybody here is sorry to see you willing to go.
> >>
> >> Now the bad news... Regarding the MD takeover, there I think I see a
> >> problem...
> >> The MD code is very tersely commented, compared to its complexity!
> >
> > That is certainly true, but seems to be true across much of the kernel, and
> > probably most code in general (though I'm sure such a comment will lead to
> > people wanting to tell me their favourite exceptions ... so I'll start with
> > "TeX").
> >
> > This is one of the reasons I offered "mentoring" to any likely candidate.
> >
> >
> >
> >>
> >> - there is not much explanation of overall strategies, or the structure
> >> of code. Also the flow of data between the functions is not much
> >> explained most of the times.
> >>
> >> - It's not obvious to me what is the entry point for kernel processes
> >> related to MD arrays, how are they triggered and where do they run...
> >> E.g. in the past I tried to understand how did resync work, but I
> >> couldn't. I thought there was a kernel process controlling resync
> >> advancement, but I couldn't really find the boundaries of code inside
> >> which it was executing.
> >
> > md_do_sync() is the heart of the resync process. it calls into the
> > personality's sync_request() function.
> >
> > The kernel thread is started by md_check_recovery() if it appears to be
> > needed. md_check_recovery() is regularly run by each personality's main
> > controlling thread.
> >
> >>
> >> - it's not clear what the various functions do or in what occasion they
> >> are called. Except from their own name, most of them have no comments
> >> just before the definition.
> >
> > How about this:
> > - you identify some functions for which the purpose or use isn't clear
> > - I'll explain to you when/how/why they are used
> > - You create a patch which adds comments which explains it all
> > - I'll apply that patch.
> >
> > deal??
> >
> >>
> >> - the algoritms within the functions are very long and complex, but only
> >> rarely they are explained by comments. I am now seeing pieces having 5
> >> levels of if's nested one inside the other, and there are almost no
> >> comments.
> >
> > I feel your pain. I really should factor out the deeply nested levels into
> > separate functions. Sometimes I have done that but there is plenty more do
> > to. Again, I would be much more motivated to do this if I were working with
> > someone who would be directly helped by it. So if you identify specific
> > problems, it'll be a lot easier for me to help fix them.
> >
> >
> >>
> >> - last but not least, variables have very short names, and for most of
> >> them, it is not explained what they mean. This is mostly for local
> >> variables, but sometimes even for the structs which go into metadata
> >> e.g. in struct r1_private_data_s most members do not have an
> >> explanation. This is pretty serious, to me at least, for understanding
> >> the code.
> >
> > Does this help?
> >
> > diff --git a/drivers/md/raid1.h b/drivers/md/raid1.h
> > index e0d676b..feb44ad 100644
> > --- a/drivers/md/raid1.h
> > +++ b/drivers/md/raid1.h
> > @@ -28,42 +28,67 @@ struct r1_private_data_s {
> > mddev_t *mddev;
> > mirror_info_t *mirrors;
> > int raid_disks;
> > +
> > + /* When choose the best device for a read (read_balance())
> > + * we try to keep sequential reads one the same device
> > + * using 'last_used' and 'next_seq_sect'
> > + */
> > int last_used;
> > sector_t next_seq_sect;
> > + /* During resync, read_balancing is only allowed on the part
> > + * of the array that has been resynced. 'next_resync' tells us
> > + * where that is.
> > + */
> > + sector_t next_resync;
> > +
> > spinlock_t device_lock;
> >
> > + /* list of 'r1bio_t' that need to be processed by raid1d, whether
> > + * to retry a read, writeout a resync or recovery block, or
> > + * anything else.
> > + */
> > struct list_head retry_list;
> > - /* queue pending writes and submit them on unplug */
> > - struct bio_list pending_bio_list;
> >
> > - /* for use when syncing mirrors: */
> > + /* queue pending writes to be submitted on unplug */
> > + struct bio_list pending_bio_list;
> >
> > + /* for use when syncing mirrors:
> > + * We don't allow both normal IO and resync/recovery IO at
> > + * the same time - resync/recovery can only happen when there
> > + * is no other IO. So when either is active, the other has to wait.
> > + * See more details description in raid1.c near raise_barrier().
> > + */
> > + wait_queue_head_t wait_barrier;
> > spinlock_t resync_lock;
> > int nr_pending;
> > int nr_waiting;
> > int nr_queued;
> > int barrier;
> > - sector_t next_resync;
> > - int fullsync; /* set to 1 if a full sync is needed,
> > - * (fresh device added).
> > - * Cleared when a sync completes.
> > - */
> > - int recovery_disabled; /* when the same as
> > - * mddev->recovery_disabled
> > - * we don't allow recovery
> > - * to be attempted as we
> > - * expect a read error
> > - */
> >
> > - wait_queue_head_t wait_barrier;
> > + /* Set to 1 if a full sync is needed, (fresh device added).
> > + * Cleared when a sync completes.
> > + */
> > + int fullsync
> >
> > - struct pool_info *poolinfo;
> > + /* When the same as mddev->recovery_disabled we don't allow
> > + * recovery to be attempted as we expect a read error.
> > + */
> > + int recovery_disabled;
> >
> > - struct page *tmppage;
> >
> > + /* poolinfo contains information about the content of the
> > + * mempools - it changes when the array grows or shrinks
> > + */
> > + struct pool_info *poolinfo;
> > mempool_t *r1bio_pool;
> > mempool_t *r1buf_pool;
> >
> > + /* temporary buffer to synchronous IO when attempting to repair
> > + * a read error.
> > + */
> > + struct page *tmppage;
> > +
> > +
> > /* When taking over an array from a different personality, we store
> > * the new thread here until we fully activate the array.
> > */
> >
> >>
> >> - ... maybe more I can't think of right now ...
> >>
> >> Your code is of excellent quality, as I wrote, I wish there were more
> >> programmers like you, but if you now want to leave, THEN I start to be
> >> worried! Would you please comment it (much) more before leaving? Fully
> >> understanding your code I think is going to take other people a lot of
> >> time otherwise, and you might not find a replacement easily and/or s/he
> >> might do mistakes.
> >
> > I'm not planning on leaving - not for quite some time anyway.
> > But I know the code so well that it is hard to see which bits need
> > documenting, and what sort of documentation would really help.
> > I would love it if you (or anyone) would review the code and point to parts
> > that particularly need improvement.
> >
> >
> >>
> >> There were times in the past when I had ideas and I wanted to contribute
> >> code, but when I looked inside MD and tried to understand where should I
> >> put my changes, I realized I wasn't able to understand what current code
> >> was doing. Maybe I am not a good enough C programmer, but I was able to
> >> change things in other occasions.
> >
> > Don't be afraid to ask... But sometimes you do need a bit of persistence
> > though. :-) Not always easy to find time for that.
> >
> >
> >>
> >>
> >> I hope you won't get these critiques bad...
> >
> > Not at all.
> >
> >> and thanks for all your efforts, really, in the name of, I think, everybody.
> >> Aapo L.
> >
> > Thanks for your valuable feedback.
> > Being able to see problems is of significant value. One of the reasons that
> > I pay close attention to this list is because it shows me where the problems
> > with md and mdadm are. People often try things that I would never even dream
> > of trying (because I know they won't work). See this helps me know where the
> > code and be improved - either so what they try does work, or so it fails more
> > gracefully and helpfully.
> >
> > Thanks,
> > NeilBrown
> >
Attachment:
signature.asc
Description: PGP signature
