Re: [RESEND PATCH 1/5] loop: remove 'rw' parameter from lo_rw_aio()

[Christoph Hellwig <hch@infradead.org>]

On Sun, Mar 09, 2025 at 12:23:05AM +0800, Ming Lei wrote:
> lo_rw_aio() is only called for READ/WRITE operation, which can be
> figured out from request directly, so remove 'rw' parameter from
> lo_rw_aio(), meantime rename the local variable as 'dir' which matches
> the actual use more.
> 
> Meantime merge lo_read_simple() and lo_write_simple() into
> lo_rw_simple().

That's two entirely separate things, please split them into separate
patches.

static int lo_rw_simple(struct loop_device *lo, struct request *rq, loff_t pos)
>  {
>  	struct bio_vec bvec;
>  	struct req_iterator iter;
>  	struct iov_iter i;
>  	ssize_t len;
>  
> +	if (req_op(rq) == REQ_OP_WRITE) {
> +		int ret;
> +
> +		rq_for_each_segment(bvec, rq, iter) {
> +			ret = lo_write_bvec(lo->lo_backing_file, &bvec, &pos);
> +			if (ret < 0)
> +				break;
> +			cond_resched();
> +		}
> +		return ret;
> +	}
> +
>  	rq_for_each_segment(bvec, rq, iter) {

.. and nothing is really merged here.  So unless you actually merge
some code later this part doesn't seem particularly useful.

>  	struct request *rq = blk_mq_rq_from_pdu(cmd);
> +	int dir = (req_op(rq) == REQ_OP_READ) ? ITER_DEST : ITER_SOURCE;
>  	struct bio *bio = rq->bio;
>  	struct file *file = lo->lo_backing_file;
>  	struct bio_vec tmp;
> @@ -448,7 +442,7 @@ static int lo_rw_aio(struct loop_device *lo, struct loop_cmd *cmd,
>  	}
>  	atomic_set(&cmd->ref, 2);
>  
> -	iov_iter_bvec(&iter, rw, bvec, nr_bvec, blk_rq_bytes(rq));
> +	iov_iter_bvec(&iter, dir, bvec, nr_bvec, blk_rq_bytes(rq));
>  	iter.iov_offset = offset;
>  
>  	cmd->iocb.ki_pos = pos;
> @@ -457,7 +451,7 @@ static int lo_rw_aio(struct loop_device *lo, struct loop_cmd *cmd,
>  	cmd->iocb.ki_flags = IOCB_DIRECT;
>  	cmd->iocb.ki_ioprio = IOPRIO_PRIO_VALUE(IOPRIO_CLASS_NONE, 0);
>  
> -	if (rw == ITER_SOURCE)
> +	if (dir == ITER_SOURCE)


i'd just use the request direction check here, and then open code
the iter source/dest in the iov_iter_bvec call.

>  	case REQ_OP_READ:
>  		if (cmd->use_aio)
> -			return lo_rw_aio(lo, cmd, pos, ITER_DEST);
> +			return lo_rw_aio(lo, cmd, pos);
>  		else
> -			return lo_read_simple(lo, rq, pos);
> +			return lo_rw_simple(lo, rq, pos);

Not entirely new here, but there is no reason to use an else after a
return.