[PATCH 1/4] block: add function disk_name_format() into block core

[PATCH 1/4] block: add function disk_name_format() into block core

[Ren Mingxin]

  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@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);

[PATCH 2/4] scsi: replace sd_format_disk_name() to disk_name_format()

[Ren Mingxin]

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

[PATCH 3/4] block: replace rssd_disk_name_format() to disk_name_format()

[Ren Mingxin]

  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@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);

[PATCH 4/4] virtio_blk: use disk_name_format() to support mass of disks naming

[Ren Mingxin]

  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@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);

Re: [PATCH 0/4] block: move sd_format_disk_name() into block core as disk_name_format()

[James Bottomley]

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

Re: [PATCH 0/4] block: move sd_format_disk_name() into block core as disk_name_format()

[Ren Mingxin]

  Hi, James:

On 03/30/2012 05:56 PM, James Bottomley wrote:
> 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.

Thank you very much for so quick checking. I'll merge these
two patches into one in the next version.

> Other than that, everything looks fine to me.
>
>

Thanks,
Ren

Re: [PATCH 4/4] virtio_blk: use disk_name_format() to support mass of disks naming

[Asias He]

Hi,  Ren

On Fri, Mar 30, 2012 at 5:53 PM, Ren Mingxin <renmx@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@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@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html



-- 
Asias He
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH 4/4] virtio_blk: use disk_name_format() to support mass of disks naming

[Ren Mingxin]

  Hi, He:

On 03/30/2012 07:22 PM, Asias He wrote:
> On Fri, Mar 30, 2012 at 5:53 PM, Ren Mingxin<renmx@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 ).

Sorry, I intended to type 26^3+ (26^2+26)...
It may still be a restriction which should be improved though.

Thanks,
Ren

Re: [PATCH 4/4] virtio_blk: use disk_name_format() to support mass of disks naming

[Asias He]

On Sat, Mar 31, 2012 at 9:14 AM, Ren Mingxin <renmx@cn.fujitsu.com> wrote:
>  Hi, He:
>
>
> On 03/30/2012 07:22 PM, Asias He wrote:
>>
>> On Fri, Mar 30, 2012 at 5:53 PM, Ren Mingxin<renmx@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 ).
>
>
> Sorry, I intended to type 26^3+ (26^2+26)...
> It may still be a restriction which should be improved though.

OK.

-- 
Asias He
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH 4/4] virtio_blk: use disk_name_format() to support mass of disks naming

[Tejun Heo]

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

Re: [PATCH 4/4] virtio_blk: use disk_name_format() to support mass of disks naming

[Tejun Heo]

On Fri, Mar 30, 2012 at 08:26:06AM -0700, Tejun Heo wrote:
> 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@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".

And also let's make that clear in the function name - say,
format_legacy_disk_name() or something.

Thanks.

-- 
tejun

Re: [PATCH 4/4] virtio_blk: use disk_name_format() to support mass of disks naming

[Ren Mingxin]

  On 03/30/2012 11:28 PM, Tejun Heo wrote:
> On Fri, Mar 30, 2012 at 08:26:06AM -0700, Tejun Heo wrote:
>> 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@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".
> And also let's make that clear in the function name - say,
> format_legacy_disk_name() or something.

So, to legacy disks [sh]d, we'd name them as [sh]d[a-z]{1,}. To new devices
like vd, we'd name them as vd<index>(vd<index>p<partno> as partitions)?
And how about the rssd in the patch 3 then?

Besides, does anybody have comments?
Looking forward to your replies ;-)

-- 
Thanks,
Ren

Re: [PATCH 4/4] virtio_blk: use disk_name_format() to support mass of disks naming

[Michael S. Tsirkin]

On Mon, Apr 02, 2012 at 09:19:05AM +0800, Ren Mingxin wrote:
>  On 03/30/2012 11:28 PM, Tejun Heo wrote:
> >On Fri, Mar 30, 2012 at 08:26:06AM -0700, Tejun Heo wrote:
> >>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@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".
> >And also let's make that clear in the function name - say,
> >format_legacy_disk_name() or something.
> 
> So, to legacy disks [sh]d, we'd name them as [sh]d[a-z]{1,}. To new devices
> like vd, we'd name them as vd<index>(vd<index>p<partno> as partitions)?

