Re: [PATCH 1/7] aoe: support more AoE addresses with dynamic block device minor numbers
On Oct 2, 2012, at 5:12 PM, Andrew Morton wrote:
...
>> +static int
>> +minor_get(ulong *minor)
>> {
>> -struct aoedev *d;
>> ulong flags;
>> +ulong n;
>> +int error = 0;
>> +
>> +spin_lock_irqsave(_minors_lock, flags);
>> +n = find_first_zero_bit(used_minors, N_DEVS);
>> +if (n < N_DEVS)
>> +set_bit(n, used_minors);
>> +else
>> +error = -1;
>> +spin_unlock_irqrestore(_minors_lock, flags);
>> +
>> +*minor = n * AOE_PARTITIONS;
>> +return error;
>> +}
>
> - can use the more efficient __set_bit() inside that spinlock.
Thanks for that observation. Because this operation occurs on target
discovery, which is expected to be relatively infrequent, my inclination is to
leave it in its atomic form, though, and leave the __set_bit() for another time
when optimization is needed. Like you said, this is a minor point. I wouldn't
mind changing it, though, if you think it's worth me resubmitting the patch.
Just let me know.
> - could avoid setting *minor if we're returning an error.
Yes. The only caller of aoedev.c:minor_get() handles that correctly. Again,
just let me know if you think this is worth a resubmission of the patch.
Otherwise I'll just make a note to myself to try to avoid setting output
parameters on error in the future.
--
Ed Cashin
ecas...@coraid.com
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 1/7] aoe: support more AoE addresses with dynamic block device minor numbers
On Mon, 1 Oct 2012 18:59:12 -0700
Ed Cashin wrote:
> The ATA over Ethernet protocol uses a major (shelf) and
> minor (slot) address to identify a particular storage target.
> These changes remove an artificial limitation the aoe driver
> imposes on the use of AoE addresses. For example, without these
> changes, the slot address has a maximum of 15, but users commonly
> use slot numbers much greater than that.
>
> The AoE shelf and slot address space is often used sparsely.
> Instead of using a static mapping between AoE addresses and the
> block device minor number, the block device minor numbers are now
> allocated on demand.
>
> ...
Very minor things...
>
> ...
>
> +static int
> +minor_get(ulong *minor)
> {
> - struct aoedev *d;
> ulong flags;
> + ulong n;
> + int error = 0;
> +
> + spin_lock_irqsave(_minors_lock, flags);
> + n = find_first_zero_bit(used_minors, N_DEVS);
> + if (n < N_DEVS)
> + set_bit(n, used_minors);
> + else
> + error = -1;
> + spin_unlock_irqrestore(_minors_lock, flags);
> +
> + *minor = n * AOE_PARTITIONS;
> + return error;
> +}
- can use the more efficient __set_bit() inside that spinlock.
- could avoid setting *minor if we're returning an error.
> - spin_lock_irqsave(_lock, flags);
> +static void
> +minor_free(ulong minor)
> +{
> + ulong flags;
>
> - for (d=devlist; d; d=d->next)
> - if (d->aoemajor == maj && d->aoeminor == min) {
> - d->ref++;
> - break;
> - }
> + minor /= AOE_PARTITIONS;
> + BUG_ON(minor >= N_DEVS);
>
> - spin_unlock_irqrestore(_lock, flags);
> - return d;
> + spin_lock_irqsave(_minors_lock, flags);
> + BUG_ON(!test_bit(minor, used_minors));
> + clear_bit(minor, used_minors);
> + spin_unlock_irqrestore(_minors_lock, flags);
> }
Could use
BUG_ON(!__test_and_clear_bit(...));
This will work, but I think it's bad form to put an expression with
side-effects inside an assert macro.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 1/7] aoe: support more AoE addresses with dynamic block device minor numbers
I should have mentioned that these seven patches were made for linux-next/akpm,
which contains the last 14-patch series for aoe.
On Oct 1, 2012, at 9:59 PM, "Ed Cashin" wrote:
> The ATA over Ethernet protocol uses a major (shelf) and
> minor (slot) address to identify a particular storage target.
> These changes remove an artificial limitation the aoe driver
> imposes on the use of AoE addresses. For example, without these
> changes, the slot address has a maximum of 15, but users commonly
> use slot numbers much greater than that.
>
> The AoE shelf and slot address space is often used sparsely.
> Instead of using a static mapping between AoE addresses and the
> block device minor number, the block device minor numbers are now
> allocated on demand.
>
> Signed-off-by: Ed Cashin
> ---
> drivers/block/aoe/aoe.h|6 ++--
> drivers/block/aoe/aoeblk.c |2 +-
> drivers/block/aoe/aoechr.c |2 +-
> drivers/block/aoe/aoecmd.c | 25 -
> drivers/block/aoe/aoedev.c | 86 ++--
> 5 files changed, 72 insertions(+), 49 deletions(-)
>
> diff --git a/drivers/block/aoe/aoe.h b/drivers/block/aoe/aoe.h
> index 27d0a21..7b694f7 100644
> --- a/drivers/block/aoe/aoe.h
> +++ b/drivers/block/aoe/aoe.h
> @@ -49,6 +49,8 @@ struct aoe_hdr {
>__be32 tag;
> };
>
> +#define AOE_MAXSHELF (0x-1)/* one less than the broadcast shelf
> address */
> +
> struct aoe_atahdr {
>unsigned char aflags;
>unsigned char errfeat;
> @@ -211,8 +213,7 @@ void aoe_ktstop(struct ktstate *k);
>
> int aoedev_init(void);
> void aoedev_exit(void);
> -struct aoedev *aoedev_by_aoeaddr(int maj, int min);
> -struct aoedev *aoedev_by_sysminor_m(ulong sysminor);
> +struct aoedev *aoedev_by_aoeaddr(ulong maj, int min, int do_alloc);
> void aoedev_downdev(struct aoedev *d);
> int aoedev_flush(const char __user *str, size_t size);
> void aoe_failbuf(struct aoedev *, struct buf *);
> @@ -223,4 +224,3 @@ void aoenet_exit(void);
> void aoenet_xmit(struct sk_buff_head *);
> int is_aoe_netif(struct net_device *ifp);
> int set_aoe_iflist(const char __user *str, size_t size);
> -
> diff --git a/drivers/block/aoe/aoeblk.c b/drivers/block/aoe/aoeblk.c
> index 83160ab..00dfc50 100644
> --- a/drivers/block/aoe/aoeblk.c
> +++ b/drivers/block/aoe/aoeblk.c
> @@ -249,7 +249,7 @@ aoeblk_gdalloc(void *vp)
>q->queuedata = d;
>d->gd = gd;
>gd->major = AOE_MAJOR;
> -gd->first_minor = d->sysminor * AOE_PARTITIONS;
> +gd->first_minor = d->sysminor;
>gd->fops = _bdops;
>gd->private_data = d;
>set_capacity(gd, d->ssize);
> diff --git a/drivers/block/aoe/aoechr.c b/drivers/block/aoe/aoechr.c
> index deb30c1..ed57a89 100644
> --- a/drivers/block/aoe/aoechr.c
> +++ b/drivers/block/aoe/aoechr.c
> @@ -91,7 +91,7 @@ revalidate(const char __user *str, size_t size)
>pr_err("aoe: invalid device specification %s\n", buf);
>return -EINVAL;
>}
> -d = aoedev_by_aoeaddr(major, minor);
> +d = aoedev_by_aoeaddr(major, minor, 0);
>if (!d)
>return -EINVAL;
>spin_lock_irqsave(>lock, flags);
> diff --git a/drivers/block/aoe/aoecmd.c b/drivers/block/aoe/aoecmd.c
> index 39dacdb..94e810c 100644
> --- a/drivers/block/aoe/aoecmd.c
> +++ b/drivers/block/aoe/aoecmd.c
> @@ -1149,7 +1149,7 @@ aoecmd_ata_rsp(struct sk_buff *skb)
>
>h = (struct aoe_hdr *) skb->data;
>aoemajor = be16_to_cpu(get_unaligned(>major));
> -d = aoedev_by_aoeaddr(aoemajor, h->minor);
> +d = aoedev_by_aoeaddr(aoemajor, h->minor, 0);
>if (d == NULL) {
>snprintf(ebuf, sizeof ebuf, "aoecmd_ata_rsp: ata response "
>"for unknown device %d.%d\n",
> @@ -1330,7 +1330,7 @@ aoecmd_cfg_rsp(struct sk_buff *skb)
>struct aoe_hdr *h;
>struct aoe_cfghdr *ch;
>struct aoetgt *t;
> -ulong flags, sysminor, aoemajor;
> +ulong flags, aoemajor;
>struct sk_buff *sl;
>struct sk_buff_head queue;
>u16 n;
> @@ -1349,18 +1349,15 @@ aoecmd_cfg_rsp(struct sk_buff *skb)
>"Check shelf dip switches.\n");
>return;
>}
> -if (h->minor >= NPERSHELF) {
> -pr_err("aoe: e%ld.%d %s, %d\n",
> -aoemajor, h->minor,
> -"slot number larger than the maximum",
> -NPERSHELF-1);
> +if (aoemajor > AOE_MAXSHELF) {
> +pr_info("aoe: e%ld.%d: shelf number too large\n",
> +aoemajor, (int) h->minor);
>return;
>}
>
> -sysminor = SYSMINOR(aoemajor, h->minor);
> -if (sysminor * AOE_PARTITIONS + AOE_PARTITIONS > MINORMASK) {
> -printk(KERN_INFO "aoe: e%ld.%d: minor number too large\n",
> -aoemajor, (int) h->minor);
> +d = aoedev_by_aoeaddr(aoemajor, h->minor, 1);
> +if (d == NULL) {
> +pr_info("aoe: device allocation failure\n");
>return;
>}
>
> @@ -1368,12 +1365,6 @@ aoecmd_cfg_rsp(struct sk_buff *skb)
>if (n > aoe_maxout)/* keep it reasonable */
>n = aoe_maxout;
>
> -d =
Re: [PATCH 1/7] aoe: support more AoE addresses with dynamic block device minor numbers
I should have mentioned that these seven patches were made for linux-next/akpm,
which contains the last 14-patch series for aoe.
On Oct 1, 2012, at 9:59 PM, Ed Cashin ecas...@coraid.com wrote:
The ATA over Ethernet protocol uses a major (shelf) and
minor (slot) address to identify a particular storage target.
These changes remove an artificial limitation the aoe driver
imposes on the use of AoE addresses. For example, without these
changes, the slot address has a maximum of 15, but users commonly
use slot numbers much greater than that.
The AoE shelf and slot address space is often used sparsely.
Instead of using a static mapping between AoE addresses and the
block device minor number, the block device minor numbers are now
allocated on demand.
Signed-off-by: Ed Cashin ecas...@coraid.com
---
drivers/block/aoe/aoe.h|6 ++--
drivers/block/aoe/aoeblk.c |2 +-
drivers/block/aoe/aoechr.c |2 +-
drivers/block/aoe/aoecmd.c | 25 -
drivers/block/aoe/aoedev.c | 86 ++--
5 files changed, 72 insertions(+), 49 deletions(-)
diff --git a/drivers/block/aoe/aoe.h b/drivers/block/aoe/aoe.h
index 27d0a21..7b694f7 100644
--- a/drivers/block/aoe/aoe.h
+++ b/drivers/block/aoe/aoe.h
@@ -49,6 +49,8 @@ struct aoe_hdr {
__be32 tag;
};
+#define AOE_MAXSHELF (0x-1)/* one less than the broadcast shelf
address */
+
struct aoe_atahdr {
unsigned char aflags;
unsigned char errfeat;
@@ -211,8 +213,7 @@ void aoe_ktstop(struct ktstate *k);
int aoedev_init(void);
void aoedev_exit(void);
-struct aoedev *aoedev_by_aoeaddr(int maj, int min);
-struct aoedev *aoedev_by_sysminor_m(ulong sysminor);
+struct aoedev *aoedev_by_aoeaddr(ulong maj, int min, int do_alloc);
void aoedev_downdev(struct aoedev *d);
int aoedev_flush(const char __user *str, size_t size);
void aoe_failbuf(struct aoedev *, struct buf *);
@@ -223,4 +224,3 @@ void aoenet_exit(void);
void aoenet_xmit(struct sk_buff_head *);
int is_aoe_netif(struct net_device *ifp);
int set_aoe_iflist(const char __user *str, size_t size);
-
diff --git a/drivers/block/aoe/aoeblk.c b/drivers/block/aoe/aoeblk.c
index 83160ab..00dfc50 100644
--- a/drivers/block/aoe/aoeblk.c
+++ b/drivers/block/aoe/aoeblk.c
@@ -249,7 +249,7 @@ aoeblk_gdalloc(void *vp)
q-queuedata = d;
d-gd = gd;
gd-major = AOE_MAJOR;
-gd-first_minor = d-sysminor * AOE_PARTITIONS;
+gd-first_minor = d-sysminor;
gd-fops = aoe_bdops;
gd-private_data = d;
set_capacity(gd, d-ssize);
diff --git a/drivers/block/aoe/aoechr.c b/drivers/block/aoe/aoechr.c
index deb30c1..ed57a89 100644
--- a/drivers/block/aoe/aoechr.c
+++ b/drivers/block/aoe/aoechr.c
@@ -91,7 +91,7 @@ revalidate(const char __user *str, size_t size)
pr_err(aoe: invalid device specification %s\n, buf);
return -EINVAL;
}
-d = aoedev_by_aoeaddr(major, minor);
+d = aoedev_by_aoeaddr(major, minor, 0);
if (!d)
return -EINVAL;
spin_lock_irqsave(d-lock, flags);
diff --git a/drivers/block/aoe/aoecmd.c b/drivers/block/aoe/aoecmd.c
index 39dacdb..94e810c 100644
--- a/drivers/block/aoe/aoecmd.c
+++ b/drivers/block/aoe/aoecmd.c
@@ -1149,7 +1149,7 @@ aoecmd_ata_rsp(struct sk_buff *skb)
h = (struct aoe_hdr *) skb-data;
aoemajor = be16_to_cpu(get_unaligned(h-major));
-d = aoedev_by_aoeaddr(aoemajor, h-minor);
+d = aoedev_by_aoeaddr(aoemajor, h-minor, 0);
if (d == NULL) {
snprintf(ebuf, sizeof ebuf, aoecmd_ata_rsp: ata response
for unknown device %d.%d\n,
@@ -1330,7 +1330,7 @@ aoecmd_cfg_rsp(struct sk_buff *skb)
struct aoe_hdr *h;
struct aoe_cfghdr *ch;
struct aoetgt *t;
-ulong flags, sysminor, aoemajor;
+ulong flags, aoemajor;
struct sk_buff *sl;
struct sk_buff_head queue;
u16 n;
@@ -1349,18 +1349,15 @@ aoecmd_cfg_rsp(struct sk_buff *skb)
Check shelf dip switches.\n);
return;
}
-if (h-minor = NPERSHELF) {
-pr_err(aoe: e%ld.%d %s, %d\n,
-aoemajor, h-minor,
-slot number larger than the maximum,
-NPERSHELF-1);
+if (aoemajor AOE_MAXSHELF) {
+pr_info(aoe: e%ld.%d: shelf number too large\n,
+aoemajor, (int) h-minor);
return;
}
-sysminor = SYSMINOR(aoemajor, h-minor);
-if (sysminor * AOE_PARTITIONS + AOE_PARTITIONS MINORMASK) {
-printk(KERN_INFO aoe: e%ld.%d: minor number too large\n,
-aoemajor, (int) h-minor);
+d = aoedev_by_aoeaddr(aoemajor, h-minor, 1);
+if (d == NULL) {
+pr_info(aoe: device allocation failure\n);
return;
}
@@ -1368,12 +1365,6 @@ aoecmd_cfg_rsp(struct sk_buff *skb)
if (n aoe_maxout)/* keep it reasonable */
n = aoe_maxout;
-d = aoedev_by_sysminor_m(sysminor);
-if (d == NULL) {
-printk(KERN_INFO aoe: device sysminor_m failure\n);
-return;
-}
Re: [PATCH 1/7] aoe: support more AoE addresses with dynamic block device minor numbers
On Mon, 1 Oct 2012 18:59:12 -0700
Ed Cashin ecas...@coraid.com wrote:
The ATA over Ethernet protocol uses a major (shelf) and
minor (slot) address to identify a particular storage target.
These changes remove an artificial limitation the aoe driver
imposes on the use of AoE addresses. For example, without these
changes, the slot address has a maximum of 15, but users commonly
use slot numbers much greater than that.
The AoE shelf and slot address space is often used sparsely.
Instead of using a static mapping between AoE addresses and the
block device minor number, the block device minor numbers are now
allocated on demand.
...
Very minor things...
...
+static int
+minor_get(ulong *minor)
{
- struct aoedev *d;
ulong flags;
+ ulong n;
+ int error = 0;
+
+ spin_lock_irqsave(used_minors_lock, flags);
+ n = find_first_zero_bit(used_minors, N_DEVS);
+ if (n N_DEVS)
+ set_bit(n, used_minors);
+ else
+ error = -1;
+ spin_unlock_irqrestore(used_minors_lock, flags);
+
+ *minor = n * AOE_PARTITIONS;
+ return error;
+}
- can use the more efficient __set_bit() inside that spinlock.
- could avoid setting *minor if we're returning an error.
- spin_lock_irqsave(devlist_lock, flags);
+static void
+minor_free(ulong minor)
+{
+ ulong flags;
- for (d=devlist; d; d=d-next)
- if (d-aoemajor == maj d-aoeminor == min) {
- d-ref++;
- break;
- }
+ minor /= AOE_PARTITIONS;
+ BUG_ON(minor = N_DEVS);
- spin_unlock_irqrestore(devlist_lock, flags);
- return d;
+ spin_lock_irqsave(used_minors_lock, flags);
+ BUG_ON(!test_bit(minor, used_minors));
+ clear_bit(minor, used_minors);
+ spin_unlock_irqrestore(used_minors_lock, flags);
}
Could use
BUG_ON(!__test_and_clear_bit(...));
This will work, but I think it's bad form to put an expression with
side-effects inside an assert macro.
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 1/7] aoe: support more AoE addresses with dynamic block device minor numbers
On Oct 2, 2012, at 5:12 PM, Andrew Morton wrote:
...
+static int
+minor_get(ulong *minor)
{
-struct aoedev *d;
ulong flags;
+ulong n;
+int error = 0;
+
+spin_lock_irqsave(used_minors_lock, flags);
+n = find_first_zero_bit(used_minors, N_DEVS);
+if (n N_DEVS)
+set_bit(n, used_minors);
+else
+error = -1;
+spin_unlock_irqrestore(used_minors_lock, flags);
+
+*minor = n * AOE_PARTITIONS;
+return error;
+}
- can use the more efficient __set_bit() inside that spinlock.
Thanks for that observation. Because this operation occurs on target
discovery, which is expected to be relatively infrequent, my inclination is to
leave it in its atomic form, though, and leave the __set_bit() for another time
when optimization is needed. Like you said, this is a minor point. I wouldn't
mind changing it, though, if you think it's worth me resubmitting the patch.
Just let me know.
- could avoid setting *minor if we're returning an error.
Yes. The only caller of aoedev.c:minor_get() handles that correctly. Again,
just let me know if you think this is worth a resubmission of the patch.
Otherwise I'll just make a note to myself to try to avoid setting output
parameters on error in the future.
--
Ed Cashin
ecas...@coraid.com
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
