diff mbox series

[v2,12/13] block: make __register_blkdev() return an error

Message ID 20211103122157.1215783-13-mcgrof@kernel.org (mailing list archive)
State New, archived
Headers show
Series block: add_disk() error handling stragglers | expand

Commit Message

Luis Chamberlain Nov. 3, 2021, 12:21 p.m. UTC
This makes __register_blkdev() return an error, and also changes the
probe() call to return an error as well.

We expand documentation for the probe call to ensure that if the block
device already exists we don't return on error on that condition. We do
this as otherwise we loose ability to handle concurrent requests if the
block device already existed.

Cc: Tetsuo Handa <penguin-kernel@i-love.sakura.ne.jp>
Signed-off-by: Luis Chamberlain <mcgrof@kernel.org>
---
 block/bdev.c            |  5 ++++-
 block/genhd.c           | 21 +++++++++++++++------
 drivers/block/ataflop.c | 19 ++++++++++++++-----
 drivers/block/brd.c     |  7 +++++--
 drivers/block/floppy.c  | 17 +++++++++++++----
 drivers/block/loop.c    | 11 ++++++++---
 drivers/md/md.c         | 12 +++++++++---
 drivers/scsi/sd.c       |  3 ++-
 include/linux/genhd.h   |  4 ++--
 9 files changed, 72 insertions(+), 27 deletions(-)

Comments

Christoph Hellwig Nov. 3, 2021, 4:09 p.m. UTC | #1
On Wed, Nov 03, 2021 at 05:21:56AM -0700, Luis Chamberlain wrote:
> This makes __register_blkdev() return an error, and also changes the
> probe() call to return an error as well.
> 
> We expand documentation for the probe call to ensure that if the block
> device already exists we don't return on error on that condition. We do
> this as otherwise we loose ability to handle concurrent requests if the
> block device already existed.

I'm still not really sold on this - if the probe fails no bdev will
be registered and the lookup will fail.  What is the benefit of
propagating the exact error here?
Luis Chamberlain Nov. 3, 2021, 4:44 p.m. UTC | #2
On Wed, Nov 03, 2021 at 05:09:33PM +0100, Christoph Hellwig wrote:
> On Wed, Nov 03, 2021 at 05:21:56AM -0700, Luis Chamberlain wrote:
> > This makes __register_blkdev() return an error, and also changes the
> > probe() call to return an error as well.
> > 
> > We expand documentation for the probe call to ensure that if the block
> > device already exists we don't return on error on that condition. We do
> > this as otherwise we loose ability to handle concurrent requests if the
> > block device already existed.
> 
> I'm still not really sold on this - if the probe fails no bdev will
> be registered and the lookup will fail.  What is the benefit of
> propagating the exact error here?

Here's the thing, prober call a form of add_disk(), and so do we want
to always ignore the errors on probe? If so we should document why that
is sane then. I think this approach is a bit more sane though.

  Luis
Christoph Hellwig Nov. 3, 2021, 5 p.m. UTC | #3
On Wed, Nov 03, 2021 at 09:44:53AM -0700, Luis Chamberlain wrote:
> Here's the thing, prober call a form of add_disk(), and so do we want
> to always ignore the errors on probe? If so we should document why that
> is sane then. I think this approach is a bit more sane though.

I suspect the right thing is to just kill of ->probe.

The only thing it supports is pre-devtmpfs, pre-udev semantics that
want to magically create disks when their pre-created device node
is accesses.  But if we don't remove it, yes I think not reporting
the error is best.  Just clean up whatever local resources were set
up in the ->probe method and let the open fail without the need of
passing on the actual error.
Luis Chamberlain Nov. 3, 2021, 5:03 p.m. UTC | #4
On Wed, Nov 03, 2021 at 06:00:49PM +0100, Christoph Hellwig wrote:
> On Wed, Nov 03, 2021 at 09:44:53AM -0700, Luis Chamberlain wrote:
> > Here's the thing, prober call a form of add_disk(), and so do we want
> > to always ignore the errors on probe? If so we should document why that
> > is sane then. I think this approach is a bit more sane though.
> 
> I suspect the right thing is to just kill of ->probe.
> 
> The only thing it supports is pre-devtmpfs, pre-udev semantics that
> want to magically create disks when their pre-created device node
> is accesses.

That sounds like a possible userspace impact? And so not for v5.16 for
sure.

> But if we don't remove it, yes I think not reporting
> the error is best.  Just clean up whatever local resources were set
> up in the ->probe method and let the open fail without the need of
> passing on the actual error.

Alright, I'll do that and send a final v3 for the last 2 patches.

  Luis
diff mbox series

Patch

