Re: [PATCH 1/2] block: loop: convert to per-device workqueue

[Dave Kleikamp <dave.kleikamp@oracle.com>]

On 05/05/2015 06:49 AM, Ming Lei wrote:
> Documentation/workqueue.txt:
> 	If there is dependency among multiple work items used
> 	during memory reclaim, they should be queued to separate
> 	wq each with WQ_MEM_RECLAIM.
> 
> Loop devices can be stacked, so we have to convert to per-device
> workqueue. One example is Fedora live CD.

I'm just now looking at this, but I found a minor nit. I'm not sure if
it's worth replacing the patch or just fixing it with a trivial cleanup
patch.

> 
> Fixes: b5dd2f6047ca108001328aac0e8588edd15f1778
> Cc: stable@vger.kernel.org (v4.0)
> Cc: Justin M. Forbes <jforbes@fedoraproject.org>
> Signed-off-by: Ming Lei <ming.lei@canonical.com>
> ---
>  drivers/block/loop.c | 30 ++++++++++++++----------------
>  drivers/block/loop.h |  1 +
>  2 files changed, 15 insertions(+), 16 deletions(-)
> 
> diff --git a/drivers/block/loop.c b/drivers/block/loop.c
> index ae3fcb4..3dc1598 100644
> --- a/drivers/block/loop.c
> +++ b/drivers/block/loop.c
> @@ -86,8 +86,6 @@ static DEFINE_MUTEX(loop_index_mutex);
>  static int max_part;
>  static int part_shift;
>  
> -static struct workqueue_struct *loop_wq;
> -
>  static int transfer_xor(struct loop_device *lo, int cmd,
>  			struct page *raw_page, unsigned raw_off,
>  			struct page *loop_page, unsigned loop_off,
> @@ -725,6 +723,12 @@ static int loop_set_fd(struct loop_device *lo, fmode_t mode,
>  	size = get_loop_size(lo, file);
>  	if ((loff_t)(sector_t)size != size)
>  		goto out_putf;
> +	error = -ENOMEM;
> +	lo->wq = alloc_workqueue("kloopd%d",
> +			WQ_MEM_RECLAIM | WQ_HIGHPRI | WQ_UNBOUND, 0,
> +			lo->lo_number);
> +	if (!lo->wq)
> +		goto out_putf;
>  
>  	error = 0;
>  
> @@ -872,6 +876,8 @@ static int loop_clr_fd(struct loop_device *lo)
>  	lo->lo_flags = 0;
>  	if (!part_shift)
>  		lo->lo_disk->flags |= GENHD_FL_NO_PART_SCAN;
> +	destroy_workqueue(lo->wq);
> +	lo->wq = NULL;
>  	mutex_unlock(&lo->lo_ctl_mutex);
>  	/*
>  	 * Need not hold lo_ctl_mutex to fput backing file.
> @@ -1425,9 +1431,13 @@ static int loop_queue_rq(struct blk_mq_hw_ctx *hctx,
>  		const struct blk_mq_queue_data *bd)
>  {
>  	struct loop_cmd *cmd = blk_mq_rq_to_pdu(bd->rq);
> +	struct loop_device *lo = cmd->rq->q->queuedata;
>  
>  	blk_mq_start_request(bd->rq);
>  
> +	if (lo->lo_state != Lo_bound)
> +		return -EIO;
> +
>  	if (cmd->rq->cmd_flags & REQ_WRITE) {
>  		struct loop_device *lo = cmd->rq->q->queuedata;

The above definition of lo becomes redundant now.

>  		bool need_sched = true;
> @@ -1441,9 +1451,9 @@ static int loop_queue_rq(struct blk_mq_hw_ctx *hctx,
>  		spin_unlock_irq(&lo->lo_lock);
>  
>  		if (need_sched)
> -			queue_work(loop_wq, &lo->write_work);
> +			queue_work(lo->wq, &lo->write_work);
>  	} else {
> -		queue_work(loop_wq, &cmd->read_work);
> +		queue_work(lo->wq, &cmd->read_work);
>  	}
>  
>  	return BLK_MQ_RQ_QUEUE_OK;
> @@ -1455,9 +1465,6 @@ static void loop_handle_cmd(struct loop_cmd *cmd)
>  	struct loop_device *lo = cmd->rq->q->queuedata;
>  	int ret = -EIO;
>  
> -	if (lo->lo_state != Lo_bound)
> -		goto failed;
> -
>  	if (write && (lo->lo_flags & LO_FLAGS_READ_ONLY))
>  		goto failed;
>  
> @@ -1806,13 +1813,6 @@ static int __init loop_init(void)
>  		goto misc_out;
>  	}
>  
> -	loop_wq = alloc_workqueue("kloopd",
> -			WQ_MEM_RECLAIM | WQ_HIGHPRI | WQ_UNBOUND, 0);
> -	if (!loop_wq) {
> -		err = -ENOMEM;
> -		goto misc_out;
> -	}
> -
>  	blk_register_region(MKDEV(LOOP_MAJOR, 0), range,
>  				  THIS_MODULE, loop_probe, NULL, NULL);
>  
> @@ -1850,8 +1850,6 @@ static void __exit loop_exit(void)
>  	blk_unregister_region(MKDEV(LOOP_MAJOR, 0), range);
>  	unregister_blkdev(LOOP_MAJOR, "loop");
>  
> -	destroy_workqueue(loop_wq);
> -
>  	misc_deregister(&loop_misc);
>  }
>  
> diff --git a/drivers/block/loop.h b/drivers/block/loop.h
> index 301c27f..49564ed 100644
> --- a/drivers/block/loop.h
> +++ b/drivers/block/loop.h
> @@ -54,6 +54,7 @@ struct loop_device {
>  	gfp_t		old_gfp_mask;
>  
>  	spinlock_t		lo_lock;
> +	struct workqueue_struct *wq;
>  	struct list_head	write_cmd_head;
>  	struct work_struct	write_work;
>  	bool			write_started;
>