linux-kernel - Re: [PATCH v1 19/23] cachefiles: implement .demand

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <99c94a78-58c4-f0af-e1d4-9aaa51bab281@linux.alibaba.com>
Date:   Wed, 12 Jan 2022 17:02:13 +0800
From:   JeffleXu <jefflexu@...ux.alibaba.com>
To:     Matthew Wilcox <willy@...radead.org>
Cc:     dhowells@...hat.com, linux-cachefs@...hat.com, xiang@...nel.org,
        chao@...nel.org, linux-erofs@...ts.ozlabs.org,
        linux-fsdevel@...r.kernel.org, joseph.qi@...ux.alibaba.com,
        bo.liu@...ux.alibaba.com, tao.peng@...ux.alibaba.com,
        gerry@...ux.alibaba.com, eguan@...ux.alibaba.com,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH v1 19/23] cachefiles: implement .demand_read() for demand
 read



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