[1/2] fs/fuse kio: add FUSE_S_FAIL_IMMEDIATELY check in pcs_fuse_submit()

Submitted by Pavel Butsykin on Jan. 24, 2019, 1:12 p.m.

Details

Message ID 20190124131245.29372-1-pbutsykin@virtuozzo.com
State New
Series "Series without cover letter"
Headers show

Commit Message

Pavel Butsykin Jan. 24, 2019, 1:12 p.m.
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 add the status check
in pcs_fuse_submit().

Signed-off-by: Pavel Butsykin <pbutsykin@virtuozzo.com>
---
Note:
applies on top of "[PATCH v2] fs/fuse kio: invalidate files for kio"

 fs/fuse/kio/pcs/pcs_fuse_kdirect.c | 5 +++++
 1 file changed, 5 insertions(+)

Patch hide | download patch | download mbox

diff --git a/fs/fuse/kio/pcs/pcs_fuse_kdirect.c b/fs/fuse/kio/pcs/pcs_fuse_kdirect.c
index da4b5fba03fb..3ca1ce2d6bd5 100644
--- a/fs/fuse/kio/pcs/pcs_fuse_kdirect.c
+++ b/fs/fuse/kio/pcs/pcs_fuse_kdirect.c
@@ -986,6 +986,11 @@  error:
 
 submit:
 	spin_lock(&di->kq_lock);
+	if (req->ff && test_bit(FUSE_S_FAIL_IMMEDIATELY, &req->ff->ff_state)) {
+		spin_unlock(&di->kq_lock);
+		req->out.h.error = -EIO;
+		goto error;
+	}
 	list_add_tail(&req->list, &di->kq);
 	spin_unlock(&di->kq_lock);
 

Comments

