[PATCHSET] libata: implement ->set_capacity()

[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

[PATCH 1/4] block: restart partition scan after resizing a device

[Tejun Heo]

Device resize via ->set_capacity() can reveal new partitions (e.g. in
chained partition table formats such as dos extended parts).  Restart
partition scan from the beginning after resizing a device.

Signed-off-by: Tejun Heo <tj@kernel.org>
Reported-by: Ben Hutchings <ben@decadent.org.uk>
---
 fs/partitions/check.c |    7 ++++---
 1 files changed, 4 insertions(+), 3 deletions(-)

diff --git a/fs/partitions/check.c b/fs/partitions/check.c
index e238ab2..f80a58d 100644
--- a/fs/partitions/check.c
+++ b/fs/partitions/check.c
@@ -550,7 +550,7 @@ int rescan_partitions(struct gendisk *disk, struct block_device *bdev)
 	res = invalidate_partition(disk, 0);
 	if (res)
 		return res;
-
+rescan:
 	disk_part_iter_init(&piter, disk, DISK_PITER_INCL_EMPTY);
 	while ((part = disk_part_iter_next(&piter)))
 		delete_partition(disk, part->partno);
@@ -581,7 +581,7 @@ int rescan_partitions(struct gendisk *disk, struct block_device *bdev)
 	/* add partitions */
 	for (p = 1; p < state->limit; p++) {
 		sector_t size, from;
-try_scan:
+
 		size = state->parts[p].size;
 		if (!size)
 			continue;
@@ -612,7 +612,8 @@ try_scan:
 					check_disk_size_change(disk, bdev);
 					bdev->bd_invalidated = 0;
 				}
-				goto try_scan;
+				kfree(state);
+				goto rescan;
 			} else {
 				/*
 				 * we can not ignore partitions of broken tables
-- 
1.6.4.2

[PATCH 2/4] SCSI: implement sd_set_capacity()

[Tejun Heo]

Implement sd_set_capacity() method which calls into
hostt->set_capacity() if implemented.  This will be invoked by block
layer if partitions extend beyond the end of the device and can be
used to implement, for example, on-demand ATA host protected area
unlocking.

Signed-off-by: Tejun Heo <tj@kernel.org>
Cc: Ben Hutchings <ben@decadent.org.uk>
---
 drivers/scsi/sd.c        |   26 ++++++++++++++++++++++++++
 include/scsi/scsi_host.h |   11 +++++++++++
 2 files changed, 37 insertions(+), 0 deletions(-)

diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c
index 8b827f3..59d5e8f 100644
--- a/drivers/scsi/sd.c
+++ b/drivers/scsi/sd.c
@@ -97,6 +97,8 @@ MODULE_ALIAS_SCSI_DEVICE(TYPE_RBC);
 #endif
 
 static int  sd_revalidate_disk(struct gendisk *);
+static unsigned long long sd_set_capacity(struct gendisk *disk,
+					  unsigned long long new_capacity);
 static int  sd_probe(struct device *);
 static int  sd_remove(struct device *);
 static void sd_shutdown(struct device *);
@@ -1100,6 +1102,7 @@ static const struct block_device_operations sd_fops = {
 #endif
 	.media_changed		= sd_media_changed,
 	.revalidate_disk	= sd_revalidate_disk,
+	.set_capacity		= sd_set_capacity,
 };
 
 static unsigned int sd_completed_bytes(struct scsi_cmnd *scmd)
@@ -2101,6 +2104,29 @@ static int sd_revalidate_disk(struct gendisk *disk)
 }
 
 /**
+ *	sd_set_capacity - set disk capacity
+ *	@disk: struct gendisk to set capacity for
+ *	@new_capacity: new target capacity
+ *
+ *	Block layer calls this function if it detects that partitions
+ *	on @disk reach beyond the end of the device.  If the SCSI host
+ *	implements set_capacity method, it's invoked to give it a
+ *	chance to adjust the device capacity.
+ *
+ *	CONTEXT:
+ *	Defined by block layer.  Might sleep.
+ */
+static unsigned long long sd_set_capacity(struct gendisk *disk,
+					  unsigned long long new_capacity)
+{
+	struct scsi_device *sdev = scsi_disk(disk)->device;
+
+	if (sdev->host->hostt->set_capacity)
+		return sdev->host->hostt->set_capacity(sdev, new_capacity);
+	return 0;
+}
+
+/**
  *	sd_format_disk_name - format disk name
  *	@prefix: name prefix - ie. "sd" for SCSI disks
  *	@index: index of the disk to format name for
diff --git a/include/scsi/scsi_host.h b/include/scsi/scsi_host.h
index c50a97f..31dba89 100644
--- a/include/scsi/scsi_host.h
+++ b/include/scsi/scsi_host.h
@@ -327,6 +327,17 @@ struct scsi_host_template {
 			sector_t, int []);
 
 	/*
+	 * This function is called when one or more partitions on the
+	 * device reach beyond the end of the device.  This function
+	 * should return the new capacity if disk was successfully
+	 * enlarged.  Return values smaller than the current capacity
+	 * are ignored.
+	 *
+	 * Status: OPTIONAL
+	 */
+	sector_t (*set_capacity)(struct scsi_device *, sector_t);
+
+	/*
 	 * Can be used to export driver statistics and other infos to the
 	 * world outside the kernel ie. userspace and it also provides an
 	 * interface to feed the driver with information.
-- 
1.6.4.2

[PATCH 3/4] libata: use the enlarged capacity after late HPA unlock

[Tejun Heo]

After late HPA unlock, libata kept using the original capacity
ignoring the new larger native capacity.  Enlarging device on the fly
doesn't cause any harm.  Use the larger native capacity instead.  This
will enable on-demand HPA unlocking.

Signed-off-by: Tejun Heo <tj@kernel.org>
Cc: Ben Hutchings <ben@decadent.org.uk>
---
 drivers/ata/libata-core.c |    5 ++---
 1 files changed, 2 insertions(+), 3 deletions(-)

diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c
index 86f405b..9d6e92d 100644
--- a/drivers/ata/libata-core.c
+++ b/drivers/ata/libata-core.c
@@ -4212,9 +4212,8 @@ int ata_dev_revalidate(struct ata_device *dev, unsigned int new_class,
 	    dev->n_sectors > n_sectors && dev->n_sectors == n_native_sectors) {
 		ata_dev_printk(dev, KERN_WARNING,
 			       "new n_sectors matches native, probably "
-			       "late HPA unlock, continuing\n");
-		/* keep using the old n_sectors */
-		dev->n_sectors = n_sectors;
+			       "late HPA unlock, n_sectors updated\n");
+		/* use the larger n_sectors */
 		return 0;
 	}
 
