Re: [PATCH 3/4] ublk: merge stop_work and quiesce_work

[Ming Lei <ming.lei@redhat.com>]

On Mon, Jun 17, 2024 at 01:44:50PM -0600, Uday Shankar wrote:
> Save some lines by merging stop_work and quiesce_work into nosrv_work,
> which looks at the recovery flags and does the right thing when the "no
> ublk server" condition is detected.
> 
> Signed-off-by: Uday Shankar <ushankar@purestorage.com>
> ---
>  drivers/block/ublk_drv.c | 64 ++++++++++++++++------------------------
>  1 file changed, 25 insertions(+), 39 deletions(-)
> 
> diff --git a/drivers/block/ublk_drv.c b/drivers/block/ublk_drv.c
> index e8cca58a71bc..0496fa372cc1 100644
> --- a/drivers/block/ublk_drv.c
> +++ b/drivers/block/ublk_drv.c
> @@ -182,8 +182,7 @@ struct ublk_device {
>  	unsigned int		nr_queues_ready;
>  	unsigned int		nr_privileged_daemon;
>  
> -	struct work_struct	quiesce_work;
> -	struct work_struct	stop_work;
> +	struct work_struct	nosrv_work;
>  };
>  
>  /* header of ublk_params */
> @@ -1229,10 +1228,7 @@ static enum blk_eh_timer_return ublk_timeout(struct request *rq)
>  		struct ublk_device *ub = ubq->dev;
>  
>  		if (ublk_abort_requests(ub, ubq)) {
> -			if (ublk_nosrv_should_stop_dev(ub))
> -				schedule_work(&ub->stop_work);
> -			else
> -				schedule_work(&ub->quiesce_work);
> +			schedule_work(&ub->nosrv_work);
>  		}
>  		return BLK_EH_DONE;
>  	}
> @@ -1482,10 +1478,7 @@ static void ublk_uring_cmd_cancel_fn(struct io_uring_cmd *cmd,
>  	ublk_cancel_cmd(ubq, io, issue_flags);
>  
>  	if (need_schedule) {
> -		if (ublk_nosrv_should_stop_dev(ub))
> -			schedule_work(&ub->stop_work);
> -		else
> -			schedule_work(&ub->quiesce_work);
> +		schedule_work(&ub->nosrv_work);
>  	}
>  }
>  
> @@ -1548,20 +1541,6 @@ static void __ublk_quiesce_dev(struct ublk_device *ub)
>  	ub->dev_info.state = UBLK_S_DEV_QUIESCED;
>  }
>  
> -static void ublk_quiesce_work_fn(struct work_struct *work)
> -{
> -	struct ublk_device *ub =
> -		container_of(work, struct ublk_device, quiesce_work);
> -
> -	mutex_lock(&ub->mutex);
> -	if (ub->dev_info.state != UBLK_S_DEV_LIVE)
> -		goto unlock;
> -	__ublk_quiesce_dev(ub);
> - unlock:
> -	mutex_unlock(&ub->mutex);
> -	ublk_cancel_dev(ub);
> -}
> -
>  static void ublk_unquiesce_dev(struct ublk_device *ub)
>  {
>  	int i;
> @@ -1610,6 +1589,25 @@ static void ublk_stop_dev(struct ublk_device *ub)
>  	ublk_cancel_dev(ub);
>  }
>  
> +static void ublk_nosrv_work(struct work_struct *work)
> +{
> +	struct ublk_device *ub =
> +		container_of(work, struct ublk_device, nosrv_work);
> +
> +	if (ublk_nosrv_should_stop_dev(ub)) {
> +		ublk_stop_dev(ub);
> +		return;
> +	}
> +
> +	mutex_lock(&ub->mutex);
> +	if (ub->dev_info.state != UBLK_S_DEV_LIVE)
> +		goto unlock;
> +	__ublk_quiesce_dev(ub);
> + unlock:
> +	mutex_unlock(&ub->mutex);
> +	ublk_cancel_dev(ub);
> +}
> +
>  /* device can only be started after all IOs are ready */
>  static void ublk_mark_io_ready(struct ublk_device *ub, struct ublk_queue *ubq)
>  {
> @@ -2124,14 +2122,6 @@ static int ublk_add_chdev(struct ublk_device *ub)
>  	return ret;
>  }
>  
> -static void ublk_stop_work_fn(struct work_struct *work)
> -{
> -	struct ublk_device *ub =
> -		container_of(work, struct ublk_device, stop_work);
> -
> -	ublk_stop_dev(ub);
> -}
> -
>  /* align max io buffer size with PAGE_SIZE */
>  static void ublk_align_max_io_size(struct ublk_device *ub)
>  {
> @@ -2156,8 +2146,7 @@ static int ublk_add_tag_set(struct ublk_device *ub)
>  static void ublk_remove(struct ublk_device *ub)
>  {
>  	ublk_stop_dev(ub);
> -	cancel_work_sync(&ub->stop_work);
> -	cancel_work_sync(&ub->quiesce_work);
> +	cancel_work_sync(&ub->nosrv_work);
>  	cdev_device_del(&ub->cdev, &ub->cdev_dev);
>  	ublk_put_device(ub);
>  	ublks_added--;
> @@ -2412,8 +2401,7 @@ static int ublk_ctrl_add_dev(struct io_uring_cmd *cmd)
>  		goto out_unlock;
>  	mutex_init(&ub->mutex);
>  	spin_lock_init(&ub->lock);
> -	INIT_WORK(&ub->quiesce_work, ublk_quiesce_work_fn);
> -	INIT_WORK(&ub->stop_work, ublk_stop_work_fn);
> +	INIT_WORK(&ub->nosrv_work, ublk_nosrv_work);
>  
>  	ret = ublk_alloc_dev_number(ub, header->dev_id);
>  	if (ret < 0)
> @@ -2548,9 +2536,7 @@ static inline void ublk_ctrl_cmd_dump(struct io_uring_cmd *cmd)
>  static int ublk_ctrl_stop_dev(struct ublk_device *ub)
>  {
>  	ublk_stop_dev(ub);
> -	cancel_work_sync(&ub->stop_work);
> -	cancel_work_sync(&ub->quiesce_work);
> -
> +	cancel_work_sync(&ub->nosrv_work);
>  	return 0;
>  }

Looks fine,

Reviewed-by: Ming Lei <ming.lei@redhat.com>

Thanks,
Ming