Re: [PATCH 6/8] SCSI: implement sd_unlock_native_capacity()

[James Bottomley]

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@xxxxxxx>

James


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/