[RFC 0/5] block: use flag enhancement for del_gendisk()

[RFC 0/5] block: use flag enhancement for del_gendisk()

[Luis Chamberlain]

This is the third group of driver conversions for adding *add_disk*()
error handling. In this series we address the new flag added and use
on del_gendisk() so that it's easier to deal with error handling.

Luis Chamberlain (5):
  bcache: remove no longer needed add_disk() check
  bcache: add error handling support for add_disk()
  block/sx8: remove the GENHD_FL_UP check before del_gendisk()
  block/sx8: add helper carm_free_all_disks()
  block/sx8: add error handling support for add_disk()

 drivers/block/sx8.c       | 58 +++++++++++++++++++++++----------------
 drivers/md/bcache/super.c | 23 +++++++++-------
 2 files changed, 47 insertions(+), 34 deletions(-)

-- 
2.27.0

[RFC 1/5] bcache: remove no longer needed add_disk() check

[Luis Chamberlain]

After the patch "block: add flag for add_disk() completion
notation" we no longer need to check the disk was added prior
to calling del_gendisk(), del_gendisk() now deals with the
check for us.

Signed-off-by: Luis Chamberlain <mcgrof@kernel.org>
---
 drivers/md/bcache/super.c | 6 +-----
 1 file changed, 1 insertion(+), 5 deletions(-)

diff --git a/drivers/md/bcache/super.c b/drivers/md/bcache/super.c
index 185246a0d855..70ee7a22b2a3 100644
--- a/drivers/md/bcache/super.c
+++ b/drivers/md/bcache/super.c
@@ -885,11 +885,7 @@ static void bcache_device_free(struct bcache_device *d)
 		bcache_device_detach(d);
 
 	if (disk) {
-		bool disk_added = (disk->flags & GENHD_FL_UP) != 0;
-
-		if (disk_added)
-			del_gendisk(disk);
-
+		del_gendisk(disk);
 		blk_cleanup_disk(disk);
 		ida_simple_remove(&bcache_device_idx,
 				  first_minor_to_idx(disk->first_minor));
-- 
2.27.0

[RFC 2/5] bcache: add error handling support for add_disk()

[Luis Chamberlain]

We never checked for errors on add_disk() as this function
returned void. Now that this is fixed, use the shiny new
error handling.

This driver doesn't do any unwinding with blk_cleanup_disk()
even on errors after add_disk() and so we follow that
tradition.

Signed-off-by: Luis Chamberlain <mcgrof@kernel.org>
---
 drivers/md/bcache/super.c | 17 ++++++++++++-----
 1 file changed, 12 insertions(+), 5 deletions(-)

diff --git a/drivers/md/bcache/super.c b/drivers/md/bcache/super.c
index 70ee7a22b2a3..85da34e234d6 100644
--- a/drivers/md/bcache/super.c
+++ b/drivers/md/bcache/super.c
@@ -1077,7 +1077,9 @@ int bch_cached_dev_run(struct cached_dev *dc)
 		closure_sync(&cl);
 	}
 