diff --git a/block/bdev.c b/block/bdev.c
index b4dab2fb6a74..876306c6ba6e 100644
--- a/block/bdev.c
+++ b/block/bdev.c
@@ -736,10 +736,13 @@  struct block_device *blkdev_get_no_open(dev_t dev)
 {
 	struct block_device *bdev;
 	struct inode *inode;
+	int ret;
 
 	inode = ilookup(blockdev_superblock, dev);
 	if (!inode) {
-		blk_request_module(dev);
+		ret = blk_request_module(dev);
+		if (ret)
+			return NULL;
 		inode = ilookup(blockdev_superblock, dev);
 		if (!inode)
 			return NULL;
diff --git a/block/genhd.c b/block/genhd.c
index febaaa55125a..7b56767e9e32 100644
--- a/block/genhd.c
+++ b/block/genhd.c
@@ -183,7 +183,7 @@  static struct blk_major_name {
 	struct blk_major_name *next;
 	int major;
 	char name[16];
-	void (*probe)(dev_t devt);
+	int (*probe)(dev_t devt);
 } *major_names[BLKDEV_MAJOR_HASH_SIZE];
 static DEFINE_MUTEX(major_names_lock);
 static DEFINE_SPINLOCK(major_names_spinlock);
@@ -213,7 +213,13 @@  void blkdev_show(struct seq_file *seqf, off_t offset)
  * @major: the requested major device number [1..BLKDEV_MAJOR_MAX-1]. If
  *         @major = 0, try to allocate any unused major number.
  * @name: the name of the new block device as a zero terminated string
- * @probe: allback that is called on access to any minor number of @major
+ * @probe: callback that is called on access to any minor number of @major.
+ * 	This will return 0 if the block device is already present or was
+ * 	not present and it succcessfully added a new one. If the block device
+ * 	was not already present but it was a valid request an error reflecting
+ * 	the reason why adding the block device is returned. An error should not
+ * 	be returned if the block device already exists as otherwise concurrent
+ * 	requests to open an existing block device would fail.
  *
  * The @name must be unique within the system.
  *
@@ -231,7 +237,7 @@  void blkdev_show(struct seq_file *seqf, off_t offset)
  * Use register_blkdev instead for any new code.
  */
 int __register_blkdev(unsigned int major, const char *name,
-		void (*probe)(dev_t devt))
+		int (*probe)(dev_t devt))
 {
 	struct blk_major_name **n, *p;
 	int index, ret = 0;
@@ -672,17 +678,18 @@  static ssize_t disk_badblocks_store(struct device *dev,
 	return badblocks_store(disk->bb, page, len, 0);
 }
 
-void blk_request_module(dev_t devt)
+int blk_request_module(dev_t devt)
 {
 	unsigned int major = MAJOR(devt);
 	struct blk_major_name **n;
+	int err;
 
 	mutex_lock(&major_names_lock);
 	for (n = &major_names[major_to_index(major)]; *n; n = &(*n)->next) {
 		if ((*n)->major == major && (*n)->probe) {
-			(*n)->probe(devt);
+			err = (*n)->probe(devt);
 			mutex_unlock(&major_names_lock);
-			return;
+			return err;
 		}
 	}
 	mutex_unlock(&major_names_lock);
@@ -690,6 +697,8 @@  void blk_request_module(dev_t devt)
 	if (request_module("block-major-%d-%d", MAJOR(devt), MINOR(devt)) > 0)
 		/* Make old-style 2.4 aliases work */
 		request_module("block-major-%d", MAJOR(devt));
+
+	return 0;
 }
 
 /*
diff --git a/drivers/block/ataflop.c b/drivers/block/ataflop.c
index 170dd193cef6..805c2d12e358 100644
--- a/drivers/block/ataflop.c
+++ b/drivers/block/ataflop.c
@@ -2008,22 +2008,31 @@  static int ataflop_alloc_disk(unsigned int drive, unsigned int type)
 	return 0;
 }
 
-static void ataflop_probe(dev_t dev)
+static int ataflop_probe(dev_t dev)
 {
 	int drive = MINOR(dev) & 3;
 	int type  = MINOR(dev) >> 2;
+	int err = 0;
 
 	if (type)
 		type--;
 
 	if (drive >= FD_MAX_UNITS || type >= NUM_DISK_MINORS)
-		return;
+		return -EINVAL;
+
 	if (!unit[drive].disk[type]) {
-		if (ataflop_alloc_disk(drive, type) == 0) {
-			add_disk(unit[drive].disk[type]);
-			unit[drive].registered[type] = true;
+		err = ataflop_alloc_disk(drive, type);
+		if (err == 0) {
+			err = add_disk(unit[drive].disk[type]);
+			if (err) {
+				blk_cleanup_disk(unit[drive].disk[type]);
+				unit[drive].disk[type] = NULL;
+			} else
+				unit[drive].registered[type] = true;
 		}
 	}
+
+	return err;
 }
 
 static void atari_floppy_cleanup(void)
diff --git a/drivers/block/brd.c b/drivers/block/brd.c
index a896ee175d86..fa6f532035fc 100644
--- a/drivers/block/brd.c
+++ b/drivers/block/brd.c
@@ -437,9 +437,12 @@  static int brd_alloc(int i)
 	return err;
 }
 
-static void brd_probe(dev_t dev)
+static int brd_probe(dev_t dev)
 {
-	brd_alloc(MINOR(dev) / max_part);
+	int ret =  brd_alloc(MINOR(dev) / max_part);
+	if (ret == -EEXIST)
+		return 0;
+	return ret;
 }
 
 static void brd_del_one(struct brd_device *brd)
diff --git a/drivers/block/floppy.c b/drivers/block/floppy.c
index 3873e789478e..ff3422f517a6 100644
--- a/drivers/block/floppy.c
+++ b/drivers/block/floppy.c
@@ -4518,21 +4518,30 @@  static int floppy_alloc_disk(unsigned int drive, unsigned int type)
 
 static DEFINE_MUTEX(floppy_probe_lock);
 
-static void floppy_probe(dev_t dev)
+static int floppy_probe(dev_t dev)
 {
 	unsigned int drive = (MINOR(dev) & 3) | ((MINOR(dev) & 0x80) >> 5);
 	unsigned int type = (MINOR(dev) >> 2) & 0x1f;
+	int err = 0;
 
 	if (drive >= N_DRIVE || !floppy_available(drive) ||
 	    type >= ARRAY_SIZE(floppy_type))
-		return;
+		return -EINVAL;
 
 	mutex_lock(&floppy_probe_lock);
 	if (!disks[drive][type]) {
-		if (floppy_alloc_disk(drive, type) == 0)
-			add_disk(disks[drive][type]);
+		err = floppy_alloc_disk(drive, type);
+		if (err == 0) {
+			err = add_disk(disks[drive][type]);
+			if (err) {
+				blk_cleanup_disk(disks[drive][type]);
+				disks[drive][type] = NULL;
+			}
+		}
 	}
 	mutex_unlock(&floppy_probe_lock);
+
+	return err;
 }
 
 static int __init do_floppy_init(void)
diff --git a/drivers/block/loop.c b/drivers/block/loop.c
index 3c09a33fa1c7..dfc2859274a4 100644
--- a/drivers/block/loop.c
+++ b/drivers/block/loop.c
@@ -2089,13 +2089,18 @@  static void loop_remove(struct loop_device *lo)
 	kfree(lo);
 }
 
-static void loop_probe(dev_t dev)
+static int loop_probe(dev_t dev)
 {
 	int idx = MINOR(dev) >> part_shift;
+	int ret;
 
 	if (max_loop && idx >= max_loop)
-		return;
-	loop_add(idx);
+		return -EINVAL;
+	ret = loop_add(idx);
+	if (ret == -EEXIST)
+		return 0;
+
+	return ret;
 }
 
 static int loop_control_remove(int idx)
diff --git a/drivers/md/md.c b/drivers/md/md.c
index 5111ed966947..cdfabb90acb5 100644
--- a/drivers/md/md.c
+++ b/drivers/md/md.c
@@ -5737,12 +5737,18 @@  static int md_alloc(dev_t dev, char *name)
 	return error;
 }
 
-static void md_probe(dev_t dev)
+static int md_probe(dev_t dev)
 {
+	int error = 0;
+
 	if (MAJOR(dev) == MD_MAJOR && MINOR(dev) >= 512)
-		return;
+		return -EINVAL;
 	if (create_on_open)
-		md_alloc(dev, NULL);
+		error = md_alloc(dev, NULL);
+	if (error == -EEXIST)
+		return 0;
+
+	return error;
 }
 
 static int add_named_array(const char *val, const struct kernel_param *kp)
diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c
index 65875a598d62..a135d5c48829 100644
--- a/drivers/scsi/sd.c
+++ b/drivers/scsi/sd.c
@@ -632,8 +632,9 @@  static struct scsi_driver sd_template = {
  * Don't request a new module, as that could deadlock in multipath
  * environment.
  */
-static void sd_default_probe(dev_t devt)
+static int sd_default_probe(dev_t devt)
 {
+	return 0;
 }
 
 /*
diff --git a/include/linux/genhd.h b/include/linux/genhd.h
index 59eabbc3a36b..016623245725 100644
--- a/include/linux/genhd.h
+++ b/include/linux/genhd.h
@@ -290,7 +290,7 @@  struct gendisk *__blk_alloc_disk(int node, struct lock_class_key *lkclass);
 void blk_cleanup_disk(struct gendisk *disk);
 
 int __register_blkdev(unsigned int major, const char *name,
-		void (*probe)(dev_t devt));
+		int (*probe)(dev_t devt));
 #define register_blkdev(major, name) \
 	__register_blkdev(major, name, NULL)
 void unregister_blkdev(unsigned int major, const char *name);
@@ -322,7 +322,7 @@  static inline int bd_register_pending_holders(struct gendisk *disk)
 dev_t part_devt(struct gendisk *disk, u8 partno);
 void inc_diskseq(struct gendisk *disk);
 dev_t blk_lookup_devt(const char *name, int partno);
-void blk_request_module(dev_t devt);
+int blk_request_module(dev_t devt);
 #ifdef CONFIG_BLOCK
 void printk_all_partitions(void);
 #else /* CONFIG_BLOCK */