diff mbox series

scsi: sd_zbc: Improve zone revalidation

Message ID 20200730134024.548177-1-damien.lemoal@wdc.com (mailing list archive)
State Superseded
Headers show
Series scsi: sd_zbc: Improve zone revalidation | expand

Commit Message

Damien Le Moal July 30, 2020, 1:40 p.m. UTC 
Currently, for zoned disks, since blk_revalidate_disk_zones() requires
the disk capacity to be set already to operate correctly, zones
revalidation can only be done on the second revalidate scan once the
gendisk capacity is set at the end of the first scan. As a result, if
zones revalidation fails, there is no second chance to recover from the
failure and the disk capacity is changed to 0, with the disk left
unusable.

This can be improved by shuffling around code, specifically, by moving
the call to sd_zbc_revalidate_zones() from sd_zbc_read_zones() to the
end of sd_revalidate_disk(), after set_capacity_revalidate_and_notify()
is called to set the gendisk capacity. With this change, if
sd_zbc_revalidate_zones() fails on the first scan, the second scan will
call it again to recover, if possible.

Using the new struct scsi_disk fields rev_nr_zones and rev_zone_blocks,
sd_zbc_revalidate_zones() does actual work only if it detects a change
with the disk zone configuration. This means that for a successful
zones revalidation on the first scan, the second scan will not cause
another heavy full check.

While at it, remove the unecesary "extern" declaration of
sd_zbc_read_zones().

Signed-off-by: Damien Le Moal <damien.lemoal@wdc.com>
---
 drivers/scsi/sd.c     | 10 ++++-
 drivers/scsi/sd.h     | 11 +++--
 drivers/scsi/sd_zbc.c | 95 ++++++++++++++++++++-----------------------
 3 files changed, 61 insertions(+), 55 deletions(-)

Comments

kernel test robot July 30, 2020, 8:54 p.m. UTC | #1 
Hi Damien,

I love your patch! Perhaps something to improve:

