[GIT PATCH] block,scsi,libata: implement alt_size, take#2

[GIT PATCH] block,scsi,libata: implement alt_size, take#2

[Tejun Heo]

Hello,

Upon ack, please pull from the following git tree.

  git://git.kernel.org/pub/scm/linux/kernel/git/tj/misc.git block-alt_size

This is the second take of implement-alt_size patchset.  Other than
being refreshed to apply on the current block tree, there's no real
change from the last take[L].

Jeff reluctantly acked it and Dan Williams is gonna handle mdadm part
of userland, so I guess it's about time to push this in.

This patchset is on top of the current block#for-2.6.31(f68adec3) and
can be automatically merged with block-peek-fetch branch by pulling.
diffstat follows.

 block/genhd.c              |   26 ++++++++++++++++++++++++++
 drivers/ata/libata-core.c  |    2 ++
 drivers/ata/libata-scsi.c  |    2 ++
 drivers/scsi/sd.c          |    1 +
 include/linux/genhd.h      |   13 ++++++++++++-
 include/linux/libata.h     |    1 +
 include/scsi/scsi_device.h |    1 +
 7 files changed, 45 insertions(+), 1 deletion(-)

Thanks.

--
tejun

[L] http://thread.gmane.org/gmane.linux.kernel.device-mapper.devel/7235

[GIT PATCH] block,scsi,libata: implement alt_size, take#2

[Tejun Heo]

Hello,

Upon ack, please pull from the following git tree.

  git://git.kernel.org/pub/scm/linux/kernel/git/tj/misc.git block-alt_size

This is the second take of implement-alt_size patchset.  Other than
being refreshed to apply on the current block tree, there's no real
change from the last take[L].

Jeff reluctantly acked it and Dan Williams is gonna handle mdadm part
of userland, so I guess it's about time to push this in.

This patchset is on top of the current block#for-2.6.31(f68adec3) and
can be automatically merged with block-peek-fetch branch by pulling.
diffstat follows.

 block/genhd.c              |   26 ++++++++++++++++++++++++++
 drivers/ata/libata-core.c  |    2 ++
 drivers/ata/libata-scsi.c  |    2 ++
 drivers/scsi/sd.c          |    1 +
 include/linux/genhd.h      |   13 ++++++++++++-
 include/linux/libata.h     |    1 +
 include/scsi/scsi_device.h |    1 +
 7 files changed, 45 insertions(+), 1 deletion(-)

Thanks.

--
tejun

[L] http://thread.gmane.org/gmane.linux.kernel.device-mapper.devel/7235

[PATCH 1/3] block: add alt_size

[Tejun Heo]

ATA disks have two sizes - the user visible size and native size.  The
user visible size is usually used by the BIOS to hide certain area
(e.g. recovery partition) or trim size for various reasons.  IDE and
libata can unlock these HPA areas and IDE does so by default and
libata depending on kernel parameters (some distros default to unlock
to maintain compatibility with IDE).

This usually doesn't matter but certain tools need to know what BIOS
wants the disk to look like.  For example, certain BIOS RAID formats
put the metadata at the end of the disk where the 'end' is the trimmed
size, so without knowing the BIOS size, dmraid can't work reliably if
HPA is unlocked.

This patch adds alt_size to genhd and exports it via sysfs.  alt_size
is always smaller than the regular size.  Being a hint, alt_size can
also be set from userland by writing to the sysfs node mostly for
debugging and testing.  Because initialization order isn't enforced,
the alt_size < size limitation is applied while getting the attribute.

Signed-off-by: Tejun Heo <tj@kernel.org>
Cc: Jens Axboe <jens.axboe@oracle.com>
Cc: Dan Williams <dan.j.williams@gmail.com>
Cc: Jeff Garzik <jeff@garzik.org>
Cc: James Bottomley <James.Bottomley@HansenPartnership.com>
---
 block/genhd.c         |   26 ++++++++++++++++++++++++++
 include/linux/genhd.h |   13 ++++++++++++-
 2 files changed, 38 insertions(+), 1 deletions(-)

diff --git a/block/genhd.c b/block/genhd.c
index 1a4916e..e6a3fdb 100644
--- a/block/genhd.c
+++ b/block/genhd.c
@@ -844,6 +844,29 @@ static ssize_t disk_ro_show(struct device *dev,
 	return sprintf(buf, "%d\n", get_disk_ro(disk) ? 1 : 0);
 }
 
