Linux Virtualization - Mar 2012 - [PATCH 0/4] block: move sd_format_disk_name() into block core as disk_name_format()

This patch series renames "sd_format_disk_name()" to
"disk_name_format()" and moves it into block core. So
that who needs formatting disk name can use it, instead
of duplicating these similar help functions.

Ren Mingxin (4):
   block: add function disk_name_format() into block core
   scsi: replace sd_format_disk_name() to disk_name_format()
   block: replace rssd_disk_name_format() to disk_name_format()
   virtio_blk: use disk_name_format() to support mass of disks naming

  block/genhd.c                     |   49
++++++++++++++++++++++++++++++++++++++
  drivers/block/mtip32xx/mtip32xx.c |   33 -------------------------
  drivers/block/virtio_blk.c        |   13 ----------
  drivers/scsi/sd.c                 |   48 -------------------------------------
  include/linux/genhd.h             |    2 +
  5 files changed, 54 insertions(+), 91 deletions(-)

Any comment will be appreciated.

Thanks,
Ren

According to 3e1a7ff8a0a7b948f2684930166954f9e8e776fe,
I copied function "sd_format_disk_name()" into block core with
the name of "disk_name_format()".

Signed-off-by: Ren Mingxin <renmx at cn.fujitsu.com>
---
  block/genhd.c         |   49 
+++++++++++++++++++++++++++++++++++++++++++++++++
  include/linux/genhd.h |    2 ++
  2 files changed, 51 insertions(+)

diff --git a/block/genhd.c b/block/genhd.c
index df9816e..10c04b6 100644
--- a/block/genhd.c
+++ b/block/genhd.c
@@ -357,6 +357,55 @@ void unregister_blkdev(unsigned int major, const 
char *name)

  EXPORT_SYMBOL(unregister_blkdev);

+/*
+ *     disk_name_format - format disk name
+ *     @prefix: name prefix - ie. "sd" for SCSI disks
+ *     @index: index of the disk to format name for
+ *     @buf: output buffer
+ *     @buflen: length of the output buffer
+ *
+ *     Take SCSI disks for example. Disk names starts at sda.  The
+ *     26th device is sdz and the 27th is sdaa.  The last one for
+ *     two lettered suffix is sdzz which is followed by sdaaa.
+ *
+ *     This is basically 26 base counting with one extra 'nil' entry
+ *     at the beginning from the second digit on and can be
+ *     determined using similar method as 26 base conversion with the
+ *     index shifted -1 after each digit is computed.
+ *
+ *     CONTEXT:
+ *     Don't care.
+ *
+ *     RETURNS:
+ *     0 on success, -errno on failure.
+ */
+
+int disk_name_format(char *prefix, int index, char *buf, int buflen)
+{
+       const int base = 'z' - 'a' + 1;
+       char *begin = buf + strlen(prefix);
+       char *end = buf + buflen;
+       char *p;
+       int unit;
+
+       p = end - 1;
+       *p = '\0';
+       unit = base;
+       do {
+               if (p == begin)
+                       return -EINVAL;
+               *--p = 'a' + (index % unit);
+               index = (index / unit) - 1;
+       } while (index >= 0);
+
+       memmove(begin, p, end - p);
+       memcpy(buf, prefix, strlen(prefix));
+
+       return 0;
+}
+
+EXPORT_SYMBOL(disk_name_format);
+
  static struct kobj_map *bdev_map;

  /**
diff --git a/include/linux/genhd.h b/include/linux/genhd.h
index e61d319..cd3d5e5 100644
--- a/include/linux/genhd.h
+++ b/include/linux/genhd.h
@@ -407,6 +407,8 @@ static inline void free_part_info(struct hd_struct 
*part)
  extern void part_round_stats(int cpu, struct hd_struct *part);

  /* block/genhd.c */
+extern int disk_name_format(char *prefix, int index, char *buf, int 
buflen);
+
  extern void add_disk(struct gendisk *disk);
  extern void del_gendisk(struct gendisk *gp);
  extern struct gendisk *get_gendisk(dev_t dev, int *partno);

Since "sd_format_disk_name()"has been copied into block
core as "disk_name_format()", the original function should
be removed, and the place used original function should be
replaced by the renamed function.

Signed-off-by: Ren Mingxin <renmx at cn.fujitsu.com>
---
  sd.c |   48 +-----------------------------------------------
  1 file changed, 1 insertion(+), 47 deletions(-)

diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c
index 09e3df4..b82156a 100644
--- a/drivers/scsi/sd.c
+++ b/drivers/scsi/sd.c
@@ -2525,52 +2525,6 @@ static void sd_unlock_native_capacity(struct 
gendisk *disk)
                 sdev->host->hostt->unlock_native_capacity(sdev);
  }

-/**
- *     sd_format_disk_name - format disk name
- *     @prefix: name prefix - ie. "sd" for SCSI disks
- *     @index: index of the disk to format name for
- *     @buf: output buffer
- *     @buflen: length of the output buffer
- *
- *     SCSI disk names starts at sda.  The 26th device is sdz and the
- *     27th is sdaa.  The last one for two lettered suffix is sdzz
- *     which is followed by sdaaa.
- *
- *     This is basically 26 base counting with one extra 'nil' entry
- *     at the beginning from the second digit on and can be
- *     determined using similar method as 26 base conversion with the
- *     index shifted -1 after each digit is computed.
- *
- *     CONTEXT:
- *     Don't care.
- *
- *     RETURNS:
- *     0 on success, -errno on failure.
- */
-static int sd_format_disk_name(char *prefix, int index, char *buf, int 
buflen)
-{
-       const int base = 'z' - 'a' + 1;
-       char *begin = buf + strlen(prefix);
-       char *end = buf + buflen;
-       char *p;
-       int unit;
-
-       p = end - 1;
-       *p = '\0';
-       unit = base;
-       do {
-               if (p == begin)
-                       return -EINVAL;
-               *--p = 'a' + (index % unit);
-               index = (index / unit) - 1;
-       } while (index >= 0);
-
-       memmove(begin, p, end - p);
-       memcpy(buf, prefix, strlen(prefix));
-
-       return 0;
-}
-
  /*
   * The asynchronous part of sd_probe
   */
@@ -2685,7 +2639,7 @@ static int sd_probe(struct device *dev)
                 goto out_put;
         }

