Re: [PATCH v4 7/7] block: Assign a lock_class per gendisk used for wait_for_completion()
From: Byungchul Park
Date: Wed Oct 25 2017 - 02:34:52 EST
On Wed, Oct 25, 2017 at 08:01:24AM +0200, Ingo Molnar wrote:
> Beyond the #ifdef reduction I mentioned in the other thread, there's four other
> things I noticed that need to be fixed in this patch:
>
> - Please write out 'minor' instead of the 'm' abbreviation that is meaningless.
> 'm' is only used for trivial wrappers, but this wrapper is not trivial - so
> proper canonical variable names should be used.
>
> - Since __key and __name is already double underscores that is customary for
> macros to avoid variable name shadowing, why is 'ret' not such a name?
>
> - But, 'ret' is the typical name used for integer returns, not for pointers!
> Please check the gendisk code for what the typical name for gendisk pointers
> is and use that instead of making up new, weird patterns ...
>
> - The "(complete)"#minor"("#id")" generated name is pretty bad. Firstly
> "complete" is a verb (or adjective), while lock(dep) symbol names should be
> nouns! But even "completion" is pretty opaque, how about "gendisk_completion"?
>
> More careful patches please!
I am sorry for that. I will do my best not to repeat them.
And I re-spined ASAP to make you able to review with the latest version
only including all your suggestions at the previous version.