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

Submitted by Pavel Butsykin on April 23, 2019, 9:45 a.m.

Details

Message ID 950048d7-7cce-1bc3-081d-089c365b8874@virtuozzo.com
State New
Series "fuse:Backport of ms locking patches part 2"
Headers show

Commit Message

Pavel Butsykin April 23, 2019, 9:45 a.m.
On 23.04.2019 12:35, Kirill Tkhai wrote:
> On 23.04.2019 12:33, Pavel Butsykin wrote:

>>

>>

>> On 23.04.2019 12:21, Kirill Tkhai wrote:

>>> On 23.04.2019 11:51, Pavel Butsykin wrote:

>>>>

>>>>

>>>> 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);

>>>

>>> Yeah, but fc->lock is still *not needed*.

>>

>> I said - "loсk is needed here, but fc->bg_lock instead of fc->lock".

>>

>>> We may just move immediate checking logic into fuse_request_queue_background():

>>>

>>> fuse: do not take fc->lock in fuse_request_send_background() - fixup

>>>

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

>>> ---

>>>    fs/fuse/dev.c |   24 ++++++------------------

>>>    1 file changed, 6 insertions(+), 18 deletions(-)

>>>

>>> diff --git a/fs/fuse/dev.c b/fs/fuse/dev.c

>>> index 1355f4a0a8e4..bfc792c9b5dd 100644

>>> --- a/fs/fuse/dev.c

>>> +++ b/fs/fuse/dev.c

>>> @@ -610,7 +610,10 @@ bool fuse_request_queue_background(struct fuse_conn *fc, struct fuse_req *req)

>>>    	}

>>>    	__set_bit(FR_ISREPLY, &req->flags);

>>>    	spin_lock(&fc->bg_lock);

>>> -	if (likely(fc->connected)) {

>>> +	if (unlikely(req->page_cache && req->ff &&

>>> +		     test_bit(FUSE_S_FAIL_IMMEDIATELY, &req->ff->ff_state)))

>>> +		req->out.h.error = -EIO;

>>

>> There's another place where synchronization with FUSE_S_FAIL_IMMEDIATELY will

>> still be broken:

>> /* Called under fi->lock, may release and reacquire it */

>> static void fuse_send_writepage(struct fuse_conn *fc, struct fuse_req *req)

>> __releases(fi->lock)

>> __acquires(fi->lock)

>> {

>> ...

>> if (test_bit(FUSE_S_FAIL_IMMEDIATELY, &req->ff->ff_state)) {

> 

> No, after immediate check is moved into fuse_request_queue_background(),

> everything is OK. We just need to remove this excess the check

> from fuse_send_writepage().


Only read requests have setted req->page_cache, so we'll just lose
FUSE_S_FAIL_IMMEDIATELY check in case we remove 'excess' the check.

What about this patch? (see attach)

>>> +	else if (likely(fc->connected)) {

>>>    		fc->num_background++;

>>>    		if (fc->num_background == fc->max_background)

>>>    			fc->blocked = 1;

>>> @@ -640,29 +643,14 @@ bool fuse_request_queue_background(struct fuse_conn *fc, struct fuse_req *req)

>>>    

>>>    void fuse_request_send_background(struct fuse_conn *fc, struct fuse_req *req)

>>>    {

>>> -	bool fail;

>>>    	WARN_ON(!req->end);

>>>    

>>>    	if (fc->kio.op && !fc->kio.op->req_send(fc, req, true, false))

>>>    		return;

>>>    

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

>>> -	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);

>>

>> 		I'm not sure we can drop this stuff.

>>

>>> -		request_end(fc, req);

>>> -		return;

>>> -	}

>>> -

>>>    	if (!fuse_request_queue_background(fc, req)) {

>>> -		req->out.h.error = -ENOTCONN;

>>> +		if (!req->out.h.error)

>>> +			req->out.h.error = -ENOTCONN;

>>>    		req->end(fc, req);

>>>    		fuse_put_request(fc, req);

>>>    	}

>>>

>

Patch hide | download patch | download mbox

From 501c989cdc8575e9d3cb9ad000d767466a1f8a6f Mon Sep 17 00:00:00 2001
From: Pavel Butsykin <pbutsykin@virtuozzo.com>
Date: Tue, 23 Apr 2019 12:43:39 +0300
Subject: [PATCH] fix

---
 fs/fuse/dev.c    | 66 ++++++++++++++++++++++++++------------------------------
 fs/fuse/file.c   | 13 +++++++----
 fs/fuse/fuse_i.h |  2 +-
 3 files changed, 40 insertions(+), 41 deletions(-)

diff --git a/fs/fuse/dev.c b/fs/fuse/dev.c
index a43d0bf..bf0b1cb 100644
--- a/fs/fuse/dev.c
+++ b/fs/fuse/dev.c
@@ -585,10 +585,9 @@  void fuse_request_send(struct fuse_conn *fc, struct fuse_req *req)
 }
 EXPORT_SYMBOL_GPL(fuse_request_send);
 
-bool fuse_request_queue_background(struct fuse_conn *fc, struct fuse_req *req)
+bool fuse_request_queue_background_locked(struct fuse_conn *fc, struct fuse_req *req)
 {
 	struct fuse_iqueue *fiq = req->fiq;
-	bool queued = false;
 
 	WARN_ON(!test_bit(FR_BACKGROUND, &req->flags));
 	if (!test_bit(FR_WAITING, &req->flags)) {
@@ -596,59 +595,54 @@  bool fuse_request_queue_background(struct fuse_conn *fc, struct fuse_req *req)
 		atomic_inc(&fc->num_waiting);
 	}
 	__set_bit(FR_ISREPLY, &req->flags);
-	spin_lock(&fc->bg_lock);
-	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 (unlikely(!fc->connected))
+		return false;
 
-		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;
-		}
+	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);
+	}
 