-       error = sd_format_disk_name("sd", index, gd->disk_name, 
DISK_NAME_LEN);
+       error = disk_name_format("sd", index, gd->disk_name,
DISK_NAME_LEN);
         if (error) {
                 sdev_printk(KERN_WARNING, sdp, "SCSI disk (sd) name 
length exceeded.\n");
                 goto out_free_index;

Currently, block core has been supplied "disk_name_format()", so
we should remove duplicate function "rssd_disk_name_format()"
and use the new function to format rssd disk names.

Signed-off-by: Ren Mingxin <renmx at cn.fujitsu.com>
---
  mtip32xx.c |   33 +--------------------------------
  1 file changed, 1 insertion(+), 32 deletions(-)

diff --git a/drivers/block/mtip32xx/mtip32xx.c 
b/drivers/block/mtip32xx/mtip32xx.c
index 8eb81c9..8950bb5 100644
--- a/drivers/block/mtip32xx/mtip32xx.c
+++ b/drivers/block/mtip32xx/mtip32xx.c
@@ -2836,37 +2836,6 @@ static int mtip_hw_resume(struct driver_data *dd)
  }

  /*
- * Helper function for reusing disk name
- * upon hot insertion.
- */
-static int rssd_disk_name_format(char *prefix,
-                                int index,
-                                char *buf,
-                                int buflen)
-{
-       const int base = 'z' - 'a' + 1;
-       char *begin = buf + strlen(prefix);
-       char *end = buf + buflen;
-       char *p;
-       int unit;
-
-       p = end - 1;
-       *p = '\0';
-       unit = base;
-       do {
-               if (p == begin)
-                       return -EINVAL;
-               *--p = 'a' + (index % unit);
-               index = (index / unit) - 1;
-       } while (index >= 0);
-
-       memmove(begin, p, end - p);
-       memcpy(buf, prefix, strlen(prefix));
-
-       return 0;
-}
-
-/*
   * Block layer IOCTL handler.
   *
   * @dev Pointer to the block_device structure.
@@ -3140,7 +3109,7 @@ static int mtip_block_initialize(struct 
driver_data *dd)
         if (rv)
                 goto ida_get_error;

-       rv = rssd_disk_name_format("rssd",
+       rv = disk_name_format("rssd",
                                 index,
                                 dd->disk->disk_name,
                                 DISK_NAME_LEN);

The current virtblk's naming algorithm only supports 263  disks.
If there are mass of virtblks(exceeding 263), there will be disks
with the same name.

By renaming "sd_format_disk_name()" to "disk_name_format()"
and moving it into block core, virtio_blk can use this function to
support mass of disks.

Signed-off-by: Ren Mingxin <renmx at cn.fujitsu.com>
---
   virtio_blk.c |   13 +------------
  1 file changed, 1 insertion(+), 12 deletions(-)

diff --git a/drivers/block/virtio_blk.c b/drivers/block/virtio_blk.c
index c4a60ba..54612c2 100644
--- a/drivers/block/virtio_blk.c
+++ b/drivers/block/virtio_blk.c
@@ -442,18 +442,7 @@ static int __devinit virtblk_probe(struct 
virtio_device *vdev)

         q->queuedata = vblk;

-       if (index < 26) {
-               sprintf(vblk->disk->disk_name, "vd%c",
'a' + index % 26);
-       } else if (index < (26 + 1) * 26) {
-               sprintf(vblk->disk->disk_name, "vd%c%c",
-                       'a' + index / 26 - 1, 'a' + index % 26);
-       } else {
-               const unsigned int m1 = (index / 26 - 1) / 26 - 1;
-               const unsigned int m2 = (index / 26 - 1) % 26;
-               const unsigned int m3 =  index % 26;
-               sprintf(vblk->disk->disk_name, "vd%c%c%c",
-                       'a' + m1, 'a' + m2, 'a' + m3);
-       }
+       disk_name_format("vd", index, vblk->disk->disk_name,
DISK_NAME_LEN);

         vblk->disk->major = major;
         vblk->disk->first_minor = index_to_minor(index);

On Fri, 2012-03-30 at 17:50 +0800, Ren Mingxin wrote:
> 
> This patch series renames "sd_format_disk_name()" to
> "disk_name_format()" and moves it into block core. So
> that who needs formatting disk name can use it, instead
> of duplicating these similar help functions.
> 
> Ren Mingxin (4):
>    block: add function disk_name_format() into block core
>    scsi: replace sd_format_disk_name() to disk_name_format()
These two should be a single patch so it makes it obvious to anyone
looking through the change sets that the code is merely being moved.

Other than that, everything looks fine to me.

James

Hi,  Ren

On Fri, Mar 30, 2012 at 5:53 PM, Ren Mingxin <renmx at cn.fujitsu.com>
wrote:
> ?The current virtblk's naming algorithm only supports 263 ?disks.
> If there are mass of virtblks(exceeding 263), there will be disks
> with the same name.
This fix is pretty nice. However, current virtblk's naming still
supports up to 18278 disks, no?
( index 0  -> vda, index 18277 -> vdzzz ).
> By renaming "sd_format_disk_name()" to
"disk_name_format()"
> and moving it into block core, virtio_blk can use this function to
> support mass of disks.
>
> Signed-off-by: Ren Mingxin <renmx at cn.fujitsu.com>
> ---
> ?virtio_blk.c | ? 13 +------------
> ?1 file changed, 1 insertion(+), 12 deletions(-)
>
> diff --git a/drivers/block/virtio_blk.c b/drivers/block/virtio_blk.c
> index c4a60ba..54612c2 100644
> --- a/drivers/block/virtio_blk.c
> +++ b/drivers/block/virtio_blk.c
> @@ -442,18 +442,7 @@ static int __devinit virtblk_probe(struct
virtio_device
> *vdev)
>
> ? ? ? ?q->queuedata = vblk;
>
> - ? ? ? if (index < 26) {
> - ? ? ? ? ? ? ? sprintf(vblk->disk->disk_name, "vd%c",
'a' + index % 26);
> - ? ? ? } else if (index < (26 + 1) * 26) {
> - ? ? ? ? ? ? ? sprintf(vblk->disk->disk_name, "vd%c%c",
> - ? ? ? ? ? ? ? ? ? ? ? 'a' + index / 26 - 1, 'a' + index %
26);
> - ? ? ? } else {
> - ? ? ? ? ? ? ? const unsigned int m1 = (index / 26 - 1) / 26 - 1;
> - ? ? ? ? ? ? ? const unsigned int m2 = (index / 26 - 1) % 26;
> - ? ? ? ? ? ? ? const unsigned int m3 = ?index % 26;
> - ? ? ? ? ? ? ? sprintf(vblk->disk->disk_name, "vd%c%c%c",
> - ? ? ? ? ? ? ? ? ? ? ? 'a' + m1, 'a' + m2, 'a' +
m3);
> - ? ? ? }
> + ? ? ? disk_name_format("vd", index,
vblk->disk->disk_name, DISK_NAME_LEN);
>
> ? ? ? ?vblk->disk->major = major;
> ? ? ? ?vblk->disk->first_minor = index_to_minor(index);
>
> --
> To unsubscribe from this list: send the line "unsubscribe
linux-scsi" in
> the body of a message to majordomo at vger.kernel.org
> More majordomo info at ?http://vger.kernel.org/majordomo-info.html


-- 
Asias He

On Fri, Mar 30, 2012 at 05:53:52PM +0800, Ren Mingxin
wrote:
>  The current virtblk's naming algorithm only supports 263  disks.
> If there are mass of virtblks(exceeding 263), there will be disks
> with the same name.
> 
> By renaming "sd_format_disk_name()" to
"disk_name_format()"
> and moving it into block core, virtio_blk can use this function to
> support mass of disks.
> 
> Signed-off-by: Ren Mingxin <renmx at cn.fujitsu.com>
I guess it's already way too late but why couldn't they have been
named vdD-P where both D and P are integers denoting disk number and
partition number?  [sh]dX's were created when there weren't supposed
to be too many disks, so we had to come up with the horrible alphabet
based numbering scheme but vd is new enough.  I mean, naming is one
thing but who wants to figure out which sequence is or guess what
comes next vdzz9?  :(

If we're gonna move it to block layer, let's add big blinking red
comment saying "don't ever use it for any new driver".

Thanks.

-- 
tejun

On 3/30/2012 2:53 AM, Ren Mingxin wrote:
>  Currently, block core has been supplied "disk_name_format()", so
> we should remove duplicate function "rssd_disk_name_format()"
> and use the new function to format rssd disk names.
> 
> Signed-off-by: Ren Mingxin <renmx at cn.fujitsu.com>
> ---
>  mtip32xx.c |   33 +--------------------------------
>  1 file changed, 1 insertion(+), 32 deletions(-)

Looks fine.

Should the subject be "mtip32xx: ..." instead of "block:
..."? I
understand "block:" as relating to block core. I am fairly new here.
If
"block:" can be used for block drivers too, that is fine too.

--
Regards,
Asai Thambi

On Fri, Mar 30, 2012 at 05:50:12PM +0800, Ren Mingxin
wrote:
> 
> 
> This patch series renames "sd_format_disk_name()" to
> "disk_name_format()" and moves it into block core. So
> that who needs formatting disk name can use it, instead
> of duplicating these similar help functions.
I see there are some responses about naming and comments,
so that would need to be fixed.
Besides this, Jens, would you like to take the patchset
through your tree or prefer for me to merge it through virtio tree? 
Block tree seems more appropriate, right?
Also this is a bugfix technically since it fixes setups
with a ton of disks, so 3.4 material?
> Ren Mingxin (4):
>   block: add function disk_name_format() into block core
>   scsi: replace sd_format_disk_name() to disk_name_format()
>   block: replace rssd_disk_name_format() to disk_name_format()
>   virtio_blk: use disk_name_format() to support mass of disks naming
> 
>  block/genhd.c                     |   49
++++++++++++++++++++++++++++++++++++++
>  drivers/block/mtip32xx/mtip32xx.c |   33 -------------------------
>  drivers/block/virtio_blk.c        |   13 ----------
>  drivers/scsi/sd.c                 |   48
-------------------------------------
>  include/linux/genhd.h             |    2 +
>  5 files changed, 54 insertions(+), 91 deletions(-)
> 
> Any comment will be appreciated.
> 
> Thanks,
> Ren
>

On 03/31/2012 07:54 AM, Asai Thambi Samymuthu Pattrayasamy 
(asamymuthupa) [CONT - Type 2] wrote:
> On 3/30/2012 2:53 AM, Ren Mingxin wrote:
>
>>   Currently, block core has been supplied
"disk_name_format()", so
>> we should remove duplicate function "rssd_disk_name_format()"
>> and use the new function to format rssd disk names.
>>
>> Signed-off-by: Ren Mingxin<renmx at cn.fujitsu.com>
>> ---
>>   mtip32xx.c |   33 +--------------------------------
>>   1 file changed, 1 insertion(+), 32 deletions(-)
>
> Looks fine.
>
> Should the subject be "mtip32xx: ..." instead of "block:
..."? I
> understand "block:" as relating to block core. I am fairly new
here. If
> "block:" can be used for block drivers too, that is fine too.
>
>
Good idea! Will be fixed in the next version.

-- 
Thanks,
Ren