Kirill Tkhai Feb. 1, 2019, 2:24 p.m.
On 24.01.2019 16:12, 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 add the status check
> in pcs_fuse_submit().
> 
> Signed-off-by: Pavel Butsykin <pbutsykin@virtuozzo.com>
> ---
> Note:
> applies on top of "[PATCH v2] fs/fuse kio: invalidate files for kio"
> 
>  fs/fuse/kio/pcs/pcs_fuse_kdirect.c | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/fs/fuse/kio/pcs/pcs_fuse_kdirect.c b/fs/fuse/kio/pcs/pcs_fuse_kdirect.c
> index da4b5fba03fb..3ca1ce2d6bd5 100644
> --- a/fs/fuse/kio/pcs/pcs_fuse_kdirect.c
> +++ b/fs/fuse/kio/pcs/pcs_fuse_kdirect.c
> @@ -986,6 +986,11 @@ error:
>  
>  submit:
>  	spin_lock(&di->kq_lock);
> +	if (req->ff && test_bit(FUSE_S_FAIL_IMMEDIATELY, &req->ff->ff_state)) {

FUSE_S_FAIL_IMMEDIATELY is set under fc->lock, while it's checked under kq_lock.
How do you guarantee visibility?

> +		spin_unlock(&di->kq_lock);
> +		req->out.h.error = -EIO;
> +		goto error;
> +	}
>  	list_add_tail(&req->list, &di->kq);
>  	spin_unlock(&di->kq_lock);
>  
>
Pavel Butsykin Feb. 1, 2019, 3:13 p.m.
On 01.02.2019 17:24, Kirill Tkhai wrote:
> On 24.01.2019 16:12, 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 add the status check
>> in pcs_fuse_submit().
>>
>> Signed-off-by: Pavel Butsykin <pbutsykin@virtuozzo.com>
>> ---
>> Note:
>> applies on top of "[PATCH v2] fs/fuse kio: invalidate files for kio"
>>
>>   fs/fuse/kio/pcs/pcs_fuse_kdirect.c | 5 +++++
>>   1 file changed, 5 insertions(+)
>>
>> diff --git a/fs/fuse/kio/pcs/pcs_fuse_kdirect.c b/fs/fuse/kio/pcs/pcs_fuse_kdirect.c
>> index da4b5fba03fb..3ca1ce2d6bd5 100644
>> --- a/fs/fuse/kio/pcs/pcs_fuse_kdirect.c
>> +++ b/fs/fuse/kio/pcs/pcs_fuse_kdirect.c
>> @@ -986,6 +986,11 @@ error:
>>   
>>   submit:
>>   	spin_lock(&di->kq_lock);
>> +	if (req->ff && test_bit(FUSE_S_FAIL_IMMEDIATELY, &req->ff->ff_state)) {
> 
> FUSE_S_FAIL_IMMEDIATELY is set under fc->lock, while it's checked under kq_lock.

Actually this lock is needed to protect listing fi->rw_files, not for
set_bit.

> How do you guarantee visibility?

It's atomic operation, so I should just guarantee
synchronization with fuse_invalidate_files() which is carried out by
set_bit(FUSE_S_FAIL_IMMEDIATELY, ) + kpcs_kill_requests() and the locked
check.

> 
>> +		spin_unlock(&di->kq_lock);
>> +		req->out.h.error = -EIO;
>> +		goto error;
>> +	}
>>   	list_add_tail(&req->list, &di->kq);
>>   	spin_unlock(&di->kq_lock);
>>   
>>
Pavel Butsykin Feb. 1, 2019, 3:21 p.m.
On 01.02.2019 18:13, Pavel Butsykin wrote:
> 
> 
> On 01.02.2019 17:24, Kirill Tkhai wrote:
>> On 24.01.2019 16:12, 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 add the 
>>> status check
>>> in pcs_fuse_submit().
>>>
>>> Signed-off-by: Pavel Butsykin <pbutsykin@virtuozzo.com>
>>> ---
>>> Note:
>>> applies on top of "[PATCH v2] fs/fuse kio: invalidate files for kio"
>>>
>>>   fs/fuse/kio/pcs/pcs_fuse_kdirect.c | 5 +++++
>>>   1 file changed, 5 insertions(+)
>>>
>>> diff --git a/fs/fuse/kio/pcs/pcs_fuse_kdirect.c 
>>> b/fs/fuse/kio/pcs/pcs_fuse_kdirect.c
>>> index da4b5fba03fb..3ca1ce2d6bd5 100644
>>> --- a/fs/fuse/kio/pcs/pcs_fuse_kdirect.c
>>> +++ b/fs/fuse/kio/pcs/pcs_fuse_kdirect.c
>>> @@ -986,6 +986,11 @@ error:
>>>   submit:
>>>       spin_lock(&di->kq_lock);
>>> +    if (req->ff && test_bit(FUSE_S_FAIL_IMMEDIATELY, 
>>> &req->ff->ff_state)) {
>>
>> FUSE_S_FAIL_IMMEDIATELY is set under fc->lock, while it's checked 
>> under kq_lock.
> 
> Actually this lock is needed to protect listing fi->rw_files, not for
> set_bit.
> 
>> How do you guarantee visibility?
> 
> It's atomic operation, so I should just guarantee
> synchronization with fuse_invalidate_files() which is carried out by
> set_bit(FUSE_S_FAIL_IMMEDIATELY, ) + kpcs_kill_requests() and the locked
> check.

But perhaps for architectures "not x86" need barriers.

>>
>>> +        spin_unlock(&di->kq_lock);
>>> +        req->out.h.error = -EIO;
>>> +        goto error;
>>> +    }
>>>       list_add_tail(&req->list, &di->kq);
>>>       spin_unlock(&di->kq_lock);
>>>
Kirill Tkhai Feb. 1, 2019, 3:25 p.m.
On 24.01.2019 16:12, 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 add the status check
> in pcs_fuse_submit().
> 
> Signed-off-by: Pavel Butsykin <pbutsykin@virtuozzo.com>

Reviewed-by: Kirill Tkhai <ktkhai@virtuozzo.com>

> ---
> Note:
> applies on top of "[PATCH v2] fs/fuse kio: invalidate files for kio"
> 
>  fs/fuse/kio/pcs/pcs_fuse_kdirect.c | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/fs/fuse/kio/pcs/pcs_fuse_kdirect.c b/fs/fuse/kio/pcs/pcs_fuse_kdirect.c
> index da4b5fba03fb..3ca1ce2d6bd5 100644
> --- a/fs/fuse/kio/pcs/pcs_fuse_kdirect.c
> +++ b/fs/fuse/kio/pcs/pcs_fuse_kdirect.c
> @@ -986,6 +986,11 @@ error:
>  
>  submit:
>  	spin_lock(&di->kq_lock);
> +	if (req->ff && test_bit(FUSE_S_FAIL_IMMEDIATELY, &req->ff->ff_state)) {
> +		spin_unlock(&di->kq_lock);
> +		req->out.h.error = -EIO;
> +		goto error;
> +	}
>  	list_add_tail(&req->list, &di->kq);
>  	spin_unlock(&di->kq_lock);
>  
>