Re: [PATCHSET] libata: implement ->set_capacity()

Re: [PATCHSET] libata: implement ->set_capacity()

[Zoltan Boszormenyi]

Hi,

> On Thu, May 13 2010, Tejun Heo wrote:
> > Hello,
> > 
> > On 05/13/2010 06:06 PM, James Bottomley wrote:
> > > I'm not sure this is such a good interface ... it sounds very error
> > > prone for what is effectively a binary lock/unlock.
> > 
> > Well, the original block interface was like that.  It has been used as
> > binary switch tho.  The requested capacity is always ~0ULL and return
> > value smaller than the current capacity is ignored.  I'm all for
> > dropping the capacity parameter and the return value from
> > ->set_capacity() so that it just unlocks native capacity and directly
> > sets the new capacity.  Jens?
>
> Is there a valid case for setting the capacity less than the unlocked
> capacity? I would think the unlock/lock bool api is saner.
>
> -- 
> Jens Axboe
>   

I heard about a so called "short stroking", that limits a larger capacity
harddisk to say one third or one fourth of the capacity but the gains are
increased average throughput and decreased seek time. Is it possible
under Linux ATM, or do we need the patch series to correctly do it?

I know I can partition the harddisk to only use the beginning of it
to have the same gains, but I still have a question: as I read on e.g.
tomshardware.com, this is done by instructing the harddisk, not via
partitioning. Does this "short stroking" change the amount of spare
sectors available internally to SATA disks? I suppose it would and
so the harddisk would only lose some speed not actual data when
the disk starts aging. I would have more time to discover the
problem and replace the disk, no? I am only asking...

Best regards,
Zoltán Böszörményi

Re: [PATCHSET] libata: implement ->set_capacity()

[Ben Hutchings]

[-- Attachment #1: Type: text/plain, Size: 1239 bytes --]

On Thu, 2010-05-13 at 17:56 +0200, Tejun Heo wrote:
> Hello, Jens, James, Jeff,
> 
> This patchset implements ->set_capacity() in libata so that HPA can be
> unlocked on demand.

Thanks Tejun.

>  0001-block-restart-partition-scan-after-resizing-a-device.patch
>  0002-SCSI-implement-sd_set_capacity.patch
>  0003-libata-use-the-enlarged-capacity-after-late-HPA-unlo.patch
>  0004-libata-implement-on-demand-HPA-unlocking.patch
> 
> 0001 makes partition scan code to restart after ->set_capacity().
> This makes sure that partitions which start beyond the HPA limit are
> discovered.
> 
> 0002 implements ->set_capacity() in sd.
> 
> 0003 makes libata accept device capacity larger than the initial one.
> 
> 0004 implements ->set_capacity() in libata which asks libata EH to
> unlock HPA, waits and returns the new capacity.
> 
> Ben Hutchings suggeseted implementing ->set_capacity() in libata and
> also reported the bug in the current partition scan code where it
> fails to discover partitions which start beyond the HPA limit.
[...]

And I've already successfully tested the combination of patches 2-4.

Ben.

-- 
Ben Hutchings
Once a job is fouled up, anything done to improve it makes it worse.

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 828 bytes --]

Re: [PATCHSET] libata: implement ->set_capacity()

[Tejun Heo]

Hello, James.

On 05/13/2010 07:18 PM, James Bottomley wrote:
> So I think the bit I missed was you're trying to do this programatically
> from boot time partition read to detect if the user previously disabled
> the HPA and partitioned the device?  In which case you still have user
> context, you can call BLKRRPART here too.

Yeah, that's what I'm trying to do but the functionality is already in
the block layer and has been used by IDE layer for a long time.  We
just forgot to implement it for libata.  The only thing I did was
fixing up corner case handling in block layer a bit and adding a
callback to sd so that the block layer call can be passed down to
libata.  So, it doesn't have anything to do with sysfs attributes,
block or libata.

We sure can trigger the rescan by invoking BLKRRPART from inside the
partition scan code but I'm afraid that would be more convoluted than
the currently existing implementation of restarting the scan inside
the scan function.

The only thing is that the existing block method is somewhat overly
generic without any good reason, so simplifying that would be a good
idea.

Thanks.

-- 
tejun

Re: [PATCHSET] libata: implement ->set_capacity()

[Tejun Heo]

Hello,

On 05/13/2010 07:40 PM, Jens Axboe wrote:
> Is there a valid case for setting the capacity less than the unlocked
> capacity? I would think the unlock/lock bool api is saner.

IDE currently is the only user (and probably has been that way the
whole time), so it is a binary thing.  I have no idea why the original
interface was designed that way.  Looks like it tried to be too
generic.  Anyways, for the task at hand, the following should be
enough.

	void (*unlock_native_capacity)(void);

This simple signalling is how the current interface is being used
anyway.  If nobody objects, I'll replace ->set_capacity() with the
above.

Thanks.

-- 
tejun

Re: [PATCHSET] libata: implement ->set_capacity()

[Jens Axboe]

