Re: [PATCH 3/3] md: 'array_size' sysfs attribute

[Andre Noll <maan@systemlinux.org>]

[-- Attachment #1: Type: text/plain, Size: 3203 bytes --]

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, &sectors);
> +		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 #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 189 bytes --]