[Devel] [PATCH 1/2] fs/fuse kio_pcs: handle error of submit_size_grow()

[Kirill Tkhai]

On 07.06.2018 17:40, Pavel Butsykin wrote:
> Before continuing write requests, we need to check the procedure size grow was
> successful. If the size attribute of a file failed to increase, it makes no
> sense to continue to push write requests because they will not be able to
> succeed until the file size will match.
> 
> Signed-off-by: Pavel Butsykin <pbutsykin at virtuozzo.com>
> ---
>  fs/fuse/kio/pcs/pcs_fuse_kdirect.c | 14 +++++++++++++-
>  fs/fuse/kio/pcs/pcs_map.c          | 34 ----------------------------------
>  fs/fuse/kio/pcs/pcs_req.c          | 31 +++++++++++++++++++++++++++++++
>  fs/fuse/kio/pcs/pcs_req.h          |  2 ++
>  4 files changed, 46 insertions(+), 35 deletions(-)
> 
> diff --git a/fs/fuse/kio/pcs/pcs_fuse_kdirect.c b/fs/fuse/kio/pcs/pcs_fuse_kdirect.c
> index cf97a5ce0e50..1a6776f7977a 100644
> --- a/fs/fuse/kio/pcs/pcs_fuse_kdirect.c
> +++ b/fs/fuse/kio/pcs/pcs_fuse_kdirect.c
> @@ -654,6 +654,7 @@ static void fuse_size_grow_work(struct work_struct *w)
>  	struct inode *inode = &di->inode->inode;
>  	struct pcs_int_request* ireq, *next;
>  	unsigned long long size = 0;
> +	int err;
>  	LIST_HEAD(to_submit);
>  
>  	spin_lock(&di->lock);
> @@ -673,7 +674,18 @@ static void fuse_size_grow_work(struct work_struct *w)
>  	}
>  	di->size.required = size;
>  	spin_unlock(&di->lock);
> -	submit_size_grow(inode, size);
> +
> +	err = submit_size_grow(inode, size);
> +	if (err) {
> +		LIST_HEAD(to_fail);
> +
> +		spin_lock(&di->lock);
> +		list_splice_tail_init(&di->size.grow_queue, &to_fail);
> +		spin_unlock(&di->lock);
> +
> +		pcs_ireq_queue_fail(&to_fail, err);
> +		return;

In case of we return here, we never reset di->size.required. So, next time
we enter fuse_size_grow_work(), it will exit on the first "if", and this
work will never work.

> +	}
>  
>  	spin_lock(&di->lock);
>  	BUG_ON(di->size.shrink);
> diff --git a/fs/fuse/kio/pcs/pcs_map.c b/fs/fuse/kio/pcs/pcs_map.c
> index 90e311f7e995..78e773536bb4 100644
> --- a/fs/fuse/kio/pcs/pcs_map.c
> +++ b/fs/fuse/kio/pcs/pcs_map.c
> @@ -26,8 +26,6 @@
>  */
>  #define MAP_BATCH 16
>  
> -static void pcs_ireq_queue_fail(struct list_head *queue, int error);
> -
>  abs_time_t get_real_time_ms(void)
>  {
>  	struct timespec tv = current_kernel_time();
> @@ -138,7 +136,6 @@ static void pcs_map_reset(struct pcs_map_entry * m)
>  {
>  	m->state &= ~(PCS_MAP_READABLE|PCS_MAP_WRITEABLE);
>  }
> -static void pcs_ireq_queue_fail(struct list_head *queue, int error);
>  static void map_sync_work_add(struct pcs_map_entry *m, unsigned long timeout);
>  static void map_sync_work_del(struct pcs_map_entry *m);
>  
> @@ -724,37 +721,6 @@ void pcs_map_notify_addr_change(struct pcs_cs * cs)
>  	}
>  }
>  
> -noinline static void pcs_ireq_queue_fail(struct list_head *queue, int error)
> -{
> -	while (!list_empty(queue)) {
> -		struct pcs_int_request *ireq = list_first_entry(queue, struct pcs_int_request, list);
> -
> -		list_del_init(&ireq->list);
> -
> -		pcs_set_local_error(&ireq->error, error);
> -
> -		if (ireq->type == PCS_IREQ_TRUNCATE) {
> -			ireq_on_error(ireq);
> -
> -			if (!(ireq->flags & IREQ_F_FATAL)) {
> -				if (ireq_is_timed_out(ireq)) {
> -					pcs_log(LOG_ERR, "timeout while truncate(%d) request on \"" DENTRY_FMT "\" last err=%u",
> -						ireq->type, DENTRY_ARGS(ireq->dentry), ireq->error.value);
> -					BUG();
> -				}
> -				pcs_clear_error(&ireq->error);
> -
> -				TRACE("requeue truncate(%d) %llu@" DENTRY_FMT "\n", ireq->type,
> -				      (unsigned long long)ireq->truncreq.offset, DENTRY_ARGS(ireq->dentry));
> -
> -				ireq_delay(ireq);
> -				continue;
> -			}
> -		}
> -		ireq_complete(ireq);
> -	}
> -}

