Re: [PATCH v2 0/3] ata,sd: Fix reading concurrent positioning ranges

[Damien Le Moal <damien.lemoal@opensource.wdc.com>]

On 6/3/22 14:23, Christoph Hellwig wrote:
> On Fri, Jun 03, 2022 at 10:30:04AM +0900, Damien Le Moal wrote:
>> Looks all good to me. I tested this and really wonder how I did not catch
>> these mistakes earlier :)
>>
>> Using a tcmu emulator for various concurrent positioning range configs to
>> test, I got a lockdep splat when unplugging the drive:
> 
> You probably want something like this:
> 
> ---
> From 4340b85be3532149310326b5f0caf329e1f4c748 Mon Sep 17 00:00:00 2001
> From: Christoph Hellwig <hch@lst.de>
> Date: Mon, 23 May 2022 09:18:44 +0200
> Subject: block: don't take sysfs_lock in blk_ia_range_sysfs_show
> 
> sysfs already synchronizes internally against kobject removal, so remove
> the extra lock.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---
>  block/blk-ia-ranges.c  | 8 +-------
>  include/linux/blkdev.h | 1 -
>  2 files changed, 1 insertion(+), 8 deletions(-)
> 
> diff --git a/block/blk-ia-ranges.c b/block/blk-ia-ranges.c
> index 853be76b9808b..e9e7ebf02737f 100644
> --- a/block/blk-ia-ranges.c
> +++ b/block/blk-ia-ranges.c
> @@ -54,13 +54,8 @@ static ssize_t blk_ia_range_sysfs_show(struct kobject *kobj,
>  		container_of(attr, struct blk_ia_range_sysfs_entry, attr);
>  	struct blk_independent_access_range *iar =
>  		container_of(kobj, struct blk_independent_access_range, kobj);
> -	ssize_t ret;
>  
> -	mutex_lock(&iar->queue->sysfs_lock);
> -	ret = entry->show(iar, buf);
> -	mutex_unlock(&iar->queue->sysfs_lock);
> -
> -	return ret;
> +	return entry->show(iar, buf);

I sent a patch that Jens applied doing exactly that. But forgot to add the
removal of the queue pointer you have below. Sending an incremental patch
for that.

>  }
>  
>  static const struct sysfs_ops blk_ia_range_sysfs_ops = {
> @@ -149,7 +144,6 @@ int disk_register_independent_access_ranges(struct gendisk *disk,
>  	}
>  
>  	for (i = 0; i < iars->nr_ia_ranges; i++) {
> -		iars->ia_range[i].queue = q;
>  		ret = kobject_init_and_add(&iars->ia_range[i].kobj,
>  					   &blk_ia_range_ktype, &iars->kobj,
>  					   "%d", i);
> diff --git a/include/linux/blkdev.h b/include/linux/blkdev.h
> index a72203ed25454..0ceb85ca52af4 100644
> --- a/include/linux/blkdev.h
> +++ b/include/linux/blkdev.h
> @@ -348,7 +348,6 @@ static inline int blkdev_zone_mgmt_ioctl(struct block_device *bdev,
>   */
>  struct blk_independent_access_range {
>  	struct kobject		kobj;
> -	struct request_queue	*queue;
>  	sector_t		sector;
>  	sector_t		nr_sectors;
>  };


-- 
Damien Le Moal
Western Digital Research