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

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

[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

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

[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

[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