Re: [PATCH V1 1/1] Fix race between iscsi logout and systemd-udevd

[Ming Lei <ming.lei@redhat.com>]

On Wed, May 12, 2021 at 08:35:05AM +0200, Christoph Hellwig wrote:
> On Wed, May 12, 2021 at 11:23:59AM +0800, Ming Lei wrote:
> > 
> > 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:
> 
> If we move the clearing of GENHD_FL_UP earlier we can do away with
> bdev_lookup_sem entirely I think.  Something like this untested patch:
> 
> diff --git a/block/genhd.c b/block/genhd.c
> index a5847560719c..ef717084b343 100644
> --- a/block/genhd.c
> +++ b/block/genhd.c
> @@ -29,8 +29,6 @@
>  
>  static struct kobject *block_depr;
>  
> -DECLARE_RWSEM(bdev_lookup_sem);
> -
>  /* for extended dynamic devt allocation, currently only one major is used */
>  #define NR_EXT_DEVT		(1 << MINORBITS)
>  static DEFINE_IDA(ext_devt_ida);
> @@ -609,13 +607,8 @@ void del_gendisk(struct gendisk *disk)
>  	blk_integrity_del(disk);
>  	disk_del_events(disk);
>  
> -	/*
> -	 * Block lookups of the disk until all bdevs are unhashed and the
> -	 * disk is marked as dead (GENHD_FL_UP cleared).
> -	 */
> -	down_write(&bdev_lookup_sem);
> -
>  	mutex_lock(&disk->open_mutex);
> +	disk->flags &= ~GENHD_FL_UP;
>  	blk_drop_partitions(disk);
>  	mutex_unlock(&disk->open_mutex);
>  
> @@ -627,10 +620,7 @@ void del_gendisk(struct gendisk *disk)
>  	 * up any more even if openers still hold references to it.
>  	 */
>  	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)) {
>  		sysfs_remove_link(&disk_to_dev(disk)->kobj, "bdi");
> diff --git a/fs/block_dev.c b/fs/block_dev.c
> index 8dd8e2fd1401..bde23940190f 100644
> --- a/fs/block_dev.c
> +++ b/fs/block_dev.c
> @@ -1377,33 +1377,24 @@ struct block_device *blkdev_get_no_open(dev_t dev)
>  	struct block_device *bdev;
>  	struct gendisk *disk;
>  
> -	down_read(&bdev_lookup_sem);
>  	bdev = bdget(dev);
>  	if (!bdev) {
> -		up_read(&bdev_lookup_sem);
>  		blk_request_module(dev);
> -		down_read(&bdev_lookup_sem);
> -
>  		bdev = bdget(dev);
>  		if (!bdev)
> -			goto unlock;
> +			return NULL;
>  	}
>  
>  	disk = bdev->bd_disk;
>  	if (!kobject_get_unless_zero(&disk_to_dev(disk)->kobj))
>  		goto bdput;
> -	if ((disk->flags & (GENHD_FL_UP | GENHD_FL_HIDDEN)) != GENHD_FL_UP)
> -		goto put_disk;
>  	if (!try_module_get(bdev->bd_disk->fops->owner))
>  		goto put_disk;
> -	up_read(&bdev_lookup_sem);
>  	return bdev;
>  put_disk:
>  	put_disk(disk);
>  bdput:
>  	bdput(bdev);
> -unlock:
> -	up_read(&bdev_lookup_sem);
>  	return NULL;
>  }
>  
> @@ -1462,7 +1453,10 @@ struct block_device *blkdev_get_by_dev(dev_t dev, fmode_t mode, void *holder)
>  
>  	disk_block_events(disk);
>  
> +	ret = -ENXIO;
>  	mutex_lock(&disk->open_mutex);
> +	if ((disk->flags & (GENHD_FL_UP | GENHD_FL_HIDDEN)) != GENHD_FL_UP)
> +		goto abort_claiming;
>  	if (bdev_is_partition(bdev))
>  		ret = blkdev_get_part(bdev, mode);
>  	else

This patch looks fine, and new openers can be prevented really with help
of ->open_mutex.

Thanks, 
Ming