lkml.org 
[lkml]   [2006]   [Jan]   [20]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [PATCH 001 of 5] md: Split disks array out of raid5 conf structure so it is easier to grow.
    >>>>> "Neil" == Neil Brown <neilb@suse.de> writes:

    Neil> On Tuesday January 17, john@stoffel.org wrote:
    >> >>>>> "NeilBrown" == NeilBrown <neilb@suse.de> writes:
    >>
    NeilBrown> Previously the array of disk information was included in
    NeilBrown> the raid5 'conf' structure which was allocated to an
    NeilBrown> appropriate size. This makes it awkward to change the size
    NeilBrown> of that array. So we split it off into a separate
    NeilBrown> kmalloced array which will require a little extra indexing,
    NeilBrown> but is much easier to grow.

    >> Instead of setting mddev->private = NULL, should you be doing a kfree
    >> on it as well when you are in an abort state?

    Neil> The only times I set
    mddev-> private = NULL
    Neil> it is immediately after
    Neil> kfree(conf)
    Neil> and as conf is the thing that is assigned to mddev->private, this
    Neil> should be doing exactly what you suggest.

    Neil> Does that make sense?

    Now that I've had some time to actually apply your patches to
    2.6.16-rc1 and look them over more carefully, I see my mistake. I
    overlooked the assignment of

    conf = mddev->private

    In the lines just below there, and I see how you do clean it up. I
    guess I would have just done it the other way around:

    conf = kzalloc()
    if (!conf)
    goto abort:
    .
    .
    .
    mddev->private = conf;

    Though now that I look at it, don't we have a circular reference
    here? Let me quote the code section, which starts of with where I was
    confused:

    mddev->private = kzalloc(sizeof (raid5_conf_t), GFP_KERNEL);
    if ((conf = mddev->private) == NULL)
    goto abort;
    conf->disks = kzalloc(mddev->raid_disks * sizeof(struct disk_info),
    GFP_KERNEL);
    if (!conf->disks)
    goto abort;

    conf->mddev = mddev;

    if ((conf->stripe_hashtbl = kzalloc(PAGE_SIZE, GFP_KERNEL)) == NULL)
    goto abort;


    Now we seem to end up with:

    mddev->private = conf;
    conf->mddev = mddev;

    which looks a little strange to me, and possibly something that could
    lead to endless loops. But I need to find the time to sit down and
    try to understand the code, so don't waste much time educating me
    here.

    Thanks for all your work on this Neil, I for one really appreciate it!

    John
    -
    To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
    the body of a message to majordomo@vger.kernel.org
    More majordomo info at http://vger.kernel.org/majordomo-info.html
    Please read the FAQ at http://www.tux.org/lkml/

    \
     
     \ /
      Last update: 2006-01-21 04:40    [W:6.150 / U:0.060 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site