[PATCH 0/3] Minor cleanups to fuse

[PATCH 0/3] Minor cleanups to fuse

[Kemeng Shi]

This series aims to remove unneeded usage of FR_WATING flag. More
details can be found in respective patches. Thanks!

Kemeng Shi (3):
  fuse: move FR_WAITING set from fuse_request_queue_background to
    fuse_simple_background
  fuse: remove usage of FR_WATING flag
  fuse: move fuse_put_request a bit to remove forward declaration

 fs/fuse/dev.c    | 55 ++++++++++++++++++++----------------------------
 fs/fuse/fuse_i.h |  2 --
 2 files changed, 23 insertions(+), 34 deletions(-)

-- 
2.30.0

[PATCH 1/3] fuse: move FR_WAITING set from fuse_request_queue_background to fuse_simple_background

[Kemeng Shi]

Current way to set FR_WAITING in fuse_simple_background:
fuse_simple_background
	if (args->force)
		fuse_request_alloc
		/* need to increase num_waiting before request is queued */
	else
		fuse_get_req
			atomic_inc(&fc->num_waiting);
			__set_bit(FR_WAITING, &req->flags);

	fuse_request_queue_background
		if (!test_bit(FR_WAITING, &req->flags)
			__set_bit(FR_WAITING, &req->flags);
			atomic_inc(&fc->num_waiting);

We only need to increase num_waiting for force allocated reqeust in
fuse_request_queue_background. Simply increase num_waiting in force block
to remove unnecessary test_bit(FR_WAITING).
This patch also makes it more intuitive to remove FR_WATING usage in next
commit.

Signed-off-by: Kemeng Shi <shikemeng@huaweicloud.com>
---
 fs/fuse/dev.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/fs/fuse/dev.c b/fs/fuse/dev.c
index 1a8f82f478cb..59e1357d4880 100644
--- a/fs/fuse/dev.c
+++ b/fs/fuse/dev.c
@@ -528,10 +528,6 @@ static bool fuse_request_queue_background(struct fuse_req *req)
 	bool queued = false;
 
 	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);
 	if (likely(fc->connected)) {
@@ -553,10 +549,14 @@ int fuse_simple_background(struct fuse_mount *fm, struct fuse_args *args,
 	struct fuse_req *req;
 
 	if (args->force) {
+		struct fuse_conn *fc = fm->fc;
+
 		WARN_ON(!args->nocreds);
 		req = fuse_request_alloc(fm, gfp_flags);
 		if (!req)
 			return -ENOMEM;
+		atomic_inc(&fc->num_waiting);
+		__set_bit(FR_WAITING, &req->flags);
 		__set_bit(FR_BACKGROUND, &req->flags);
 	} else {
 		WARN_ON(args->nocreds);
-- 
2.30.0

Re: [PATCH 1/3] fuse: move FR_WAITING set from fuse_request_queue_background to fuse_simple_background

[Bernd Schubert]

On 9/4/23 16:30, Kemeng Shi wrote:
> Current way to set FR_WAITING in fuse_simple_background:
> fuse_simple_background
> 	if (args->force)
> 		fuse_request_alloc
> 		/* need to increase num_waiting before request is queued */
> 	else
> 		fuse_get_req
> 			atomic_inc(&fc->num_waiting);
> 			__set_bit(FR_WAITING, &req->flags);
> 
> 	fuse_request_queue_background
> 		if (!test_bit(FR_WAITING, &req->flags)
> 			__set_bit(FR_WAITING, &req->flags);
> 			atomic_inc(&fc->num_waiting);
> 
> We only need to increase num_waiting for force allocated reqeust in
> fuse_request_queue_background. Simply increase num_waiting in force block
> to remove unnecessary test_bit(FR_WAITING).
> This patch also makes it more intuitive to remove FR_WATING usage in next
> commit.

Very minor nit 'FR_WAITING'. The patch looks good to me.

> 
> Signed-off-by: Kemeng Shi <shikemeng@huaweicloud.com>
> ---
>   fs/fuse/dev.c | 8 ++++----
>   1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/fs/fuse/dev.c b/fs/fuse/dev.c
> index 1a8f82f478cb..59e1357d4880 100644
> --- a/fs/fuse/dev.c
> +++ b/fs/fuse/dev.c
> @@ -528,10 +528,6 @@ static bool fuse_request_queue_background(struct fuse_req *req)
>   	bool queued = false;
>   
>   	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);
>   	if (likely(fc->connected)) {
> @@ -553,10 +549,14 @@ int fuse_simple_background(struct fuse_mount *fm, struct fuse_args *args,
>   	struct fuse_req *req;
>   
>   	if (args->force) {
> +		struct fuse_conn *fc = fm->fc;
> +
>   		WARN_ON(!args->nocreds);
>   		req = fuse_request_alloc(fm, gfp_flags);
>   		if (!req)
>   			return -ENOMEM;
> +		atomic_inc(&fc->num_waiting);
> +		__set_bit(FR_WAITING, &req->flags);
>   		__set_bit(FR_BACKGROUND, &req->flags);
>   	} else {
>   		WARN_ON(args->nocreds);

[PATCH 2/3] fuse: remove usage of FR_WATING flag

[Kemeng Shi]

Each allocated request from fuse_request_alloc counts to num_waiting
before request is freed.
Simply drop num_waiting without FR_WAITING flag check in fuse_put_request
to remove unneeded usage of FR_WAITING flag.

Signed-off-by: Kemeng Shi <shikemeng@huaweicloud.com>
---
 fs/fuse/dev.c    | 9 +--------
 fs/fuse/fuse_i.h | 2 --
 2 files changed, 1 insertion(+), 10 deletions(-)

diff --git a/fs/fuse/dev.c b/fs/fuse/dev.c
index 59e1357d4880..4f49b1946635 100644
--- a/fs/fuse/dev.c
+++ b/fs/fuse/dev.c
@@ -139,7 +139,6 @@ static struct fuse_req *fuse_get_req(struct fuse_mount *fm, bool for_background)
 	req->in.h.gid = from_kgid(fc->user_ns, current_fsgid());
 	req->in.h.pid = pid_nr_ns(task_pid(current), fc->pid_ns);
 
-	__set_bit(FR_WAITING, &req->flags);
 	if (for_background)
 		__set_bit(FR_BACKGROUND, &req->flags);
 
@@ -171,11 +170,7 @@ static void fuse_put_request(struct fuse_req *req)
 			spin_unlock(&fc->bg_lock);
 		}
 
-		if (test_bit(FR_WAITING, &req->flags)) {
-			__clear_bit(FR_WAITING, &req->flags);
-			fuse_drop_waiting(fc);
-		}
-
+		fuse_drop_waiting(fc);
 		fuse_request_free(req);
 	}
 }
@@ -495,7 +490,6 @@ ssize_t fuse_simple_request(struct fuse_mount *fm, struct fuse_args *args)
 		if (!args->nocreds)
 			fuse_force_creds(req);
 
-		__set_bit(FR_WAITING, &req->flags);
 		__set_bit(FR_FORCE, &req->flags);
 	} else {
 		WARN_ON(args->nocreds);
@@ -556,7 +550,6 @@ int fuse_simple_background(struct fuse_mount *fm, struct fuse_args *args,
 		if (!req)
 			return -ENOMEM;
 		atomic_inc(&fc->num_waiting);
-		__set_bit(FR_WAITING, &req->flags);
 		__set_bit(FR_BACKGROUND, &req->flags);
 	} else {
 		WARN_ON(args->nocreds);
diff --git a/fs/fuse/fuse_i.h b/fs/fuse/fuse_i.h
index 9b7fc7d3c7f1..45da5553bae3 100644
--- a/fs/fuse/fuse_i.h
+++ b/fs/fuse/fuse_i.h
@@ -307,7 +307,6 @@ struct fuse_io_priv {
  * FR_ISREPLY:		set if the request has reply
  * FR_FORCE:		force sending of the request even if interrupted
  * FR_BACKGROUND:	request is sent in the background
- * FR_WAITING:		request is counted as "waiting"
  * FR_ABORTED:		the request was aborted
  * FR_INTERRUPTED:	the request has been interrupted
  * FR_LOCKED:		data is being copied to/from the request
@@ -321,7 +320,6 @@ enum fuse_req_flag {
 	FR_ISREPLY,
 	FR_FORCE,
 	FR_BACKGROUND,
-	FR_WAITING,
 	FR_ABORTED,
 	FR_INTERRUPTED,
 	FR_LOCKED,
-- 
2.30.0

Re: [PATCH 2/3] fuse: remove usage of FR_WATING flag

[Bernd Schubert]

On 9/4/23 16:30, Kemeng Shi wrote:
> Each allocated request from fuse_request_alloc counts to num_waiting
> before request is freed.
> Simply drop num_waiting without FR_WAITING flag check in fuse_put_request
> to remove unneeded usage of FR_WAITING flag.
> 
> Signed-off-by: Kemeng Shi <shikemeng@huaweicloud.com>
> ---
>   fs/fuse/dev.c    | 9 +--------
>   fs/fuse/fuse_i.h | 2 --
>   2 files changed, 1 insertion(+), 10 deletions(-)
> 
> diff --git a/fs/fuse/dev.c b/fs/fuse/dev.c
> index 59e1357d4880..4f49b1946635 100644
> --- a/fs/fuse/dev.c
> +++ b/fs/fuse/dev.c
> @@ -139,7 +139,6 @@ static struct fuse_req *fuse_get_req(struct fuse_mount *fm, bool for_background)
>   	req->in.h.gid = from_kgid(fc->user_ns, current_fsgid());
>   	req->in.h.pid = pid_nr_ns(task_pid(current), fc->pid_ns);
>   
> -	__set_bit(FR_WAITING, &req->flags);
>   	if (for_background)
>   		__set_bit(FR_BACKGROUND, &req->flags);
>   
> @@ -171,11 +170,7 @@ static void fuse_put_request(struct fuse_req *req)
>   			spin_unlock(&fc->bg_lock);
>   		}
>   
> -		if (test_bit(FR_WAITING, &req->flags)) {
> -			__clear_bit(FR_WAITING, &req->flags);
> -			fuse_drop_waiting(fc);
> -		}
> -
> +		fuse_drop_waiting(fc);
>   		fuse_request_free(req);
>   	}
>   }
> @@ -495,7 +490,6 @@ ssize_t fuse_simple_request(struct fuse_mount *fm, struct fuse_args *args)
>   		if (!args->nocreds)
>   			fuse_force_creds(req);
>   
> -		__set_bit(FR_WAITING, &req->flags);
>   		__set_bit(FR_FORCE, &req->flags);
>   	} else {
>   		WARN_ON(args->nocreds);
> @@ -556,7 +550,6 @@ int fuse_simple_background(struct fuse_mount *fm, struct fuse_args *args,
>   		if (!req)
>   			return -ENOMEM;
>   		atomic_inc(&fc->num_waiting);
> -		__set_bit(FR_WAITING, &req->flags);
>   		__set_bit(FR_BACKGROUND, &req->flags);
>   	} else {
>   		WARN_ON(args->nocreds);
> diff --git a/fs/fuse/fuse_i.h b/fs/fuse/fuse_i.h
> index 9b7fc7d3c7f1..45da5553bae3 100644
> --- a/fs/fuse/fuse_i.h
> +++ b/fs/fuse/fuse_i.h
> @@ -307,7 +307,6 @@ struct fuse_io_priv {
>    * FR_ISREPLY:		set if the request has reply
>    * FR_FORCE:		force sending of the request even if interrupted
>    * FR_BACKGROUND:	request is sent in the background
> - * FR_WAITING:		request is counted as "waiting"
>    * FR_ABORTED:		the request was aborted
>    * FR_INTERRUPTED:	the request has been interrupted
>    * FR_LOCKED:		data is being copied to/from the request
> @@ -321,7 +320,6 @@ enum fuse_req_flag {
>   	FR_ISREPLY,
>   	FR_FORCE,
>   	FR_BACKGROUND,
> -	FR_WAITING,
>   	FR_ABORTED,
>   	FR_INTERRUPTED,
>   	FR_LOCKED,

Yeah, at best it is a debug information, but as it is set anyway (also before
patch 1) before queuing it, there is not much to gain from it. Looks good to
me.

[PATCH 3/3] fuse: move fuse_put_request a bit to remove forward declaration

[Kemeng Shi]

Move fuse_put_request before fuse_get_req to remove forward declaration.

Signed-off-by: Kemeng Shi <shikemeng@huaweicloud.com>
---
 fs/fuse/dev.c | 42 ++++++++++++++++++++----------------------
 1 file changed, 20 insertions(+), 22 deletions(-)

diff --git a/fs/fuse/dev.c b/fs/fuse/dev.c
index 4f49b1946635..deda8b036de7 100644
--- a/fs/fuse/dev.c
+++ b/fs/fuse/dev.c
@@ -101,7 +101,26 @@ static void fuse_drop_waiting(struct fuse_conn *fc)
 	}
 }
 
-static void fuse_put_request(struct fuse_req *req);
+static void fuse_put_request(struct fuse_req *req)
+{
+	struct fuse_conn *fc = req->fm->fc;
+
+	if (refcount_dec_and_test(&req->count)) {
+		if (test_bit(FR_BACKGROUND, &req->flags)) {
+			/*
+			 * We get here in the unlikely case that a background
+			 * request was allocated but not sent
+			 */
+			spin_lock(&fc->bg_lock);
+			if (!fc->blocked)
+				wake_up(&fc->blocked_waitq);
+			spin_unlock(&fc->bg_lock);
+		}
+
+		fuse_drop_waiting(fc);
+		fuse_request_free(req);
+	}
+}
 
 static struct fuse_req *fuse_get_req(struct fuse_mount *fm, bool for_background)
 {
@@ -154,27 +173,6 @@ static struct fuse_req *fuse_get_req(struct fuse_mount *fm, bool for_background)
 	return ERR_PTR(err);
 }
 
-static void fuse_put_request(struct fuse_req *req)
-{
-	struct fuse_conn *fc = req->fm->fc;
-
-	if (refcount_dec_and_test(&req->count)) {
-		if (test_bit(FR_BACKGROUND, &req->flags)) {
-			/*
-			 * We get here in the unlikely case that a background
-			 * request was allocated but not sent
-			 */
-			spin_lock(&fc->bg_lock);
-			if (!fc->blocked)
-				wake_up(&fc->blocked_waitq);
-			spin_unlock(&fc->bg_lock);
-		}
-
-		fuse_drop_waiting(fc);
-		fuse_request_free(req);
-	}
-}
-
 unsigned int fuse_len_args(unsigned int numargs, struct fuse_arg *args)
 {
 	unsigned nbytes = 0;
-- 
2.30.0

Re: [PATCH 3/3] fuse: move fuse_put_request a bit to remove forward declaration

[Bernd Schubert]

On 9/4/23 16:30, Kemeng Shi wrote:
> Move fuse_put_request before fuse_get_req to remove forward declaration.
> 
> Signed-off-by: Kemeng Shi <shikemeng@huaweicloud.com>
> ---
>   fs/fuse/dev.c | 42 ++++++++++++++++++++----------------------
>   1 file changed, 20 insertions(+), 22 deletions(-)
> 
> diff --git a/fs/fuse/dev.c b/fs/fuse/dev.c
> index 4f49b1946635..deda8b036de7 100644
> --- a/fs/fuse/dev.c
> +++ b/fs/fuse/dev.c
> @@ -101,7 +101,26 @@ static void fuse_drop_waiting(struct fuse_conn *fc)
>   	}
>   }
>   
> -static void fuse_put_request(struct fuse_req *req);
> +static void fuse_put_request(struct fuse_req *req)
> +{
> +	struct fuse_conn *fc = req->fm->fc;
> +
> +	if (refcount_dec_and_test(&req->count)) {
> +		if (test_bit(FR_BACKGROUND, &req->flags)) {
> +			/*
> +			 * We get here in the unlikely case that a background
> +			 * request was allocated but not sent
> +			 */
> +			spin_lock(&fc->bg_lock);
> +			if (!fc->blocked)
> +				wake_up(&fc->blocked_waitq);
> +			spin_unlock(&fc->bg_lock);
> +		}
> +
> +		fuse_drop_waiting(fc);
> +		fuse_request_free(req);
> +	}
> +}
>   
>   static struct fuse_req *fuse_get_req(struct fuse_mount *fm, bool for_background)
>   {
> @@ -154,27 +173,6 @@ static struct fuse_req *fuse_get_req(struct fuse_mount *fm, bool for_background)
>   	return ERR_PTR(err);
>   }
>   
> -static void fuse_put_request(struct fuse_req *req)
> -{
> -	struct fuse_conn *fc = req->fm->fc;
> -
> -	if (refcount_dec_and_test(&req->count)) {
> -		if (test_bit(FR_BACKGROUND, &req->flags)) {
> -			/*
> -			 * We get here in the unlikely case that a background
> -			 * request was allocated but not sent
> -			 */
> -			spin_lock(&fc->bg_lock);
> -			if (!fc->blocked)
> -				wake_up(&fc->blocked_waitq);
> -			spin_unlock(&fc->bg_lock);
> -		}
> -
> -		fuse_drop_waiting(fc);
> -		fuse_request_free(req);
> -	}
> -}
> -
>   unsigned int fuse_len_args(unsigned int numargs, struct fuse_arg *args)
>   {
>   	unsigned nbytes = 0;

Hmm yeah, but it makes it harder to get history with git annotate/blame?

Thanks,
Bernd

Re: [PATCH 3/3] fuse: move fuse_put_request a bit to remove forward declaration

[Kemeng Shi]

on 9/8/2023 6:55 AM, Bernd Schubert wrote:
> 
> 
> On 9/4/23 16:30, Kemeng Shi wrote:
>> Move fuse_put_request before fuse_get_req to remove forward declaration.
>>
>> Signed-off-by: Kemeng Shi <shikemeng@huaweicloud.com>
>> ---
>>   fs/fuse/dev.c | 42 ++++++++++++++++++++----------------------
>>   1 file changed, 20 insertions(+), 22 deletions(-)
>>
>> diff --git a/fs/fuse/dev.c b/fs/fuse/dev.c
>> index 4f49b1946635..deda8b036de7 100644
>> --- a/fs/fuse/dev.c
>> +++ b/fs/fuse/dev.c
>> @@ -101,7 +101,26 @@ static void fuse_drop_waiting(struct fuse_conn *fc)
>>       }
>>   }
>>   -static void fuse_put_request(struct fuse_req *req);
>> +static void fuse_put_request(struct fuse_req *req)
>> +{
>> +    struct fuse_conn *fc = req->fm->fc;
>> +
>> +    if (refcount_dec_and_test(&req->count)) {
>> +        if (test_bit(FR_BACKGROUND, &req->flags)) {
>> +            /*
>> +             * We get here in the unlikely case that a background
>> +             * request was allocated but not sent
>> +             */
>> +            spin_lock(&fc->bg_lock);
>> +            if (!fc->blocked)
>> +                wake_up(&fc->blocked_waitq);
>> +            spin_unlock(&fc->bg_lock);
>> +        }
>> +
>> +        fuse_drop_waiting(fc);
>> +        fuse_request_free(req);
>> +    }
>> +}
>>     static struct fuse_req *fuse_get_req(struct fuse_mount *fm, bool for_background)
>>   {
>> @@ -154,27 +173,6 @@ static struct fuse_req *fuse_get_req(struct fuse_mount *fm, bool for_background)
>>       return ERR_PTR(err);
>>   }
>>   -static void fuse_put_request(struct fuse_req *req)
>> -{
>> -    struct fuse_conn *fc = req->fm->fc;
>> -
>> -    if (refcount_dec_and_test(&req->count)) {
>> -        if (test_bit(FR_BACKGROUND, &req->flags)) {
>> -            /*
>> -             * We get here in the unlikely case that a background
>> -             * request was allocated but not sent
>> -             */
>> -            spin_lock(&fc->bg_lock);
>> -            if (!fc->blocked)
>> -                wake_up(&fc->blocked_waitq);
>> -            spin_unlock(&fc->bg_lock);
>> -        }
>> -
>> -        fuse_drop_waiting(fc);
>> -        fuse_request_free(req);
>> -    }
>> -}
>> -
>>   unsigned int fuse_len_args(unsigned int numargs, struct fuse_arg *args)
>>   {
>>       unsigned nbytes = 0;
> 
> Hmm yeah, but it makes it harder to get history with git annotate/blame?
> 
Thanks for the feedbak, I will drop this and fix typo in patch 1 in next
version.
> Thanks,
> Bernd
>