[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Message-ID: <3051133b-1408-2ccb-b22f-e5ee990bdc4f@linux.alibaba.com>
Date: Thu, 13 Oct 2022 09:47:11 +0800
From: JeffleXu <jefflexu@...ux.alibaba.com>
To: Jia Zhu <zhujia.zj@...edance.com>, dhowells@...hat.com,
xiang@...nel.org
Cc: linux-cachefs@...hat.com, linux-erofs@...ts.ozlabs.org,
linux-fsdevel@...r.kernel.org, linux-kernel@...r.kernel.org,
yinxin.x@...edance.com
Subject: Re: [External] Re: [PATCH 3/5] cachefiles: resend an open request if
the read request's object is closed
On 10/12/22 11:37 PM, Jia Zhu wrote:
>
>
> 在 2022/10/12 15:53, JeffleXu 写道:
>>
>>
>> On 10/11/22 9:15 PM, Jia Zhu wrote:
>>> @@ -254,12 +282,18 @@ ssize_t cachefiles_ondemand_daemon_read(struct
>>> cachefiles_cache *cache,
>>> * request distribution fair.
>>> */
>>> xa_lock(&cache->reqs);
>>> - req = xas_find_marked(&xas, UINT_MAX, CACHEFILES_REQ_NEW);
>>> - if (!req && cache->req_id_next > 0) {
>>> - xas_set(&xas, 0);
>>> - req = xas_find_marked(&xas, cache->req_id_next - 1,
>>> CACHEFILES_REQ_NEW);
>>> +retry:
>>> + xas_for_each_marked(&xas, req, xa_max, CACHEFILES_REQ_NEW) {
>>> + if (cachefiles_ondemand_skip_req(req))
>>> + continue;
>>> + break;
>>> }
>>> if (!req) {
>>> + if (cache->req_id_next > 0 && xa_max == ULONG_MAX) {
>>> + xas_set(&xas, 0);
>>> + xa_max = cache->req_id_next - 1;
>>> + goto retry;
>>> + }
>>
>> I would suggest abstracting the "xas_for_each_marked(...,
>> CACHEFILES_REQ_NEW)" part into a helper function to avoid the "goto
>> retry".
>>
> Hi JingBo,
>
> Thanks for your advice. Are the following revises appropriate?
>
> static struct cachefiles_req *cachefiles_ondemand_select_req(struct
> xa_state *xas, unsigned long xa_max)
> {
> struct cachefiles_req *req;
> struct cachefiles_ondemand_info *info;
>
> xas_for_each_marked(xas, req, xa_max, CACHEFILES_REQ_NEW) {
> if (!req || req->msg.opcode != CACHEFILES_OP_READ)
xas_for_each_marked() will guarantee that @req won't be NULL, and thus
the NULL check here in unnecessary. Otherwise LGTM.
> return req;
> info = req->object->private;
> if (info->state == CACHEFILES_ONDEMAND_OBJSTATE_close) {
> cachefiles_ondemand_set_object_reopening(req->object);
> queue_work(fscache_wq, &info->work);
> continue;
> } else if (info->state == CACHEFILES_ONDEMAND_OBJSTATE_reopening) {
> continue;
> }
> return req;
> }
> return NULL;
> }
>
> ...
>
> xa_lock(&cache->reqs);
> req = cachefiles_ondemand_select_req(&xas, ULONG_MAX);
> if (!req && cache->req_id_next > 0) {
> xas_set(&xas, 0);
> req = cachefiles_ondemand_select_req(&xas, cache->req_id_next - 1);
> }
> if (!req) {
> xa_unlock(&cache->reqs);
> return 0;
> }
>>
--
Thanks,
Jingbo
Powered by blists - more mailing lists