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?
> +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.
> + 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.
> @@ -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?
> 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? 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.
Regards
Andre
--
The only person who always got his work done by Friday was Robinson Crusoe
Attachment:
signature.asc
Description: Digital signature