On Thu, May 13 2010, Tejun Heo wrote:
> Hello,
> 
> On 05/13/2010 06:06 PM, James Bottomley wrote:
> > I'm not sure this is such a good interface ... it sounds very error
> > prone for what is effectively a binary lock/unlock.
> 
> Well, the original block interface was like that.  It has been used as
> binary switch tho.  The requested capacity is always ~0ULL and return
> value smaller than the current capacity is ignored.  I'm all for
> dropping the capacity parameter and the return value from
> ->set_capacity() so that it just unlocks native capacity and directly
> sets the new capacity.  Jens?

Is there a valid case for setting the capacity less than the unlocked
capacity? I would think the unlock/lock bool api is saner.

-- 
Jens Axboe

Re: [PATCHSET] libata: implement ->set_capacity()

[James Bottomley]

On Thu, 2010-05-13 at 18:54 +0200, Tejun Heo wrote:
> Hello,
> 
> On 05/13/2010 06:38 PM, James Bottomley wrote:
> > Instead of making this a block sysfs attribute, since HPA is SATA only,
> > why not make it a libata attribute for the disk?
> > 
> > That way on unlock, you can unlock the HPA and then trigger a partition
> > rescan of the block device (BLKRRPART) ... this is an ioctl, so you need
> > user context, but you have it if you do it from the sysfs write routine.
> > This looks to be a lot simpler than threading it up through SCSI and
> > block.
> 
> This doesn't have anything to do with sysfs.  It's called from block
> partition scan code when it detects a partition extends beyond the end
> of the device.  No user intervention at all and the mechanism has been
> there for quite some years and possibly predates sysfs.  Am I being
> really slow or are you looking at something else?

OK, so maybe I'm misunderstanding what you're trying to do.  I thought
this set_capacity thing was trying to unlock the HPA and then cause
block to see a new capacity ... the latter is what calling BLKRRPART
from the sysfs write would do.

If it's just to unlock the HPA, then forget the RRPART piece.

So I think the bit I missed was you're trying to do this programatically
from boot time partition read to detect if the user previously disabled
the HPA and partitioned the device?  In which case you still have user
context, you can call BLKRRPART here too.

James

Re: [PATCHSET] libata: implement ->set_capacity()

[Alan Cox]

On Thu, 13 May 2010 11:38:07 -0500
James Bottomley <James.Bottomley@suse.de> wrote:

> On Thu, 2010-05-13 at 18:22 +0200, Tejun Heo wrote:
> > > Instead of just saying unlock the HPA and show me the new capacity
> > > (with a rescan), you have to echo the right number of sectors to the
> > > set_capacity variable.  Isn't a hpa_unlock libata specific attribute
> > > better (you could even call BLKRRPART from the user context of the
> > > write)?
> > 
> > Hmmm... I lost you.  What are you talking about?
> 
> Instead of making this a block sysfs attribute, since HPA is SATA only,
> why not make it a libata attribute for the disk?