Pleae don't rename virtio disks, it is way too late for that:
virtio block driver was merged around 2007, it is not new by
any measure, and there are many systems out there using
the current naming scheme.

> And how about the rssd in the patch 3 then?

Probably same. Renaming existing devices will break setups.
I think the idea is to avoid using the
legacy naming in new drivers *that will be added from now on*.

> Besides, does anybody have comments?
> Looking forward to your replies ;-)
> 
> -- 
> Thanks,
> Ren

Re: [PATCH 4/4] virtio_blk: use disk_name_format() to support mass of disks naming

[Tejun Heo]

Hello,

On Mon, Apr 02, 2012 at 10:20:09AM +0300, Michael S. Tsirkin wrote:
> Pleae don't rename virtio disks, it is way too late for that:
> virtio block driver was merged around 2007, it is not new by
> any measure, and there are many systems out there using
> the current naming scheme.

There's no point in renaming device nodes of already deployed drivers.
It'll cause a lot of pain.

> > And how about the rssd in the patch 3 then?
> 
> Probably same. Renaming existing devices will break setups.
> I think the idea is to avoid using the
> legacy naming in new drivers *that will be added from now on*.

Yeap.

Thanks.

-- 
tejun

Re: [PATCH 4/4] virtio_blk: use disk_name_format() to support mass of disks naming

[James Bottomley]

On Mon, 2012-04-02 at 11:52 -0700, Tejun Heo wrote:
> > Probably same. Renaming existing devices will break setups.
> > I think the idea is to avoid using the
> > legacy naming in new drivers *that will be added from now on*.
> 
> Yeap.

So if we're agreed no other devices going forwards should ever use this
interface, is there any point unifying the interface?  No matter how
many caveats you hedge it round with, putting the API in a central place
will be a bit like a honey trap for careless bears.   It might be safer
just to leave it buried in the three current drivers.

James

Re: [PATCH 4/4] virtio_blk: use disk_name_format() to support mass of disks naming

[Tejun Heo]

Hello, James.

On Mon, Apr 02, 2012 at 11:56:18AM -0700, James Bottomley wrote:
> So if we're agreed no other devices going forwards should ever use this
> interface, is there any point unifying the interface?  No matter how
> many caveats you hedge it round with, putting the API in a central place
> will be a bit like a honey trap for careless bears.   It might be safer
> just to leave it buried in the three current drivers.

Yeah, that was my hope but I think it would be easier to enforce to
have a common function which is clearly marked legacy so that new
driver writers can go look for the naming code in the existing ones,
find out they're all using the same function which is marked legacy
and explains what to do for newer drivers.

Thanks.

-- 
tejun

Re: [PATCH 4/4] virtio_blk: use disk_name_format() to support mass of disks naming

[Michael S. Tsirkin]

On Mon, Apr 02, 2012 at 12:00:45PM -0700, Tejun Heo wrote:
> Hello, James.
> 
> On Mon, Apr 02, 2012 at 11:56:18AM -0700, James Bottomley wrote:
> > So if we're agreed no other devices going forwards should ever use this
> > interface, is there any point unifying the interface?  No matter how
> > many caveats you hedge it round with, putting the API in a central place
> > will be a bit like a honey trap for careless bears.   It might be safer
> > just to leave it buried in the three current drivers.
> 
> Yeah, that was my hope but I think it would be easier to enforce to
> have a common function which is clearly marked legacy so that new
> driver writers can go look for the naming code in the existing ones,
> find out they're all using the same function which is marked legacy
> and explains what to do for newer drivers.
> 
> Thanks.

I think I'm not the only one to be confused about the
preferred direction here.
James, do you agree to the approach above?

It would be nice to fix virtio block for 3.4, so
how about this:
- I'll just apply the original bugfix patch for 3.4 -
  it only affects virtio
- Ren will repost the refactoring patch on top, and we can
  keep up the discussion

Ren if you agree, can you make this a two patch series please?

> -- 
> tejun
> _______________________________________________
> Virtualization mailing list
> Virtualization@lists.linux-foundation.org
> https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH 4/4] virtio_blk: use disk_name_format() to support mass of disks naming

