Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <029453d7-1b2d-3d3c-cd8e-64f5be4420bf@bytedance.com>
Date:   Wed, 29 Mar 2023 19:42:47 +0800
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.15; rv:102.0)
 Gecko/20100101 Thunderbird/102.9.0
Subject: Re: Re: [PATCH V4 3/5] cachefiles: resend an open request if the read
 request's object is closed
To:     David Howells <dhowells@redhat.com>
Cc:     linux-cachefs@redhat.com, linux-erofs@lists.ozlabs.org,
        linux-kernel@vger.kernel.org, Xin Yin <yinxin.x@bytedance.com>,
        Jingbo Xu <jefflexu@linux.alibaba.com>, zhujia.zj@bytedance.com
References: <20230111052515.53941-4-zhujia.zj@bytedance.com>
 <20230111052515.53941-1-zhujia.zj@bytedance.com>
 <132777.1680012744@warthog.procyon.org.uk>
From:   Jia Zhu <zhujia.zj@bytedance.com>
In-Reply-To: <132777.1680012744@warthog.procyon.org.uk>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 8bit
Precedence: bulk


在 2023/3/28 22:12, David Howells 写道:
> Jia Zhu <zhujia.zj@bytedance.com> wrote:
> 
>> +	struct cachefiles_object *object =
>> +		((struct cachefiles_ondemand_info *)work)->object;
> 
> container_of().
Thanks, will revise it.
> 
>> +			continue;
>> +		} else if (cachefiles_ondemand_object_is_reopening(object)) {
> 
> The "else" is unnecessary.
Will remove it.
> 
>> +static void ondemand_object_worker(struct work_struct *work)
>> +{
>> +	struct cachefiles_object *object =
>> +		((struct cachefiles_ondemand_info *)work)->object;
>> +
>> +	cachefiles_ondemand_init_object(object);
>> +}
> 
> I can't help but feel there's some missing exclusion/locking.  

It's indeed kind of complicated here since the async operation.
Thus we paid much attention to catching the race scenarios during coding
and reviewing.

Here are several corner case have been considered:

1. Don't repeatedly push the @work of same object into workqueue:
Use <reopening> state to represent this object. Once the object is
set to <reopening> atomicly, which means the work has been pushed to
workqueue. And other concurrent threads will not pick the <reopening>
object to workqueue.

2. Don't repeatedly set <reopening> state for the same object:
Hold the xa_lock during searching reqs and setting it to <reopening>.
Once object is set to <reopening>, the same object will be skipped.

3. etc.

Would you mind providing more hints for this issue?

> This feels like
> it really ought to be driven from the fscache object state machine.

It's a great idea. But the problem is if we add a new state to indicate
this reopening status and use fscache state machine to drive the cookie
to do reopen(), thus reopen() (in fscache module) ought to invoke
cachefiles_ondemand_init_object() (in cachefile module) to require user
daemon to open the backend file.
But it seems that fscache module should not depend on cachefiles module.
>