Re: [PATCH 001 of 5] md: Split disks array out of raid5 conf structure so it is easier to grow.

["John Stoffel" <john@stoffel.org>]

>>>>> "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/