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@xxxxxxxxxx/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