-	add_disk(d->disk);
+	ret = add_disk(d->disk);
+	if (ret)
+		goto out;
 	bd_link_disk_holder(dc->bdev, dc->disk.disk);
 	/*
 	 * won't show up in the uevent file, use udevadm monitor -e instead
@@ -1526,10 +1528,11 @@ static void flash_dev_flush(struct closure *cl)
 
 static int flash_dev_run(struct cache_set *c, struct uuid_entry *u)
 {
+	int err = -ENOMEM;
 	struct bcache_device *d = kzalloc(sizeof(struct bcache_device),
 					  GFP_KERNEL);
 	if (!d)
-		return -ENOMEM;
+		goto err_ret;
 
 	closure_init(&d->cl, NULL);
 	set_closure_fn(&d->cl, flash_dev_flush, system_wq);
@@ -1543,9 +1546,12 @@ static int flash_dev_run(struct cache_set *c, struct uuid_entry *u)
 	bcache_device_attach(d, c, u - c->uuids);
 	bch_sectors_dirty_init(d);
 	bch_flash_dev_request_init(d);
-	add_disk(d->disk);
+	err = add_disk(d->disk);
+	if (err)
+		goto err;
 
-	if (kobject_add(&d->kobj, &disk_to_dev(d->disk)->kobj, "bcache"))
+	err = kobject_add(&d->kobj, &disk_to_dev(d->disk)->kobj, "bcache");
+	if (err)
 		goto err;
 
 	bcache_device_link(d, c, "volume");
@@ -1559,7 +1565,8 @@ static int flash_dev_run(struct cache_set *c, struct uuid_entry *u)
 	return 0;
 err:
 	kobject_put(&d->kobj);
-	return -ENOMEM;
+err_ret:
+	return err;
 }
 
 static int flash_devs_run(struct cache_set *c)
-- 
2.27.0

Re: [RFC 2/5] bcache: add error handling support for add_disk()

[kernel test robot]

[-- Attachment #1: Type: text/plain, Size: 4603 bytes --]

Hi Luis,

[FYI, it's a private test report for your RFC patch.]
[auto build test ERROR on block/for-next]
[also build test ERROR on linus/master v5.14-rc1 next-20210715]
[cannot apply to linux/master]
[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/Luis-Chamberlain/block-use-flag-enhancement-for-del_gendisk/20210716-043130
base:   https://git.kernel.org/pub/scm/linux/kernel/git/axboe/linux-block.git for-next
config: x86_64-randconfig-a004-20210715 (attached as .config)
compiler: clang version 13.0.0 (https://github.com/llvm/llvm-project 0e49c54a8cbd3e779e5526a5888c683c01cc3c50)
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
        # https://github.com/0day-ci/linux/commit/75d7d3207d699f24abb7502f1dc8c1ff00a9ac1a
        git remote add linux-review https://github.com/0day-ci/linux
        git fetch --no-tags linux-review Luis-Chamberlain/block-use-flag-enhancement-for-del_gendisk/20210716-043130
        git checkout 75d7d3207d699f24abb7502f1dc8c1ff00a9ac1a
        # save the attached .config to linux build tree
        mkdir build_dir
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross O=build_dir ARCH=x86_64 SHELL=/bin/bash drivers/block/ drivers/md/bcache/ drivers/scsi/

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

All errors (new ones prefixed by >>):

>> drivers/md/bcache/super.c:1080:6: error: assigning to 'int' from incompatible type 'void'
           ret = add_disk(d->disk);
               ^ ~~~~~~~~~~~~~~~~~
   drivers/md/bcache/super.c:1549:6: error: assigning to 'int' from incompatible type 'void'
           err = add_disk(d->disk);
               ^ ~~~~~~~~~~~~~~~~~
   2 errors generated.


vim +1080 drivers/md/bcache/super.c

  1041	
  1042	
  1043	int bch_cached_dev_run(struct cached_dev *dc)
  1044	{
  1045		int ret = 0;
  1046		struct bcache_device *d = &dc->disk;
  1047		char *buf = kmemdup_nul(dc->sb.label, SB_LABEL_SIZE, GFP_KERNEL);
  1048		char *env[] = {
  1049			"DRIVER=bcache",
  1050			kasprintf(GFP_KERNEL, "CACHED_UUID=%pU", dc->sb.uuid),
  1051			kasprintf(GFP_KERNEL, "CACHED_LABEL=%s", buf ? : ""),
  1052			NULL,
  1053		};
  1054	
  1055		if (dc->io_disable) {
  1056			pr_err("I/O disabled on cached dev %s\n",
  1057			       dc->backing_dev_name);
  1058			ret = -EIO;
  1059			goto out;
  1060		}
  1061	
  1062		if (atomic_xchg(&dc->running, 1)) {
  1063			pr_info("cached dev %s is running already\n",
  1064			       dc->backing_dev_name);
  1065			ret = -EBUSY;
  1066			goto out;
  1067		}
  1068	
  1069		if (!d->c &&
  1070		    BDEV_STATE(&dc->sb) != BDEV_STATE_NONE) {
  1071			struct closure cl;
  1072	
  1073			closure_init_stack(&cl);
  1074	
  1075			SET_BDEV_STATE(&dc->sb, BDEV_STATE_STALE);
  1076			bch_write_bdev_super(dc, &cl);
  1077			closure_sync(&cl);
  1078		}
  1079	
> 1080		ret = add_disk(d->disk);
  1081		if (ret)
  1082			goto out;
  1083		bd_link_disk_holder(dc->bdev, dc->disk.disk);
  1084		/*
  1085		 * won't show up in the uevent file, use udevadm monitor -e instead
  1086		 * only class / kset properties are persistent
  1087		 */
  1088		kobject_uevent_env(&disk_to_dev(d->disk)->kobj, KOBJ_CHANGE, env);
  1089	
  1090		if (sysfs_create_link(&d->kobj, &disk_to_dev(d->disk)->kobj, "dev") ||
  1091		    sysfs_create_link(&disk_to_dev(d->disk)->kobj,
  1092				      &d->kobj, "bcache")) {
  1093			pr_err("Couldn't create bcache dev <-> disk sysfs symlinks\n");
  1094			ret = -ENOMEM;
  1095			goto out;
  1096		}
  1097	
  1098		dc->status_update_thread = kthread_run(cached_dev_status_update,
  1099						       dc, "bcache_status_update");
  1100		if (IS_ERR(dc->status_update_thread)) {
  1101			pr_warn("failed to create bcache_status_update kthread, continue to run without monitoring backing device status\n");
  1102		}
  1103	
  1104	out:
  1105		kfree(env[1]);
  1106		kfree(env[2]);
  1107		kfree(buf);
  1108		return ret;
  1109	}
  1110	

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all(a)lists.01.org

