On Sat, March 28, 2009 3:19 am, Andre Noll wrote:

On 19:53, NeilBrown wrote:

quoted

-			if (logical_sector >= conf->expand_progress) {
+			if (mddev->delta_disks < 0
+			    ? logical_sector < conf->reshape_progress
+			    : logical_sector >= conf->reshape_progress) {
 				disks = conf->previous_raid_disks;
 				previous = 1;
 			} else {
-				if (logical_sector >= conf->expand_lo) {
+				if (mddev->delta_disks < 0
+				    ? logical_sector < conf->reshape_safe
+				    : logical_sector >= conf->reshape_safe) {
 					spin_unlock_irq(&conf->device_lock);
 					schedule();
 					goto retry;

Is it only me who finds such code hard to comprehend?  Given that
the patch adds checks of the form

	(delta < 0 && s < r) || (delta >= 0 && s >= r)

They are really of the form
       delta < 0 ? s < r : s >= r

I guess I could use something like

 static inline inorder(mddev_t *mddev, sector_t a, sector_t b)
 {
      if (mddev->delta_disks < 0)
                return b > a;
      else
                return a <= b;
 }

However sometimes it is '<' vs '>=' and sometimes  '<' vs '>',
so I'm not sure it would apply universally.....

at several locations, it might make sense to introduce a marco or an
inline function for this check.

quoted

+	/* reshape_progress is the leading edge of a 'reshape'
+	 * It has value MaxSector when no expand is happening

s/expand/reshape

Thanks.
Fixed.


NeilBrown

Regards
Andre
--
The only person who always got his work done by Friday was Robinson Crusoe