On 7/30/2015 08:36, J. Bruce Fields wrote:
> On Thu, Jul 30, 2015 at 07:48:24AM +1000, NeilBrown wrote:
>> On Wed, 29 Jul 2015 15:41:55 -0400 "J. Bruce Fields"
>> <bfields@xxxxxxxxxxxx> wrote:
>>
>>> On Wed, Jul 29, 2015 at 10:25:19AM +1000, NeilBrown wrote:
>>>> On Mon, 27 Jul 2015 11:06:53 +0800 Kinglong Mee <kinglongmee@xxxxxxxxx>
>>>> wrote:
>>>>
>>>>> Without initialized, done in fs_pin at stack space may
>>>>> contains strange value.
>>>>>
>>>>> v8, same as v3
>>>>> Adds macro for header file
>>>>>
>>>>> Signed-off-by: Kinglong Mee <kinglongmee@xxxxxxxxx>
>>>>
>>>> Reviewed-by: NeilBrown <neilb@xxxxxxxx>
>>>>
>>>> It would be really good if some of these early patches could be applied
>>>> to the relevant trees so they appear in -next and we only need to keep
>>>> reviewing the more interesting code at the end.
>>>
>>> This patch seems a little bikeshed-y. I'd rather just drop it or save
>>> it for some other day. It's not necessary to the series.
>>
>> ???
>>
>> I accept that:
>>
>>
>>>>> @@ -1,3 +1,6 @@
>>>>> +#ifndef _LINUX_FS_PIN_H
>>>>> +#define _LINUX_FS_PIN_H
>>>>> +
>>>>> #include <linux/wait.h>
>>
>> could be a little bike-shed-y, not that I've seen much bike shedding
>> going on.
>>
>> However:
>>>>>
>>>>> struct fs_pin {
>>>>> @@ -16,9 +19,12 @@ static inline void init_fs_pin(struct fs_pin *p, void (*kill)(struct fs_pin *))
>>>>> INIT_HLIST_NODE(&p->s_list);
>>>>> INIT_HLIST_NODE(&p->m_list);
>>>>> p->kill = kill;
>>>>> + p->done = 0;
>>>>> }
>>>>>
>>
>> is quite important.
>> Without that assignment we would probably need to rename the function to
>> init_most_of_fs_pin
>> or
>> init_fs_pin_if_already_zeroed
>> or maybe just
>> __init_fs_pin
>> with the time honoured interpretation that it sort-of does what the
>> name says, but maybe not exactly how you think and please use with care.
>>
>> Then in nfsd code we would need to add the assignment ourselves, or use
>> kzalloc where it would otherwise be completely unnecessary.
>
> Right, the existing users do something like kzalloc, last I checked.
> Maybe it'd be an improvement not to.
>
> I don't really care, but since Al's a bit overloaded, and you don't want
> to see this reposted, and it's not really essential to the series, why
> not drop it for now?
What's your opinion about this patch, Al?
Drop? needs update?
thanks,
Kinglong Mee
--
To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
