Re: [PATCH V3 06/12] scsi: sd_zbc: Rearrange code
On Sat, 2017-09-16 at 07:35 +0900, Damien Le Moal wrote: > rw16 is mandatory for ZBC drives. So it has to be set to true. If the > HBA does not support rw16 (why would that happen ?), then the disk > should not be used. It's good that all HBAs support rw16. But it's nontrivial to analyze whether or not use_1[06]_for_rw are set before these are used so maybe we should think about how we can restructure the SCSI code such that it becomes easier to verify that use_1[06]_for_rw are set before these are used. Bart.
Re: [PATCH V3 06/12] scsi: sd_zbc: Rearrange code
On 9/16/17 06:02, Bart Van Assche wrote: > On Fri, 2017-09-15 at 19:51 +0200, h...@lst.de wrote: >> On Fri, Sep 15, 2017 at 02:51:03PM +, Bart Van Assche wrote: >>> On Fri, 2017-09-15 at 19:06 +0900, Damien Le Moal wrote: Rearrange sd_zbc_setup() to include use_16_for_rw and use_10_for_rw assignments and move the calculation of sdkp->zone_shift together with the assignment of the verified zone_blocks value in sd_zbc_check_zone_size(). >>> >>> Both functions are called from inside >>> block_device_operations.revalidate_disk. >>> Isn't that too late to set use_1[06]_for_rw? Shouldn't both be set just >>> after >>> the slave_alloc callback has been called? >> >> Hmm. sd_read_capacity already is called from the same path and seems >> to work.. > > Hello Christoph, > > My concern is that if both a SCSI LLD and the ZBC code set use_1[06]_for_rw > that these settings may conflict. Should the ZBC code e.g. refuse that the sd > driver is attached to a ZBC disk controlled by a SCSI LLD that supports > use_10_for_rw but not use_16_for_rw? rw16 is mandatory for ZBC drives. So it has to be set to true. If the HBA does not support rw16 (why would that happen ?), then the disk should not be used. Cheers. -- Damien Le Moal Western Digital Research
Re: [PATCH V3 06/12] scsi: sd_zbc: Rearrange code
On Fri, 2017-09-15 at 19:51 +0200, h...@lst.de wrote: > On Fri, Sep 15, 2017 at 02:51:03PM +, Bart Van Assche wrote: > > On Fri, 2017-09-15 at 19:06 +0900, Damien Le Moal wrote: > > > Rearrange sd_zbc_setup() to include use_16_for_rw and use_10_for_rw > > > assignments and move the calculation of sdkp->zone_shift together > > > with the assignment of the verified zone_blocks value in > > > sd_zbc_check_zone_size(). > > > > Both functions are called from inside > > block_device_operations.revalidate_disk. > > Isn't that too late to set use_1[06]_for_rw? Shouldn't both be set just > > after > > the slave_alloc callback has been called? > > Hmm. sd_read_capacity already is called from the same path and seems > to work.. Hello Christoph, My concern is that if both a SCSI LLD and the ZBC code set use_1[06]_for_rw that these settings may conflict. Should the ZBC code e.g. refuse that the sd driver is attached to a ZBC disk controlled by a SCSI LLD that supports use_10_for_rw but not use_16_for_rw? Thanks, Bart.
Re: [PATCH V3 06/12] scsi: sd_zbc: Rearrange code
On Fri, Sep 15, 2017 at 02:51:03PM +, Bart Van Assche wrote: > On Fri, 2017-09-15 at 19:06 +0900, Damien Le Moal wrote: > > Rearrange sd_zbc_setup() to include use_16_for_rw and use_10_for_rw > > assignments and move the calculation of sdkp->zone_shift together > > with the assignment of the verified zone_blocks value in > > sd_zbc_check_zone_size(). > > Both functions are called from inside block_device_operations.revalidate_disk. > Isn't that too late to set use_1[06]_for_rw? Shouldn't both be set just after > the slave_alloc callback has been called? Hmm. sd_read_capacity already is called from the same path and seems to work.. > > Thanks, > > Bart.---end quoted text---
Re: [PATCH V3 06/12] scsi: sd_zbc: Rearrange code
Looks fine, Reviewed-by: Christoph Hellwig
Re: [PATCH V3 06/12] scsi: sd_zbc: Rearrange code
On Fri, 2017-09-15 at 19:06 +0900, Damien Le Moal wrote: > Rearrange sd_zbc_setup() to include use_16_for_rw and use_10_for_rw > assignments and move the calculation of sdkp->zone_shift together > with the assignment of the verified zone_blocks value in > sd_zbc_check_zone_size(). Both functions are called from inside block_device_operations.revalidate_disk. Isn't that too late to set use_1[06]_for_rw? Shouldn't both be set just after the slave_alloc callback has been called? Thanks, Bart.
Re: [PATCH V3 06/12] scsi: sd_zbc: Rearrange code
On 09/15/2017 12:06 PM, Damien Le Moal wrote: > Rearrange sd_zbc_setup() to include use_16_for_rw and use_10_for_rw > assignments and move the calculation of sdkp->zone_shift together > with the assignment of the verified zone_blocks value in > sd_zbc_check_zone_size(). > > No functional change is introduced by this patch. > > Signed-off-by: Damien Le Moal > --- > drivers/scsi/sd_zbc.c | 10 +- > 1 file changed, 5 insertions(+), 5 deletions(-) > Reviewed-by: Hannes Reinecke Cheers, Hannes -- Dr. Hannes ReineckeTeamlead Storage & Networking h...@suse.de +49 911 74053 688 SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg GF: F. Imendörffer, J. Smithard, J. Guild, D. Upmanyu, G. Norton HRB 21284 (AG Nürnberg)