[-- Attachment #2: config.gz --]
[-- Type: application/gzip, Size: 43732 bytes --]

[RFC 3/5] block/sx8: remove the GENHD_FL_UP check before del_gendisk()

[Luis Chamberlain]

This is no longer required, given the block layer adds a flag
now for us on a disk once add_disk() completes successfully,
and so del_gendisk() will only really run if that flag is set.

Signed-off-by: Luis Chamberlain <mcgrof@kernel.org>
---
 drivers/block/sx8.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/block/sx8.c b/drivers/block/sx8.c
index 7b54353ee92b..e4dfee5acf08 100644
--- a/drivers/block/sx8.c
+++ b/drivers/block/sx8.c
@@ -1373,8 +1373,7 @@ static void carm_free_disk(struct carm_host *host, unsigned int port_no)
 	if (!disk)
 		return;
 
-	if (disk->flags & GENHD_FL_UP)
-		del_gendisk(disk);
+	del_gendisk(disk);
 	blk_cleanup_disk(disk);
 }
 
-- 
2.27.0

[RFC 4/5] block/sx8: add helper carm_free_all_disks()

[Luis Chamberlain]

Share the code of unregistering disks in a common helper.
Code is shifted a above so that we can later re-use this
helper in other places.

Signed-off-by: Luis Chamberlain <mcgrof@kernel.org>
---
 drivers/block/sx8.c | 42 +++++++++++++++++++++++-------------------
 1 file changed, 23 insertions(+), 19 deletions(-)

diff --git a/drivers/block/sx8.c b/drivers/block/sx8.c
index e4dfee5acf08..6a6dc3fffa5c 100644
--- a/drivers/block/sx8.c
+++ b/drivers/block/sx8.c
@@ -1092,6 +1092,27 @@ static irqreturn_t carm_interrupt(int irq, void *__host)
 	return IRQ_RETVAL(handled);
 }
 
