Re: [md PATCH 14/22] md: support updating bitmap parameters via sysfs.

[Andre Noll <maan@systemlinux.org>]

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

On 17:48, NeilBrown wrote:
> A new attribute directory 'bitmap' in 'md' is created which
> contains files for configuring the bitmap.

Nice.

> 'location' identifies where the bitmap is, either 'none',
> or 'file' or 'sector offset from metadata'.
> Writing 'location' can create or remove a bitmap.
> Adding a 'file' bitmap this way is not yet supported.
> 
> 'chunksize' can be set before creating a bitmap, but is
> currently always over-ridden by the bitmap superblock.
> 
> 'time_base' and 'backlog' can be updated at any time.

This text should probably go into the mdadm documentation as well.

> +static ssize_t
> +location_show(mddev_t *mddev, char *page)
> +{
> +	ssize_t len = 0;
> +	if (mddev->bitmap_info.file) {
> +		len = sprintf(page, "file");
> +	} else if (mddev->bitmap_info.offset) {
> +		len = sprintf(page, "%+lld", (long long)mddev->bitmap_info.offset);
> +	} else
> +		len = sprintf(page, "none");
> +	len += sprintf(page+len, "\n");
> +	return len;
> +}

Pointless initialization to zero.

> +	} else {
> +		/* No bitmap, OK to set a location */
> +		long long offset;
> +		if (strncmp(buf, "none", 4) == 0)
> +			/* nothing to be done */;
> +		else if (strncmp(buf, "file:", 5) == 0) {
> +			/* Not supported yet */
> +			return -EINVAL;
> +		} else {
> +			int rv;
> +			if (buf[0] == '+')
> +				rv = strict_strtoll(buf+1, 10, &offset);
> +			else
> +				rv = strict_strtoll(buf, 10, &offset);
> +			if (rv)
> +				return rv;
> +			mddev->bitmap_info.offset = offset;

Introducing a sanity check here would probably save some sysadmin's
feet.

> +static struct md_sysfs_entry bitmap_location =
> +__ATTR(location, S_IRUGO|S_IWUSR, location_show, location_store);
> +
> +static ssize_t
> +timeout_show(mddev_t *mddev, char *page)
> +{
> +	ssize_t len = 0;
> +	unsigned long secs = mddev->bitmap_info.daemon_sleep / HZ;
> +	unsigned long jifs = mddev->bitmap_info.daemon_sleep % HZ;
> +	
> +	len = sprintf(page, "%lu", secs);
> +	if (jifs)
> +		len += sprintf(page+len, ".%03u", jiffies_to_msecs(jifs));
> +	len += sprintf(page+len, "\n");
> +	return len;
> +}

Again, no need to initialize len.

> +
> +int strict_strtoul_scaled(const char *cp, unsigned long *res, int scale)
> +{
> +	unsigned long result = 0;
> +	long decimals = -1;
> +	while (isdigit(*cp) || (*cp == '.' && decimals < 0)) {
> +		if (*cp == '.')
> +			decimals = 0;
> +		else if (decimals < scale) {
> +			unsigned int value;
> +			value = *cp - '0';
> +			result = result * 10 + value;
> +			if (decimals >= 0)
> +				decimals++;
> +		}
> +		cp++;
> +	}
> +	if (*cp == '\n')
> +		cp++;
> +	if (*cp)
> +		return -EINVAL;
> +	if (decimals < 0)
> +		decimals = 0;
> +	while (decimals < scale) {
> +		result *= 10;
> +		decimals ++;
> +	}
> +	*res = result;
> +	return 0;
> +}

A comment which describes what exactly this function is supposed to
to would be nice. It has only one caller and could be made static BTW.
OTOH, it probably belongs to lib/vsprintf.c.

> +static ssize_t
> +timeout_store(mddev_t *mddev, const char *buf, size_t len)
> +{
> +	/* timeout can be set at any time */
> +	unsigned long timeout;
> +	int rv = strict_strtoul_scaled(buf, &timeout, 4);
> +	if (rv)
> +		return rv;
> +
> +	timeout = timeout * HZ / 10000;

This might overflow. But maybe that's OK as only insane values (written
by root) will cause an overflow.

> +
> +	if (timeout > MAX_SCHEDULE_TIMEOUT)
> +		timeout = MAX_SCHEDULE_TIMEOUT;
> +	if (timeout < 1)
> +		timeout = 1;
> +	mddev->bitmap_info.daemon_sleep = timeout;
> +	if (mddev->thread) {
> +		if (mddev->thread->timeout < MAX_SCHEDULE_TIMEOUT) {
> +			mddev->thread->timeout = timeout;
> +			md_wakeup_thread(mddev->thread);
> +		}

I don't get this. Why do we want to set mddev->thread->timeout only
if mddev->thread->timeout < MAX_SCHEDULE_TIMEOUT)?

Also, if the timeout is being set while the array is stopped, the
command succeeds but has no effect (as mddev->thread is NULL). Maybe
it's better to return an error in this case.

> +static ssize_t
> +backlog_store(mddev_t *mddev, const char *buf, size_t len)
> +{
> +	unsigned long backlog;
> +	int rv = strict_strtoul(buf, 10, &backlog);
> +	if (rv)
> +		return rv;
> +	if (backlog > COUNTER_MAX)
> +		backlog = COUNTER_MAX;
> +	mddev->bitmap_info.max_write_behind = backlog;
> +	return len;
> +}

If the given value is too large this code silently ignores that value
and uses the maximal possible value instead. Even if we'd prefer "I
know better what you really want" over "you asked for it you got it",
we should at least be consistent. min_sync_store() for example returns
EINVAL if the given value is not suitable. So does chunksize_store()
below.

The same comment applies to timeout_store() of course.

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 --]