From b9c3040b10b60350ece42aed8c29972031fb14a0 Mon Sep 17 00:00:00 2001 From: Rob N Date: Sat, 30 Mar 2024 08:51:33 +1100 Subject: [PATCH] vdev_disk: clean up spa/bdev mode conversion 43e8f6e37 introduced a subtle API misuse, in that it passed the output from vdev_bdev_mode() back into itself. Fortunately, the SPA_MODE_(READ|WRITE) bit values exactly map to the FMODE_(READ|WRITE) & BLK_OPEN_(READ|WRITE) bit values, so it didn't result in a bug, but it was hard to read and understand, so I cleaned it up. In doing so, I noticed that the only call to vdev_bdev_mode() without the "exclusive" flag set was in that misuse, and actually, we never do a non-exclusive blkdev_get_by_path(). So I've just made exclusive be always-on. Sponsored-by: Klara, Inc. Sponsored-by: Wasabi Technology, Inc. Reviewed by: Brian Behlendorf Reviewed-by: Allan Jude Signed-off-by: Rob Norris Closes #15995 --- module/os/linux/zfs/vdev_disk.c | 81 ++++++++++++++++----------------- 1 file changed, 39 insertions(+), 42 deletions(-) diff --git a/module/os/linux/zfs/vdev_disk.c b/module/os/linux/zfs/vdev_disk.c index 223b41068..35e2a573f 100644 --- a/module/os/linux/zfs/vdev_disk.c +++ b/module/os/linux/zfs/vdev_disk.c @@ -97,38 +97,41 @@ static uint_t zfs_vdev_open_timeout_ms = 1000; static unsigned int zfs_vdev_failfast_mask = 1; +/* + * Convert SPA mode flags into bdev open mode flags. + */ #ifdef HAVE_BLK_MODE_T -static blk_mode_t +typedef blk_mode_t vdev_bdev_mode_t; +#define VDEV_BDEV_MODE_READ BLK_OPEN_READ +#define VDEV_BDEV_MODE_WRITE BLK_OPEN_WRITE +#define VDEV_BDEV_MODE_EXCL BLK_OPEN_EXCL +#define VDEV_BDEV_MODE_MASK (BLK_OPEN_READ|BLK_OPEN_WRITE|BLK_OPEN_EXCL) #else -static fmode_t +typedef fmode_t vdev_bdev_mode_t; +#define VDEV_BDEV_MODE_READ FMODE_READ +#define VDEV_BDEV_MODE_WRITE FMODE_WRITE +#define VDEV_BDEV_MODE_EXCL FMODE_EXCL +#define VDEV_BDEV_MODE_MASK (FMODE_READ|FMODE_WRITE|FMODE_EXCL) #endif -vdev_bdev_mode(spa_mode_t spa_mode, boolean_t exclusive) + +static vdev_bdev_mode_t +vdev_bdev_mode(spa_mode_t smode) { -#ifdef HAVE_BLK_MODE_T - blk_mode_t mode = 0; + ASSERT3U(smode, !=, SPA_MODE_UNINIT); + ASSERT0(smode & ~(SPA_MODE_READ|SPA_MODE_WRITE)); - if (spa_mode & SPA_MODE_READ) - mode |= BLK_OPEN_READ; + vdev_bdev_mode_t bmode = VDEV_BDEV_MODE_EXCL; - if (spa_mode & SPA_MODE_WRITE) - mode |= BLK_OPEN_WRITE; + if (smode & SPA_MODE_READ) + bmode |= VDEV_BDEV_MODE_READ; - if (exclusive) - mode |= BLK_OPEN_EXCL; -#else - fmode_t mode = 0; + if (smode & SPA_MODE_WRITE) + bmode |= VDEV_BDEV_MODE_WRITE; - if (spa_mode & SPA_MODE_READ) - mode |= FMODE_READ; + ASSERT(bmode & VDEV_BDEV_MODE_MASK); + ASSERT0(bmode & ~VDEV_BDEV_MODE_MASK); - if (spa_mode & SPA_MODE_WRITE) - mode |= FMODE_WRITE; - - if (exclusive) - mode |= FMODE_EXCL; -#endif - - return (mode); + return (bmode); } /* @@ -235,30 +238,28 @@ vdev_disk_kobj_evt_post(vdev_t *v) } static zfs_bdev_handle_t * -vdev_blkdev_get_by_path(const char *path, spa_mode_t mode, void *holder) +vdev_blkdev_get_by_path(const char *path, spa_mode_t smode, void *holder) { + vdev_bdev_mode_t bmode = vdev_bdev_mode(smode); + #if defined(HAVE_BDEV_OPEN_BY_PATH) - return (bdev_open_by_path(path, - vdev_bdev_mode(mode, B_TRUE), holder, NULL)); + return (bdev_open_by_path(path, bmode, holder, NULL)); #elif defined(HAVE_BLKDEV_GET_BY_PATH_4ARG) - return (blkdev_get_by_path(path, - vdev_bdev_mode(mode, B_TRUE), holder, NULL)); + return (blkdev_get_by_path(path, bmode, holder, NULL)); #else - return (blkdev_get_by_path(path, - vdev_bdev_mode(mode, B_TRUE), holder)); + return (blkdev_get_by_path(path, bmode, holder)); #endif } static void -vdev_blkdev_put(zfs_bdev_handle_t *bdh, spa_mode_t mode, void *holder) +vdev_blkdev_put(zfs_bdev_handle_t *bdh, spa_mode_t smode, void *holder) { #if defined(HAVE_BDEV_RELEASE) return (bdev_release(bdh)); #elif defined(HAVE_BLKDEV_PUT_HOLDER) return (blkdev_put(BDH_BDEV(bdh), holder)); #else - return (blkdev_put(BDH_BDEV(bdh), - vdev_bdev_mode(mode, B_TRUE))); + return (blkdev_put(BDH_BDEV(bdh), vdev_bdev_mode(smode))); #endif } @@ -267,11 +268,7 @@ vdev_disk_open(vdev_t *v, uint64_t *psize, uint64_t *max_psize, uint64_t *logical_ashift, uint64_t *physical_ashift) { zfs_bdev_handle_t *bdh; -#ifdef HAVE_BLK_MODE_T - blk_mode_t mode = vdev_bdev_mode(spa_mode(v->vdev_spa), B_FALSE); -#else - fmode_t mode = vdev_bdev_mode(spa_mode(v->vdev_spa), B_FALSE); -#endif + spa_mode_t smode = spa_mode(v->vdev_spa); hrtime_t timeout = MSEC2NSEC(zfs_vdev_open_timeout_ms); vdev_disk_t *vd; @@ -322,16 +319,16 @@ vdev_disk_open(vdev_t *v, uint64_t *psize, uint64_t *max_psize, reread_part = B_TRUE; } - vdev_blkdev_put(bdh, mode, zfs_vdev_holder); + vdev_blkdev_put(bdh, smode, zfs_vdev_holder); } if (reread_part) { - bdh = vdev_blkdev_get_by_path(disk_name, mode, + bdh = vdev_blkdev_get_by_path(disk_name, smode, zfs_vdev_holder); if (!BDH_IS_ERR(bdh)) { int error = vdev_bdev_reread_part(BDH_BDEV(bdh)); - vdev_blkdev_put(bdh, mode, zfs_vdev_holder); + vdev_blkdev_put(bdh, smode, zfs_vdev_holder); if (error == 0) { timeout = MSEC2NSEC( zfs_vdev_open_timeout_ms * 2); @@ -376,7 +373,7 @@ vdev_disk_open(vdev_t *v, uint64_t *psize, uint64_t *max_psize, hrtime_t start = gethrtime(); bdh = BDH_ERR_PTR(-ENXIO); while (BDH_IS_ERR(bdh) && ((gethrtime() - start) < timeout)) { - bdh = vdev_blkdev_get_by_path(v->vdev_path, mode, + bdh = vdev_blkdev_get_by_path(v->vdev_path, smode, zfs_vdev_holder); if (unlikely(BDH_PTR_ERR(bdh) == -ENOENT)) { /*