[Ren Mingxin]

  On 04/04/2012 04:01 PM, Michael S. Tsirkin wrote:
> On Mon, Apr 02, 2012 at 12:00:45PM -0700, Tejun Heo wrote:
>> On Mon, Apr 02, 2012 at 11:56:18AM -0700, James Bottomley wrote:
>>> So if we're agreed no other devices going forwards should ever use this
>>> interface, is there any point unifying the interface?  No matter how
>>> many caveats you hedge it round with, putting the API in a central place
>>> will be a bit like a honey trap for careless bears.   It might be safer
>>> just to leave it buried in the three current drivers.
>> Yeah, that was my hope but I think it would be easier to enforce to
>> have a common function which is clearly marked legacy so that new
>> driver writers can go look for the naming code in the existing ones,
>> find out they're all using the same function which is marked legacy
>> and explains what to do for newer drivers.
> I think I'm not the only one to be confused about the
> preferred direction here.
> James, do you agree to the approach above?
>
> It would be nice to fix virtio block for 3.4, so
> how about this:
> - I'll just apply the original bugfix patch for 3.4 -
>    it only affects virtio

Sorry, about only affects virtio, I'm not very clear here:
1) Just duplicate the disk name format function in virtio_blk
     like the original patch: https://lkml.org/lkml/2012/3/28/45
2) Move the disk name format function into block core like
     this patch series but only affects virtio(not affect mtip32xx).
Do you mean the 2) one or something else?

> - Ren will repost the refactoring patch on top, and we can
>    keep up the discussion
>
> Ren if you agree, can you make this a two patch series please?
>

Sure.

-- 
Thanks,
Ren

Re: [PATCH 4/4] virtio_blk: use disk_name_format() to support mass of disks naming

[Michael S. Tsirkin]

On Mon, Apr 09, 2012 at 11:47:51AM +0800, Ren Mingxin wrote:
>  On 04/04/2012 04:01 PM, Michael S. Tsirkin wrote:
> >On Mon, Apr 02, 2012 at 12:00:45PM -0700, Tejun Heo wrote:
> >>On Mon, Apr 02, 2012 at 11:56:18AM -0700, James Bottomley wrote:
> >>>So if we're agreed no other devices going forwards should ever use this
> >>>interface, is there any point unifying the interface?  No matter how
> >>>many caveats you hedge it round with, putting the API in a central place
> >>>will be a bit like a honey trap for careless bears.   It might be safer
> >>>just to leave it buried in the three current drivers.
> >>Yeah, that was my hope but I think it would be easier to enforce to
> >>have a common function which is clearly marked legacy so that new
> >>driver writers can go look for the naming code in the existing ones,
> >>find out they're all using the same function which is marked legacy
> >>and explains what to do for newer drivers.
> >I think I'm not the only one to be confused about the
> >preferred direction here.
> >James, do you agree to the approach above?
> >
> >It would be nice to fix virtio block for 3.4, so
> >how about this:
> >- I'll just apply the original bugfix patch for 3.4 -
> >   it only affects virtio
> 
> Sorry, about only affects virtio, I'm not very clear here:
> 1) Just duplicate the disk name format function in virtio_blk
>     like the original patch: https://lkml.org/lkml/2012/3/28/45
> 2) Move the disk name format function into block core like
>     this patch series but only affects virtio(not affect mtip32xx).
> Do you mean the 2) one or something else?

I mean 1) - I'll apply the original patch.

> >- Ren will repost the refactoring patch on top, and we can
> >   keep up the discussion
> >
> >Ren if you agree, can you make this a two patch series please?
> >
> 
> Sure.
> 
> -- 
> Thanks,
> Ren

Re: [PATCH 4/4] virtio_blk: use disk_name_format() to support mass of disks naming

[Michael S. Tsirkin]

