| Message ID | 1273946974-29131-7-git-send-email-tj@kernel.org |
|---|---|
| State | Not Applicable |
| Delegated to: | David Miller |
| Headers | show |
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
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
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.
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
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.
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
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.
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
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.
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(-)