-		list_add_tail(&req->list, &fc->bg_queue);
-		flush_bg_queue(fc, fiq);
-		queued = true;
-        }
-unlock:
-	spin_unlock(&fc->bg_lock);
-	return queued;
+	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);
+		return false;
+	}
+
+	list_add_tail(&req->list, &fc->bg_queue);
+	flush_bg_queue(fc, fiq);
+	return true;
 }
 
 void fuse_request_send_background(struct fuse_conn *fc, struct fuse_req *req)
 {
-	bool fail;
+	bool queued;
 	WARN_ON(!req->end);
 
 	if (fc->kio.op && !fc->kio.op->req_send(fc, req, true, false))
 		return;
 
-	spin_lock(&fc->lock);
-	fail = (req->page_cache && req->ff &&
-		test_bit(FUSE_S_FAIL_IMMEDIATELY, &req->ff->ff_state));
-	spin_unlock(&fc->lock);
-
-	if (fail) {
-		/* FIXME */
+	spin_lock(&fc->bg_lock);
+	if (req->page_cache && req->ff &&
+	    test_bit(FUSE_S_FAIL_IMMEDIATELY, &req->ff->ff_state)) {
 		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_lock(&fc->bg_lock);
 		request_end(fc, req);
 		return;
 	}
+	queued = fuse_request_queue_background_locked(fc, req);
+	spin_lock(&fc->bg_lock);
 
-	if (!fuse_request_queue_background(fc, req)) {
+	if (unlikely(!queued)) {
 		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 7929f4b..fc0d51f 100644
--- a/fs/fuse/file.c
+++ b/fs/fuse/file.c
@@ -1949,8 +1949,6 @@  __acquires(fi->lock)
 	__u64 data_size = req->num_pages * PAGE_CACHE_SIZE;
 	bool queued;
 
-	if (test_bit(FUSE_S_FAIL_IMMEDIATELY, &req->ff->ff_state))
-		goto out_free;
 
 	if (inarg->offset + data_size <= size) {
 		inarg->size = data_size;
@@ -1960,9 +1958,16 @@  __acquires(fi->lock)
 		/* Got truncated off completely */
 		goto out_free;
 	}
-
 	req->in.args[1].size = inarg->size;
-	queued = fuse_request_queue_background(fc, req);
+
+	spin_lock(&fc->bg_lock);
+	if (test_bit(FUSE_S_FAIL_IMMEDIATELY, &req->ff->ff_state)) {
+		spin_unlock(&fc->bg_lock);
+		goto out_free;
+	}
+	queued = fuse_request_queue_background_locked(fc, req);
+	spin_unlock(&fc->bg_lock);
+
 	/* Fails on broken connection only */
 	if (unlikely(!queued))
 		goto out_free;
diff --git a/fs/fuse/fuse_i.h b/fs/fuse/fuse_i.h
index f7415fd..3f9dbc6 100644
--- a/fs/fuse/fuse_i.h
+++ b/fs/fuse/fuse_i.h
@@ -987,7 +987,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);
 
-bool fuse_request_queue_background(struct fuse_conn *fc, struct fuse_req *req);
+bool fuse_request_queue_background_locked(struct fuse_conn *fc, struct fuse_req *req);
 
 /* Abort all requests */
 void fuse_abort_conn(struct fuse_conn *fc);
-- 
1.8.3.1