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

Submitted by Pavel Butsykin on June 7, 2018, 2:40 p.m.

Details

Message ID 20180607144012.22213-2-pbutsykin@virtuozzo.com
State New
Series "fuse_kdirect: fsync hungs after unlink fixes"
Headers show

Commit Message

Pavel Butsykin June 7, 2018, 2:40 p.m.
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@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(-)

Patch hide | download patch | download mbox

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;
+	}
 
 	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);
-	}
-}
-
 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_ */

Comments

Pavel Butsykin June 7, 2018, 2:56 p.m.
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.
> 

Alexey,

I wonder whether we should abort pending requests for all errors or just
for fatal? for which errors we can retry FUSE_SETATTR request?

> Signed-off-by: Pavel Butsykin <pbutsykin@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;
> +	}
>   
>   	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);
> -	}
> -}
> -
>   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_ */
>
Kirill Tkhai June 8, 2018, 3:12 p.m.
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@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_ */
>