Re: [PATCH 2/2] block: fix accounting bug on cross partition merges

[Jens Axboe <jaxboe@fusionio.com>]

On 2011-01-05 23:16, Greg KH wrote:
> On Wed, Jan 05, 2011 at 10:38:15PM +0100, Jens Axboe wrote:
>> On 2011-01-05 21:08, Greg KH wrote:
>>> On Wed, Jan 05, 2011 at 07:46:32PM +0100, Jens Axboe wrote:
>>>> On 2011-01-05 16:58, Greg KH wrote:
>>>>> On Wed, Jan 05, 2011 at 02:55:51PM +0100, Jens Axboe wrote:
>>>>>> On 2011-01-04 22:00, Greg KH wrote:
>>>>>>> On Tue, Jan 04, 2011 at 04:55:13PM +0100, Jerome Marchand wrote:
>>>>>>>> Also add a refcount to struct hd_struct to keep the partition in
>>>>>>>> memory as long as users exist. We use kref_test_and_get() to ensure
>>>>>>>> we don't add a reference to a partition which is going away.
>>>>>>>
>>>>>>> No, don't do this, use a kref correctly and no such function should be
>>>>>>> needed.
>>>>>>>
>>>>>>>> +	} else {
>>>>>>>> +		part = disk_map_sector_rcu(rq->rq_disk, blk_rq_pos(rq));
>>>>>>>
>>>>>>> That is the function that should properly increment the reference count
>>>>>>> on the object.  If the object is "being removed", then it will return
>>>>>>> NULL and you need to check that.  Do that and you do not need to add:
>>>>>>
>>>>>> It doesn't matter if you do it in there of after the fact, since the
>>>>>> "lock" (RCU) is being held across the call. See my original suggestion
>>>>>> here:
>>>>>>
>>>>>> https://lkml.org/lkml/2010/12/17/275
>>>>>
>>>>> Ok, that's fine, just do it without adding that kref function and I have
>>>>> no objection :)
>>>>
>>>> Why? The code is perfectly fine. I originally objected to making an API
>>>> like this for simple reference counting - seems I was right. Please
>>>> actually look at the code and use. Alexey asked whether this was a toy
>>>> API or a real one, I'd like to know that as well. If this is meant just
>>>> for very basic get/put references, fine, then document that. But then
>>>> what's the point of having this API in the first place?
>>>
>>> The point is that you shouldn't have to roll your own reference count
>>> code all over the place, 99% of the time, you should just use the
>>> debugged, and documented, interface that the kernel provides with the
>>> kref interface.
>>>
>>> As for it being a "toy", it properly handles a very large majority of
>>> the kernel reference counting logic today, in a race-free manner, so I
>>> would not call that a "toy" at all.
>>
>> Dunno, then perhaps pointless. It's not like the API is saving lots of
>> typing or easier to use than just atomics, imho.
> 
> Remember what Andrew said when you complained about this last time?
> I'll paraphrase:
> 	When we see someone use an atomic value for a reference count,
> 	we then need to go audit the code to make sure they got the
> 	flushing right, and all the other little stuff you need to do in
> 	order to ensure the code works properly.
> 
> 	If you use a kref, then we know that all is correct, and we can
> 	then focus on how the kref is used (very easy to audit) and
> 	worry about the rest of the code.  It saves us time as
> 	reviewers and maintainers also.

And I agree with that. Which is even more reason to add advanced
functionality to it, instead of just bog basic stuff. It's not like
basic reference counts are black magic by any stretch of the
imagination. If you just document with kerneldoc, there's a chance that
people will actually read the documentation.

>>> Just use it properly.  As this patch series points out, adding this type
>>
>> By adding pointless locks?
> 
> No.

Then how would you suggest to use it properly, giving the context of
this thread? Did you read more of the code than just the single kref
function that was added?

>> Your suggestion of doing the referencing
>> inside the function being called is moot, since RCU is held off over the
>> call. The point of the addition to the API is to _not_ grab a reference
>> if someone has done the final put. We know that the RCU grace period has
>> not ended, so the kref is valid. But if it is going away _in the future_
>> after we drop the part lock, then we don't want a reference to it.
>>
>> So to "use it properly", I would have to slow down a fast path. No
>> thanks.
> 
> Then don't use it.  a kref is NOT for a fast path, use RCU for that.
> Heck, an atomic value is also not good for a fast path also, as it
> causes major stalls, so you might want to reconsider not even using that
> if you are thinking you should roll your own.

There's a major difference between needing a reference count in a fast
path, or needing a reference count and screwing locking just to be able
to do that.

>>> of function to the api is not a good idea, as it will be incorrect when
>>> used.
>>
>> The code is fine, the use is fine. I think the only thing we have
>> established here is that Jerome made the mistake of using the kref API
>> for this. I'll rewrite that part to handle its own references.
> 
> Yes, that is true.  If you are already using RCU, then by all means,
> don't use a kref.  There are lots of reference counts in the kernel that
> don't use a kref, nor should they.

I'll do that.

-- 
Jens Axboe