[lustre-devel] [PATCH 08/12] lustre: mdc: propagate changelog errors to readers

[NeilBrown]

On Sun, Nov 25 2018, James Simmons wrote:

> From: "John L. Hammond" <jhammond at whamcloud.com>
>
> Store errors encountered by the changelog llog reader thread in the
> crs_err field of struct changelog_reader_state so that they can be
> propageted to changelog readers. In chlg_read() improve the error and
> EOF reporting. Return -ERESTARTSYS when the blocked reader is
> interrupted. Replace uses of wait_event_idle() with
> ait_event_interruptible().
>
> Signed-off-by: John L. Hammond <jhammond at whamcloud.com>
> WC-bug-id: https://jira.whamcloud.com/browse/LU-10218
> Reviewed-on: https://review.whamcloud.com/30040
> Reviewed-by: Quentin Bouget <quentin.bouget at cea.fr>
> Reviewed-by: Henri Doreau <henri.doreau at cea.fr>
> Reviewed-by: Oleg Drokin <green at whamcloud.com>
> Signed-off-by: James Simmons <jsimmons at infradead.org>
> ---
>  drivers/staging/lustre/lustre/mdc/mdc_changelog.c | 45 ++++++++++++++---------
>  1 file changed, 28 insertions(+), 17 deletions(-)
>
> diff --git a/drivers/staging/lustre/lustre/mdc/mdc_changelog.c b/drivers/staging/lustre/lustre/mdc/mdc_changelog.c
> index becdee8..811a36a 100644
> --- a/drivers/staging/lustre/lustre/mdc/mdc_changelog.c
> +++ b/drivers/staging/lustre/lustre/mdc/mdc_changelog.c
> @@ -71,7 +71,7 @@ struct chlg_reader_state {
>  	/* Producer thread (if any) */
>  	struct task_struct	*crs_prod_task;
>  	/* An error occurred that prevents from reading further */
> -	bool			 crs_err;
> +	int			 crs_err;
>  	/* EOF, no more records available */
>  	bool			 crs_eof;
>  	/* Desired start position */
> @@ -148,9 +148,9 @@ static int chlg_read_cat_process_cb(const struct lu_env *env,
>  	       PFID(&rec->cr.cr_tfid), PFID(&rec->cr.cr_pfid),
>  	       rec->cr.cr_namelen, changelog_rec_name(&rec->cr));
>  
> -	wait_event_idle(crs->crs_waitq_prod,
> -			(crs->crs_rec_count < CDEV_CHLG_MAX_PREFETCH ||
> -			 kthread_should_stop()));
> +	wait_event_interruptible(crs->crs_waitq_prod,
> +				 crs->crs_rec_count < CDEV_CHLG_MAX_PREFETCH ||
> +				 kthread_should_stop());

This is wrong.  Not harmful, but wrong.
This is in a kernel thread, so it doesn't expect to receive signals.
So allowing an abort on a signal is pointless and misleading.

So I've removed this chunk from the patch, and also a later chunk which
uses wait_event_interruptible() in a kthread.

Other places where wait_event_interruptible() are used make sense.

Thanks,
NeilBrown


>  
>  	if (kthread_should_stop())
>  		return LLOG_PROC_BREAK;
> @@ -231,7 +231,7 @@ static int chlg_load(void *args)
>  
>  err_out:
>  	if (rc < 0)
> -		crs->crs_err = true;
> +		crs->crs_err = rc;
>  
>  	wake_up_all(&crs->crs_waitq_cons);
>  
> @@ -241,7 +241,7 @@ static int chlg_load(void *args)
>  	if (ctx)
>  		llog_ctxt_put(ctx);
>  
> -	wait_event_idle(crs->crs_waitq_prod, kthread_should_stop());
> +	wait_event_interruptible(crs->crs_waitq_prod, kthread_should_stop());
>  
>  	return rc;
>  }
> @@ -264,13 +264,20 @@ static ssize_t chlg_read(struct file *file, char __user *buff, size_t count,
>  	struct chlg_reader_state *crs = file->private_data;
>  	struct chlg_rec_entry *rec;
>  	struct chlg_rec_entry *tmp;
> -	ssize_t  written_total = 0;
> +	ssize_t written_total = 0;
>  	LIST_HEAD(consumed);
> +	size_t rc;
> +
> +	if (file->f_flags & O_NONBLOCK && crs->crs_rec_count == 0) {
> +		if (crs->crs_err < 0)
> +			return crs->crs_err;
> +		else if (crs->crs_eof)
> +			return 0;
> +		else
> +			return -EAGAIN;
> +	}
>  
> -	if (file->f_flags & O_NONBLOCK && crs->crs_rec_count == 0)
> -		return -EAGAIN;
> -
> -	wait_event_idle(crs->crs_waitq_cons,
> +	rc = wait_event_interruptible(crs->crs_waitq_cons,
>  			crs->crs_rec_count > 0 || crs->crs_eof || crs->crs_err);
>  
>  	mutex_lock(&crs->crs_lock);
> @@ -279,8 +286,7 @@ static ssize_t chlg_read(struct file *file, char __user *buff, size_t count,
>  			break;
>  
>  		if (copy_to_user(buff, rec->enq_record, rec->enq_length)) {
> -			if (written_total == 0)
> -				written_total = -EFAULT;
> +			rc = -EFAULT;
>  			break;
>  		}
>  
> @@ -294,15 +300,19 @@ static ssize_t chlg_read(struct file *file, char __user *buff, size_t count,
>  	}
>  	mutex_unlock(&crs->crs_lock);
>  
> -	if (written_total > 0)
> +	if (written_total > 0) {
> +		rc = written_total;
>  		wake_up_all(&crs->crs_waitq_prod);
> +	} else if (rc == 0) {
> +		rc = crs->crs_err;
> +	}
>  
>  	list_for_each_entry_safe(rec, tmp, &consumed, enq_linkage)
>  		enq_record_delete(rec);
>  
>  	*ppos = crs->crs_start_offset;
>  
> -	return written_total;
> +	return rc;
>  }
>  
>  /**
> @@ -509,15 +519,16 @@ static int chlg_release(struct inode *inode, struct file *file)
>  	struct chlg_reader_state *crs = file->private_data;
>  	struct chlg_rec_entry *rec;
>  	struct chlg_rec_entry *tmp;
> +	int rc = 0;
>  
>  	if (crs->crs_prod_task)
> -		kthread_stop(crs->crs_prod_task);
> +		rc = kthread_stop(crs->crs_prod_task);
>  
>  	list_for_each_entry_safe(rec, tmp, &crs->crs_rec_queue, enq_linkage)
>  		enq_record_delete(rec);
>  
>  	kfree(crs);
> -	return 0;
> +	return rc;
>  }
>  
>  /**
> -- 
> 1.8.3.1
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 832 bytes
Desc: not available
URL: <http://lists.lustre.org/pipermail/lustre-devel-lustre.org/attachments/20181127/82e37de0/attachment-0001.sig>