[Devel] [PATCH] fs/fuse: move FUSE_S_FAIL_IMMEDIATELY check before kio req send

[Pavel Butsykin]

Yes, I missed this synchronization idea, the check and list_add should
be together, will fix.

On 24.01.2019 11:45, Kirill Tkhai wrote:
> On 23.01.2019 20:22, Pavel Butsykin wrote:
>>
>> 23.01.2019 16:55, Kirill Tkhai пишет:
>>> On 23.01.2019 14:49, Pavel Butsykin wrote:
>>>> Fuse file with FUSE_S_FAIL_IMMEDIATELY state should not allow to execute new
>>>> requests. But in case of kio requests it doesn't work because the status check
>>>> is located behind kio.op->req_send(). To fix this let's move the status check
>>>> before kio.op->req_send().
>>>>
>>>> Note: We can drop hunk with req->end(fc, req) in __fuse_request_send() because
>>>> it was only needed to clenup kio setattr request after pcs_kio_setattr_handle().
>>>>
>>>> Signed-off-by: Pavel Butsykin <pbutsykin at virtuozzo.com>
>>> Why is this safe?
>>>
>>> After you move the check out of fc->lock, it becomes racy, and fuse_invalidate_files()
>>> may become work not as expected.
>>
>> test_bit is atomic operation. Which type of race do you mean?
> 
> fuse_request_send_background()                                 fuse_invalidate_files()
>    test_bit(FUSE_S_FAIL_IMMEDIATELY, &req->ff->ff_state))
> 
>                                                                   spin_lock(&fc->lock);
>                                                                   list_for_each_entry(ff, &fi->rw_files, rw_entry)
>                                                                     set_bit(FUSE_S_FAIL_IMMEDIATELY, &ff->ff_state);
>                                                                   spin_unlock(&fc->lock);
> 
>                                                                   spin_lock(&fc->lock);
>                                                                   fuse_kill_requests(fc, inode, &fc->bg_queue);
>                                                                   spin_unlock(&fc->lock);
> 
> 
>    spin_lock(&fc->lock);
>    if (fc->connected) {
>      fuse_request_send_background_locked(fc, req); <-- queuing request after fuse_invalidate_files() thinks that
>                                                        requests for all immediate files already killed.
> 
>