+static void carm_free_disk(struct carm_host *host, unsigned int port_no)
+{
+	struct carm_port *port = &host->port[port_no];
+	struct gendisk *disk = port->disk;
+
+	if (!disk)
+		return;
+
+	del_gendisk(disk);
+	blk_cleanup_disk(disk);
+}
+
+static void carm_free_all_disks(struct carm_host *host)
+{
+	unsigned int i;
+
+	for (i = 0; i < CARM_MAX_PORTS; i++)
+		carm_free_disk(host, i);
+	unregister_blkdev(host->major, host->name);
+}
+
 static void carm_fsm_task (struct work_struct *work)
 {
 	struct carm_host *host =
@@ -1365,18 +1386,6 @@ static int carm_init_disk(struct carm_host *host, unsigned int port_no)
 	return 0;
 }
 
-static void carm_free_disk(struct carm_host *host, unsigned int port_no)
-{
-	struct carm_port *port = &host->port[port_no];
-	struct gendisk *disk = port->disk;
-
-	if (!disk)
-		return;
-
-	del_gendisk(disk);
-	blk_cleanup_disk(disk);
-}
-
 static int carm_init_shm(struct carm_host *host)
 {
 	host->shm = dma_alloc_coherent(&host->pdev->dev, CARM_SHM_SIZE,
@@ -1520,9 +1529,7 @@ static int carm_init_one (struct pci_dev *pdev, const struct pci_device_id *ent)
 err_out_free_irq:
 	free_irq(pdev->irq, host);
 err_out_blkdev_disks:
-	for (i = 0; i < CARM_MAX_PORTS; i++)
-		carm_free_disk(host, i);
-	unregister_blkdev(host->major, host->name);
+	carm_free_all_disks(host);
 err_out_free_majors:
 	if (host->major == 160)
 		clear_bit(0, &carm_major_alloc);
@@ -1546,7 +1553,6 @@ static int carm_init_one (struct pci_dev *pdev, const struct pci_device_id *ent)
 static void carm_remove_one (struct pci_dev *pdev)
 {
 	struct carm_host *host = pci_get_drvdata(pdev);
-	unsigned int i;
 
 	if (!host) {
 		printk(KERN_ERR PFX "BUG: no host data for PCI(%s)\n",
@@ -1555,9 +1561,7 @@ static void carm_remove_one (struct pci_dev *pdev)
 	}
 
 	free_irq(pdev->irq, host);
-	for (i = 0; i < CARM_MAX_PORTS; i++)
-		carm_free_disk(host, i);
-	unregister_blkdev(host->major, host->name);
+	carm_free_all_disks(host);
 	if (host->major == 160)
 		clear_bit(0, &carm_major_alloc);
 	else if (host->major == 161)
-- 
2.27.0

[RFC 5/5] block/sx8: add error handling support for add_disk()

[Luis Chamberlain]

We never checked for errors on add_disk() as this function
returned void. Now that this is fixed, use the shiny new
error handling.

A completion is used to notify the initial probe what is
happening and so we must defer error handling on completion.
Do this by remembering the error and using the shared cleanup
function.

The tags are shared and so are hanlded later for the
driver already.

Signed-off-by: Luis Chamberlain <mcgrof@kernel.org>
---
 drivers/block/sx8.c | 15 +++++++++++----
 1 file changed, 11 insertions(+), 4 deletions(-)

diff --git a/drivers/block/sx8.c b/drivers/block/sx8.c
index 6a6dc3fffa5c..34bbd033e2fd 100644
--- a/drivers/block/sx8.c
+++ b/drivers/block/sx8.c
@@ -297,6 +297,7 @@ struct carm_host {
 
 	struct work_struct		fsm_task;
 
+	int probe_err;
 	struct completion		probe_comp;
 };
 
@@ -1202,8 +1203,11 @@ static void carm_fsm_task (struct work_struct *work)
 				struct gendisk *disk = port->disk;
 
 				set_capacity(disk, port->capacity);
-				add_disk(disk);
-				activated++;
+				host->probe_err = add_disk(disk);
+				if (!host->probe_err)
+					activated++;
+				else
+					break;
 			}
 
 		printk(KERN_INFO DRV_NAME "(%s): %d ports activated\n",
@@ -1213,11 +1217,9 @@ static void carm_fsm_task (struct work_struct *work)
 		reschedule = 1;
 		break;
 	}
-
 	case HST_PROBE_FINISHED:
 		complete(&host->probe_comp);
 		break;
-
 	case HST_ERROR:
 		/* FIXME: TODO */
 		break;
@@ -1515,7 +1517,10 @@ static int carm_init_one (struct pci_dev *pdev, const struct pci_device_id *ent)
 		goto err_out_free_irq;
 
 	DPRINTK("waiting for probe_comp\n");
+	host->probe_err = -ENODEV;
 	wait_for_completion(&host->probe_comp);
+	if (host->probe_err)
+		goto err_out_disks;
 
 	printk(KERN_INFO "%s: pci %s, ports %d, io %llx, irq %u, major %d\n",
 	       host->name, pci_name(pdev), (int) CARM_MAX_PORTS,
@@ -1526,6 +1531,8 @@ static int carm_init_one (struct pci_dev *pdev, const struct pci_device_id *ent)
 	pci_set_drvdata(pdev, host);
 	return 0;
 
+err_out_disks:
+	carm_free_all_disks(host);
 err_out_free_irq:
 	free_irq(pdev->irq, host);
 err_out_blkdev_disks:
-- 
2.27.0

Re: [RFC 5/5] block/sx8: add error handling support for add_disk()

[kernel test robot]

[-- Attachment #1: Type: text/plain, Size: 5781 bytes --]

Hi Luis,

[FYI, it's a private test report for your RFC patch.]
[auto build test ERROR on block/for-next]
[also build test ERROR on linus/master v5.14-rc1 next-20210715]
[cannot apply to linux/master]
[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/Luis-Chamberlain/block-use-flag-enhancement-for-del_gendisk/20210716-043130
base:   https://git.kernel.org/pub/scm/linux/kernel/git/axboe/linux-block.git for-next
config: arm64-randconfig-r036-20210715 (attached as .config)
compiler: clang version 13.0.0 (https://github.com/llvm/llvm-project 0e49c54a8cbd3e779e5526a5888c683c01cc3c50)
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 arm64 cross compiling tool for clang build
        # apt-get install binutils-aarch64-linux-gnu
        # https://github.com/0day-ci/linux/commit/6b028ffa67d111371215cfeb9ea04ed6e3d30723
        git remote add linux-review https://github.com/0day-ci/linux
        git fetch --no-tags linux-review Luis-Chamberlain/block-use-flag-enhancement-for-del_gendisk/20210716-043130
        git checkout 6b028ffa67d111371215cfeb9ea04ed6e3d30723
        # save the attached .config to linux build tree
        mkdir build_dir
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross O=build_dir ARCH=arm64 SHELL=/bin/bash drivers/block/

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

All errors (new ones prefixed by >>):

>> drivers/block/sx8.c:1206:21: error: assigning to 'int' from incompatible type 'void'
                                   host->probe_err = add_disk(disk);
                                                   ^ ~~~~~~~~~~~~~~
   1 error generated.


vim +1206 drivers/block/sx8.c

  1116	
  1117	static void carm_fsm_task (struct work_struct *work)
  1118	{
  1119		struct carm_host *host =
  1120			container_of(work, struct carm_host, fsm_task);
  1121		unsigned long flags;
  1122		unsigned int state;
  1123		int rc, i, next_dev;
  1124		int reschedule = 0;
  1125		int new_state = HST_INVALID;
  1126	
  1127		spin_lock_irqsave(&host->lock, flags);
  1128		state = host->state;
  1129		spin_unlock_irqrestore(&host->lock, flags);
  1130	
  1131		DPRINTK("ENTER, state == %s\n", state_name[state]);
  1132	
  1133		switch (state) {
  1134		case HST_PROBE_START:
  1135			new_state = HST_ALLOC_BUF;
  1136			reschedule = 1;
  1137			break;
  1138	
  1139		case HST_ALLOC_BUF:
  1140			rc = carm_send_special(host, carm_fill_alloc_buf);
  1141			if (rc) {
  1142				new_state = HST_ERROR;
  1143				reschedule = 1;
  1144			}
  1145			break;
  1146	
  1147		case HST_SYNC_TIME:
  1148			rc = carm_send_special(host, carm_fill_sync_time);
  1149			if (rc) {
  1150				new_state = HST_ERROR;
  1151				reschedule = 1;
  1152			}
  1153			break;
  1154	
  1155		case HST_GET_FW_VER:
  1156			rc = carm_send_special(host, carm_fill_get_fw_ver);
  1157			if (rc) {
  1158				new_state = HST_ERROR;
  1159				reschedule = 1;
  1160			}
  1161			break;
  1162	
  1163		case HST_PORT_SCAN:
  1164			rc = carm_send_special(host, carm_fill_scan_channels);
  1165			if (rc) {
  1166				new_state = HST_ERROR;
  1167				reschedule = 1;
  1168			}
  1169			break;
  1170	
  1171		case HST_DEV_SCAN_START:
  1172			host->cur_scan_dev = -1;
  1173			new_state = HST_DEV_SCAN;
  1174			reschedule = 1;
  1175			break;
  1176	
  1177		case HST_DEV_SCAN:
  1178			next_dev = -1;
  1179			for (i = host->cur_scan_dev + 1; i < CARM_MAX_PORTS; i++)
  1180				if (host->dev_present & (1 << i)) {
  1181					next_dev = i;
  1182					break;
  1183				}
  1184	
  1185			if (next_dev >= 0) {
  1186				host->cur_scan_dev = next_dev;
  1187				rc = carm_array_info(host, next_dev);
  1188				if (rc) {
  1189					new_state = HST_ERROR;
  1190					reschedule = 1;
  1191				}
  1192			} else {
  1193				new_state = HST_DEV_ACTIVATE;
  1194				reschedule = 1;
  1195			}
  1196			break;
  1197	
  1198		case HST_DEV_ACTIVATE: {
  1199			int activated = 0;
  1200			for (i = 0; i < CARM_MAX_PORTS; i++)
  1201				if (host->dev_active & (1 << i)) {
  1202					struct carm_port *port = &host->port[i];
  1203					struct gendisk *disk = port->disk;
  1204	
  1205					set_capacity(disk, port->capacity);
> 1206					host->probe_err = add_disk(disk);
  1207					if (!host->probe_err)
  1208						activated++;
  1209					else
  1210						break;
  1211				}
  1212	
  1213			printk(KERN_INFO DRV_NAME "(%s): %d ports activated\n",
  1214			       pci_name(host->pdev), activated);
  1215	
  1216			new_state = HST_PROBE_FINISHED;
  1217			reschedule = 1;
  1218			break;
  1219		}
  1220		case HST_PROBE_FINISHED:
  1221			complete(&host->probe_comp);
  1222			break;
  1223		case HST_ERROR:
  1224			/* FIXME: TODO */
  1225			break;
  1226	
  1227		default:
  1228			/* should never occur */
  1229			printk(KERN_ERR PFX "BUG: unknown state %d\n", state);
  1230			assert(0);
  1231			break;
  1232		}
  1233	
  1234		if (new_state != HST_INVALID) {
  1235			spin_lock_irqsave(&host->lock, flags);
  1236			host->state = new_state;
  1237			spin_unlock_irqrestore(&host->lock, flags);
  1238		}
  1239		if (reschedule)
  1240			schedule_work(&host->fsm_task);
  1241	}
  1242	

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all(a)lists.01.org

[-- Attachment #2: config.gz --]
[-- Type: application/gzip, Size: 36868 bytes --]