Re: [PATCH v1 19/23] cachefiles: implement .demand_read() for demand read

[JeffleXu <jefflexu@linux.alibaba.com>]

On 12/28/21 8:33 PM, JeffleXu wrote:
> 
> 
> On 12/27/21 11:36 PM, Matthew Wilcox wrote:
>> On Mon, Dec 27, 2021 at 08:54:40PM +0800, Jeffle Xu wrote:
>>> +	spin_lock(&cache->reqs_lock);
>>> +	ret = idr_alloc(&cache->reqs, req, 0, 0, GFP_KERNEL);
>>
>> GFP_KERNEL while holding a spinlock?
> 
> Right. Thanks for pointing it out.
> 
>>
>> You should be using an XArray instead of an IDR in new code anyway.
>>
> 
> Regards.
> 

Hi Matthew,

I'm afraid IDR can't be replaced by xarray here. Because we need an 'ID'
for each pending read request, so that after fetching data from remote,
user daemon could notify kernel which read request has finished by this
'ID'.

Currently this 'ID' is get from idr_alloc(), and actually identifies the
position of corresponding read request inside the IDR tree. I can't find
similar API of xarray implementing similar function, i.e., returning an
'ID'.

As for the issue of GFP_KERNEL while holding a spinlock, I'm going to
fix this with idr_preload(), somehing like

+       idr_preload(GFP_KERNEL);
+       idr_lock(&cache->reqs);
+
+       ret = idr_alloc(&cache->reqs, req, 0, 0, GFP_ATOMIC);
+       if (ret >= 0)
+               req->req_in.id = ret;
+
+       idr_unlock(&cache->reqs);
+       idr_preload_end();

Please correct me if I'm wrong.

-- 
Thanks,
Jeffle