-- 
1.6.4.2

[PATCH 4/4] libata: implement on-demand HPA unlocking

[Tejun Heo]

Implement ata_scsi_set_capacity() which will be called through SCSI
layer when block layer notices that partitions on a device extend
beyond the end of the device.  ata_scsi_set_capacity() requests EH to
unlock HPA, waits for completion and returns the current device
capacity.

This allows libata to unlock HPA on demand instead of having to decide
whether to unlock upfront.  Unlocking on demand is safer than
unlocking by upfront because some BIOSes write private data to the
area beyond HPA limit.  This was suggested by Ben Hutchings.

Signed-off-by: Tejun Heo <tj@kernel.org>
Suggested-by: Ben Hutchings <ben@decadent.org.uk>
---
 drivers/ata/libata-core.c |    1 +
 drivers/ata/libata-scsi.c |   42 ++++++++++++++++++++++++++++++++++++++++++
 include/linux/libata.h    |    3 +++
 3 files changed, 46 insertions(+), 0 deletions(-)

diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c
index 9d6e92d..56badb4 100644
--- a/drivers/ata/libata-core.c
+++ b/drivers/ata/libata-core.c
@@ -6797,6 +6797,7 @@ EXPORT_SYMBOL_GPL(ata_dummy_port_info);
 EXPORT_SYMBOL_GPL(ata_link_next);
 EXPORT_SYMBOL_GPL(ata_dev_next);
 EXPORT_SYMBOL_GPL(ata_std_bios_param);
