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

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

On Tue, Jan 07, 2020 at 07:40:10PM +0800, Hou Tao wrote:
> 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 ?

It isn't necessary to do that way, the parent disk's refcount isn't released
until device_del(part_to_dev(part)). So it is enough to hold disk's refcount
before calling device_del(part).

> 
> 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.

Yeah, that is why I added 'disk' field, which can be put with other
fields not accessed in fast path.

BTW, 'struct percpu_ref ref' should have been put just after 'nr_sects',
then these fast path fields can share single cache line.


Thanks,
Ming