Re: [PATCH v3 05/12] cachefiles: remove err_put_fd label in cachefiles_ondemand_daemon_read()

[Jingbo Xu]

On 5/22/24 7:43 PM, libaokun@xxxxxxxxxxxxxxx wrote:
> From: Baokun Li <libaokun1@xxxxxxxxxx>
> 
> The err_put_fd label is only used once, so remove it to make the code
> more readable. In addition, the logic for deleting error request and
> CLOSE request is merged to simplify the code.
> 
> Signed-off-by: Baokun Li <libaokun1@xxxxxxxxxx>
> Acked-by: Jeff Layton <jlayton@xxxxxxxxxx>
> Reviewed-by: Jia Zhu <zhujia.zj@xxxxxxxxxxxxx>
> Reviewed-by: Gao Xiang <hsiangkao@xxxxxxxxxxxxxxxxx>

LGTM.

Reviewed-by: Jingbo Xu <jefflexu@xxxxxxxxxxxxxxxxx>


> ---
>  fs/cachefiles/ondemand.c | 45 ++++++++++++++--------------------------
>  1 file changed, 16 insertions(+), 29 deletions(-)
> 
> diff --git a/fs/cachefiles/ondemand.c b/fs/cachefiles/ondemand.c
> index 3dd002108a87..bb94ef6a6f61 100644
> --- a/fs/cachefiles/ondemand.c
> +++ b/fs/cachefiles/ondemand.c
> @@ -305,7 +305,6 @@ ssize_t cachefiles_ondemand_daemon_read(struct cachefiles_cache *cache,
>  {
>  	struct cachefiles_req *req;
>  	struct cachefiles_msg *msg;
> -	unsigned long id = 0;
>  	size_t n;
>  	int ret = 0;
>  	XA_STATE(xas, &cache->reqs, cache->req_id_next);
> @@ -340,49 +339,37 @@ ssize_t cachefiles_ondemand_daemon_read(struct cachefiles_cache *cache,
>  	cachefiles_grab_object(req->object, cachefiles_obj_get_read_req);
>  	xa_unlock(&cache->reqs);
>  
> -	id = xas.xa_index;
> -
>  	if (msg->opcode == CACHEFILES_OP_OPEN) {
>  		ret = cachefiles_ondemand_get_fd(req);
>  		if (ret) {
>  			cachefiles_ondemand_set_object_close(req->object);
> -			goto error;
> +			goto out;
>  		}
>  	}
>  
> -	msg->msg_id = id;
> +	msg->msg_id = xas.xa_index;
>  	msg->object_id = req->object->ondemand->ondemand_id;
>  
>  	if (copy_to_user(_buffer, msg, n) != 0) {
>  		ret = -EFAULT;
> -		goto err_put_fd;
> -	}
> -
> -	cachefiles_put_object(req->object, cachefiles_obj_put_read_req);
> -	/* CLOSE request has no reply */
> -	if (msg->opcode == CACHEFILES_OP_CLOSE) {
> -		xa_erase(&cache->reqs, id);
> -		complete(&req->done);
> +		if (msg->opcode == CACHEFILES_OP_OPEN)
> +			close_fd(((struct cachefiles_open *)msg->data)->fd);
>  	}
> -
> -	cachefiles_req_put(req);
> -	return n;
> -
> -err_put_fd:
> -	if (msg->opcode == CACHEFILES_OP_OPEN)
> -		close_fd(((struct cachefiles_open *)msg->data)->fd);
> -error:
> +out:
>  	cachefiles_put_object(req->object, cachefiles_obj_put_read_req);
> -	xas_reset(&xas);
> -	xas_lock(&xas);
> -	if (xas_load(&xas) == req) {
> -		req->error = ret;
> -		complete(&req->done);
> -		xas_store(&xas, NULL);
> +	/* Remove error request and CLOSE request has no reply */
> +	if (ret || msg->opcode == CACHEFILES_OP_CLOSE) {
> +		xas_reset(&xas);
> +		xas_lock(&xas);
> +		if (xas_load(&xas) == req) {
> +			req->error = ret;
> +			complete(&req->done);
> +			xas_store(&xas, NULL);
> +		}
> +		xas_unlock(&xas);
>  	}
> -	xas_unlock(&xas);
>  	cachefiles_req_put(req);
> -	return ret;
> +	return ret ? ret : n;
>  }
>  
>  typedef int (*init_req_fn)(struct cachefiles_req *req, void *private);

-- 
Thanks,
Jingbo