Re: [PATCH 1/6] ioprio: move io priority from task_struct to io_context

[Andrew Morton <akpm@linux-foundation.org>]

> On Tue, 22 Jan 2008 10:49:16 +0100 Jens Axboe <jens.axboe@oracle.com> wrote:
> This is where it belongs and then it doesn't take up space for a
> process that doesn't do IO.
> 
> ...
>
>  struct io_context *get_io_context(gfp_t gfp_flags, int node)
>  {
> -	struct io_context *ret;
> -	ret = current_io_context(gfp_flags, node);
> -	if (likely(ret))
> -		atomic_inc(&ret->refcount);
> +	struct io_context *ret = NULL;
> +
> +	do {
> +		ret = current_io_context(gfp_flags, node);
> +		if (unlikely(!ret))
> +			break;
> +	} while (!atomic_inc_not_zero(&ret->refcount));

Looks weird.  Could do with a comment.  Or unweirding ;)

What's going on here?

>  	return ret;
>  }
>  EXPORT_SYMBOL(get_io_context);
> diff --git a/fs/ioprio.c b/fs/ioprio.c
> index e4e01bc..a760040 100644
> --- a/fs/ioprio.c
> +++ b/fs/ioprio.c
> @@ -41,18 +41,29 @@ static int set_task_ioprio(struct task_struct *task, int ioprio)
>  		return err;
>  
>  	task_lock(task);
> +	do {
> +		ioc = task->io_context;
> +		/* see wmb() in current_io_context() */
> +		smp_read_barrier_depends();
> +		if (ioc)
> +			break;
>  
> -	task->ioprio = ioprio;
> -
> -	ioc = task->io_context;
> -	/* see wmb() in current_io_context() */
> -	smp_read_barrier_depends();
> +		ioc = alloc_io_context(GFP_ATOMIC, -1);
> +		if (!ioc) {
> +			err = -ENOMEM;
> +			break;
> +		}
> +		task->io_context = ioc;
> +		ioc->task = task;
> +	} while (1);

argh.  Can't sit there in a loop retrying GFP_ATOMIC!

> -	if (ioc)
> +	if (!err) {
> +		ioc->ioprio = ioprio;
>  		ioc->ioprio_changed = 1;
> +	}
>  
>  	task_unlock(task);
> -	return 0;
> +	return err;
>  }
>  
>  asmlinkage long sys_ioprio_set(int which, int who, int ioprio)
>
> ...
>
>  void put_io_context(struct io_context *ioc);
>  void exit_io_context(void);
>  struct io_context *get_io_context(gfp_t gfp_flags, int node);
> +struct io_context *alloc_io_context(gfp_t, int);
>  void copy_io_context(struct io_context **pdst, struct io_context **psrc);
>  void swap_io_context(struct io_context **ioc1, struct io_context **ioc2);

The rest of the declarations around here nicely name their args.

> +static int copy_io(struct task_struct *tsk)
> +{
> +	struct io_context *ioc = current->io_context;
> +
> +	if (!ioc)
> +		return 0;
> +
> +	if (ioprio_valid(ioc->ioprio)) {
> +		tsk->io_context = alloc_io_context(GFP_KERNEL, -1);
> +		if (unlikely(!tsk->io_context))
> +			return -ENOMEM;
> +
> +		tsk->io_context->task = tsk;
> +		tsk->io_context->ioprio = ioc->ioprio;
> +	}
> +
> +	return 0;
> +}

Should this depend on CONFIG_BLOCK?