[auto build test WARNING on scsi/for-next]
[also build test WARNING on mkp-scsi/for-next next-20200730]
[cannot apply to target/for-next v5.8-rc7]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/0day-ci/linux/commits/Damien-Le-Moal/scsi-sd_zbc-Improve-zone-revalidation/20200730-214245
base:   https://git.kernel.org/pub/scm/linux/kernel/git/jejb/scsi.git for-next
config: x86_64-randconfig-a013-20200730 (attached as .config)
compiler: clang version 12.0.0 (https://github.com/llvm/llvm-project 6700f4b9fe6321ef704efa4890af5bc351a124f0)
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # install x86_64 cross compiling tool for clang build
        # apt-get install binutils-x86-64-linux-gnu
        # save the attached .config to linux build tree
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross ARCH=x86_64 

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

All warnings (new ones prefixed by >>):

>> drivers/scsi/sd_zbc.c:678:6: warning: variable 'ret' is used uninitialized whenever 'if' condition is true [-Wsometimes-uninitialized]
           if (sdkp->zone_blocks == zone_blocks &&
               ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
   drivers/scsi/sd_zbc.c:714:9: note: uninitialized use occurs here
           return ret;
                  ^~~
   drivers/scsi/sd_zbc.c:678:2: note: remove the 'if' if its condition is always false
           if (sdkp->zone_blocks == zone_blocks &&
           ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
   drivers/scsi/sd_zbc.c:667:9: note: initialize the variable 'ret' to silence this warning
           int ret;
                  ^
                   = 0
   1 warning generated.

vim +678 drivers/scsi/sd_zbc.c

5795eb44306014 Johannes Thumshirn 2020-05-12  659  
3dd6d693a9953a Damien Le Moal     2020-07-30  660  int sd_zbc_revalidate_zones(struct scsi_disk *sdkp)
5795eb44306014 Johannes Thumshirn 2020-05-12  661  {
5795eb44306014 Johannes Thumshirn 2020-05-12  662  	struct gendisk *disk = sdkp->disk;
3dd6d693a9953a Damien Le Moal     2020-07-30  663  	struct request_queue *q = disk->queue;
3dd6d693a9953a Damien Le Moal     2020-07-30  664  	u32 zone_blocks = sdkp->rev_zone_blocks;
3dd6d693a9953a Damien Le Moal     2020-07-30  665  	unsigned int nr_zones = sdkp->rev_nr_zones;
3dd6d693a9953a Damien Le Moal     2020-07-30  666  	u32 max_append;
3dd6d693a9953a Damien Le Moal     2020-07-30  667  	int ret;
3dd6d693a9953a Damien Le Moal     2020-07-30  668  
3dd6d693a9953a Damien Le Moal     2020-07-30  669  	if (!sd_is_zoned(sdkp))
3dd6d693a9953a Damien Le Moal     2020-07-30  670  		return 0;
5795eb44306014 Johannes Thumshirn 2020-05-12  671  
5795eb44306014 Johannes Thumshirn 2020-05-12  672  	/*
5795eb44306014 Johannes Thumshirn 2020-05-12  673  	 * Make sure revalidate zones are serialized to ensure exclusive
5795eb44306014 Johannes Thumshirn 2020-05-12  674  	 * updates of the scsi disk data.
5795eb44306014 Johannes Thumshirn 2020-05-12  675  	 */
5795eb44306014 Johannes Thumshirn 2020-05-12  676  	mutex_lock(&sdkp->rev_mutex);
5795eb44306014 Johannes Thumshirn 2020-05-12  677  
5795eb44306014 Johannes Thumshirn 2020-05-12 @678  	if (sdkp->zone_blocks == zone_blocks &&
5795eb44306014 Johannes Thumshirn 2020-05-12  679  	    sdkp->nr_zones == nr_zones &&
5795eb44306014 Johannes Thumshirn 2020-05-12  680  	    disk->queue->nr_zones == nr_zones)
5795eb44306014 Johannes Thumshirn 2020-05-12  681  		goto unlock;
5795eb44306014 Johannes Thumshirn 2020-05-12  682  
3dd6d693a9953a Damien Le Moal     2020-07-30  683  	sdkp->zone_blocks = zone_blocks;
3dd6d693a9953a Damien Le Moal     2020-07-30  684  	sdkp->nr_zones = nr_zones;
5795eb44306014 Johannes Thumshirn 2020-05-12  685  	sdkp->rev_wp_offset = kvcalloc(nr_zones, sizeof(u32), GFP_NOIO);
5795eb44306014 Johannes Thumshirn 2020-05-12  686  	if (!sdkp->rev_wp_offset) {
5795eb44306014 Johannes Thumshirn 2020-05-12  687  		ret = -ENOMEM;
5795eb44306014 Johannes Thumshirn 2020-05-12  688  		goto unlock;
5795eb44306014 Johannes Thumshirn 2020-05-12  689  	}
5795eb44306014 Johannes Thumshirn 2020-05-12  690  
5795eb44306014 Johannes Thumshirn 2020-05-12  691  	ret = blk_revalidate_disk_zones(disk, sd_zbc_revalidate_zones_cb);
5795eb44306014 Johannes Thumshirn 2020-05-12  692  
5795eb44306014 Johannes Thumshirn 2020-05-12  693  	kvfree(sdkp->rev_wp_offset);
5795eb44306014 Johannes Thumshirn 2020-05-12  694  	sdkp->rev_wp_offset = NULL;
5795eb44306014 Johannes Thumshirn 2020-05-12  695  
3dd6d693a9953a Damien Le Moal     2020-07-30  696  	if (ret) {
3dd6d693a9953a Damien Le Moal     2020-07-30  697  		sdkp->zone_blocks = 0;
3dd6d693a9953a Damien Le Moal     2020-07-30  698  		sdkp->nr_zones = 0;
3dd6d693a9953a Damien Le Moal     2020-07-30  699  		sdkp->capacity = 0;
3dd6d693a9953a Damien Le Moal     2020-07-30  700  		goto unlock;
3dd6d693a9953a Damien Le Moal     2020-07-30  701  	}
3dd6d693a9953a Damien Le Moal     2020-07-30  702  
3dd6d693a9953a Damien Le Moal     2020-07-30  703  	max_append = min_t(u32, logical_to_sectors(sdkp->device, zone_blocks),
3dd6d693a9953a Damien Le Moal     2020-07-30  704  			   q->limits.max_segments << (PAGE_SHIFT - 9));
3dd6d693a9953a Damien Le Moal     2020-07-30  705  	max_append = min_t(u32, max_append, queue_max_hw_sectors(q));
3dd6d693a9953a Damien Le Moal     2020-07-30  706  
3dd6d693a9953a Damien Le Moal     2020-07-30  707  	blk_queue_max_zone_append_sectors(q, max_append);
3dd6d693a9953a Damien Le Moal     2020-07-30  708  
3dd6d693a9953a Damien Le Moal     2020-07-30  709  	sd_zbc_print_zones(sdkp);
3dd6d693a9953a Damien Le Moal     2020-07-30  710  
5795eb44306014 Johannes Thumshirn 2020-05-12  711  unlock:
5795eb44306014 Johannes Thumshirn 2020-05-12  712  	mutex_unlock(&sdkp->rev_mutex);
5795eb44306014 Johannes Thumshirn 2020-05-12  713  
5795eb44306014 Johannes Thumshirn 2020-05-12  714  	return ret;
5795eb44306014 Johannes Thumshirn 2020-05-12  715  }
5795eb44306014 Johannes Thumshirn 2020-05-12  716  

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org
diff mbox series

Patch

diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c
index acde0ca35769..95018e650f2d 100644
--- a/drivers/scsi/sd.c
+++ b/drivers/scsi/sd.c
@@ -2578,8 +2578,6 @@  sd_print_capacity(struct scsi_disk *sdkp,
 		sd_printk(KERN_NOTICE, sdkp,
 			  "%u-byte physical blocks\n",
 			  sdkp->physical_block_size);
-
-	sd_zbc_print_zones(sdkp);
 }
 
 /* called with buffer of length 512 */
@@ -3220,6 +3218,14 @@  static int sd_revalidate_disk(struct gendisk *disk)
 	sd_config_write_same(sdkp);
 	kfree(buffer);
 
+	/*
+	 * For a zoned drive, revalidating the zones can be done only once
+	 * the gendisk capacity is set. So if this fails, set back the gendisk
+	 * capacity to 0.
+	 */
+	if (sd_zbc_revalidate_zones(sdkp))
+		set_capacity_revalidate_and_notify(disk, 0, false);
+
  out:
 	return 0;
 }
diff --git a/drivers/scsi/sd.h b/drivers/scsi/sd.h
index 27c0f4e9b1d4..4933e7daf17d 100644
--- a/drivers/scsi/sd.h
+++ b/drivers/scsi/sd.h
@@ -75,7 +75,9 @@  struct scsi_disk {
 	struct opal_dev *opal_dev;
 #ifdef CONFIG_BLK_DEV_ZONED
 	u32		nr_zones;
+	u32		rev_nr_zones;
 	u32		zone_blocks;
+	u32		rev_zone_blocks;
 	u32		zones_optimal_open;
 	u32		zones_optimal_nonseq;
 	u32		zones_max_open;
@@ -215,8 +217,8 @@  static inline int sd_is_zoned(struct scsi_disk *sdkp)
 
 int sd_zbc_init_disk(struct scsi_disk *sdkp);
 void sd_zbc_release_disk(struct scsi_disk *sdkp);
-extern int sd_zbc_read_zones(struct scsi_disk *sdkp, unsigned char *buffer);
-extern void sd_zbc_print_zones(struct scsi_disk *sdkp);
+int sd_zbc_read_zones(struct scsi_disk *sdkp, unsigned char *buffer);
+int sd_zbc_revalidate_zones(struct scsi_disk *sdkp);
 blk_status_t sd_zbc_setup_zone_mgmt_cmnd(struct scsi_cmnd *cmd,
 					 unsigned char op, bool all);
 unsigned int sd_zbc_complete(struct scsi_cmnd *cmd, unsigned int good_bytes,
@@ -242,7 +244,10 @@  static inline int sd_zbc_read_zones(struct scsi_disk *sdkp,
 	return 0;
 }
 
-static inline void sd_zbc_print_zones(struct scsi_disk *sdkp) {}
+static inline int sd_zbc_revalidate_zones(struct scsi_disk *sdkp)
+{
+	return 0;
+}
 
 static inline blk_status_t sd_zbc_setup_zone_mgmt_cmnd(struct scsi_cmnd *cmd,
 						       unsigned char op,
diff --git a/drivers/scsi/sd_zbc.c b/drivers/scsi/sd_zbc.c
index 35d929b00a0b..6532f3fab4cc 100644
--- a/drivers/scsi/sd_zbc.c
+++ b/drivers/scsi/sd_zbc.c
@@ -633,6 +633,23 @@  static int sd_zbc_check_capacity(struct scsi_disk *sdkp, unsigned char *buf,
 	return 0;
 }
 
+static void sd_zbc_print_zones(struct scsi_disk *sdkp)
+{
+	if (!sd_is_zoned(sdkp) || !sdkp->capacity)
+		return;
+
+	if (sdkp->capacity & (sdkp->zone_blocks - 1))
+		sd_printk(KERN_NOTICE, sdkp,
+			  "%u zones of %u logical blocks + 1 runt zone\n",
+			  sdkp->nr_zones - 1,
+			  sdkp->zone_blocks);
+	else
+		sd_printk(KERN_NOTICE, sdkp,
+			  "%u zones of %u logical blocks\n",
+			  sdkp->nr_zones,
+			  sdkp->zone_blocks);
+}
+
 static void sd_zbc_revalidate_zones_cb(struct gendisk *disk)
 {
 	struct scsi_disk *sdkp = scsi_disk(disk);
@@ -640,12 +657,17 @@  static void sd_zbc_revalidate_zones_cb(struct gendisk *disk)
 	swap(sdkp->zones_wp_offset, sdkp->rev_wp_offset);
 }
 
-static int sd_zbc_revalidate_zones(struct scsi_disk *sdkp,
-				   u32 zone_blocks,
-				   unsigned int nr_zones)
+int sd_zbc_revalidate_zones(struct scsi_disk *sdkp)
 {
 	struct gendisk *disk = sdkp->disk;
-	int ret = 0;
+	struct request_queue *q = disk->queue;
+	u32 zone_blocks = sdkp->rev_zone_blocks;
+	unsigned int nr_zones = sdkp->rev_nr_zones;
+	u32 max_append;
+	int ret;
+
+	if (!sd_is_zoned(sdkp))
+		return 0;
 
 	/*
 	 * Make sure revalidate zones are serialized to ensure exclusive
@@ -653,23 +675,13 @@  static int sd_zbc_revalidate_zones(struct scsi_disk *sdkp,
 	 */
 	mutex_lock(&sdkp->rev_mutex);
 
-	/*
-	 * Revalidate the disk zones to update the device request queue zone
-	 * bitmaps and the zone write pointer offset array. Do this only once
-	 * the device capacity is set on the second revalidate execution for
-	 * disk scan or if something changed when executing a normal revalidate.
-	 */
-	if (sdkp->first_scan) {
-		sdkp->zone_blocks = zone_blocks;
-		sdkp->nr_zones = nr_zones;
-		goto unlock;
-	}
-
 	if (sdkp->zone_blocks == zone_blocks &&
 	    sdkp->nr_zones == nr_zones &&
 	    disk->queue->nr_zones == nr_zones)
 		goto unlock;
 
+	sdkp->zone_blocks = zone_blocks;
+	sdkp->nr_zones = nr_zones;
 	sdkp->rev_wp_offset = kvcalloc(nr_zones, sizeof(u32), GFP_NOIO);
 	if (!sdkp->rev_wp_offset) {
 		ret = -ENOMEM;
@@ -681,6 +693,21 @@  static int sd_zbc_revalidate_zones(struct scsi_disk *sdkp,
 	kvfree(sdkp->rev_wp_offset);
 	sdkp->rev_wp_offset = NULL;
 
+	if (ret) {
+		sdkp->zone_blocks = 0;
+		sdkp->nr_zones = 0;
+		sdkp->capacity = 0;
+		goto unlock;
+	}
+
+	max_append = min_t(u32, logical_to_sectors(sdkp->device, zone_blocks),
+			   q->limits.max_segments << (PAGE_SHIFT - 9));
+	max_append = min_t(u32, max_append, queue_max_hw_sectors(q));
+
+	blk_queue_max_zone_append_sectors(q, max_append);
+
+	sd_zbc_print_zones(sdkp);
+
 unlock:
 	mutex_unlock(&sdkp->rev_mutex);
 
@@ -693,7 +720,6 @@  int sd_zbc_read_zones(struct scsi_disk *sdkp, unsigned char *buf)
 	struct request_queue *q = disk->queue;
 	unsigned int nr_zones;
 	u32 zone_blocks = 0;
-	u32 max_append;
 	int ret;
 
 	if (!sd_is_zoned(sdkp))
@@ -722,22 +748,8 @@  int sd_zbc_read_zones(struct scsi_disk *sdkp, unsigned char *buf)
 	sdkp->device->use_16_for_rw = 1;
 	sdkp->device->use_10_for_rw = 0;
 
-	ret = sd_zbc_revalidate_zones(sdkp, zone_blocks, nr_zones);
-	if (ret)
-		goto err;
-
-	/*
-	 * On the first scan 'chunk_sectors' isn't setup yet, so calling
-	 * blk_queue_max_zone_append_sectors() will result in a WARN(). Defer
-	 * this setting to the second scan.
-	 */
-	if (sdkp->first_scan)
-		return 0;
-
-	max_append = min_t(u32, logical_to_sectors(sdkp->device, zone_blocks),
-			   q->limits.max_segments << (PAGE_SHIFT - 9));
-
-	blk_queue_max_zone_append_sectors(q, max_append);
+	sdkp->rev_nr_zones = nr_zones;
+	sdkp->rev_zone_blocks = zone_blocks;
 
 	return 0;
 
@@ -747,23 +759,6 @@  int sd_zbc_read_zones(struct scsi_disk *sdkp, unsigned char *buf)
 	return ret;
 }
 
-void sd_zbc_print_zones(struct scsi_disk *sdkp)
-{
-	if (!sd_is_zoned(sdkp) || !sdkp->capacity)
-		return;
-
-	if (sdkp->capacity & (sdkp->zone_blocks - 1))
-		sd_printk(KERN_NOTICE, sdkp,
-			  "%u zones of %u logical blocks + 1 runt zone\n",
-			  sdkp->nr_zones - 1,
-			  sdkp->zone_blocks);
-	else
-		sd_printk(KERN_NOTICE, sdkp,
-			  "%u zones of %u logical blocks\n",
-			  sdkp->nr_zones,
-			  sdkp->zone_blocks);
-}
-
 int sd_zbc_init_disk(struct scsi_disk *sdkp)
 {
 	if (!sd_is_zoned(sdkp))