[3/8] fuse: do not take fc->lock in fuse_request_send_background()

Submitted by Kirill Tkhai on April 3, 2019, 3:37 p.m.

Details

Message ID 155430583689.24379.14151995982096763926.stgit@localhost.localdomain
State New
Series "fuse:Backport of ms locking patches part 2"
Headers show

Commit Message

Kirill Tkhai April 3, 2019, 3:37 p.m.
ms commit 63825b4e1da5

Currently, we take fc->lock there only to check for fc->connected.
But this flag is changed only on connection abort, which is very
rare operation.

So allow checking fc->connected under just fc->bg_lock and use this lock
(as well as fc->lock) when resetting fc->connected.

Signed-off-by: Kirill Tkhai <ktkhai@virtuozzo.com>
Signed-off-by: Miklos Szeredi <mszeredi@redhat.com>
---
 fs/fuse/dev.c    |   73 +++++++++++++++++++++++++++---------------------------
 fs/fuse/file.c   |    4 ++-
 fs/fuse/fuse_i.h |    3 +-
 3 files changed, 41 insertions(+), 39 deletions(-)

Patch hide | download patch | download mbox

diff --git a/fs/fuse/dev.c b/fs/fuse/dev.c
index 1ffc10ff18ba..1355f4a0a8e4 100644
--- a/fs/fuse/dev.c
+++ b/fs/fuse/dev.c
@@ -598,69 +598,70 @@  void fuse_request_send(struct fuse_conn *fc, struct fuse_req *req)
 }
 EXPORT_SYMBOL_GPL(fuse_request_send);
 