+EXPORT_SYMBOL_GPL(ata_scsi_set_capacity);
 EXPORT_SYMBOL_GPL(ata_host_init);
 EXPORT_SYMBOL_GPL(ata_host_alloc);
 EXPORT_SYMBOL_GPL(ata_host_alloc_pinfo);
diff --git a/drivers/ata/libata-scsi.c b/drivers/ata/libata-scsi.c
index 0088cde..2523943 100644
--- a/drivers/ata/libata-scsi.c
+++ b/drivers/ata/libata-scsi.c
@@ -415,6 +415,48 @@ int ata_std_bios_param(struct scsi_device *sdev, struct block_device *bdev,
 }
 
 /**
+ *	ata_scsi_set_capacity - adjust device capacity
+ *	@sdev: SCSI device to adjust device capacity for
+ *	@new_capacity: new target capacity
+ *
+ *	This function is called if a partition on @sdev extends beyond
+ *	the end of the device.  It requests EH to unlock HPA and
+ *	returns the possibly adjusted capacity.
+ *
+ *	LOCKING:
+ *	Defined by the SCSI layer.  Might sleep.
+ *
+ *	RETURNS:
+ *	New capacity if adjusted successfully.  0 if device is not
+ *	found.
+ */
+sector_t ata_scsi_set_capacity(struct scsi_device *sdev, sector_t new_capacity)
+{
+	struct ata_port *ap = ata_shost_to_port(sdev->host);
+	sector_t capacity = 0;
+	struct ata_device *dev;
+	unsigned long flags;
+
+	spin_lock_irqsave(ap->lock, flags);
+
+	dev = ata_scsi_find_dev(ap, sdev);
+	if (dev && dev->n_sectors < new_capacity &&
+	    dev->n_sectors < dev->n_native_sectors) {
+		struct ata_eh_info *ehi = &dev->link->eh_info;
+
+		dev->flags |= ATA_DFLAG_UNLOCK_HPA;
+		ehi->action |= ATA_EH_RESET;
+		ata_port_schedule_eh(ap);
+		spin_unlock_irqrestore(ap->lock, flags);
+		ata_port_wait_eh(ap);
+		capacity = dev->n_sectors;
+	} else
+		spin_unlock_irqrestore(ap->lock, flags);
+
+	return capacity;
+}
+
+/**
  *	ata_get_identity - Handler for HDIO_GET_IDENTITY ioctl
  *	@ap: target port
  *	@sdev: SCSI device to get identify data for
diff --git a/include/linux/libata.h b/include/linux/libata.h
index 242eb26..1082956 100644
--- a/include/linux/libata.h
+++ b/include/linux/libata.h
@@ -1027,6 +1027,8 @@ extern void ata_scsi_simulate(struct ata_device *dev, struct scsi_cmnd *cmd,
 extern int ata_std_bios_param(struct scsi_device *sdev,
 			      struct block_device *bdev,
 			      sector_t capacity, int geom[]);
+extern sector_t ata_scsi_set_capacity(struct scsi_device *sdev,
+				      sector_t new_capacity);
 extern int ata_scsi_slave_config(struct scsi_device *sdev);
 extern void ata_scsi_slave_destroy(struct scsi_device *sdev);
 extern int ata_scsi_change_queue_depth(struct scsi_device *sdev,
@@ -1181,6 +1183,7 @@ extern struct device_attribute *ata_common_sdev_attrs[];
 	.slave_configure	= ata_scsi_slave_config,	\
 	.slave_destroy		= ata_scsi_slave_destroy,	\
 	.bios_param		= ata_std_bios_param,		\
+	.set_capacity		= ata_scsi_set_capacity,	\
 	.sdev_attrs		= ata_common_sdev_attrs
 
 #define ATA_NCQ_SHT(drv_name)					\
-- 
1.6.4.2

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

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 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: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()

[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()

[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()

[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()

[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()

[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()

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