On Fri, Mar 6, 2009 at 9:15 AM, Andre Noll <maan@xxxxxxxxxxxxxxx> wrote:
> On 17:24, Dan Williams wrote:
>> Allow userspace to set the size of the array according to the following
>> semantics:
>>
>> 1/ size must be <= to the size returned by mddev->pers->size(mddev, 0, 0)
>> a) If size is set before the array is running, do_md_run will fail
>> if size is greater than the default size
>> b) A reshape attempt that reduces the default size to less than the set
>> array size should be blocked
>> 2/ once userspace sets the size the kernel will not change it
>
> This holds only until the array is stopped and reassembled (for
> example due to a reboot). Is that correct and intended?
Yes. For arrays that need this the size is recorded in the metadata,
so it will be persistent across reboots. For the reshape-to-smaller
size cases mdadm will need to remember to set this accordingly.
>
>> +static ssize_t
>> +array_size_store(mddev_t *mddev, const char *buf, size_t len)
>> +{
>> + unsigned long long sectors;
>> + struct block_device *bdev;
>> +
>> + if (strncmp(buf, "default", 7) == 0) {
>> + if (mddev->pers)
>> + sectors = mddev->pers->size(mddev, 0, 0);
>> + else
>> + sectors = mddev->array_sectors;
>> +
>> + mddev->external_size = 0;
>> + } else {
>> + int err;
>> + sector_t new;
>> +
>> + err = strict_strtoull(buf, 10, §ors);
>> + if (err < 0)
>> + return err;
>> + sectors *= 2;
>> + new = sectors;
>> + if (new != sectors) /* overflow */
>> + return -EINVAL;
>
> Is it possible that already the "sectors *= 2" overflows? If this
> happens a much too small value is going to be stored by set_capacity()
> later.
That is true. If we overflow the blocks->sectors conversion we should
be returning an error. This is also needs to be fixed up in
rdev_size_store and size_store.
>
>> + mddev->array_sectors = sectors;
>> + set_capacity(mddev->gendisk, mddev->array_sectors);
>> + if (mddev->pers) {
>> + 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);
>> + }
>> + }
>
> bdev is only used inside the if (mddev->pers). No need to define it at
> the top of the function.
>
ok
>> @@ -4043,7 +4108,17 @@ static int do_md_run(mddev_t * mddev)
>> err = mddev->pers->run(mddev);
>> if (err)
>> printk(KERN_ERR "md: pers->run() failed ...\n");
>> - else if (mddev->pers->sync_request) {
>> + else if (mddev->pers->size(mddev, 0, 0) < mddev->array_sectors) {
>> + WARN_ONCE(!mddev->external_size, "%s: default size too small,"
>> + " but 'external_size' not in effect?\n", __func__);
>> + printk(KERN_ERR
>> + "md: invalid array_size %llu > default size %llu\n",
>> + (unsigned long long)mddev->array_sectors / 2,
>> + (unsigned long long)mddev->pers->size(mddev, 0, 0) / 2);
>> + err = -EINVAL;
>> + mddev->pers->stop(mddev);
>> + }
>
> What's the point of the WARN_ONCE() if it is followed by a printk() that
> is always being printed?
>
There is a subtle difference. In both cases it is an error, but the
WARN_ONCE is checking for a kernel coding bug while the printk will
also trigger on a bad value from userspace.
>> void md_set_size(mddev_t *mddev, sector_t array_sectors)
>> {
>> + WARN(!mddev_is_locked(mddev), "%s: unlocked mddev!\n", __func__);
>
> An unlocked mddev would actually be a bug, no?
A bug, yes, but not a fatal one. The user can now report the warning
and fix up the size because the system stayed running.
> Also, the name
> "md_set_size" is a bit unfortunate as an exported function name because
> it's not clear from the name that the size should be specified in
> sectors. "md_set_sector_count" or "md_set_array_sectors" is probably
> clearer.
Admittedly a minor issue, not much different than the lack of clarity
for set_capacity(), but I will go ahead and make this
md_set_array_sectors.
Thanks,
Dan
--
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