I don't understand why we need to move this function. In case of we need to do that,
this should be made in a separate patch and write, that it's just a move
Otherwise, it's not possible to understand either the function was changed
or not.

>  void transfer_sync_data(struct pcs_cs_list * new_cs_list, struct pcs_cs_list * old_cs_list)
>  {
>  	int i, k;
> diff --git a/fs/fuse/kio/pcs/pcs_req.c b/fs/fuse/kio/pcs/pcs_req.c
> index a05f1e43c94c..9516768c47d4 100644
> --- a/fs/fuse/kio/pcs/pcs_req.c
> +++ b/fs/fuse/kio/pcs/pcs_req.c
> @@ -121,3 +121,34 @@ void ireq_handle_hole(struct pcs_int_request *ireq)
>  
>  	ireq_complete(ireq);
>  }
> +
> +noinline void pcs_ireq_queue_fail(struct list_head *queue, int error)
> +{
> +	while (!list_empty(queue)) {
> +		struct pcs_int_request *ireq = list_first_entry(queue, struct pcs_int_request, list);
> +
> +		list_del_init(&ireq->list);
> +
> +		pcs_set_local_error(&ireq->error, error);
> +
> +		if (ireq->type == PCS_IREQ_TRUNCATE) {
> +			ireq_on_error(ireq);
> +
> +			if (!(ireq->flags & IREQ_F_FATAL)) {
> +				if (ireq_is_timed_out(ireq)) {
> +					pcs_log(LOG_ERR, "timeout while truncate(%d) request on \"" DENTRY_FMT "\" last err=%u",
> +						ireq->type, DENTRY_ARGS(ireq->dentry), ireq->error.value);
> +					BUG();
> +				}
> +				pcs_clear_error(&ireq->error);
> +
> +				TRACE("requeue truncate(%d) %llu@" DENTRY_FMT "\n", ireq->type,
> +				      (unsigned long long)ireq->truncreq.offset, DENTRY_ARGS(ireq->dentry));
> +
> +				ireq_delay(ireq);
> +				continue;
> +			}
> +		}
> +		ireq_complete(ireq);
> +	}
> +}
> diff --git a/fs/fuse/kio/pcs/pcs_req.h b/fs/fuse/kio/pcs/pcs_req.h
> index 6f49018e3988..557e8e476856 100644
> --- a/fs/fuse/kio/pcs/pcs_req.h
> +++ b/fs/fuse/kio/pcs/pcs_req.h
> @@ -332,4 +332,6 @@ void ireq_handle_hole(struct pcs_int_request *ireq);
>  
>  void pcs_process_ireq(struct pcs_int_request *ireq);
>  
> +void pcs_ireq_queue_fail(struct list_head *queue, int error);
> +
>  #endif /* _PCS_REQ_H_ */
>