Re: [PATCH] block: make sure last_lookup set as NULL after part deleted

[Hou Tao <houtao1@huawei.com>]

Hi,

On 2020/1/6 18:05, Ming Lei wrote:
> On Mon, Jan 06, 2020 at 05:41:45PM +0800, Hou Tao wrote:
>> Hi,
>>
[snipped]

>> Yes. The solution you proposed also adds an invocation of percpu_ref_tryget_live()
>> in the fast path. Not sure which one will have a better performance. However the
>> reason we prefer the index caching is the simplicity instead of performance.
> 
> No, hd_struct_try_get() and hd_struct_get() is always called once for one IO, the
> patch I proposed changes nothing about this usage.
> 
> Please take a close look at the patch:
> 
> https://lore.kernel.org/linux-block/5cc465cc-d68c-088e-0729-2695279c7853@huawei.com/T/#m8f3e6b4e77eadf006ce142a84c966f50f3a9ae26
> 
> which just moves hd_struct_try_get() from blk_account_io_start() into
> disk_map_sector_rcu(), doesn't it?
> 
Yes, you are right. And a little suggestion for your patch:

@@ -283,8 +289,9 @@ void delete_partition(struct gendisk *disk, int partno)
 	if (!part)
 		return;

+	get_device(disk_to_dev(disk));
 	rcu_assign_pointer(ptbl->part[partno], NULL);
-	rcu_assign_pointer(ptbl->last_lookup, NULL);
+
 	kobject_put(part->holder_dir);
 	device_del(part_to_dev(part));

Could we move the call of get_device() into add_partition, and that will make the assignment of
disk to p->disk in add_partition() be natural ?

Maybe there is no need to add a new disk field in hd_struct, because the kobject of gendisk
is already the parent of hd_struct. But make use of part->__dev.parent after the calling
of device_del() is a bad_idea.

Regards,
Tao


> 
> Thanks,
> Ming
> 
> 
> .
>