Hi Neil,
On Wed, May 20, 2009 at 4:28 AM, Neil Brown <neilb@xxxxxxx> wrote:
> On Tuesday May 19, sandeepksinha@xxxxxxxxx wrote:
>> Hi,
>>
>> In linear_add( ), the mddev is gets changed.
>> Don't we require to lock/unlock around mddev inorder to avoid races?
>>
>
> A good question.
>
> To be able to answer it, we must also ask "what races do we need to
> avoid". Did you have some possible races in mind?
>
I see mddev being referenced in md.c (md layer) for lot of operations.
Actually for almost all operations we pass through it. i.e get array
info.
> In linear add there are 3 lines which are in an important order:
>
> mddev->private = newconf;
> mddev->raid_disks++;
> ....
> set_capacity(mddev->gendisk, mddev->array_sectors);
>
> Until raid_disks in incremented, there will be no attempt to access
> the extra information in ->private.
> Until the set_capacity, there will be no attempt to send IO beyond the
> reach of raid_disks.
> So except that it might be wise to insert some barrier()s, it looks
> OK.... or it did.
>
> I just noticed that the new 'which_dev' checks mddev->raid_disks,
> where as the old which_dev did not.
>
> So if:
> which_dev takes a copy of mddev->private,
> linear_add changes mddev->private
> linear_add changes raid_disks
> which_dev takes a copy of mddev->raid_disks
>
> then which_dev will have a raid_disks which points beyond the end of
> conf->disks. If the target sector were beyond the end of the original
> array, we could fall off the end of that array which would be bad.
> However as the sector number will have been tested again the size of
> the array (in bio_check_eod from generic_make_request), that cannot
> happen. Unless which_dev was called from linear_mergeable_bvec...
>
Yes, this is true.
> I suspect in light of this it would be best to:
> 1/ insert a barrier() between setting ->private and setting
> ->raid_disks
> 2/ in which_dev, read ->raid_disks before ->private, and put a
> barrier in between.
>
Do you suggest implementing barrier functions for linear raid?
Can you elaborate on this a bit more please?
> We could possibly use rcu_dereference and rcu_assign_pointer, and
> maybe even make use of rcu to free the old conf structure promptly
> rather than keeping it around until the array is stopped.
> That would require careful placement of rcu_read_{,un}lock around
> access to ->private.
>
> Probably best to just put barriers in for now, and leave the rcu
> stuff.
>
k.
> NeilBrown
>
>
>
--
Regards,
Sandeep.
“To learn is to change. Education is a process that changes the learner.”
--
To unsubscribe from this list: send the line "unsubscribe linux-raid" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
