Patchwork [6/8] SCSI: implement sd_unlock_native_capacity()

login
register
mail settings
Submitter Tejun Heo
Date May 15, 2010, 6:09 p.m.
Message ID <1273946974-29131-7-git-send-email-tj@kernel.org>
Download mbox | patch
Permalink /patch/52719/
State Not Applicable
Delegated to: David Miller
Headers show

Comments

Tejun Heo - May 15, 2010, 6:09 p.m.
Implement sd_unlock_native_capacity() method which calls into
hostt->unlock_native_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        |   22 ++++++++++++++++++++++
 include/scsi/scsi_host.h |    8 ++++++++
 2 files changed, 30 insertions(+), 0 deletions(-)
David Miller - May 16, 2010, 7:18 a.m.
From: Tejun Heo <tj@kernel.org>
Date: Sat, 15 May 2010 20:09:32 +0200

> Implement sd_unlock_native_capacity() method which calls into
> hostt->unlock_native_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>

Acked-by: David S. Miller <davem@davemloft.net>
--
To unsubscribe from this list: send the line "unsubscribe linux-ide" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
James Bottomley - May 16, 2010, 1:39 p.m.
On Sat, 2010-05-15 at 20:09 +0200, Tejun Heo wrote:
> Implement sd_unlock_native_capacity() method which calls into
> hostt->unlock_native_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        |   22 ++++++++++++++++++++++
>  include/scsi/scsi_host.h |    8 ++++++++
>  2 files changed, 30 insertions(+), 0 deletions(-)
> 
> diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c
> index 8b827f3..b85906e 100644
> --- a/drivers/scsi/sd.c
> +++ b/drivers/scsi/sd.c
> @@ -97,6 +97,7 @@ MODULE_ALIAS_SCSI_DEVICE(TYPE_RBC);
>  #endif
>  
>  static int  sd_revalidate_disk(struct gendisk *);
> +static void sd_unlock_native_capacity(struct gendisk *disk);
>  static int  sd_probe(struct device *);
>  static int  sd_remove(struct device *);
>  static void sd_shutdown(struct device *);
> @@ -1100,6 +1101,7 @@ static const struct block_device_operations sd_fops = {
>  #endif
>  	.media_changed		= sd_media_changed,
>  	.revalidate_disk	= sd_revalidate_disk,
> +	.unlock_native_capacity	= sd_unlock_native_capacity,
>  };
>  
>  static unsigned int sd_completed_bytes(struct scsi_cmnd *scmd)
> @@ -2101,6 +2103,26 @@ static int sd_revalidate_disk(struct gendisk *disk)
>  }
>  
>  /**
> + *	sd_unlock_native_capacity - unlock native capacity
> + *	@disk: struct gendisk to set capacity for
> + *
> + *	Block layer calls this function if it detects that partitions
> + *	on @disk reach beyond the end of the device.  If the SCSI host
> + *	implements ->unlock_native_capacity() method, it's invoked to
> + *	give it a chance to adjust the device capacity.
> + *
> + *	CONTEXT:
> + *	Defined by block layer.  Might sleep.
> + */
> +static void sd_unlock_native_capacity(struct gendisk *disk)
> +{
> +	struct scsi_device *sdev = scsi_disk(disk)->device;
> +
> +	if (sdev->host->hostt->unlock_native_capacity)
> +		sdev->host->hostt->unlock_native_capacity(sdev);
> +}
> +
> +/**
>   *	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..b7bdecb 100644
> --- a/include/scsi/scsi_host.h
> +++ b/include/scsi/scsi_host.h
> @@ -327,6 +327,14 @@ 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.
> +	 *
> +	 * Status: OPTIONAL
> +	 */
> +	void (*unlock_native_capacity)(struct scsi_device *);
> +
> +	/*

I still principally dislike this threading of the operation up and down
the stack.  Although we've done it before, we're trying to get away from
the paradigm of driver conditions to scsi and scsi conditions to block
for all stuff where driver could condition straight to block (alignment
is a classic example of the new way of doing things).

The reason you need the threading is because the block ops gets hidden
at the top of the stack in the upper layers (this was a design choice to
try to prevent the lower layers mucking with it).  The way we get around
this is to use a queue operation instead.  As far as I can tell, there's
no logical reason for the unlock to be a block op, so lets make it a
queue op instead.  The consumer in 

static bool disk_unlock_native_capacity(struct gendisk *disk)

then becomes

	blk_unlock_native_capacity(disk->queue)

Block's implementation of this would probably call directly through the
queue.

You place the usual function in the queue template so the ata slave
configure can say

	blk_queue_unlock_native_function(rq, <unlock_function>)

Then there's no need for anything at all in SCSI ... and it becomes a
whole lot more obvious how to do legacy ide (if we ever get problems
there).

James


--
To unsubscribe from this list: send the line "unsubscribe linux-ide" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Ben Hutchings - May 16, 2010, 3:07 p.m.
On Sun, 2010-05-16 at 08:39 -0500, James Bottomley wrote:
[...]
> Then there's no need for anything at all in SCSI ... and it becomes a
> whole lot more obvious how to do legacy ide (if we ever get problems
> there).

The block device set_capacity() interface is already defined and was
implemented for the IDE disk driver some time ago.  The purpose of
Tejun's work on sd and libata is to implement this existing interface
and so avoid a regression when users switch from IDE to libata-based
drivers.  I don't see why he should have to replace the interface as
well.

Ben.
James Bottomley - May 16, 2010, 4 p.m.
On Sun, 2010-05-16 at 16:07 +0100, Ben Hutchings wrote:
> On Sun, 2010-05-16 at 08:39 -0500, James Bottomley wrote:
> [...]
> > Then there's no need for anything at all in SCSI ... and it becomes a
> > whole lot more obvious how to do legacy ide (if we ever get problems
> > there).
> 
> The block device set_capacity() interface is already defined and was
> implemented for the IDE disk driver some time ago.  The purpose of
> Tejun's work on sd and libata is to implement this existing interface
> and so avoid a regression when users switch from IDE to libata-based
> drivers.  I don't see why he should have to replace the interface as
> well.

I thought we already reached the conclusion that set_capacity wasn't the
right interface, hence the redo as a binary lock/unlock of native
capacity ... which is revoking the set_capacity interface in this patch.

The point I was making is that bdops just works for ide because it's
completely monolithic.  It gives us a layering problem in SCSI because
you have to hook it into the lower layers which don't have access to
bdops (since they're an upper layer thing in SCSI).

This layering problem is partly the fault of libata ... if we had an ATA
native disk driver, it would be able to unlock the capacity on its own.
It's just we're using SCSI which has no SAT command it can issue for
this, so the functional request has to be pushed down to libata ...
leading to the need to thread it through the host template.

I was just pointing out that the whole thing is simplified if we use a
block queue function approach instead.  ide_disk_t has access to the
queue, as does gendisk, so it would all "just work" with a simple call
site change if we used queue ops instead of block dev ops.  The plus
side of doing it this way is that the SCSI threading becomes
unnecessary: libata gets directly hooked into the unlock function
instead of having to do it via an intermediary.

James


--
To unsubscribe from this list: send the line "unsubscribe linux-ide" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Tejun Heo - May 16, 2010, 5 p.m.
Hello, James.

On 05/16/2010 06:00 PM, James Bottomley wrote:
> This layering problem is partly the fault of libata ... if we had an ATA
> native disk driver, it would be able to unlock the capacity on its own.

Yeap.

> It's just we're using SCSI which has no SAT command it can issue for
> this, so the functional request has to be pushed down to libata ...
> leading to the need to thread it through the host template.
> 
> I was just pointing out that the whole thing is simplified if we use a
> block queue function approach instead.  ide_disk_t has access to the
> queue, as does gendisk, so it would all "just work" with a simple call
> site change if we used queue ops instead of block dev ops.  The plus
> side of doing it this way is that the SCSI threading becomes
> unnecessary: libata gets directly hooked into the unlock function
> instead of having to do it via an intermediary.

Yeah, it can be made to work via a queue callback but I'm afraid that
would be a genuine layering violation (although going through SCSI is
extra layering, it isn't really a layering violation).

These are request_queue methods.

	request_fn_proc		*request_fn;
	make_request_fn		*make_request_fn;
	prep_rq_fn		*prep_rq_fn;
	unplug_fn		*unplug_fn;
	merge_bvec_fn		*merge_bvec_fn;
	prepare_flush_fn	*prepare_flush_fn;
	softirq_done_fn		*softirq_done_fn;
	rq_timed_out_fn		*rq_timed_out_fn;
	dma_drain_needed_fn	*dma_drain_needed;
	lld_busy_fn		*lld_busy_fn;

These are gendisk methods.

	int (*open) (struct block_device *, fmode_t);
	int (*release) (struct gendisk *, fmode_t);
	int (*locked_ioctl) (struct block_device *, fmode_t, unsigned, unsigned long);
	int (*ioctl) (struct block_device *, fmode_t, unsigned, unsigned long);
	int (*compat_ioctl) (struct block_device *, fmode_t, unsigned, unsigned long);
	int (*direct_access) (struct block_device *, sector_t,
						void **, unsigned long *);
	int (*media_changed) (struct gendisk *);
	void (*unlock_native_capacity) (struct gendisk *);
	int (*revalidate_disk) (struct gendisk *);
	int (*getgeo)(struct block_device *, struct hd_geometry *);

request_queue is (or at least supposed to be) oblivious about genhd
and its attributes including capacity.  After all, request_queue can
exist w/o genhd associated, so it would be quite odd to have capacity
related method living in request_queue.

Another thing is that there is no generic way to reach the associated
genhd from request_queue and I can't think of a clean way to map
request_queue to the associated ata device w/o in-flight requests (can
you even do that from SCSI?).

Unfortunately, libata is still properly layered below SCSI, so I'm
afraid threading through sd is clumsy yet the cleanest way to do it.

Thanks.
James Bottomley - May 16, 2010, 7:23 p.m.
On Sun, 2010-05-16 at 19:00 +0200, Tejun Heo wrote:
> Hello, James.
> 
> On 05/16/2010 06:00 PM, James Bottomley wrote:
> > This layering problem is partly the fault of libata ... if we had an ATA
> > native disk driver, it would be able to unlock the capacity on its own.
> 
> Yeap.
> 
> > It's just we're using SCSI which has no SAT command it can issue for
> > this, so the functional request has to be pushed down to libata ...
> > leading to the need to thread it through the host template.
> > 
> > I was just pointing out that the whole thing is simplified if we use a
> > block queue function approach instead.  ide_disk_t has access to the
> > queue, as does gendisk, so it would all "just work" with a simple call
> > site change if we used queue ops instead of block dev ops.  The plus
> > side of doing it this way is that the SCSI threading becomes
> > unnecessary: libata gets directly hooked into the unlock function
> > instead of having to do it via an intermediary.
> 
> Yeah, it can be made to work via a queue callback but I'm afraid that
> would be a genuine layering violation (although going through SCSI is
> extra layering, it isn't really a layering violation).
> 
> These are request_queue methods.
> 
> 	request_fn_proc		*request_fn;
> 	make_request_fn		*make_request_fn;
> 	prep_rq_fn		*prep_rq_fn;
> 	unplug_fn		*unplug_fn;
> 	merge_bvec_fn		*merge_bvec_fn;
> 	prepare_flush_fn	*prepare_flush_fn;
> 	softirq_done_fn		*softirq_done_fn;
> 	rq_timed_out_fn		*rq_timed_out_fn;
> 	dma_drain_needed_fn	*dma_drain_needed;
> 	lld_busy_fn		*lld_busy_fn;
> 
> These are gendisk methods.
> 
> 	int (*open) (struct block_device *, fmode_t);
> 	int (*release) (struct gendisk *, fmode_t);
> 	int (*locked_ioctl) (struct block_device *, fmode_t, unsigned, unsigned long);
> 	int (*ioctl) (struct block_device *, fmode_t, unsigned, unsigned long);
> 	int (*compat_ioctl) (struct block_device *, fmode_t, unsigned, unsigned long);
> 	int (*direct_access) (struct block_device *, sector_t,
> 						void **, unsigned long *);
> 	int (*media_changed) (struct gendisk *);
> 	void (*unlock_native_capacity) (struct gendisk *);
> 	int (*revalidate_disk) (struct gendisk *);
> 	int (*getgeo)(struct block_device *, struct hd_geometry *);
> 
> request_queue is (or at least supposed to be) oblivious about genhd
> and its attributes including capacity.  After all, request_queue can
> exist w/o genhd associated, so it would be quite odd to have capacity
> related method living in request_queue.

Yes, I'll sort of buy this ... although it's not quite that clean:
barrier methods, which are only used for filesystem above block devices
also live in the queue.

> Another thing is that there is no generic way to reach the associated
> genhd from request_queue and I can't think of a clean way to map
> request_queue to the associated ata device w/o in-flight requests (can
> you even do that from SCSI?).

No ... that's by design ... but you don't need it if all you're doing is
unlocking the native capacity (whether on behalf of block dev ops or
queue ops).

> Unfortunately, libata is still properly layered below SCSI, so I'm
> afraid threading through sd is clumsy yet the cleanest way to do it.

s/properly/im\&/

but yes,

Reluctantly-Acked-by: James Bottomley <James.Bottomley@suse.de>

James


--
To unsubscribe from this list: send the line "unsubscribe linux-ide" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Tejun Heo - May 17, 2010, 5:30 a.m.
Hello,

On 05/16/2010 09:23 PM, James Bottomley wrote:
>> request_queue is (or at least supposed to be) oblivious about genhd
>> and its attributes including capacity.  After all, request_queue can
>> exist w/o genhd associated, so it would be quite odd to have capacity
>> related method living in request_queue.
> 
> Yes, I'll sort of buy this ... although it's not quite that clean:
> barrier methods, which are only used for filesystem above block devices
> also live in the queue.

You mean prepare_flush_fn()?  Hmmm...

>> Another thing is that there is no generic way to reach the associated
>> genhd from request_queue and I can't think of a clean way to map
>> request_queue to the associated ata device w/o in-flight requests (can
>> you even do that from SCSI?).
> 
> No ... that's by design ... but you don't need it if all you're doing is
> unlocking the native capacity (whether on behalf of block dev ops or
> queue ops).

libata defers all those managements stuff to EH and the ata device
needs to be accessible to invoke EH.  It can be worked around by
issuing a pseudo command which is trapped and deferred to EH during
command processing but it's much better to be able to access the
device directly.

>> Unfortunately, libata is still properly layered below SCSI, so I'm
>> afraid threading through sd is clumsy yet the cleanest way to do it.
> 
> s/properly/im\&/

Heh, yeah.  :-)

> but yes,
> 
> Reluctantly-Acked-by: James Bottomley <James.Bottomley@suse.de>

Thanks.  Much appreciated.
Jeff Garzik - June 2, 2010, 5:50 p.m.
On 05/15/2010 02:09 PM, Tejun Heo wrote:
> Implement sd_unlock_native_capacity() method which calls into
> hostt->unlock_native_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        |   22 ++++++++++++++++++++++
>   include/scsi/scsi_host.h |    8 ++++++++
>   2 files changed, 30 insertions(+), 0 deletions(-)
>
> diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c
> index 8b827f3..b85906e 100644
> --- a/drivers/scsi/sd.c
> +++ b/drivers/scsi/sd.c
> @@ -97,6 +97,7 @@ MODULE_ALIAS_SCSI_DEVICE(TYPE_RBC);
>   #endif
>
>   static int  sd_revalidate_disk(struct gendisk *);
> +static void sd_unlock_native_capacity(struct gendisk *disk);
>   static int  sd_probe(struct device *);
>   static int  sd_remove(struct device *);
>   static void sd_shutdown(struct device *);
> @@ -1100,6 +1101,7 @@ static const struct block_device_operations sd_fops = {
>   #endif
>   	.media_changed		= sd_media_changed,
>   	.revalidate_disk	= sd_revalidate_disk,
> +	.unlock_native_capacity	= sd_unlock_native_capacity,
>   };
>
>   static unsigned int sd_completed_bytes(struct scsi_cmnd *scmd)
> @@ -2101,6 +2103,26 @@ static int sd_revalidate_disk(struct gendisk *disk)
>   }
>
>   /**
> + *	sd_unlock_native_capacity - unlock native capacity
> + *	@disk: struct gendisk to set capacity for
> + *
> + *	Block layer calls this function if it detects that partitions
> + *	on @disk reach beyond the end of the device.  If the SCSI host
> + *	implements ->unlock_native_capacity() method, it's invoked to
> + *	give it a chance to adjust the device capacity.
> + *
> + *	CONTEXT:
> + *	Defined by block layer.  Might sleep.
> + */
> +static void sd_unlock_native_capacity(struct gendisk *disk)
> +{
> +	struct scsi_device *sdev = scsi_disk(disk)->device;
> +
> +	if (sdev->host->hostt->unlock_native_capacity)
> +		sdev->host->hostt->unlock_native_capacity(sdev);
> +}
> +
> +/**
>    *	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..b7bdecb 100644
> --- a/include/scsi/scsi_host.h
> +++ b/include/scsi/scsi_host.h
> @@ -327,6 +327,14 @@ 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.
> +	 *
> +	 * Status: OPTIONAL
> +	 */
> +	void (*unlock_native_capacity)(struct scsi_device *);
> +
> +	/*

applied 6-8, including this SCSI patch


--
To unsubscribe from this list: send the line "unsubscribe linux-ide" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Patch

diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c
index 8b827f3..b85906e 100644
--- a/drivers/scsi/sd.c
+++ b/drivers/scsi/sd.c
@@ -97,6 +97,7 @@  MODULE_ALIAS_SCSI_DEVICE(TYPE_RBC);
 #endif
 
 static int  sd_revalidate_disk(struct gendisk *);
+static void sd_unlock_native_capacity(struct gendisk *disk);
 static int  sd_probe(struct device *);
 static int  sd_remove(struct device *);
 static void sd_shutdown(struct device *);
@@ -1100,6 +1101,7 @@  static const struct block_device_operations sd_fops = {
 #endif
 	.media_changed		= sd_media_changed,
 	.revalidate_disk	= sd_revalidate_disk,
+	.unlock_native_capacity	= sd_unlock_native_capacity,
 };
 
 static unsigned int sd_completed_bytes(struct scsi_cmnd *scmd)
@@ -2101,6 +2103,26 @@  static int sd_revalidate_disk(struct gendisk *disk)
 }
 
 /**
+ *	sd_unlock_native_capacity - unlock native capacity
+ *	@disk: struct gendisk to set capacity for
+ *
+ *	Block layer calls this function if it detects that partitions
+ *	on @disk reach beyond the end of the device.  If the SCSI host
+ *	implements ->unlock_native_capacity() method, it's invoked to
+ *	give it a chance to adjust the device capacity.
+ *
+ *	CONTEXT:
+ *	Defined by block layer.  Might sleep.
+ */
+static void sd_unlock_native_capacity(struct gendisk *disk)
+{
+	struct scsi_device *sdev = scsi_disk(disk)->device;
+
+	if (sdev->host->hostt->unlock_native_capacity)
+		sdev->host->hostt->unlock_native_capacity(sdev);
+}
+
+/**
  *	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..b7bdecb 100644
--- a/include/scsi/scsi_host.h
+++ b/include/scsi/scsi_host.h
@@ -327,6 +327,14 @@  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.
+	 *
+	 * Status: OPTIONAL
+	 */
+	void (*unlock_native_capacity)(struct scsi_device *);
+
+	/*
 	 * 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.