On Tue, Oct 24, 2017 at 12:15:51PM +0200, Ingo Molnar wrote:
> > @@ -1409,9 +1403,12 @@ struct gendisk *alloc_disk_node(int minors, int node_id)
> > disk_to_dev(disk)->type = &disk_type;
> > device_initialize(disk_to_dev(disk));
> > }
> > +
> > + lockdep_init_map(&disk->lockdep_map, lock_name, key, 0);
>
> lockdep_init_map() depends on CONFIG_DEBUG_LOCK_ALLOC IIRC, but the data structure
> change you made depends on CONFIG_LOCKDEP_COMPLETIONS:
OMG, my mistake! I am very sorry. I will fix it.
BTW, lockdep_init_map() seems to decide whether using lockdep_map or
ignoring it, depending on CONFIG_LOCKDEP than CONFIG_DEBUG_LOCK_ALLOC.
> > return disk;
> > }
> > -EXPORT_SYMBOL(alloc_disk_node);
> > +EXPORT_SYMBOL(__alloc_disk_node);
> >
> > struct kobject *get_disk(struct gendisk *disk)
> > {
> > diff --git a/include/linux/genhd.h b/include/linux/genhd.h
> > index 6d85a75..9832e3c 100644
> > --- a/include/linux/genhd.h
> > +++ b/include/linux/genhd.h
> > @@ -206,6 +206,9 @@ struct gendisk {
> > #endif /* CONFIG_BLK_DEV_INTEGRITY */
> > int node_id;
> > struct badblocks *bb;
> > +#ifdef CONFIG_LOCKDEP_COMPLETIONS
> > + struct lockdep_map lockdep_map;
> > +#endif
> > };
>
> Which is risking a future build failure at minimum.
>
> Isn't lockdep_map a zero size structure that is always defined? If yes then
> there's no need for an #ifdef.
No, a zero size structure for lockdep_map is not provided yet.
There are two options I can do:
1. Add a zero size structure for lockdep_map and remove #ifdef
2. Replace CONFIG_LOCKDEP_COMPLETIONS with CONFIG_LOCKDEP here.
Or something else?
Which one do you prefer?
> Also:
>
> >
> > static inline struct gendisk *part_to_disk(struct hd_struct *part)
> > @@ -590,8 +593,7 @@ extern struct hd_struct * __must_check add_partition(struct gendisk *disk,
> > extern void delete_partition(struct gendisk *, int);
> > extern void printk_all_partitions(void);
> >
> > -extern struct gendisk *alloc_disk_node(int minors, int node_id);
> > -extern struct gendisk *alloc_disk(int minors);
> > +extern struct gendisk *__alloc_disk_node(int minors, int node_id, struct lock_class_key *key, const char *lock_name);
> > extern struct kobject *get_disk(struct gendisk *disk);
> > extern void put_disk(struct gendisk *disk);
> > extern void blk_register_region(dev_t devt, unsigned long range,
> > @@ -615,6 +617,22 @@ extern ssize_t part_fail_store(struct device *dev,
> > const char *buf, size_t count);
> > #endif /* CONFIG_FAIL_MAKE_REQUEST */
> >
> > +#ifdef CONFIG_LOCKDEP_COMPLETIONS
> > +#define alloc_disk_node(m, id) \
> > +({ \
> > + static struct lock_class_key __key; \
> > + const char *__lock_name; \
> > + \
> > + __lock_name = "(complete)"#m"("#id")"; \
> > + \
> > + __alloc_disk_node(m, id, &__key, __lock_name); \
> > +})
> > +#else
> > +#define alloc_disk_node(m, id) __alloc_disk_node(m, id, NULL, NULL)
> > +#endif
> > +
> > +#define alloc_disk(m) alloc_disk_node(m, NUMA_NO_NODE)
> > +
> > static inline int hd_ref_init(struct hd_struct *part)
> > {
> > if (percpu_ref_init(&part->ref, __delete_partition, 0,
>
> Why is the lockdep_map passed in to the init function? Since it's wrapped in an
> ugly fashion anyway, why not introduce a clean inline function that calls
This is the way workqueue adopted for that purpose. BTW, can I make
a lock_class_key distinguishable from another of a different gendisk,
using inline function?
> lockdep_init_map() on the returned structure.
Ok. I will make it work on the returned structre instead of passing it.
> No #ifdefs required, and no uglification of the alloc_disk_node() interface.
Ok. I will remove this #ifdef.
Thank you very much.
