Hi Neil,

On Wed, May 20, 2009 at 4:28 AM, Neil Brown [off-list ref] wrote:

On Tuesday May 19, sandeepksinha@gmail.com wrote:

quoted

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@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html