HPA is both PATA and SATA (and USB SCSI pass through ATA over SCSI etc
with some of the more functional USB/ATA bridges.

Re: [PATCHSET] libata: implement ->set_capacity()

[Tejun Heo]

Hello,

On 05/13/2010 06:38 PM, James Bottomley wrote:
> Instead of making this a block sysfs attribute, since HPA is SATA only,
> why not make it a libata attribute for the disk?
> 
> That way on unlock, you can unlock the HPA and then trigger a partition
> rescan of the block device (BLKRRPART) ... this is an ioctl, so you need
> user context, but you have it if you do it from the sysfs write routine.
> This looks to be a lot simpler than threading it up through SCSI and
> block.

This doesn't have anything to do with sysfs.  It's called from block
partition scan code when it detects a partition extends beyond the end
of the device.  No user intervention at all and the mechanism has been
there for quite some years and possibly predates sysfs.  Am I being
really slow or are you looking at something else?

Thanks.

-- 
tejun

Re: [PATCHSET] libata: implement ->set_capacity()

[James Bottomley]

On Thu, 2010-05-13 at 18:22 +0200, Tejun Heo wrote:
> > Instead of just saying unlock the HPA and show me the new capacity
> > (with a rescan), you have to echo the right number of sectors to the
> > set_capacity variable.  Isn't a hpa_unlock libata specific attribute
> > better (you could even call BLKRRPART from the user context of the
> > write)?
> 
> Hmmm... I lost you.  What are you talking about?

Instead of making this a block sysfs attribute, since HPA is SATA only,
why not make it a libata attribute for the disk?

That way on unlock, you can unlock the HPA and then trigger a partition
rescan of the block device (BLKRRPART) ... this is an ioctl, so you need
user context, but you have it if you do it from the sysfs write routine.
This looks to be a lot simpler than threading it up through SCSI and
block.

James

Re: [PATCHSET] libata: implement ->set_capacity()

[Tejun Heo]

Hello,

On 05/13/2010 06:06 PM, James Bottomley wrote:
> I'm not sure this is such a good interface ... it sounds very error
> prone for what is effectively a binary lock/unlock.

Well, the original block interface was like that.  It has been used as
binary switch tho.  The requested capacity is always ~0ULL and return
value smaller than the current capacity is ignored.  I'm all for
dropping the capacity parameter and the return value from
->set_capacity() so that it just unlocks native capacity and directly
sets the new capacity.  Jens?

> Instead of just saying unlock the HPA and show me the new capacity
> (with a rescan), you have to echo the right number of sectors to the
> set_capacity variable.  Isn't a hpa_unlock libata specific attribute
> better (you could even call BLKRRPART from the user context of the
> write)?

Hmmm... I lost you.  What are you talking about?

Thanks.

-- 
tejun

Re: [PATCHSET] libata: implement ->set_capacity()

[James Bottomley]

On Thu, 2010-05-13 at 17:56 +0200, Tejun Heo wrote:
> Hello, Jens, James, Jeff,
> 
> This patchset implements ->set_capacity() in libata so that HPA can be
> unlocked on demand.
> 
>  0001-block-restart-partition-scan-after-resizing-a-device.patch
>  0002-SCSI-implement-sd_set_capacity.patch
>  0003-libata-use-the-enlarged-capacity-after-late-HPA-unlo.patch
>  0004-libata-implement-on-demand-HPA-unlocking.patch
> 
> 0001 makes partition scan code to restart after ->set_capacity().
> This makes sure that partitions which start beyond the HPA limit are
> discovered.
> 
> 0002 implements ->set_capacity() in sd.
> 
> 0003 makes libata accept device capacity larger than the initial one.
> 
> 0004 implements ->set_capacity() in libata which asks libata EH to
> unlock HPA, waits and returns the new capacity.
> 
> Ben Hutchings suggeseted implementing ->set_capacity() in libata and
> also reported the bug in the current partition scan code where it
> fails to discover partitions which start beyond the HPA limit.
> 
> Unlocking HPA on-demand seems to be the safest default way to deal
> with HPA.  Leaving HPA alone by default could fail to detect or
> truncate existing partitions while unlocking by default make it more
> prone to obscure data corruptions when combined with BIOSes beliving
> that they exclusively own the area beyond HPA limit.
> 
> 0001 should be routed through the block tree.  0002 should go through
> SCSI but given the dependency and that libata is the only user, it
> would probably much easier to route it through libata-dev#upstream
> together with 0003 and 0004.

I'm not sure this is such a good interface ... it sounds very error
prone for what is effectively a binary lock/unlock.  Instead of just
saying unlock the HPA and show me the new capacity (with a rescan), you
have to echo the right number of sectors to the set_capacity variable.
Isn't a hpa_unlock libata specific attribute better (you could even call
BLKRRPART from the user context of the write)?

James

[PATCHSET] libata: implement ->set_capacity()

[Tejun Heo]

Hello, Jens, James, Jeff,

This patchset implements ->set_capacity() in libata so that HPA can be
unlocked on demand.

 0001-block-restart-partition-scan-after-resizing-a-device.patch
 0002-SCSI-implement-sd_set_capacity.patch
 0003-libata-use-the-enlarged-capacity-after-late-HPA-unlo.patch
 0004-libata-implement-on-demand-HPA-unlocking.patch

0001 makes partition scan code to restart after ->set_capacity().
This makes sure that partitions which start beyond the HPA limit are
discovered.

0002 implements ->set_capacity() in sd.

0003 makes libata accept device capacity larger than the initial one.

0004 implements ->set_capacity() in libata which asks libata EH to
unlock HPA, waits and returns the new capacity.

Ben Hutchings suggeseted implementing ->set_capacity() in libata and
also reported the bug in the current partition scan code where it
fails to discover partitions which start beyond the HPA limit.

Unlocking HPA on-demand seems to be the safest default way to deal
with HPA.  Leaving HPA alone by default could fail to detect or
truncate existing partitions while unlocking by default make it more
prone to obscure data corruptions when combined with BIOSes beliving
that they exclusively own the area beyond HPA limit.

0001 should be routed through the block tree.  0002 should go through
SCSI but given the dependency and that libata is the only user, it
would probably much easier to route it through libata-dev#upstream
together with 0003 and 0004.

The patches are based on top of libata-dev#upstream but apply fine on
top of mainline too.

This patchset is available in the following git branch.

  git://git.kernel.org/pub/scm/linux/kernel/git/tj/libata-dev.git set-capa

and contains the following changes.

 drivers/ata/libata-core.c |    6 +++---
 drivers/ata/libata-scsi.c |   42 ++++++++++++++++++++++++++++++++++++++++++
 drivers/scsi/sd.c         |   26 ++++++++++++++++++++++++++
 fs/partitions/check.c     |    7 ++++---
 include/linux/libata.h    |    3 +++
 include/scsi/scsi_host.h  |   11 +++++++++++
 6 files changed, 89 insertions(+), 6 deletions(-)

Thanks.

--
tejun