On Tue, May 11, 2021 at 06:15:58PM +0000, Gulam Mohamed wrote:
> Problem description:
>
> During the kernel patching, customer was switching between the iscsi
> disks. To switch between the iscsi disks, it was logging out the
> currently connected iscsi disk and then logging in to the new iscsi
> disk. This was being done using a script. Customer was also using the
> "parted" command in the script to list the partition details just
> before the iscsi logout. This usage of "parted" command was creating
> an issue and we were seeing stale links of the
> disks in /sys/class/block.
>
> Analysis:
>
> As part of iscsi logout, the partitions and the disk will be removed
> in the function del_gendisk() which is done through a kworker. The
> parted command, used to list the partitions, will open the disk in
> RW mode which results in systemd-udevd re-reading the partitions. The
> ioctl used to re-read partitions is BLKRRPART. This will trigger the
> rescanning of partitions which will also delete and re-add the
> partitions. So, both iscsi logout processing (through kworker) and the
> "parted" command (through systemd-udevd) will be involved in
> add/delete of partitions. In our case, the following sequence of
> operations happened (the iscsi device is /dev/sdb with partition sdb1):
>
> 1. sdb1 was removed by PARTED
> 2. kworker, as part of iscsi logout, couldn't remove sdb1 as it was
> already removed by PARTED
> 3. sdb1 was added by parted
> 4. sdb was NOW removed as part of iscsi logout (the last part of the
> device removal after remoing the partitions)
>
> Since the symlink /sys/class/block/sdb1 points to
> /sys/class/devices/platform/hostx/sessionx/targetx:x/block/sdb/sdb1
> and since sdb is already removed, the symlink /sys/class/block/sdb1
> will be orphan and stale. So, this stale link is a result of the race
> condition in kernel between the systemd-udevd and iscsi-logout
> processing as described above. We were able to reproduce this even
> with latest upstream kernel.
>
> Fix:
>
> While Dropping/Adding partitions as part of BLKRRPART ioctl, take the
> read lock for "bdev_lookup_sem" to sync with del_gendisk().
>
> Signed-off-by: Gulam Mohamed <gulam.mohamed@xxxxxxxxxx>
> ---
> fs/block_dev.c | 15 +++++++++++++--
> 1 file changed, 13 insertions(+), 2 deletions(-)
>
> diff --git a/fs/block_dev.c b/fs/block_dev.c
> index 09d6f7229db9..e903a7edfd63 100644
> --- a/fs/block_dev.c
> +++ b/fs/block_dev.c
> @@ -1245,9 +1245,17 @@ int bdev_disk_changed(struct block_device *bdev, bool invalidate)
> lockdep_assert_held(&bdev->bd_mutex);
>
> rescan:
> + down_read(&bdev_lookup_sem);
> + if (!(disk->flags & GENHD_FL_UP)) {
> + up_read(&bdev_lookup_sem);
> + return -ENXIO;
> + }
This way might cause deadlock:
1) code path BLKRRPART:
mutex_lock(bdev->bd_mutex)
down_read(&bdev_lookup_sem);
2) del_gendisk():
down_write(&bdev_lookup_sem);
mutex_lock(&disk->part0->bd_mutex);
Given GENHD_FL_UP is only checked when opening one bdev, and
fsync_bdev() and __invalidate_device() needn't to open bdev, so
the following way may work for your issue:
diff --git a/block/genhd.c b/block/genhd.c
index 39ca97b0edc6..5eb27995d4ab 100644
--- a/block/genhd.c
+++ b/block/genhd.c
@@ -617,6 +617,7 @@ void del_gendisk(struct gendisk *disk)
mutex_lock(&disk->part0->bd_mutex);
blk_drop_partitions(disk);
+ disk->flags &= ~GENHD_FL_UP;
mutex_unlock(&disk->part0->bd_mutex);
fsync_bdev(disk->part0);
@@ -629,7 +630,6 @@ void del_gendisk(struct gendisk *disk)
remove_inode_hash(disk->part0->bd_inode);
set_capacity(disk, 0);
- disk->flags &= ~GENHD_FL_UP;
up_write(&bdev_lookup_sem);
if (!(disk->flags & GENHD_FL_HIDDEN)) {
diff --git a/fs/block_dev.c b/fs/block_dev.c
index b8abccd03e5d..06b70b8e3f67 100644
--- a/fs/block_dev.c
+++ b/fs/block_dev.c
@@ -1245,6 +1245,8 @@ int bdev_disk_changed(struct block_device *bdev, bool invalidate)
lockdep_assert_held(&bdev->bd_mutex);
rescan:
+ if(!(disk->flags & GENHD_FL_UP))
+ return -ENXIO;
if (bdev->bd_part_count)
return -EBUSY;
sync_blockdev(bdev);
Thanks,
Ming