-/*
- * Called under fc->lock
- *
- * fc->connected must have been checked previously
- */
-void fuse_request_send_background_nocheck(struct fuse_conn *fc,
-					  struct fuse_req *req)
+bool fuse_request_queue_background(struct fuse_conn *fc, struct fuse_req *req)
 {
 	struct fuse_iqueue *fiq = req->fiq;
+	bool queued = false;
 
-	BUG_ON(!test_bit(FR_BACKGROUND, &req->flags));
-
+	WARN_ON(!test_bit(FR_BACKGROUND, &req->flags));
 	if (!test_bit(FR_WAITING, &req->flags)) {
 		__set_bit(FR_WAITING, &req->flags);
 		atomic_inc(&fc->num_waiting);
 	}
 	__set_bit(FR_ISREPLY, &req->flags);
 	spin_lock(&fc->bg_lock);
-	fc->num_background++;
-	if (fc->num_background == fc->max_background)
-		fc->blocked = 1;
-	if (fc->num_background == fc->congestion_threshold &&
-	    fc->bdi_initialized) {
-		set_bdi_congested(&fc->bdi, BLK_RW_SYNC);
-		set_bdi_congested(&fc->bdi, BLK_RW_ASYNC);
-	}
+	if (likely(fc->connected)) {
+		fc->num_background++;
+		if (fc->num_background == fc->max_background)
+			fc->blocked = 1;
+		if (fc->num_background == fc->congestion_threshold && fc->sb) {
+			set_bdi_congested(fc->sb->s_bdi, BLK_RW_SYNC);
+			set_bdi_congested(fc->sb->s_bdi, BLK_RW_ASYNC);
+		}
 
-	if (test_bit(FR_NONBLOCKING, &req->flags)) {
-		fc->active_background++;
-		spin_lock(&fiq->waitq.lock);
-		req->in.h.unique = fuse_get_unique(fiq);
-		queue_request(fiq, req);
-		spin_unlock(&fiq->waitq.lock);
-		goto unlock;
-	}
+		if (test_bit(FR_NONBLOCKING, &req->flags)) {
+			fc->active_background++;
+			spin_lock(&fiq->waitq.lock);
+			req->in.h.unique = fuse_get_unique(fiq);
+			queue_request(fiq, req);
+			spin_unlock(&fiq->waitq.lock);
+			queued = true;
+			goto unlock;
+		}
 
-	list_add_tail(&req->list, &fc->bg_queue);
-	flush_bg_queue(fc, fiq);
+		list_add_tail(&req->list, &fc->bg_queue);
+		flush_bg_queue(fc, fiq);
+		queued = true;
+        }
 unlock:
 	spin_unlock(&fc->bg_lock);
+	return queued;
 }
 
 void fuse_request_send_background(struct fuse_conn *fc, struct fuse_req *req)
 {
-	BUG_ON(!req->end);
+	bool fail;
+	WARN_ON(!req->end);
 
 	if (fc->kio.op && !fc->kio.op->req_send(fc, req, true, false))
 		return;
 
 	spin_lock(&fc->lock);
-	if (req->page_cache && req->ff &&
-	    test_bit(FUSE_S_FAIL_IMMEDIATELY, &req->ff->ff_state)) {
+	fail = (req->page_cache && req->ff &&
+		test_bit(FUSE_S_FAIL_IMMEDIATELY, &req->ff->ff_state));
+	spin_unlock(&fc->lock);
+
+	if (fail) {
+		/* FIXME */
 		BUG_ON(req->in.h.opcode != FUSE_READ);
 		req->out.h.error = -EIO;
 		__clear_bit(FR_BACKGROUND, &req->flags);
 		__clear_bit(FR_PENDING, &req->flags);
-		spin_unlock(&fc->lock);
 		request_end(fc, req);
-	} else if (fc->connected) {
-		fuse_request_send_background_nocheck(fc, req);
-		spin_unlock(&fc->lock);
-	} else {
-		spin_unlock(&fc->lock);
+		return;
+	}
+
+	if (!fuse_request_queue_background(fc, req)) {
 		req->out.h.error = -ENOTCONN;
 		req->end(fc, req);
 		fuse_put_request(fc, req);
diff --git a/fs/fuse/file.c b/fs/fuse/file.c
index edc314bd7156..6bffa1eef4dd 100644
--- a/fs/fuse/file.c
+++ b/fs/fuse/file.c
@@ -1941,6 +1941,7 @@  __acquires(fc->lock)
 	loff_t size = i_size_read(req->inode);
 	struct fuse_write_in *inarg = &req->misc.write.in;
 	__u64 data_size = req->num_pages * PAGE_CACHE_SIZE;
+	bool queued;
 
 	if (!fc->connected ||
 	    test_bit(FUSE_S_FAIL_IMMEDIATELY, &req->ff->ff_state))
@@ -1957,7 +1958,8 @@  __acquires(fc->lock)
 
 	req->in.args[1].size = inarg->size;
 	fi->writectr++;
-	fuse_request_send_background_nocheck(fc, req);
+	queued = fuse_request_queue_background(fc, req);
+	WARN_ON(!queued);
 	return;
 
  out_free:
diff --git a/fs/fuse/fuse_i.h b/fs/fuse/fuse_i.h
index a6eb566f8789..ee8b367f2956 100644
--- a/fs/fuse/fuse_i.h
+++ b/fs/fuse/fuse_i.h
@@ -979,8 +979,7 @@  void fuse_request_check_and_send(struct fuse_conn *fc, struct fuse_req *req,
  */
 void fuse_request_send_background(struct fuse_conn *fc, struct fuse_req *req);
 
-void fuse_request_send_background_nocheck(struct fuse_conn *fc,
-					  struct fuse_req *req);
+bool fuse_request_queue_background(struct fuse_conn *fc, struct fuse_req *req);
 
 /* Abort all requests */
 void fuse_abort_conn(struct fuse_conn *fc);

Comments

Pavel Butsykin April 23, 2019, 6:56 a.m.
On 03.04.2019 18:37, Kirill Tkhai wrote:
> ms commit 63825b4e1da5
> 
> Currently, we take fc->lock there only to check for fc->connected.
> But this flag is changed only on connection abort, which is very
> rare operation.
> 
> So allow checking fc->connected under just fc->bg_lock and use this lock
> (as well as fc->lock) when resetting fc->connected.
> 
> Signed-off-by: Kirill Tkhai <ktkhai@virtuozzo.com>
> Signed-off-by: Miklos Szeredi <mszeredi@redhat.com>
> ---
>   fs/fuse/dev.c    |   73 +++++++++++++++++++++++++++---------------------------
>   fs/fuse/file.c   |    4 ++-
>   fs/fuse/fuse_i.h |    3 +-
>   3 files changed, 41 insertions(+), 39 deletions(-)
> 
> diff --git a/fs/fuse/dev.c b/fs/fuse/dev.c
> index 1ffc10ff18ba..1355f4a0a8e4 100644
> --- a/fs/fuse/dev.c
> +++ b/fs/fuse/dev.c
> @@ -598,69 +598,70 @@ void fuse_request_send(struct fuse_conn *fc, struct fuse_req *req)
>   }
>   EXPORT_SYMBOL_GPL(fuse_request_send);
>   
> -/*
> - * Called under fc->lock
> - *
> - * fc->connected must have been checked previously
> - */
> -void fuse_request_send_background_nocheck(struct fuse_conn *fc,
> -					  struct fuse_req *req)
> +bool fuse_request_queue_background(struct fuse_conn *fc, struct fuse_req *req)
>   {
>   	struct fuse_iqueue *fiq = req->fiq;
> +	bool queued = false;
>   
> -	BUG_ON(!test_bit(FR_BACKGROUND, &req->flags));
> -
> +	WARN_ON(!test_bit(FR_BACKGROUND, &req->flags));
>   	if (!test_bit(FR_WAITING, &req->flags)) {
>   		__set_bit(FR_WAITING, &req->flags);
>   		atomic_inc(&fc->num_waiting);
>   	}
>   	__set_bit(FR_ISREPLY, &req->flags);
>   	spin_lock(&fc->bg_lock);
> -	fc->num_background++;
> -	if (fc->num_background == fc->max_background)
> -		fc->blocked = 1;
> -	if (fc->num_background == fc->congestion_threshold &&
> -	    fc->bdi_initialized) {
> -		set_bdi_congested(&fc->bdi, BLK_RW_SYNC);
> -		set_bdi_congested(&fc->bdi, BLK_RW_ASYNC);
> -	}
> +	if (likely(fc->connected)) {
> +		fc->num_background++;
> +		if (fc->num_background == fc->max_background)
> +			fc->blocked = 1;
> +		if (fc->num_background == fc->congestion_threshold && fc->sb) {
> +			set_bdi_congested(fc->sb->s_bdi, BLK_RW_SYNC);
> +			set_bdi_congested(fc->sb->s_bdi, BLK_RW_ASYNC);
> +		}
>   
> -	if (test_bit(FR_NONBLOCKING, &req->flags)) {
> -		fc->active_background++;
> -		spin_lock(&fiq->waitq.lock);
> -		req->in.h.unique = fuse_get_unique(fiq);
> -		queue_request(fiq, req);
> -		spin_unlock(&fiq->waitq.lock);
> -		goto unlock;
> -	}
> +		if (test_bit(FR_NONBLOCKING, &req->flags)) {
> +			fc->active_background++;
> +			spin_lock(&fiq->waitq.lock);
> +			req->in.h.unique = fuse_get_unique(fiq);
> +			queue_request(fiq, req);
> +			spin_unlock(&fiq->waitq.lock);
> +			queued = true;
> +			goto unlock;
> +		}
>   
> -	list_add_tail(&req->list, &fc->bg_queue);
> -	flush_bg_queue(fc, fiq);
> +		list_add_tail(&req->list, &fc->bg_queue);
> +		flush_bg_queue(fc, fiq);
> +		queued = true;
> +        }
>   unlock:
>   	spin_unlock(&fc->bg_lock);
> +	return queued;
>   }
>   
>   void fuse_request_send_background(struct fuse_conn *fc, struct fuse_req *req)
>   {
> -	BUG_ON(!req->end);
> +	bool fail;
> +	WARN_ON(!req->end);
>   
>   	if (fc->kio.op && !fc->kio.op->req_send(fc, req, true, false))
>   		return;
>   
>   	spin_lock(&fc->lock);
> -	if (req->page_cache && req->ff &&
> -	    test_bit(FUSE_S_FAIL_IMMEDIATELY, &req->ff->ff_state)) {
> +	fail = (req->page_cache && req->ff &&
> +		test_bit(FUSE_S_FAIL_IMMEDIATELY, &req->ff->ff_state));
> +	spin_unlock(&fc->lock);
> +
> +	if (fail) {
> +		/* FIXME */

What needs to be fixed here?

>   		BUG_ON(req->in.h.opcode != FUSE_READ);
>   		req->out.h.error = -EIO;
>   		__clear_bit(FR_BACKGROUND, &req->flags);
>   		__clear_bit(FR_PENDING, &req->flags);
> -		spin_unlock(&fc->lock);
>   		request_end(fc, req);
> -	} else if (fc->connected) {
> -		fuse_request_send_background_nocheck(fc, req);
> -		spin_unlock(&fc->lock);
> -	} else {
> -		spin_unlock(&fc->lock);
> +		return;
> +	}
> +
> +	if (!fuse_request_queue_background(fc, req)) {
>   		req->out.h.error = -ENOTCONN;
>   		req->end(fc, req);
>   		fuse_put_request(fc, req);
> diff --git a/fs/fuse/file.c b/fs/fuse/file.c
> index edc314bd7156..6bffa1eef4dd 100644
> --- a/fs/fuse/file.c
> +++ b/fs/fuse/file.c
> @@ -1941,6 +1941,7 @@ __acquires(fc->lock)
>   	loff_t size = i_size_read(req->inode);
>   	struct fuse_write_in *inarg = &req->misc.write.in;
>   	__u64 data_size = req->num_pages * PAGE_CACHE_SIZE;
> +	bool queued;
>   
>   	if (!fc->connected ||
>   	    test_bit(FUSE_S_FAIL_IMMEDIATELY, &req->ff->ff_state))
> @@ -1957,7 +1958,8 @@ __acquires(fc->lock)
>   
>   	req->in.args[1].size = inarg->size;
>   	fi->writectr++;
> -	fuse_request_send_background_nocheck(fc, req);
> +	queued = fuse_request_queue_background(fc, req);
> +	WARN_ON(!queued);
>   	return;
>   
>    out_free:
> diff --git a/fs/fuse/fuse_i.h b/fs/fuse/fuse_i.h
> index a6eb566f8789..ee8b367f2956 100644
> --- a/fs/fuse/fuse_i.h
> +++ b/fs/fuse/fuse_i.h
> @@ -979,8 +979,7 @@ void fuse_request_check_and_send(struct fuse_conn *fc, struct fuse_req *req,
>    */
>   void fuse_request_send_background(struct fuse_conn *fc, struct fuse_req *req);
>   
> -void fuse_request_send_background_nocheck(struct fuse_conn *fc,
> -					  struct fuse_req *req);
> +bool fuse_request_queue_background(struct fuse_conn *fc, struct fuse_req *req);
>   
>   /* Abort all requests */
>   void fuse_abort_conn(struct fuse_conn *fc);
>
Kirill Tkhai April 23, 2019, 7:48 a.m.
On 23.04.2019 09:56, Pavel Butsykin wrote:
> 
> 
> On 03.04.2019 18:37, Kirill Tkhai wrote:
>> ms commit 63825b4e1da5
>>
>> Currently, we take fc->lock there only to check for fc->connected.
>> But this flag is changed only on connection abort, which is very
>> rare operation.
>>
>> So allow checking fc->connected under just fc->bg_lock and use this lock
>> (as well as fc->lock) when resetting fc->connected.
>>
>> Signed-off-by: Kirill Tkhai <ktkhai@virtuozzo.com>
>> Signed-off-by: Miklos Szeredi <mszeredi@redhat.com>
>> ---
>>   fs/fuse/dev.c    |   73 +++++++++++++++++++++++++++---------------------------
>>   fs/fuse/file.c   |    4 ++-
>>   fs/fuse/fuse_i.h |    3 +-
>>   3 files changed, 41 insertions(+), 39 deletions(-)
>>
>> diff --git a/fs/fuse/dev.c b/fs/fuse/dev.c
>> index 1ffc10ff18ba..1355f4a0a8e4 100644
>> --- a/fs/fuse/dev.c
>> +++ b/fs/fuse/dev.c
>> @@ -598,69 +598,70 @@ void fuse_request_send(struct fuse_conn *fc, struct fuse_req *req)
>>   }
>>   EXPORT_SYMBOL_GPL(fuse_request_send);
>>   
>> -/*
>> - * Called under fc->lock
>> - *
>> - * fc->connected must have been checked previously
>> - */
>> -void fuse_request_send_background_nocheck(struct fuse_conn *fc,
>> -					  struct fuse_req *req)
>> +bool fuse_request_queue_background(struct fuse_conn *fc, struct fuse_req *req)
>>   {
>>   	struct fuse_iqueue *fiq = req->fiq;
>> +	bool queued = false;
>>   
>> -	BUG_ON(!test_bit(FR_BACKGROUND, &req->flags));
>> -
>> +	WARN_ON(!test_bit(FR_BACKGROUND, &req->flags));
>>   	if (!test_bit(FR_WAITING, &req->flags)) {
>>   		__set_bit(FR_WAITING, &req->flags);
>>   		atomic_inc(&fc->num_waiting);
>>   	}
>>   	__set_bit(FR_ISREPLY, &req->flags);
>>   	spin_lock(&fc->bg_lock);
>> -	fc->num_background++;
>> -	if (fc->num_background == fc->max_background)
>> -		fc->blocked = 1;
>> -	if (fc->num_background == fc->congestion_threshold &&
>> -	    fc->bdi_initialized) {
>> -		set_bdi_congested(&fc->bdi, BLK_RW_SYNC);
>> -		set_bdi_congested(&fc->bdi, BLK_RW_ASYNC);
>> -	}
>> +	if (likely(fc->connected)) {
>> +		fc->num_background++;
>> +		if (fc->num_background == fc->max_background)
>> +			fc->blocked = 1;
>> +		if (fc->num_background == fc->congestion_threshold && fc->sb) {
>> +			set_bdi_congested(fc->sb->s_bdi, BLK_RW_SYNC);
>> +			set_bdi_congested(fc->sb->s_bdi, BLK_RW_ASYNC);
>> +		}
>>   
>> -	if (test_bit(FR_NONBLOCKING, &req->flags)) {
>> -		fc->active_background++;
>> -		spin_lock(&fiq->waitq.lock);
>> -		req->in.h.unique = fuse_get_unique(fiq);
>> -		queue_request(fiq, req);
>> -		spin_unlock(&fiq->waitq.lock);
>> -		goto unlock;
>> -	}
>> +		if (test_bit(FR_NONBLOCKING, &req->flags)) {
>> +			fc->active_background++;
>> +			spin_lock(&fiq->waitq.lock);
>> +			req->in.h.unique = fuse_get_unique(fiq);
>> +			queue_request(fiq, req);
>> +			spin_unlock(&fiq->waitq.lock);
>> +			queued = true;
>> +			goto unlock;
>> +		}
>>   
>> -	list_add_tail(&req->list, &fc->bg_queue);
>> -	flush_bg_queue(fc, fiq);
>> +		list_add_tail(&req->list, &fc->bg_queue);
>> +		flush_bg_queue(fc, fiq);
>> +		queued = true;
>> +        }
>>   unlock:
>>   	spin_unlock(&fc->bg_lock);
>> +	return queued;
>>   }
>>   
>>   void fuse_request_send_background(struct fuse_conn *fc, struct fuse_req *req)
>>   {
>> -	BUG_ON(!req->end);
>> +	bool fail;
>> +	WARN_ON(!req->end);
>>   
>>   	if (fc->kio.op && !fc->kio.op->req_send(fc, req, true, false))
>>   		return;
>>   
>>   	spin_lock(&fc->lock);
>> -	if (req->page_cache && req->ff &&
>> -	    test_bit(FUSE_S_FAIL_IMMEDIATELY, &req->ff->ff_state)) {
>> +	fail = (req->page_cache && req->ff &&
>> +		test_bit(FUSE_S_FAIL_IMMEDIATELY, &req->ff->ff_state));
>> +	spin_unlock(&fc->lock);
>> +
>> +	if (fail) {
>> +		/* FIXME */
> 
> What needs to be fixed here?

Commit aims to remove fc->lock from this function (and it's called "fuse: do not
take fc->lock in fuse_request_send_background()). But FUSE_S_FAIL_IMMEDIATELY
vstorage crutch is made under fc->lock, so the lock remains, and "FIXME" is there.

I think the lock is not needed, since the crutch's bit FUSE_S_FAIL_IMMEDIATELY
is set under lock, but we do not need for all queued requests in fuse_invalidate_files()
after the bit is set. So, I remain this logic, and you may decide whether you
need the lock or not.

>>   		BUG_ON(req->in.h.opcode != FUSE_READ);
>>   		req->out.h.error = -EIO;
>>   		__clear_bit(FR_BACKGROUND, &req->flags);
>>   		__clear_bit(FR_PENDING, &req->flags);
>> -		spin_unlock(&fc->lock);
>>   		request_end(fc, req);
>> -	} else if (fc->connected) {
>> -		fuse_request_send_background_nocheck(fc, req);
>> -		spin_unlock(&fc->lock);
>> -	} else {
>> -		spin_unlock(&fc->lock);
>> +		return;
>> +	}
>> +
>> +	if (!fuse_request_queue_background(fc, req)) {
>>   		req->out.h.error = -ENOTCONN;
>>   		req->end(fc, req);
>>   		fuse_put_request(fc, req);
>> diff --git a/fs/fuse/file.c b/fs/fuse/file.c
>> index edc314bd7156..6bffa1eef4dd 100644
>> --- a/fs/fuse/file.c
>> +++ b/fs/fuse/file.c
>> @@ -1941,6 +1941,7 @@ __acquires(fc->lock)
>>   	loff_t size = i_size_read(req->inode);
>>   	struct fuse_write_in *inarg = &req->misc.write.in;
>>   	__u64 data_size = req->num_pages * PAGE_CACHE_SIZE;
>> +	bool queued;
>>   
>>   	if (!fc->connected ||
>>   	    test_bit(FUSE_S_FAIL_IMMEDIATELY, &req->ff->ff_state))
>> @@ -1957,7 +1958,8 @@ __acquires(fc->lock)
>>   
>>   	req->in.args[1].size = inarg->size;
>>   	fi->writectr++;
>> -	fuse_request_send_background_nocheck(fc, req);
>> +	queued = fuse_request_queue_background(fc, req);
>> +	WARN_ON(!queued);
>>   	return;
>>   
>>    out_free:
>> diff --git a/fs/fuse/fuse_i.h b/fs/fuse/fuse_i.h
>> index a6eb566f8789..ee8b367f2956 100644
>> --- a/fs/fuse/fuse_i.h
>> +++ b/fs/fuse/fuse_i.h
>> @@ -979,8 +979,7 @@ void fuse_request_check_and_send(struct fuse_conn *fc, struct fuse_req *req,
>>    */
>>   void fuse_request_send_background(struct fuse_conn *fc, struct fuse_req *req);
>>   
>> -void fuse_request_send_background_nocheck(struct fuse_conn *fc,
>> -					  struct fuse_req *req);
>> +bool fuse_request_queue_background(struct fuse_conn *fc, struct fuse_req *req);
>>   
>>   /* Abort all requests */
>>   void fuse_abort_conn(struct fuse_conn *fc);
>>
Pavel Butsykin April 23, 2019, 8:36 a.m.
On 23.04.2019 10:48, Kirill Tkhai wrote:
> On 23.04.2019 09:56, Pavel Butsykin wrote:
>>
>>
>> On 03.04.2019 18:37, Kirill Tkhai wrote:
>>> ms commit 63825b4e1da5
>>>
>>> Currently, we take fc->lock there only to check for fc->connected.
>>> But this flag is changed only on connection abort, which is very
>>> rare operation.
>>>
>>> So allow checking fc->connected under just fc->bg_lock and use this lock
>>> (as well as fc->lock) when resetting fc->connected.
>>>
>>> Signed-off-by: Kirill Tkhai <ktkhai@virtuozzo.com>
>>> Signed-off-by: Miklos Szeredi <mszeredi@redhat.com>
>>> ---
>>>    fs/fuse/dev.c    |   73 +++++++++++++++++++++++++++---------------------------
>>>    fs/fuse/file.c   |    4 ++-
>>>    fs/fuse/fuse_i.h |    3 +-
>>>    3 files changed, 41 insertions(+), 39 deletions(-)
>>>
>>> diff --git a/fs/fuse/dev.c b/fs/fuse/dev.c
>>> index 1ffc10ff18ba..1355f4a0a8e4 100644
>>> --- a/fs/fuse/dev.c
>>> +++ b/fs/fuse/dev.c
>>> @@ -598,69 +598,70 @@ void fuse_request_send(struct fuse_conn *fc, struct fuse_req *req)
>>>    }
>>>    EXPORT_SYMBOL_GPL(fuse_request_send);
>>>    
>>> -/*
>>> - * Called under fc->lock
>>> - *
>>> - * fc->connected must have been checked previously
>>> - */
>>> -void fuse_request_send_background_nocheck(struct fuse_conn *fc,
>>> -					  struct fuse_req *req)
>>> +bool fuse_request_queue_background(struct fuse_conn *fc, struct fuse_req *req)
>>>    {
>>>    	struct fuse_iqueue *fiq = req->fiq;
>>> +	bool queued = false;
>>>    
>>> -	BUG_ON(!test_bit(FR_BACKGROUND, &req->flags));
>>> -
>>> +	WARN_ON(!test_bit(FR_BACKGROUND, &req->flags));
>>>    	if (!test_bit(FR_WAITING, &req->flags)) {
>>>    		__set_bit(FR_WAITING, &req->flags);
>>>    		atomic_inc(&fc->num_waiting);
>>>    	}
>>>    	__set_bit(FR_ISREPLY, &req->flags);
>>>    	spin_lock(&fc->bg_lock);
>>> -	fc->num_background++;
>>> -	if (fc->num_background == fc->max_background)
>>> -		fc->blocked = 1;
>>> -	if (fc->num_background == fc->congestion_threshold &&
>>> -	    fc->bdi_initialized) {
>>> -		set_bdi_congested(&fc->bdi, BLK_RW_SYNC);
>>> -		set_bdi_congested(&fc->bdi, BLK_RW_ASYNC);
>>> -	}
>>> +	if (likely(fc->connected)) {
>>> +		fc->num_background++;
>>> +		if (fc->num_background == fc->max_background)
>>> +			fc->blocked = 1;
>>> +		if (fc->num_background == fc->congestion_threshold && fc->sb) {
>>> +			set_bdi_congested(fc->sb->s_bdi, BLK_RW_SYNC);
>>> +			set_bdi_congested(fc->sb->s_bdi, BLK_RW_ASYNC);
>>> +		}
>>>    
>>> -	if (test_bit(FR_NONBLOCKING, &req->flags)) {
>>> -		fc->active_background++;
>>> -		spin_lock(&fiq->waitq.lock);
>>> -		req->in.h.unique = fuse_get_unique(fiq);
>>> -		queue_request(fiq, req);
>>> -		spin_unlock(&fiq->waitq.lock);
>>> -		goto unlock;
>>> -	}
>>> +		if (test_bit(FR_NONBLOCKING, &req->flags)) {
>>> +			fc->active_background++;
>>> +			spin_lock(&fiq->waitq.lock);
>>> +			req->in.h.unique = fuse_get_unique(fiq);
>>> +			queue_request(fiq, req);
>>> +			spin_unlock(&fiq->waitq.lock);
>>> +			queued = true;
>>> +			goto unlock;
>>> +		}
>>>    
>>> -	list_add_tail(&req->list, &fc->bg_queue);
>>> -	flush_bg_queue(fc, fiq);
>>> +		list_add_tail(&req->list, &fc->bg_queue);
>>> +		flush_bg_queue(fc, fiq);
>>> +		queued = true;
>>> +        }
>>>    unlock:
>>>    	spin_unlock(&fc->bg_lock);
>>> +	return queued;
>>>    }
>>>    
>>>    void fuse_request_send_background(struct fuse_conn *fc, struct fuse_req *req)
>>>    {
>>> -	BUG_ON(!req->end);
>>> +	bool fail;
>>> +	WARN_ON(!req->end);
>>>    
>>>    	if (fc->kio.op && !fc->kio.op->req_send(fc, req, true, false))
>>>    		return;
>>>    
>>>    	spin_lock(&fc->lock);
>>> -	if (req->page_cache && req->ff &&
>>> -	    test_bit(FUSE_S_FAIL_IMMEDIATELY, &req->ff->ff_state)) {
>>> +	fail = (req->page_cache && req->ff &&
>>> +		test_bit(FUSE_S_FAIL_IMMEDIATELY, &req->ff->ff_state));
>>> +	spin_unlock(&fc->lock);
>>> +
>>> +	if (fail) {
>>> +		/* FIXME */
>>
>> What needs to be fixed here?
> 
> Commit aims to remove fc->lock from this function (and it's called "fuse: do not
> take fc->lock in fuse_request_send_background()). But FUSE_S_FAIL_IMMEDIATELY
> vstorage crutch is made under fc->lock, so the lock remains, and "FIXME" is there.

There already fi->lock.

> I think the lock is not needed, since the crutch's bit FUSE_S_FAIL_IMMEDIATELY
> is set under lock, but we do not need for all queued requests in fuse_invalidate_files()
> after the bit is set. So, I remain this logic, and you may decide whether you
> need the lock or not.

Ok, Unfortunately loсk is needed here, but fc->bg_lock instead of
fc->lock to synchronize test_bit(FUSE_S_FAIL_IMMEDIATELY) and
fc->bg_queue. Possible race:


fuse_request_send_background(): 
fuse_invalidate_files():
test_bit(FUSE_S_FAIL_IMMEDIATELY, &ff->ff_state)
 
spin_lock(&fi->lock);
 
list_for_each_entry(ff, &fi->rw_files, rw_entry)
 
set_bit(FUSE_S_FAIL_IMMEDIATELY, &ff->ff_state);
 
spin_unlock(&fi->lock);

 
spin_lock(&fc->lock);

 
spin_lock(&fc->bg_lock);
 
fuse_kill_requests(fc, inode, &fc->bg_queue);
 
spin_unlock(&fc->bg_lock);

fuse_request_queue_background():
spin_lock(&fc->bg_lock);
list_add_tail(&req->list, &fc->bg_queue);
flush_bg_queue(fc, fiq);
spin_unlock(&fc->bg_lock);

 
spin_unlock(&fc->lock);


So, to fix that we need to have test_bit(FUSE_S_FAIL_IMMEDIATELY) and
list_add_tail(&req->list, &fc->bg_queue) together under one fc->bg_lock.


>>>    		BUG_ON(req->in.h.opcode != FUSE_READ);
>>>    		req->out.h.error = -EIO;
>>>    		__clear_bit(FR_BACKGROUND, &req->flags);
>>>    		__clear_bit(FR_PENDING, &req->flags);
>>> -		spin_unlock(&fc->lock);
>>>    		request_end(fc, req);
>>> -	} else if (fc->connected) {
>>> -		fuse_request_send_background_nocheck(fc, req);
>>> -		spin_unlock(&fc->lock);
>>> -	} else {
>>> -		spin_unlock(&fc->lock);
>>> +		return;
>>> +	}
>>> +
>>> +	if (!fuse_request_queue_background(fc, req)) {
>>>    		req->out.h.error = -ENOTCONN;
>>>    		req->end(fc, req);
>>>    		fuse_put_request(fc, req);
>>> diff --git a/fs/fuse/file.c b/fs/fuse/file.c
>>> index edc314bd7156..6bffa1eef4dd 100644
>>> --- a/fs/fuse/file.c
>>> +++ b/fs/fuse/file.c
>>> @@ -1941,6 +1941,7 @@ __acquires(fc->lock)
>>>    	loff_t size = i_size_read(req->inode);
>>>    	struct fuse_write_in *inarg = &req->misc.write.in;
>>>    	__u64 data_size = req->num_pages * PAGE_CACHE_SIZE;
>>> +	bool queued;
>>>    
>>>    	if (!fc->connected ||
>>>    	    test_bit(FUSE_S_FAIL_IMMEDIATELY, &req->ff->ff_state))
>>> @@ -1957,7 +1958,8 @@ __acquires(fc->lock)
>>>    
>>>    	req->in.args[1].size = inarg->size;
>>>    	fi->writectr++;
>>> -	fuse_request_send_background_nocheck(fc, req);
>>> +	queued = fuse_request_queue_background(fc, req);
>>> +	WARN_ON(!queued);
>>>    	return;
>>>    
>>>     out_free:
>>> diff --git a/fs/fuse/fuse_i.h b/fs/fuse/fuse_i.h
>>> index a6eb566f8789..ee8b367f2956 100644
>>> --- a/fs/fuse/fuse_i.h
>>> +++ b/fs/fuse/fuse_i.h
>>> @@ -979,8 +979,7 @@ void fuse_request_check_and_send(struct fuse_conn *fc, struct fuse_req *req,
>>>     */
>>>    void fuse_request_send_background(struct fuse_conn *fc, struct fuse_req *req);
>>>    
>>> -void fuse_request_send_background_nocheck(struct fuse_conn *fc,
>>> -					  struct fuse_req *req);
>>> +bool fuse_request_queue_background(struct fuse_conn *fc, struct fuse_req *req);
>>>    
>>>    /* Abort all requests */
>>>    void fuse_abort_conn(struct fuse_conn *fc);
>>>
>
Kirill Tkhai April 23, 2019, 8:40 a.m.
On 23.04.2019 11:36, Pavel Butsykin wrote:
> On 23.04.2019 10:48, Kirill Tkhai wrote:
>> On 23.04.2019 09:56, Pavel Butsykin wrote:
>>>
>>>
>>> On 03.04.2019 18:37, Kirill Tkhai wrote:
>>>> ms commit 63825b4e1da5
>>>>
>>>> Currently, we take fc->lock there only to check for fc->connected.
>>>> But this flag is changed only on connection abort, which is very
>>>> rare operation.
>>>>
>>>> So allow checking fc->connected under just fc->bg_lock and use this lock
>>>> (as well as fc->lock) when resetting fc->connected.
>>>>
>>>> Signed-off-by: Kirill Tkhai <ktkhai@virtuozzo.com>
>>>> Signed-off-by: Miklos Szeredi <mszeredi@redhat.com>
>>>> ---
>>>>    fs/fuse/dev.c    |   73 +++++++++++++++++++++++++++---------------------------
>>>>    fs/fuse/file.c   |    4 ++-
>>>>    fs/fuse/fuse_i.h |    3 +-
>>>>    3 files changed, 41 insertions(+), 39 deletions(-)
>>>>
>>>> diff --git a/fs/fuse/dev.c b/fs/fuse/dev.c
>>>> index 1ffc10ff18ba..1355f4a0a8e4 100644
>>>> --- a/fs/fuse/dev.c
>>>> +++ b/fs/fuse/dev.c
>>>> @@ -598,69 +598,70 @@ void fuse_request_send(struct fuse_conn *fc, struct fuse_req *req)
>>>>    }
>>>>    EXPORT_SYMBOL_GPL(fuse_request_send);
>>>>    
>>>> -/*
>>>> - * Called under fc->lock
>>>> - *
>>>> - * fc->connected must have been checked previously
>>>> - */
>>>> -void fuse_request_send_background_nocheck(struct fuse_conn *fc,
>>>> -					  struct fuse_req *req)
>>>> +bool fuse_request_queue_background(struct fuse_conn *fc, struct fuse_req *req)
>>>>    {
>>>>    	struct fuse_iqueue *fiq = req->fiq;
>>>> +	bool queued = false;
>>>>    
>>>> -	BUG_ON(!test_bit(FR_BACKGROUND, &req->flags));
>>>> -
>>>> +	WARN_ON(!test_bit(FR_BACKGROUND, &req->flags));
>>>>    	if (!test_bit(FR_WAITING, &req->flags)) {
>>>>    		__set_bit(FR_WAITING, &req->flags);
>>>>    		atomic_inc(&fc->num_waiting);
>>>>    	}
>>>>    	__set_bit(FR_ISREPLY, &req->flags);
>>>>    	spin_lock(&fc->bg_lock);
>>>> -	fc->num_background++;
>>>> -	if (fc->num_background == fc->max_background)
>>>> -		fc->blocked = 1;
>>>> -	if (fc->num_background == fc->congestion_threshold &&
>>>> -	    fc->bdi_initialized) {
>>>> -		set_bdi_congested(&fc->bdi, BLK_RW_SYNC);
>>>> -		set_bdi_congested(&fc->bdi, BLK_RW_ASYNC);
>>>> -	}
>>>> +	if (likely(fc->connected)) {
>>>> +		fc->num_background++;
>>>> +		if (fc->num_background == fc->max_background)
>>>> +			fc->blocked = 1;
>>>> +		if (fc->num_background == fc->congestion_threshold && fc->sb) {
>>>> +			set_bdi_congested(fc->sb->s_bdi, BLK_RW_SYNC);
>>>> +			set_bdi_congested(fc->sb->s_bdi, BLK_RW_ASYNC);
>>>> +		}
>>>>    
>>>> -	if (test_bit(FR_NONBLOCKING, &req->flags)) {
>>>> -		fc->active_background++;
>>>> -		spin_lock(&fiq->waitq.lock);
>>>> -		req->in.h.unique = fuse_get_unique(fiq);
>>>> -		queue_request(fiq, req);
>>>> -		spin_unlock(&fiq->waitq.lock);
>>>> -		goto unlock;
>>>> -	}
>>>> +		if (test_bit(FR_NONBLOCKING, &req->flags)) {
>>>> +			fc->active_background++;
>>>> +			spin_lock(&fiq->waitq.lock);
>>>> +			req->in.h.unique = fuse_get_unique(fiq);
>>>> +			queue_request(fiq, req);
>>>> +			spin_unlock(&fiq->waitq.lock);
>>>> +			queued = true;
>>>> +			goto unlock;
>>>> +		}
>>>>    
>>>> -	list_add_tail(&req->list, &fc->bg_queue);
>>>> -	flush_bg_queue(fc, fiq);
>>>> +		list_add_tail(&req->list, &fc->bg_queue);
>>>> +		flush_bg_queue(fc, fiq);
>>>> +		queued = true;
>>>> +        }
>>>>    unlock:
>>>>    	spin_unlock(&fc->bg_lock);
>>>> +	return queued;
>>>>    }
>>>>    
>>>>    void fuse_request_send_background(struct fuse_conn *fc, struct fuse_req *req)
>>>>    {
>>>> -	BUG_ON(!req->end);
>>>> +	bool fail;
>>>> +	WARN_ON(!req->end);
>>>>    
>>>>    	if (fc->kio.op && !fc->kio.op->req_send(fc, req, true, false))
>>>>    		return;
>>>>    
>>>>    	spin_lock(&fc->lock);
>>>> -	if (req->page_cache && req->ff &&
>>>> -	    test_bit(FUSE_S_FAIL_IMMEDIATELY, &req->ff->ff_state)) {
>>>> +	fail = (req->page_cache && req->ff &&
>>>> +		test_bit(FUSE_S_FAIL_IMMEDIATELY, &req->ff->ff_state));
>>>> +	spin_unlock(&fc->lock);
>>>> +
>>>> +	if (fail) {
>>>> +		/* FIXME */
>>>
>>> What needs to be fixed here?
>>
>> Commit aims to remove fc->lock from this function (and it's called "fuse: do not
>> take fc->lock in fuse_request_send_background()). But FUSE_S_FAIL_IMMEDIATELY
>> vstorage crutch is made under fc->lock, so the lock remains, and "FIXME" is there.
> 
> There already fi->lock.
> 
>> I think the lock is not needed, since the crutch's bit FUSE_S_FAIL_IMMEDIATELY
>> is set under lock, but we do not need for all queued requests in fuse_invalidate_files()
>> after the bit is set. So, I remain this logic, and you may decide whether you
>> need the lock or not.
> 
> Ok, Unfortunately loсk is needed here, but fc->bg_lock instead of
> fc->lock to synchronize test_bit(FUSE_S_FAIL_IMMEDIATELY) and
> fc->bg_queue. Possible race:
> 
> 
> fuse_request_send_background(): 
> fuse_invalidate_files():
> test_bit(FUSE_S_FAIL_IMMEDIATELY, &ff->ff_state)
>  
> spin_lock(&fi->lock);
>  
> list_for_each_entry(ff, &fi->rw_files, rw_entry)
>  
> set_bit(FUSE_S_FAIL_IMMEDIATELY, &ff->ff_state);
>  
> spin_unlock(&fi->lock);
> 
>  
> spin_lock(&fc->lock);
> 
>  
> spin_lock(&fc->bg_lock);
>  
> fuse_kill_requests(fc, inode, &fc->bg_queue);
>  
> spin_unlock(&fc->bg_lock);
> 
> fuse_request_queue_background():
> spin_lock(&fc->bg_lock);
> list_add_tail(&req->list, &fc->bg_queue);
> flush_bg_queue(fc, fiq);
> spin_unlock(&fc->bg_lock);
> 
>  
> spin_unlock(&fc->lock);

Your formatting lost all spaces.
 
> So, to fix that we need to have test_bit(FUSE_S_FAIL_IMMEDIATELY) and
> list_add_tail(&req->list, &fc->bg_queue) together under one fc->bg_lock.
> 
> 
>>>>    		BUG_ON(req->in.h.opcode != FUSE_READ);
>>>>    		req->out.h.error = -EIO;
>>>>    		__clear_bit(FR_BACKGROUND, &req->flags);
>>>>    		__clear_bit(FR_PENDING, &req->flags);
>>>> -		spin_unlock(&fc->lock);
>>>>    		request_end(fc, req);
>>>> -	} else if (fc->connected) {
>>>> -		fuse_request_send_background_nocheck(fc, req);
>>>> -		spin_unlock(&fc->lock);
>>>> -	} else {
>>>> -		spin_unlock(&fc->lock);
>>>> +		return;
>>>> +	}
>>>> +
>>>> +	if (!fuse_request_queue_background(fc, req)) {
>>>>    		req->out.h.error = -ENOTCONN;
>>>>    		req->end(fc, req);
>>>>    		fuse_put_request(fc, req);
>>>> diff --git a/fs/fuse/file.c b/fs/fuse/file.c
>>>> index edc314bd7156..6bffa1eef4dd 100644
>>>> --- a/fs/fuse/file.c
>>>> +++ b/fs/fuse/file.c
>>>> @@ -1941,6 +1941,7 @@ __acquires(fc->lock)
>>>>    	loff_t size = i_size_read(req->inode);
>>>>    	struct fuse_write_in *inarg = &req->misc.write.in;
>>>>    	__u64 data_size = req->num_pages * PAGE_CACHE_SIZE;
>>>> +	bool queued;
>>>>    
>>>>    	if (!fc->connected ||
>>>>    	    test_bit(FUSE_S_FAIL_IMMEDIATELY, &req->ff->ff_state))
>>>> @@ -1957,7 +1958,8 @@ __acquires(fc->lock)
>>>>    
>>>>    	req->in.args[1].size = inarg->size;
>>>>    	fi->writectr++;
>>>> -	fuse_request_send_background_nocheck(fc, req);
>>>> +	queued = fuse_request_queue_background(fc, req);
>>>> +	WARN_ON(!queued);
>>>>    	return;
>>>>    
>>>>     out_free:
>>>> diff --git a/fs/fuse/fuse_i.h b/fs/fuse/fuse_i.h
>>>> index a6eb566f8789..ee8b367f2956 100644
>>>> --- a/fs/fuse/fuse_i.h
>>>> +++ b/fs/fuse/fuse_i.h
>>>> @@ -979,8 +979,7 @@ void fuse_request_check_and_send(struct fuse_conn *fc, struct fuse_req *req,
>>>>     */
>>>>    void fuse_request_send_background(struct fuse_conn *fc, struct fuse_req *req);
>>>>    
>>>> -void fuse_request_send_background_nocheck(struct fuse_conn *fc,
>>>> -					  struct fuse_req *req);
>>>> +bool fuse_request_queue_background(struct fuse_conn *fc, struct fuse_req *req);
>>>>    
>>>>    /* Abort all requests */
>>>>    void fuse_abort_conn(struct fuse_conn *fc);
>>>>
>>
Pavel Butsykin April 23, 2019, 8:51 a.m.
On 23.04.2019 11:40, Kirill Tkhai wrote:
> On 23.04.2019 11:36, Pavel Butsykin wrote:
>> On 23.04.2019 10:48, Kirill Tkhai wrote:
>>> On 23.04.2019 09:56, Pavel Butsykin wrote:
>>>>
>>>>
>>>> On 03.04.2019 18:37, Kirill Tkhai wrote:
>>>>> ms commit 63825b4e1da5
>>>>>
>>>>> Currently, we take fc->lock there only to check for fc->connected.
>>>>> But this flag is changed only on connection abort, which is very
>>>>> rare operation.
>>>>>
>>>>> So allow checking fc->connected under just fc->bg_lock and use this lock
>>>>> (as well as fc->lock) when resetting fc->connected.
>>>>>
>>>>> Signed-off-by: Kirill Tkhai <ktkhai@virtuozzo.com>
>>>>> Signed-off-by: Miklos Szeredi <mszeredi@redhat.com>
>>>>> ---
>>>>>     fs/fuse/dev.c    |   73 +++++++++++++++++++++++++++---------------------------
>>>>>     fs/fuse/file.c   |    4 ++-
>>>>>     fs/fuse/fuse_i.h |    3 +-
>>>>>     3 files changed, 41 insertions(+), 39 deletions(-)
>>>>>
>>>>> diff --git a/fs/fuse/dev.c b/fs/fuse/dev.c
>>>>> index 1ffc10ff18ba..1355f4a0a8e4 100644
>>>>> --- a/fs/fuse/dev.c
>>>>> +++ b/fs/fuse/dev.c
>>>>> @@ -598,69 +598,70 @@ void fuse_request_send(struct fuse_conn *fc, struct fuse_req *req)
>>>>>     }
>>>>>     EXPORT_SYMBOL_GPL(fuse_request_send);
>>>>>     
>>>>> -/*
>>>>> - * Called under fc->lock
>>>>> - *
>>>>> - * fc->connected must have been checked previously
>>>>> - */
>>>>> -void fuse_request_send_background_nocheck(struct fuse_conn *fc,
>>>>> -					  struct fuse_req *req)
>>>>> +bool fuse_request_queue_background(struct fuse_conn *fc, struct fuse_req *req)
>>>>>     {
>>>>>     	struct fuse_iqueue *fiq = req->fiq;
>>>>> +	bool queued = false;
>>>>>     
>>>>> -	BUG_ON(!test_bit(FR_BACKGROUND, &req->flags));
>>>>> -
>>>>> +	WARN_ON(!test_bit(FR_BACKGROUND, &req->flags));
>>>>>     	if (!test_bit(FR_WAITING, &req->flags)) {
>>>>>     		__set_bit(FR_WAITING, &req->flags);
>>>>>     		atomic_inc(&fc->num_waiting);
>>>>>     	}
>>>>>     	__set_bit(FR_ISREPLY, &req->flags);
>>>>>     	spin_lock(&fc->bg_lock);
>>>>> -	fc->num_background++;
>>>>> -	if (fc->num_background == fc->max_background)
>>>>> -		fc->blocked = 1;
>>>>> -	if (fc->num_background == fc->congestion_threshold &&
>>>>> -	    fc->bdi_initialized) {
>>>>> -		set_bdi_congested(&fc->bdi, BLK_RW_SYNC);
>>>>> -		set_bdi_congested(&fc->bdi, BLK_RW_ASYNC);
>>>>> -	}
>>>>> +	if (likely(fc->connected)) {
>>>>> +		fc->num_background++;
>>>>> +		if (fc->num_background == fc->max_background)
>>>>> +			fc->blocked = 1;
>>>>> +		if (fc->num_background == fc->congestion_threshold && fc->sb) {
>>>>> +			set_bdi_congested(fc->sb->s_bdi, BLK_RW_SYNC);
>>>>> +			set_bdi_congested(fc->sb->s_bdi, BLK_RW_ASYNC);
>>>>> +		}
>>>>>     
>>>>> -	if (test_bit(FR_NONBLOCKING, &req->flags)) {
>>>>> -		fc->active_background++;
>>>>> -		spin_lock(&fiq->waitq.lock);
>>>>> -		req->in.h.unique = fuse_get_unique(fiq);
>>>>> -		queue_request(fiq, req);
>>>>> -		spin_unlock(&fiq->waitq.lock);
>>>>> -		goto unlock;
>>>>> -	}
>>>>> +		if (test_bit(FR_NONBLOCKING, &req->flags)) {
>>>>> +			fc->active_background++;
>>>>> +			spin_lock(&fiq->waitq.lock);
>>>>> +			req->in.h.unique = fuse_get_unique(fiq);
>>>>> +			queue_request(fiq, req);
>>>>> +			spin_unlock(&fiq->waitq.lock);
>>>>> +			queued = true;
>>>>> +			goto unlock;
>>>>> +		}
>>>>>     
>>>>> -	list_add_tail(&req->list, &fc->bg_queue);
>>>>> -	flush_bg_queue(fc, fiq);
>>>>> +		list_add_tail(&req->list, &fc->bg_queue);
>>>>> +		flush_bg_queue(fc, fiq);
>>>>> +		queued = true;
>>>>> +        }
>>>>>     unlock:
>>>>>     	spin_unlock(&fc->bg_lock);
>>>>> +	return queued;
>>>>>     }
>>>>>     
>>>>>     void fuse_request_send_background(struct fuse_conn *fc, struct fuse_req *req)
>>>>>     {
>>>>> -	BUG_ON(!req->end);
>>>>> +	bool fail;
>>>>> +	WARN_ON(!req->end);
>>>>>     
>>>>>     	if (fc->kio.op && !fc->kio.op->req_send(fc, req, true, false))
>>>>>     		return;
>>>>>     
>>>>>     	spin_lock(&fc->lock);
>>>>> -	if (req->page_cache && req->ff &&
>>>>> -	    test_bit(FUSE_S_FAIL_IMMEDIATELY, &req->ff->ff_state)) {
>>>>> +	fail = (req->page_cache && req->ff &&
>>>>> +		test_bit(FUSE_S_FAIL_IMMEDIATELY, &req->ff->ff_state));
>>>>> +	spin_unlock(&fc->lock);
>>>>> +
>>>>> +	if (fail) {
>>>>> +		/* FIXME */
>>>>
>>>> What needs to be fixed here?
>>>
>>> Commit aims to remove fc->lock from this function (and it's called "fuse: do not
>>> take fc->lock in fuse_request_send_background()). But FUSE_S_FAIL_IMMEDIATELY
>>> vstorage crutch is made under fc->lock, so the lock remains, and "FIXME" is there.
>>
>> There already fi->lock.
>>
>>> I think the lock is not needed, since the crutch's bit FUSE_S_FAIL_IMMEDIATELY
>>> is set under lock, but we do not need for all queued requests in fuse_invalidate_files()
>>> after the bit is set. So, I remain this logic, and you may decide whether you
>>> need the lock or not.
>>
>> Ok, Unfortunately loсk is needed here, but fc->bg_lock instead of
>> fc->lock to synchronize test_bit(FUSE_S_FAIL_IMMEDIATELY) and
>> fc->bg_queue. Possible race:
>>
> Your formatting lost all spaces.
>   

One more try:

fuse_request_send_background():........................fuse_invalidate_files():
test_bit(FUSE_S_FAIL_IMMEDIATELY,.&ff->ff_state)
........................................................spin_lock(&fi->lock);
........................................................list_for_each_entry(ff,.&fi->rw_files,.rw_entry)
..........................................................set_bit(FUSE_S_FAIL_IMMEDIATELY,.&ff->ff_state);
........................................................spin_unlock(&fi->lock);

........................................................spin_lock(&fc->lock);

........................................................spin_lock(&fc->bg_lock);
........................................................fuse_kill_requests(fc,.inode,.&fc->bg_queue);
........................................................spin_unlock(&fc->bg_lock);

fuse_request_queue_background():
spin_lock(&fc->bg_lock);
list_add_tail(&req->list,.&fc->bg_queue);
flush_bg_queue(fc,.fiq);
spin_unlock(&fc->bg_lock);

........................................................spin_unlock(&fc->lock);


>> So, to fix that we need to have test_bit(FUSE_S_FAIL_IMMEDIATELY) and
>> list_add_tail(&req->list, &fc->bg_queue) together under one fc->bg_lock.
>>
>>
>>>>>     		BUG_ON(req->in.h.opcode != FUSE_READ);
>>>>>     		req->out.h.error = -EIO;
>>>>>     		__clear_bit(FR_BACKGROUND, &req->flags);
>>>>>     		__clear_bit(FR_PENDING, &req->flags);
>>>>> -		spin_unlock(&fc->lock);
>>>>>     		request_end(fc, req);
>>>>> -	} else if (fc->connected) {
>>>>> -		fuse_request_send_background_nocheck(fc, req);
>>>>> -		spin_unlock(&fc->lock);
>>>>> -	} else {
>>>>> -		spin_unlock(&fc->lock);
>>>>> +		return;
>>>>> +	}
>>>>> +
>>>>> +	if (!fuse_request_queue_background(fc, req)) {
>>>>>     		req->out.h.error = -ENOTCONN;
>>>>>     		req->end(fc, req);
>>>>>     		fuse_put_request(fc, req);
>>>>> diff --git a/fs/fuse/file.c b/fs/fuse/file.c
>>>>> index edc314bd7156..6bffa1eef4dd 100644
>>>>> --- a/fs/fuse/file.c
>>>>> +++ b/fs/fuse/file.c
>>>>> @@ -1941,6 +1941,7 @@ __acquires(fc->lock)
>>>>>     	loff_t size = i_size_read(req->inode);
>>>>>     	struct fuse_write_in *inarg = &req->misc.write.in;
>>>>>     	__u64 data_size = req->num_pages * PAGE_CACHE_SIZE;
>>>>> +	bool queued;
>>>>>     
>>>>>     	if (!fc->connected ||
>>>>>     	    test_bit(FUSE_S_FAIL_IMMEDIATELY, &req->ff->ff_state))
>>>>> @@ -1957,7 +1958,8 @@ __acquires(fc->lock)
>>>>>     
>>>>>     	req->in.args[1].size = inarg->size;
>>>>>     	fi->writectr++;
>>>>> -	fuse_request_send_background_nocheck(fc, req);
>>>>> +	queued = fuse_request_queue_background(fc, req);
>>>>> +	WARN_ON(!queued);
>>>>>     	return;
>>>>>     
>>>>>      out_free:
>>>>> diff --git a/fs/fuse/fuse_i.h b/fs/fuse/fuse_i.h
>>>>> index a6eb566f8789..ee8b367f2956 100644
>>>>> --- a/fs/fuse/fuse_i.h
>>>>> +++ b/fs/fuse/fuse_i.h
>>>>> @@ -979,8 +979,7 @@ void fuse_request_check_and_send(struct fuse_conn *fc, struct fuse_req *req,
>>>>>      */
>>>>>     void fuse_request_send_background(struct fuse_conn *fc, struct fuse_req *req);
>>>>>     
>>>>> -void fuse_request_send_background_nocheck(struct fuse_conn *fc,
>>>>> -					  struct fuse_req *req);
>>>>> +bool fuse_request_queue_background(struct fuse_conn *fc, struct fuse_req *req);
>>>>>     
>>>>>     /* Abort all requests */
>>>>>     void fuse_abort_conn(struct fuse_conn *fc);
>>>>>
>>>
>