On Mon, Apr 09, 2012 at 11:47:51AM +0800, Ren Mingxin wrote:
>  On 04/04/2012 04:01 PM, Michael S. Tsirkin wrote:
> >On Mon, Apr 02, 2012 at 12:00:45PM -0700, Tejun Heo wrote:
> >>On Mon, Apr 02, 2012 at 11:56:18AM -0700, James Bottomley wrote:
> >>>So if we're agreed no other devices going forwards should ever use this
> >>>interface, is there any point unifying the interface?  No matter how
> >>>many caveats you hedge it round with, putting the API in a central place
> >>>will be a bit like a honey trap for careless bears.   It might be safer
> >>>just to leave it buried in the three current drivers.
> >>Yeah, that was my hope but I think it would be easier to enforce to
> >>have a common function which is clearly marked legacy so that new
> >>driver writers can go look for the naming code in the existing ones,
> >>find out they're all using the same function which is marked legacy
> >>and explains what to do for newer drivers.
> >I think I'm not the only one to be confused about the
> >preferred direction here.
> >James, do you agree to the approach above?
> >
> >It would be nice to fix virtio block for 3.4, so
> >how about this:
> >- I'll just apply the original bugfix patch for 3.4 -
> >   it only affects virtio
> 
> Sorry, about only affects virtio, I'm not very clear here:
> 1) Just duplicate the disk name format function in virtio_blk
>     like the original patch: https://lkml.org/lkml/2012/3/28/45

So I'd like to apply this, and we can discuss the
deduplication for 3.5. Please post a version of this that
1. isn't line-wrapped and doesn't have damaged whitespace
   so I can run git am on it
2. lists the # of duspported disks correctly as 26^3+(26^2+26)
   in the description

Thanks!

> 2) Move the disk name format function into block core like
>     this patch series but only affects virtio(not affect mtip32xx).
> Do you mean the 2) one or something else?
> 
> >- Ren will repost the refactoring patch on top, and we can
> >   keep up the discussion
> >
> >Ren if you agree, can you make this a two patch series please?
> >
> 
> Sure.
> 
> -- 
> Thanks,
> Ren

Re: [PATCH 4/4] virtio_blk: use disk_name_format() to support mass of disks naming

[Tejun Heo]

On Fri, Mar 30, 2012 at 08:26:06AM -0700, Tejun Heo wrote:
> 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@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

So, if a device name ends in digit the partition code adds delimiter
'p' automatically and this is already in use by md.  So, partitioned
md devices are named mdDpP where D and P are base 10 number indicating
the sequence like md12p4.  So, let's please add comment that new
drivers should name their devices PREFIX%d where the sequence number
can be allocated by ida.

Thanks.

-- 
tejun

Re: [PATCH 4/4] virtio_blk: use disk_name_format() to support mass of disks naming

[Ren Mingxin]

  On 03/30/2012 11:38 PM, Tejun Heo wrote:
> On Fri, Mar 30, 2012 at 08:26:06AM -0700, Tejun Heo wrote:
>> 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@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
> So, if a device name ends in digit the partition code adds delimiter
> 'p' automatically and this is already in use by md.  So, partitioned
> md devices are named mdDpP where D and P are base 10 number indicating
> the sequence like md12p4.  So, let's please add comment that new
> drivers should name their devices PREFIX%d where the sequence number
> can be allocated by ida.
>
>

Oh, got it. Just like md devices' names.

-- 
Thanks,
Ren

Re: [PATCH 4/4] virtio_blk: use disk_name_format() to support mass of disks naming

[Michael S. Tsirkin]

On Fri, Mar 30, 2012 at 08:26:06AM -0700, Tejun Heo wrote:
> 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@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?

Yes they could and yes it's way too late to change device
names for virtio - if we did this will break countless setups.

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

Right. virtio would have to use the legacy one though.

> -- 
> tejun

Re: [PATCH 4/4] virtio_blk: use disk_name_format() to support mass of disks naming

[Jeff Garzik]

On 03/30/2012 11:26 AM, Tejun Heo wrote:
> 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".

Big ACK to that...

Re: [PATCH 3/4] block: replace rssd_disk_name_format() to disk_name_format()

[Asai Thambi Samymuthu Pattrayasamy (asamymuthupa) [CONT - Type 2]]

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

Re: [PATCH 3/4] block: replace rssd_disk_name_format() to disk_name_format()

[Ren Mingxin]

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

Re: [PATCH 0/4] block: move sd_format_disk_name() into block core as disk_name_format()

[Michael S. Tsirkin]

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
> 

[PATCH 0/4] block: move sd_format_disk_name() into block core as disk_name_format()

[Ren Mingxin]

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