+static ssize_t disk_alt_size_show(struct device *dev,
+				  struct device_attribute *attr, char *buf)
+{
+	struct gendisk *disk = dev_to_disk(dev);
+
+	return sprintf(buf, "%llu\n",
+		       (unsigned long long)get_alt_capacity(disk));
+}
+
+static ssize_t disk_alt_size_store(struct device *dev,
+				   struct device_attribute *attr,
+				   const char *buf, size_t count)
+{
+	struct gendisk *disk = dev_to_disk(dev);
+	sector_t new_size = simple_strtoull(buf, NULL, 0);
+
+	set_alt_capacity(disk, new_size);
+
+	if (get_alt_capacity(disk) == new_size)
+		return count;
+	return -EINVAL;
+}
+
 static ssize_t disk_capability_show(struct device *dev,
 				    struct device_attribute *attr, char *buf)
 {
@@ -857,6 +880,8 @@ static DEVICE_ATTR(ext_range, S_IRUGO, disk_ext_range_show, NULL);
 static DEVICE_ATTR(removable, S_IRUGO, disk_removable_show, NULL);
 static DEVICE_ATTR(ro, S_IRUGO, disk_ro_show, NULL);
 static DEVICE_ATTR(size, S_IRUGO, part_size_show, NULL);
+static DEVICE_ATTR(alt_size, S_IRUGO | S_IWUSR, disk_alt_size_show,
+		   disk_alt_size_store);
 static DEVICE_ATTR(capability, S_IRUGO, disk_capability_show, NULL);
 static DEVICE_ATTR(stat, S_IRUGO, part_stat_show, NULL);
 #ifdef CONFIG_FAIL_MAKE_REQUEST
@@ -875,6 +900,7 @@ static struct attribute *disk_attrs[] = {
 	&dev_attr_removable.attr,
 	&dev_attr_ro.attr,
 	&dev_attr_size.attr,
+	&dev_attr_alt_size.attr,
 	&dev_attr_capability.attr,
 	&dev_attr_stat.attr,
 #ifdef CONFIG_FAIL_MAKE_REQUEST
diff --git a/include/linux/genhd.h b/include/linux/genhd.h
index a1a28ca..a83e4e6 100644
--- a/include/linux/genhd.h
+++ b/include/linux/genhd.h
@@ -159,7 +159,8 @@ struct gendisk {
 
 	struct timer_rand_state *random;
 
-	atomic_t sync_io;		/* RAID */
+	sector_t alt_nr_sects;	/* alt sects, should be <= part0.sects */
+	atomic_t sync_io;	/* RAID */
 	struct work_struct async_notify;
 #ifdef  CONFIG_BLK_DEV_INTEGRITY
 	struct blk_integrity *integrity;
@@ -364,10 +365,20 @@ static inline sector_t get_capacity(struct gendisk *disk)
 {
 	return disk->part0.nr_sects;
 }
+static inline sector_t get_alt_capacity(struct gendisk *disk)
+{
+	if (disk->alt_nr_sects && disk->alt_nr_sects < disk->part0.nr_sects)
+		return disk->alt_nr_sects;
+	return 0;
+}
 static inline void set_capacity(struct gendisk *disk, sector_t size)
 {
 	disk->part0.nr_sects = size;
 }
+static inline void set_alt_capacity(struct gendisk *disk, sector_t size)
+{
+	disk->alt_nr_sects = size;
+}
 
 #ifdef CONFIG_SOLARIS_X86_PARTITION
 
-- 
1.6.0.2

[PATCH 1/3] block: add alt_size

[Tejun Heo]

ATA disks have two sizes - the user visible size and native size.  The
user visible size is usually used by the BIOS to hide certain area
(e.g. recovery partition) or trim size for various reasons.  IDE and
libata can unlock these HPA areas and IDE does so by default and
libata depending on kernel parameters (some distros default to unlock
to maintain compatibility with IDE).

This usually doesn't matter but certain tools need to know what BIOS
wants the disk to look like.  For example, certain BIOS RAID formats
put the metadata at the end of the disk where the 'end' is the trimmed
size, so without knowing the BIOS size, dmraid can't work reliably if
HPA is unlocked.

This patch adds alt_size to genhd and exports it via sysfs.  alt_size
is always smaller than the regular size.  Being a hint, alt_size can
also be set from userland by writing to the sysfs node mostly for
debugging and testing.  Because initialization order isn't enforced,
the alt_size < size limitation is applied while getting the attribute.

Signed-off-by: Tejun Heo <tj@kernel.org>
Cc: Jens Axboe <jens.axboe@oracle.com>
Cc: Dan Williams <dan.j.williams@gmail.com>
Cc: Jeff Garzik <jeff@garzik.org>
Cc: James Bottomley <James.Bottomley@HansenPartnership.com>
---
 block/genhd.c         |   26 ++++++++++++++++++++++++++
 include/linux/genhd.h |   13 ++++++++++++-
 2 files changed, 38 insertions(+), 1 deletions(-)

diff --git a/block/genhd.c b/block/genhd.c
index 1a4916e..e6a3fdb 100644
--- a/block/genhd.c
+++ b/block/genhd.c
@@ -844,6 +844,29 @@ static ssize_t disk_ro_show(struct device *dev,
 	return sprintf(buf, "%d\n", get_disk_ro(disk) ? 1 : 0);
 }
 
+static ssize_t disk_alt_size_show(struct device *dev,
+				  struct device_attribute *attr, char *buf)
+{
+	struct gendisk *disk = dev_to_disk(dev);
+
+	return sprintf(buf, "%llu\n",
+		       (unsigned long long)get_alt_capacity(disk));
+}
+
+static ssize_t disk_alt_size_store(struct device *dev,
+				   struct device_attribute *attr,
+				   const char *buf, size_t count)
+{
+	struct gendisk *disk = dev_to_disk(dev);
+	sector_t new_size = simple_strtoull(buf, NULL, 0);
+
+	set_alt_capacity(disk, new_size);
+
+	if (get_alt_capacity(disk) == new_size)
+		return count;
+	return -EINVAL;
+}
+
 static ssize_t disk_capability_show(struct device *dev,
 				    struct device_attribute *attr, char *buf)
 {
@@ -857,6 +880,8 @@ static DEVICE_ATTR(ext_range, S_IRUGO, disk_ext_range_show, NULL);
 static DEVICE_ATTR(removable, S_IRUGO, disk_removable_show, NULL);
 static DEVICE_ATTR(ro, S_IRUGO, disk_ro_show, NULL);
 static DEVICE_ATTR(size, S_IRUGO, part_size_show, NULL);
+static DEVICE_ATTR(alt_size, S_IRUGO | S_IWUSR, disk_alt_size_show,
+		   disk_alt_size_store);
 static DEVICE_ATTR(capability, S_IRUGO, disk_capability_show, NULL);
 static DEVICE_ATTR(stat, S_IRUGO, part_stat_show, NULL);
 #ifdef CONFIG_FAIL_MAKE_REQUEST
@@ -875,6 +900,7 @@ static struct attribute *disk_attrs[] = {
 	&dev_attr_removable.attr,
 	&dev_attr_ro.attr,
 	&dev_attr_size.attr,
+	&dev_attr_alt_size.attr,
 	&dev_attr_capability.attr,
 	&dev_attr_stat.attr,
 #ifdef CONFIG_FAIL_MAKE_REQUEST
diff --git a/include/linux/genhd.h b/include/linux/genhd.h
index a1a28ca..a83e4e6 100644
--- a/include/linux/genhd.h
+++ b/include/linux/genhd.h
@@ -159,7 +159,8 @@ struct gendisk {
 
 	struct timer_rand_state *random;
 
-	atomic_t sync_io;		/* RAID */
+	sector_t alt_nr_sects;	/* alt sects, should be <= part0.sects */
+	atomic_t sync_io;	/* RAID */
 	struct work_struct async_notify;
 #ifdef  CONFIG_BLK_DEV_INTEGRITY
 	struct blk_integrity *integrity;
@@ -364,10 +365,20 @@ static inline sector_t get_capacity(struct gendisk *disk)
 {
 	return disk->part0.nr_sects;
 }
+static inline sector_t get_alt_capacity(struct gendisk *disk)
+{
+	if (disk->alt_nr_sects && disk->alt_nr_sects < disk->part0.nr_sects)
+		return disk->alt_nr_sects;
+	return 0;
+}
 static inline void set_capacity(struct gendisk *disk, sector_t size)
 {
 	disk->part0.nr_sects = size;
 }
+static inline void set_alt_capacity(struct gendisk *disk, sector_t size)
+{
+	disk->alt_nr_sects = size;
+}
 
 #ifdef CONFIG_SOLARIS_X86_PARTITION
 
-- 
1.6.0.2

Re: [PATCH 1/3] block: add alt_size

[Kay Sievers]

On Sat, May 9, 2009 at 02:13, Tejun Heo <tj@kernel.org> wrote:
> This patch adds alt_size to genhd and exports it via sysfs.  alt_size
> is always smaller than the regular size.  Being a hint, alt_size can
> also be set from userland by writing to the sysfs node mostly for
> debugging and testing.

What does "alt_" stand for? I think that should be more descriptive in
an exported interface.

And can we please keep the "size_*" in front of the name, so that they
group together?

Also, values with magic block counts, while there is no way to get the
blocksize with the same interface, are pretty weird. I think the
current "size" attribute is just a bug.
Not sure, how that should be solved, by adding a "blocksize" attribute
that is always in the same context as the current "size*" values, or
by just using bytes for new attributes here.

Almost all tools I've seen using these attributes, have hardcoded *
512 in there, which may cause trouble pretty soon. And this is mostly
a failure of the interface and not of the users, I think.

Thanks,
Kay
--
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

Re: [PATCH 1/3] block: add alt_size

[Kay Sievers]

On Sat, May 9, 2009 at 02:13, Tejun Heo <tj@kernel.org> wrote:
> This patch adds alt_size to genhd and exports it via sysfs.  alt_size
> is always smaller than the regular size.  Being a hint, alt_size can
> also be set from userland by writing to the sysfs node mostly for
> debugging and testing.

What does "alt_" stand for? I think that should be more descriptive in
an exported interface.

And can we please keep the "size_*" in front of the name, so that they
group together?

Also, values with magic block counts, while there is no way to get the
blocksize with the same interface, are pretty weird. I think the
current "size" attribute is just a bug.
Not sure, how that should be solved, by adding a "blocksize" attribute
that is always in the same context as the current "size*" values, or
by just using bytes for new attributes here.

Almost all tools I've seen using these attributes, have hardcoded *
512 in there, which may cause trouble pretty soon. And this is mostly
a failure of the interface and not of the users, I think.

Thanks,
Kay

Re: [PATCH 1/3] block: add alt_size

[Tejun Heo]

Hello,

Kay Sievers wrote:
> What does "alt_" stand for? I think that should be more descriptive in
> an exported interface.

Alternative.

> And can we please keep the "size_*" in front of the name, so that they
> group together?

Maybe, but size_alt?  Any better ideas?

> Also, values with magic block counts, while there is no way to get the
> blocksize with the same interface, are pretty weird. I think the
> current "size" attribute is just a bug.

Logical block size is fixed at 512 bytes.  Offset and size are always
represented in multiples of 512 bytes and only get converted to
hardware block size in the lld.

> Not sure, how that should be solved, by adding a "blocksize" attribute
> that is always in the same context as the current "size*" values, or
> by just using bytes for new attributes here.
> 
> Almost all tools I've seen using these attributes, have hardcoded *
> 512 in there, which may cause trouble pretty soon. And this is mostly
> a failure of the interface and not of the users, I think.

No, it will never break.  It will always be 512.  It's there to give
nine bit shift to allow additional 9 bit of addressing without going
to the next full blown bitwidth.  It's chosen to be the lowest common
denominator which gives enough addressing boost to hold things
together till the next bitwidth becomes popular.

For userlevel exporting, it might have been better to use just bytes
there as preformance isn't really an issue, but, well, it's already
determined, so..

Thanks.

-- 
tejun

Re: [PATCH 1/3] block: add alt_size

[Tejun Heo]

Hello,

Kay Sievers wrote:
> What does "alt_" stand for? I think that should be more descriptive in
> an exported interface.

Alternative.

> And can we please keep the "size_*" in front of the name, so that they
> group together?

Maybe, but size_alt?  Any better ideas?

> Also, values with magic block counts, while there is no way to get the
> blocksize with the same interface, are pretty weird. I think the
> current "size" attribute is just a bug.

Logical block size is fixed at 512 bytes.  Offset and size are always
represented in multiples of 512 bytes and only get converted to
hardware block size in the lld.

> Not sure, how that should be solved, by adding a "blocksize" attribute
> that is always in the same context as the current "size*" values, or
> by just using bytes for new attributes here.
> 
> Almost all tools I've seen using these attributes, have hardcoded *
> 512 in there, which may cause trouble pretty soon. And this is mostly
> a failure of the interface and not of the users, I think.

No, it will never break.  It will always be 512.  It's there to give
nine bit shift to allow additional 9 bit of addressing without going
to the next full blown bitwidth.  It's chosen to be the lowest common
denominator which gives enough addressing boost to hold things
together till the next bitwidth becomes popular.

For userlevel exporting, it might have been better to use just bytes
there as preformance isn't really an issue, but, well, it's already
determined, so..

Thanks.

-- 
tejun

Re: [PATCH 1/3] block: add alt_size

[Kay Sievers]

On Sat, May 9, 2009 at 16:04, Tejun Heo <tj@kernel.org> wrote:
> Kay Sievers wrote:
>> What does "alt_" stand for? I think that should be more descriptive in
>> an exported interface.
>
> Alternative.
>
>> And can we please keep the "size_*" in front of the name, so that they
>> group together?
>
> Maybe, but size_alt?  Any better ideas?

"size_limit"
"size_restricted"
"size_clipped"
"size_constrain"

anything that ideally would express that this is smaller than the
actual "size", and that is is a "configured" value and not some
hardware property.

>> Also, values with magic block counts, while there is no way to get the
>> blocksize with the same interface, are pretty weird. I think the
>> current "size" attribute is just a bug.
>
> Logical block size is fixed at 512 bytes.  Offset and size are always
> represented in multiples of 512 bytes and only get converted to
> hardware block size in the lld.

Oh, good. Didn't know that this will always be 512, even for devices
with a native size larger than this.

>> Not sure, how that should be solved, by adding a "blocksize" attribute
>> that is always in the same context as the current "size*" values, or
>> by just using bytes for new attributes here.
>>
>> Almost all tools I've seen using these attributes, have hardcoded *
>> 512 in there, which may cause trouble pretty soon. And this is mostly
>> a failure of the interface and not of the users, I think.
>
> No, it will never break.  It will always be 512.

Cool. No problem then. :)

> For userlevel exporting, it might have been better to use just bytes
> there as preformance isn't really an issue, but, well, it's already
> determined, so..

Right, but if it can not change, it's fine, I guess.

Thanks,
Kay

Re: [dm-devel] Re: [PATCH 1/3] block: add alt_size

[Konrad Rzeszutek]

.. snip ..
> > Also, values with magic block counts, while there is no way to get the
> > blocksize with the same interface, are pretty weird. I think the
> > current "size" attribute is just a bug.
> 
> Logical block size is fixed at 512 bytes.  Offset and size are always
> represented in multiples of 512 bytes and only get converted to
> hardware block size in the lld.

That interpretation is at odds with the work that Martin Peterson is
doing with the 4K support. In the e-mail titled: "Re: [PATCH 4 of 8] sd:
Physical block size and alignment support",
Message-ID:<yq1ab67b51p.fsf@sermon.lab.mkp.net> he says:

"
	Konrad> about what a 'logical block', and 'physical block' is
	Konrad> vs. 'hardware sector' ?

	Well, another item on my todo list is to kill the notion of hardware
	sector completely.  The protocols have been referring to logical blocks
	for ages.

	It hasn't been a big problem until now because logical block size has
	been equal to the hardware sector size.  That's no longer a valid
	assumption.
"

Are the ATA/SCSI/etc specs at odds with each other about this?

Re: Re: [PATCH 1/3] block: add alt_size

[Tejun Heo]

Konrad Rzeszutek wrote:
> .. snip ..
>>> Also, values with magic block counts, while there is no way to get the
>>> blocksize with the same interface, are pretty weird. I think the
>>> current "size" attribute is just a bug.
>> Logical block size is fixed at 512 bytes.  Offset and size are always
>> represented in multiples of 512 bytes and only get converted to
>> hardware block size in the lld.
> 
> That interpretation is at odds with the work that Martin Peterson is
> doing with the 4K support. In the e-mail titled: "Re: [PATCH 4 of 8] sd:
> Physical block size and alignment support",
> Message-ID:<yq1ab67b51p.fsf@sermon.lab.mkp.net> he says:
> 
> "
> 	Konrad> about what a 'logical block', and 'physical block' is
> 	Konrad> vs. 'hardware sector' ?
> 
> 	Well, another item on my todo list is to kill the notion of hardware
> 	sector completely.  The protocols have been referring to logical blocks
> 	for ages.
> 
> 	It hasn't been a big problem until now because logical block size has
> 	been equal to the hardware sector size.  That's no longer a valid
> 	assumption.
> "
> 
> Are the ATA/SCSI/etc specs at odds with each other about this?

Hardware specs aren't of concern here.  The logical block concept is
there simply to give 9 bit addressing advantage, nothing more, nothing
less.  If hardware's sector size doesn't match it, the lld should be
mapping the sector addresses and sizes and cdrom and a few other
drives have been doing that for ages.  There's nothing new about
devices with sectors larger than 512 bytes.

Thanks.

-- 
tejun

Re: [dm-devel] Re: [PATCH 1/3] block: add alt_size

[Tejun Heo]

Konrad Rzeszutek wrote:
> .. snip ..
>>> Also, values with magic block counts, while there is no way to get the
>>> blocksize with the same interface, are pretty weird. I think the
>>> current "size" attribute is just a bug.
>> Logical block size is fixed at 512 bytes.  Offset and size are always
>> represented in multiples of 512 bytes and only get converted to
>> hardware block size in the lld.
> 
> That interpretation is at odds with the work that Martin Peterson is
> doing with the 4K support. In the e-mail titled: "Re: [PATCH 4 of 8] sd:
> Physical block size and alignment support",
> Message-ID:<yq1ab67b51p.fsf@sermon.lab.mkp.net> he says:
> 
> "
> 	Konrad> about what a 'logical block', and 'physical block' is
> 	Konrad> vs. 'hardware sector' ?
> 
> 	Well, another item on my todo list is to kill the notion of hardware
> 	sector completely.  The protocols have been referring to logical blocks
> 	for ages.
> 
> 	It hasn't been a big problem until now because logical block size has
> 	been equal to the hardware sector size.  That's no longer a valid
> 	assumption.
> "
> 
> Are the ATA/SCSI/etc specs at odds with each other about this?

Hardware specs aren't of concern here.  The logical block concept is
there simply to give 9 bit addressing advantage, nothing more, nothing
less.  If hardware's sector size doesn't match it, the lld should be
mapping the sector addresses and sizes and cdrom and a few other
drives have been doing that for ages.  There's nothing new about
devices with sectors larger than 512 bytes.

Thanks.

-- 
tejun

[PATCH 2/3] scsi: add scsi_device->alt_capacity

[Tejun Heo]

Add scsi_device->alt_capacity and let sd pass it over to genhd.  This
is to allow SCSI low level drivers to configure alt_capacity via
slave_configure().

Signed-off-by: Tejun Heo <tj@kernel.org>
Cc: Jens Axboe <jens.axboe@oracle.com>
Cc: Dan Williams <dan.j.williams@gmail.com>
Cc: Jeff Garzik <jeff@garzik.org>
Cc: James Bottomley <James.Bottomley@HansenPartnership.com>
---
 drivers/scsi/sd.c          |    1 +
 include/scsi/scsi_device.h |    1 +
 2 files changed, 2 insertions(+), 0 deletions(-)

diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c
index 3fcb64b..f3448bf 100644
--- a/drivers/scsi/sd.c
+++ b/drivers/scsi/sd.c
@@ -1839,6 +1839,7 @@ static int sd_revalidate_disk(struct gendisk *disk)
 	blk_queue_ordered(sdkp->disk->queue, ordered, sd_prepare_flush);
 
 	set_capacity(disk, sdkp->capacity);
+	set_alt_capacity(disk, sdp->alt_capacity);
 	kfree(buffer);
 
  out:
diff --git a/include/scsi/scsi_device.h b/include/scsi/scsi_device.h
index 3f566af..b24fdeb 100644
--- a/include/scsi/scsi_device.h
+++ b/include/scsi/scsi_device.h
@@ -92,6 +92,7 @@ struct scsi_device {
 	unsigned int manufacturer;	/* Manufacturer of device, for using 
 					 * vendor-specific cmd's */
 	unsigned sector_size;	/* size in bytes */
+	size_t alt_capacity;	/* alternative capacity, used by sd */
 
 	void *hostdata;		/* available to low-level driver */
 	char type;
-- 
1.6.0.2

[PATCH 2/3] scsi: add scsi_device->alt_capacity

[Tejun Heo]

Add scsi_device->alt_capacity and let sd pass it over to genhd.  This
is to allow SCSI low level drivers to configure alt_capacity via
slave_configure().

Signed-off-by: Tejun Heo <tj@kernel.org>
Cc: Jens Axboe <jens.axboe@oracle.com>
Cc: Dan Williams <dan.j.williams@gmail.com>
Cc: Jeff Garzik <jeff@garzik.org>
Cc: James Bottomley <James.Bottomley@HansenPartnership.com>
---
 drivers/scsi/sd.c          |    1 +
 include/scsi/scsi_device.h |    1 +
 2 files changed, 2 insertions(+), 0 deletions(-)

diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c
index 3fcb64b..f3448bf 100644
--- a/drivers/scsi/sd.c
+++ b/drivers/scsi/sd.c
@@ -1839,6 +1839,7 @@ static int sd_revalidate_disk(struct gendisk *disk)
 	blk_queue_ordered(sdkp->disk->queue, ordered, sd_prepare_flush);
 
 	set_capacity(disk, sdkp->capacity);
+	set_alt_capacity(disk, sdp->alt_capacity);
 	kfree(buffer);
 
  out:
diff --git a/include/scsi/scsi_device.h b/include/scsi/scsi_device.h
index 3f566af..b24fdeb 100644
--- a/include/scsi/scsi_device.h
+++ b/include/scsi/scsi_device.h
@@ -92,6 +92,7 @@ struct scsi_device {
 	unsigned int manufacturer;	/* Manufacturer of device, for using 
 					 * vendor-specific cmd's */
 	unsigned sector_size;	/* size in bytes */
+	size_t alt_capacity;	/* alternative capacity, used by sd */
 
 	void *hostdata;		/* available to low-level driver */
 	char type;
-- 
1.6.0.2

Re: [PATCH 2/3] scsi: add scsi_device->alt_capacity

[James Bottomley]

On Sat, 2009-05-09 at 09:13 +0900, Tejun Heo wrote:
> Add scsi_device->alt_capacity and let sd pass it over to genhd.  This
> is to allow SCSI low level drivers to configure alt_capacity via
> slave_configure().
> 
> Signed-off-by: Tejun Heo <tj@kernel.org>
> Cc: Jens Axboe <jens.axboe@oracle.com>
> Cc: Dan Williams <dan.j.williams@gmail.com>
> Cc: Jeff Garzik <jeff@garzik.org>
> Cc: James Bottomley <James.Bottomley@HansenPartnership.com>
> ---
>  drivers/scsi/sd.c          |    1 +
>  include/scsi/scsi_device.h |    1 +
>  2 files changed, 2 insertions(+), 0 deletions(-)
> 
> diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c
> index 3fcb64b..f3448bf 100644
> --- a/drivers/scsi/sd.c
> +++ b/drivers/scsi/sd.c
> @@ -1839,6 +1839,7 @@ static int sd_revalidate_disk(struct gendisk *disk)
>  	blk_queue_ordered(sdkp->disk->queue, ordered, sd_prepare_flush);
>  
>  	set_capacity(disk, sdkp->capacity);
> +	set_alt_capacity(disk, sdp->alt_capacity);
>  	kfree(buffer);
>  
>   out:
> diff --git a/include/scsi/scsi_device.h b/include/scsi/scsi_device.h
> index 3f566af..b24fdeb 100644
> --- a/include/scsi/scsi_device.h
> +++ b/include/scsi/scsi_device.h
> @@ -92,6 +92,7 @@ struct scsi_device {
>  	unsigned int manufacturer;	/* Manufacturer of device, for using 
>  					 * vendor-specific cmd's */
>  	unsigned sector_size;	/* size in bytes */
> +	size_t alt_capacity;	/* alternative capacity, used by sd */
>  
>  	void *hostdata;		/* available to low-level driver */
>  	char type;

This is done at slightly the wrong level.  Capacity is actually a
property of struct scsi_disk not struct scsi_device ... shouldn't
alt_capacity be at the same level?

James

Re: [PATCH 2/3] scsi: add scsi_device->alt_capacity

[Tejun Heo]

James Bottomley wrote:
> This is done at slightly the wrong level.  Capacity is actually a
> property of struct scsi_disk not struct scsi_device ... shouldn't
> alt_capacity be at the same level?

Hmmm... I think that was my first try and then I moved it to sdev for
some reason I can't rememer now.  I'll look into it again and try to
move it into sdev.

Thanks.

-- 
tejun

Re: [PATCH 2/3] scsi: add scsi_device->alt_capacity

[Tejun Heo]

James Bottomley wrote:
> This is done at slightly the wrong level.  Capacity is actually a
> property of struct scsi_disk not struct scsi_device ... shouldn't
> alt_capacity be at the same level?

Hmmm... I think that was my first try and then I moved it to sdev for
some reason I can't rememer now.  I'll look into it again and try to
move it into sdev.

Thanks.

-- 
tejun

Re: [PATCH 2/3] scsi: add scsi_device->alt_capacity

[James Bottomley]

On Sun, 2009-05-10 at 01:09 +0900, Tejun Heo wrote:
> James Bottomley wrote:
> > This is done at slightly the wrong level.  Capacity is actually a
> > property of struct scsi_disk not struct scsi_device ... shouldn't
> > alt_capacity be at the same level?
> 
> Hmmm... I think that was my first try and then I moved it to sdev for
> some reason I can't rememer now.  I'll look into it again and try to
> move it into sdev.

Really one of the things I was wondering is why even scsi_disk ...
capacity is in there, but it's also in gendisk, so I've thought
(admittedly never translated it to action) that we could just remove the
duplication in scsi_disk.

This alt_capacity looks to be a pure ATA thing ... I can't find it in
the SCSI specs and there doesn't seem to be a SAT equivalent of the
commands.  Ideally, what should be happening is that the ata ULD would
issue the capacity commands and just set the block alt_capacity without
having to worry about transporting the value up and down the stack.
Matthew Wilcox thought we could begin an implementation of the ATA uld
using the ATA_16 command to transport it through SCSI ... this might
provide the good reason to begin that.

James

Re: [PATCH 2/3] scsi: add scsi_device->alt_capacity

[Tejun Heo]

Hello, James.

James Bottomley wrote:
> On Sun, 2009-05-10 at 01:09 +0900, Tejun Heo wrote:
>> James Bottomley wrote:
>>> This is done at slightly the wrong level.  Capacity is actually a
>>> property of struct scsi_disk not struct scsi_device ... shouldn't
>>> alt_capacity be at the same level?
>> Hmmm... I think that was my first try and then I moved it to sdev for
>> some reason I can't rememer now.  I'll look into it again and try to
>> move it into sdev.
> 
> Really one of the things I was wondering is why even scsi_disk ...
> capacity is in there, but it's also in gendisk, so I've thought
> (admittedly never translated it to action) that we could just remove the
> duplication in scsi_disk.
> 
> This alt_capacity looks to be a pure ATA thing...  I can't find it
> in the SCSI specs and there doesn't seem to be a SAT equivalent of
> the commands.  Ideally, what should be happening is that the ata ULD
> would issue the capacity commands and just set the block
> alt_capacity without having to worry about transporting the value up
> and down the stack.  Matthew Wilcox thought we could begin an
> implementation of the ATA uld using the ATA_16 command to transport
> it through SCSI ... this might provide the good reason to begin
> that.

Yeap, it's a purely ATA specific workaround.

Now I remembered what was the problem.  The problem was that the
latest hook libata can use is ->slave_configure which is called before
HLD attaches, so there's no sdkp or genhd associated during libata
configuration.  Longer term, this definitely is something which should
be stripped out of SCSI whether that happens with ATA HLD on top of
SCSI or a completely separate stack.  Any better ideas on how to work
around this for now?

Thanks.

-- 
tejun

ATA ULD (was Re: [PATCH 2/3] scsi: add scsi_device->alt_capacity)

[Jeff Garzik]

James Bottomley wrote:
> On Sun, 2009-05-10 at 01:09 +0900, Tejun Heo wrote:
>> James Bottomley wrote:
>>> This is done at slightly the wrong level.  Capacity is actually a
>>> property of struct scsi_disk not struct scsi_device ... shouldn't
>>> alt_capacity be at the same level?
>> Hmmm... I think that was my first try and then I moved it to sdev for
>> some reason I can't rememer now.  I'll look into it again and try to
>> move it into sdev.
> 
> Really one of the things I was wondering is why even scsi_disk ...
> capacity is in there, but it's also in gendisk, so I've thought
> (admittedly never translated it to action) that we could just remove the
> duplication in scsi_disk.
> 
> This alt_capacity looks to be a pure ATA thing ... I can't find it in
> the SCSI specs and there doesn't seem to be a SAT equivalent of the
> commands.  Ideally, what should be happening is that the ata ULD would
> issue the capacity commands and just set the block alt_capacity without
> having to worry about transporting the value up and down the stack.
> Matthew Wilcox thought we could begin an implementation of the ATA uld
> using the ATA_16 command to transport it through SCSI ... this might
> provide the good reason to begin that.

hmmmmm.  Well, ATA ULD is certainly the long term goal.  My main worry 
would then be user impact & confusion, and increasing the difficulty of 
separating libata from SCSI by increasing difficulty of making SCSI 
emulation an optional piece.

At this point the SCSI emulation is not only a hack to get sd.c to do 
work for us, it is also part of the ABI expected of diagnostic tools and 
the like.  That complicates changes.

It is my hope that we can separate libata-scsi.c into a separate kernel 
module, thus at least making it _optional_, thus providing a path for 
CONFIG_ATA + !CONFIG_SCSI.

I do also agree that the ATA hacks can only go on for so long, before 
they start infecting SCSI.   e.g. there was a patch to add DMI check in 
sd.c for an ATA hardware special case.

So far the "ATA infection" has been thankfully limited to things that sd 
might need anyway, like suspend/resume (start/stop)

A "libsd" does not seem viable, though.  Might be better to
	cp sd.c ata-disk.c
and then iterate from there, over time.

	Jeff

[PATCH 3/3] libata: export HPA size as alt_size

[Tejun Heo]

Export HPA size as alt_size so that userland tools which need the BIOS
size can determine it.

Signed-off-by: Tejun Heo <tj@kernel.org>
Cc: Jens Axboe <jens.axboe@oracle.com>
Cc: Dan Williams <dan.j.williams@gmail.com>
Cc: Jeff Garzik <jeff@garzik.org>
---
 drivers/ata/libata-core.c |    2 ++
 drivers/ata/libata-scsi.c |    2 ++
 include/linux/libata.h    |    1 +
 3 files changed, 5 insertions(+), 0 deletions(-)

diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c
index 17c5d48..4bec0e2 100644
--- a/drivers/ata/libata-core.c
+++ b/drivers/ata/libata-core.c
@@ -1536,6 +1536,8 @@ static int ata_hpa_resize(struct ata_device *dev)
 	}
 
 	/* let's unlock HPA */
+	dev->alt_n_sectors = sectors;
+
 	rc = ata_set_max_sectors(dev, native_sectors);
 	if (rc == -EACCES) {
 		/* if device aborted the command, skip HPA resizing */
diff --git a/drivers/ata/libata-scsi.c b/drivers/ata/libata-scsi.c
index 2733b0c..6e50cd2 100644
--- a/drivers/ata/libata-scsi.c
+++ b/drivers/ata/libata-scsi.c
@@ -1127,6 +1127,8 @@ static int ata_scsi_dev_config(struct scsi_device *sdev,
 		blk_queue_update_dma_alignment(sdev->request_queue,
 					       ATA_SECT_SIZE - 1);
 		sdev->manage_start_stop = 1;
+
+		sdev->alt_capacity = dev->alt_n_sectors;
 	}
 
 	if (dev->flags & ATA_DFLAG_AN)
diff --git a/include/linux/libata.h b/include/linux/libata.h
index 3d501db..f7a8327 100644
--- a/include/linux/libata.h
+++ b/include/linux/libata.h
@@ -588,6 +588,7 @@ struct ata_device {
 #endif
 	/* n_sector is CLEAR_BEGIN, read comment above CLEAR_BEGIN */
 	u64			n_sectors;	/* size of device, if ATA */
+	u64			alt_n_sectors;	/* size bios wants us to see */
 	unsigned int		class;		/* ATA_DEV_xxx */
 	unsigned long		unpark_deadline;
 
-- 
1.6.0.2

[PATCH 3/3] libata: export HPA size as alt_size

[Tejun Heo]

Export HPA size as alt_size so that userland tools which need the BIOS
size can determine it.

Signed-off-by: Tejun Heo <tj@kernel.org>
Cc: Jens Axboe <jens.axboe@oracle.com>
Cc: Dan Williams <dan.j.williams@gmail.com>
Cc: Jeff Garzik <jeff@garzik.org>
---
 drivers/ata/libata-core.c |    2 ++
 drivers/ata/libata-scsi.c |    2 ++
 include/linux/libata.h    |    1 +
 3 files changed, 5 insertions(+), 0 deletions(-)

diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c
index 17c5d48..4bec0e2 100644
--- a/drivers/ata/libata-core.c
+++ b/drivers/ata/libata-core.c
@@ -1536,6 +1536,8 @@ static int ata_hpa_resize(struct ata_device *dev)
 	}
 
 	/* let's unlock HPA */
+	dev->alt_n_sectors = sectors;
+
 	rc = ata_set_max_sectors(dev, native_sectors);
 	if (rc == -EACCES) {
 		/* if device aborted the command, skip HPA resizing */
diff --git a/drivers/ata/libata-scsi.c b/drivers/ata/libata-scsi.c
index 2733b0c..6e50cd2 100644
--- a/drivers/ata/libata-scsi.c
+++ b/drivers/ata/libata-scsi.c
@@ -1127,6 +1127,8 @@ static int ata_scsi_dev_config(struct scsi_device *sdev,
 		blk_queue_update_dma_alignment(sdev->request_queue,
 					       ATA_SECT_SIZE - 1);
 		sdev->manage_start_stop = 1;
+
+		sdev->alt_capacity = dev->alt_n_sectors;
 	}
 
 	if (dev->flags & ATA_DFLAG_AN)
diff --git a/include/linux/libata.h b/include/linux/libata.h
index 3d501db..f7a8327 100644
--- a/include/linux/libata.h
+++ b/include/linux/libata.h
@@ -588,6 +588,7 @@ struct ata_device {
 #endif
 	/* n_sector is CLEAR_BEGIN, read comment above CLEAR_BEGIN */
 	u64			n_sectors;	/* size of device, if ATA */
+	u64			alt_n_sectors;	/* size bios wants us to see */
 	unsigned int		class;		/* ATA_DEV_xxx */
 	unsigned long		unpark_deadline;
 
-- 
1.6.0.2

[PATCHSET] block,scsi,libata: implement alt_size

[Tejun Heo]

Hello,

This patchset implements alt_size, which is a size hint to the users
of a block device.  It's primarily to communicate the BIOS (HPA) size
on ATA devices to userland, so that dmraid can consider it when trying
to figure out BIOS raid layout.  This is critical as dmraid can be
tricked into assemblying the wrong raid when fed with the unlocked
size and if the disk content is right (or, rather, wrong) and good
number of distros are shipping with ignore_hpa=1 as default.

This patch contains the following three patches.

  0001-block-add-alt_size.patch
  0002-scsi-add-scsi_device-alt_capacity.patch
  0003-libata-export-HPA-size-as-alt_size.patch

Without the above three patches, I get the following on my nv RAID-0
if HPA unlocking is turned on.

  # ~/work/dmraid/tools/dmraid -a y
  RAID set "nvidia_djgdjigi" was activated
  # mount /dev/dm-0 /mnt/tmp
  mount: wrong fs type, bad option, bad superblock on /dev/dm-0,
	 missing codepage or helper program, or other error
	 In some cases useful info is found in syslog - try
	 dmesg | tail  or so

With the above three patches and modified dmraid,

  ck804:~/os/work/build # ~/work/dmraid/tools/dmraid -a y -v -v -v
  ...
  NOTICE: /dev/sdb: using BIOS sectors 234439535
  RAID set "nvidia_djgdjigi" was activated
  ...
  ck804:~/os/work/build # mount /dev/dm-0 /mnt/tmp
  ck804:~/os/work/build # umount /dev/dm-0

Any ideas on through which tree this should be pushed?  I'll post
dmraid patches as a reply to this thread for reference.

Thanks.

This patchset is against the current linus#master[1] and contains the
following changes.

 block/genhd.c              |   26 ++++++++++++++++++++++++++
 drivers/ata/libata-core.c  |    2 ++
 drivers/ata/libata-scsi.c  |    2 ++
 drivers/scsi/sd.c          |    1 +
 include/linux/genhd.h      |   13 ++++++++++++-
 include/linux/libata.h     |    1 +
 include/scsi/scsi_device.h |    1 +
 7 files changed, 45 insertions(+), 1 deletion(-)

--
tejun

[1] 45c82b5a770be66845687a7d027c8b52946d59af

[PATCH 2/3] scsi: add scsi_device->alt_capacity

[Tejun Heo]

Add scsi_device->alt_capacity and let sd pass it over to genhd.  This
is to allow SCSI low level drivers to configure alt_capacity via
slave_configure().

Signed-off-by: Tejun Heo <tj@kernel.org>
---
 drivers/scsi/sd.c          |    1 +
 include/scsi/scsi_device.h |    1 +
 2 files changed, 2 insertions(+), 0 deletions(-)

diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c
index d57566b..b83d850 100644
--- a/drivers/scsi/sd.c
+++ b/drivers/scsi/sd.c
@@ -1751,6 +1751,7 @@ static int sd_revalidate_disk(struct gendisk *disk)
 	blk_queue_ordered(sdkp->disk->queue, ordered, sd_prepare_flush);
 
 	set_capacity(disk, sdkp->capacity);
+	set_alt_capacity(disk, sdp->alt_capacity);
 	kfree(buffer);
 
  out:
diff --git a/include/scsi/scsi_device.h b/include/scsi/scsi_device.h
index 01a4c58..55be54a 100644
--- a/include/scsi/scsi_device.h
+++ b/include/scsi/scsi_device.h
@@ -92,6 +92,7 @@ struct scsi_device {
 	unsigned int manufacturer;	/* Manufacturer of device, for using 
 					 * vendor-specific cmd's */
 	unsigned sector_size;	/* size in bytes */
+	size_t alt_capacity;	/* alternative capacity, used by sd */
 
 	void *hostdata;		/* available to low-level driver */
 	char type;
-- 
1.6.0.2

[PATCH 2/3] scsi: add scsi_device->alt_capacity

[Tejun Heo]

Add scsi_device->alt_capacity and let sd pass it over to genhd.  This
is to allow SCSI low level drivers to configure alt_capacity via
slave_configure().

Signed-off-by: Tejun Heo <tj@kernel.org>
---
 drivers/scsi/sd.c          |    1 +
 include/scsi/scsi_device.h |    1 +
 2 files changed, 2 insertions(+), 0 deletions(-)

diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c
index d57566b..b83d850 100644
--- a/drivers/scsi/sd.c
+++ b/drivers/scsi/sd.c
@@ -1751,6 +1751,7 @@ static int sd_revalidate_disk(struct gendisk *disk)
 	blk_queue_ordered(sdkp->disk->queue, ordered, sd_prepare_flush);
 
 	set_capacity(disk, sdkp->capacity);
+	set_alt_capacity(disk, sdp->alt_capacity);
 	kfree(buffer);
 
  out:
diff --git a/include/scsi/scsi_device.h b/include/scsi/scsi_device.h
index 01a4c58..55be54a 100644
--- a/include/scsi/scsi_device.h
+++ b/include/scsi/scsi_device.h
@@ -92,6 +92,7 @@ struct scsi_device {
 	unsigned int manufacturer;	/* Manufacturer of device, for using 
 					 * vendor-specific cmd's */
 	unsigned sector_size;	/* size in bytes */
+	size_t alt_capacity;	/* alternative capacity, used by sd */
 
 	void *hostdata;		/* available to low-level driver */
 	char type;
-- 
1.6.0.2