Re: [PATCH 1/2] Don't merge different partition's IOs

[Jens Axboe <jaxboe@fusionio.com>]

On 2010-12-10 17:55, Vivek Goyal wrote:
> On Fri, Dec 10, 2010 at 05:12:04PM +0100, Jerome Marchand wrote:
>> On 12/08/2010 03:46 PM, Jens Axboe wrote:
>>>
>>> No, that's not it at all. What I mean (and what was reverted) was
>>> caching the partition lookup, and using that for the stats. The problem
>>> with that approach turned out to be the elevator queiscing logic not
>>> being fully correct. One easier way to fix that would be to reference
>>> count the part stats, instead of having to drain the queue.
>>>
>>
>> The partition is already deleted in a rcu callback function. If I
>> understand RCU correctly, we just need to use rcu_dereference() each time
>> we use rq->part. Something like the following *untested* patch.
> 
> Jerome,
> 
> I don't think that rcu_dereference() is going to solve the problem. The
> partition table will be freed as soon as one rcu period is over. So to
> make sure partition table is not freed one has to be holding
> rcu_read_lock(). It is not a good idea to keep rcu period going till
> request finishes so a better idea will to to reference count it.

Exactly. The only change you would do to partition handling is ensure
that each io grabs a reference to it and drops it at the end. You need
not even do that in the core bits outside each IO, we just need to
ensure that the partition struct persists in memory even if it is no
longer the valid partition table. The rcu call to free the memory would
happen when the ref drops to zero.

What Vivek says it completely correct, and rcu_dereference() would only
help if you also had rcu_read_lock() over each IO. That is not feasible
at all.


-- 
Jens Axboe