On Sat, March 28, 2009 3:19 am, Andre Noll wrote:
> On 19:53, NeilBrown wrote:
>
>> Never allow the size to be reduced below the minimum (4 doe raid6,
>> 3 otherwise).
>
> doe?
Left hand was one column too far left. d->f e->r.
Fixed.
>
>> @@ -3723,6 +3723,7 @@ static sector_t reshape_request(mddev_t *mddev,
>> sector_t sector_nr, int *skipped
>> int i;
>> int dd_idx;
>> sector_t writepos, safepos, gap;
>> + sector_t stripe_addr;
>>
>> if (sector_nr == 0) {
>> /* If restarting in the middle, skip the initial sectors */
>> @@ -3779,10 +3780,21 @@ static sector_t reshape_request(mddev_t *mddev,
>> sector_t sector_nr, int *skipped
>> wake_up(&conf->wait_for_overlap);
>> }
>>
>> + if (mddev->delta_disks < 0) {
>> + BUG_ON(conf->reshape_progress == 0);
>> + stripe_addr = writepos;
>> + BUG_ON((mddev->dev_sectors &
>> + ~((sector_t)mddev->chunk_size / 512 - 1))
>> + - (conf->chunk_size / 512) - stripe_addr
>> + != sector_nr);
>> + } else {
>> + BUG_ON(writepos != sector_nr + conf->chunk_size / 512);
>> + stripe_addr = writepos;
>> + }
>
> What's the point of the new stripe_addr variable? Isn't it equal to
> writepos in any case?
Yes, but it shouldn't be. In the second branch of the if, it should be
stripe_addr = sector_nr;
as I discovered yesterday while testing.
>
>> @@ -4738,14 +4755,25 @@ static int raid5_check_reshape(mddev_t *mddev)
>> raid5_conf_t *conf = mddev_to_conf(mddev);
>> int err;
>>
>> - if (mddev->delta_disks < 0 ||
>> - mddev->new_level != mddev->level)
>> - return -EINVAL; /* Cannot shrink array or change level yet */
>> if (mddev->delta_disks == 0)
>> return 0; /* nothing to do */
>> if (mddev->bitmap)
>> /* Cannot grow a bitmap yet */
>> return -EBUSY;
>> + if (mddev->degraded > conf->max_degraded)
>> + return -EINVAL;
>> + if (mddev->delta_disks < 0) {
>> + /* We might be able to shrink, but the devices must
>> + * be made bigger first.
>> + * For raid6, 4 is the minimum size.
>> + * Otherwise 2 is the minimum
>> + */
>> + int min = 2;
>> + if (mddev->level == 6)
>> + min = 4;
>> + if (mddev->raid_disks + mddev->delta_disks < min)
>> + return -EINVAL;
>> + }
>
> Hm, this code doesn't seem to do what the comment suggests. IMHO,
> we must check that
>
> (a) (raid_disks + delta_disks) * sizeof(smallest) is big enough
> and
> (b) raid_disks + delta_disks >= minimal admissible number of disks
>
> The comment says the devices must be made bigger (to satisfy (a))
> but the code only checks (b).
Yes, the comment could probably be improved.
The check for 'a' happens later when start_reshape is called.
>
>> @@ -4862,21 +4905,38 @@ static void raid5_finish_reshape(mddev_t *mddev)
>> if (!test_bit(MD_RECOVERY_INTR, &mddev->recovery)) {
>>
>> conf->previous_raid_disks = conf->raid_disks;
>> - md_set_array_sectors(mddev, raid5_size(mddev, 0, 0));
>> - set_capacity(mddev->gendisk, mddev->array_sectors);
>> - mddev->changed = 1;
>> -
>> - bdev = bdget_disk(mddev->gendisk, 0);
>> - if (bdev) {
>> - mutex_lock(&bdev->bd_inode->i_mutex);
>> - i_size_write(bdev->bd_inode,
>> - (loff_t)mddev->array_sectors << 9);
>> - mutex_unlock(&bdev->bd_inode->i_mutex);
>> - bdput(bdev);
>> - }
>> spin_lock_irq(&conf->device_lock);
>> conf->reshape_progress = MaxSector;
>> spin_unlock_irq(&conf->device_lock);
>> + if (mddev->delta_disks > 0) {
>> + conf->previous_raid_disks = conf->raid_disks;
>
> previous_raid_disks was already assigned earlier, so this can go away,
> imo.
Thanks. It is actually the first one that needs to go.
The 'else' branch of that 'if' needs to have access to the
original value of previous_raid_disks.
Thanks a lot for the review.
NeilBrown
--
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
