On Fri, Mar 6, 2009 at 9:15 AM, Andre Noll [off-list ref] wrote:

On 17:24, Dan Williams wrote:

quoted

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.

quoted

+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.

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.

quoted

+     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

quoted

@@ -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.

quoted

 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@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html