| Message ID | 20250308162312.1640828-2-ming.lei@redhat.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | loop: improve loop aio perf by IOCB_NOWAIT | expand |
Hi Ming,
kernel test robot noticed the following build warnings:
[auto build test WARNING on axboe-block/for-next]
[also build test WARNING on linus/master v6.14-rc5 next-20250307]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]
url: https://github.com/intel-lab-lkp/linux/commits/Ming-Lei/loop-remove-rw-parameter-from-lo_rw_aio/20250309-002548
base: https://git.kernel.org/pub/scm/linux/kernel/git/axboe/linux-block.git for-next
patch link: https://lore.kernel.org/r/20250308162312.1640828-2-ming.lei%40redhat.com
patch subject: [RESEND PATCH 1/5] loop: remove 'rw' parameter from lo_rw_aio()
config: arm-randconfig-001-20250309 (https://download.01.org/0day-ci/archive/20250309/202503091516.7U64QwvF-lkp@intel.com/config)
compiler: clang version 21.0.0git (https://github.com/llvm/llvm-project e15545cad8297ec7555f26e5ae74a9f0511203e7)
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20250309/202503091516.7U64QwvF-lkp@intel.com/reproduce)
If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202503091516.7U64QwvF-lkp@intel.com/
All warnings (new ones prefixed by >>):
>> drivers/block/loop.c:250:3: warning: variable 'ret' is used uninitialized whenever 'if' condition is false [-Wsometimes-uninitialized]
250 | rq_for_each_segment(bvec, rq, iter) {
| ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
include/linux/blk-mq.h:1043:2: note: expanded from macro 'rq_for_each_segment'
1043 | __rq_for_each_bio(_iter.bio, _rq) \
| ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
include/linux/blk-mq.h:1039:6: note: expanded from macro '__rq_for_each_bio'
1039 | if ((rq->bio)) \
| ^~~~~~~~~
drivers/block/loop.c:256:10: note: uninitialized use occurs here
256 | return ret;
| ^~~
drivers/block/loop.c:250:3: note: remove the 'if' if its condition is always true
250 | rq_for_each_segment(bvec, rq, iter) {
| ^
include/linux/blk-mq.h:1043:2: note: expanded from macro 'rq_for_each_segment'
1043 | __rq_for_each_bio(_iter.bio, _rq) \
| ^
include/linux/blk-mq.h:1039:2: note: expanded from macro '__rq_for_each_bio'
1039 | if ((rq->bio)) \
| ^
drivers/block/loop.c:248:10: note: initialize the variable 'ret' to silence this warning
248 | int ret;
| ^
| = 0
1 warning generated.
vim +250 drivers/block/loop.c
^1da177e4c3f41 Linus Torvalds 2005-04-16 239
e1d39dbf4716cd Ming Lei 2025-03-09 240 static int lo_rw_simple(struct loop_device *lo, struct request *rq, loff_t pos)
^1da177e4c3f41 Linus Torvalds 2005-04-16 241 {
aa4d86163e4e91 Christoph Hellwig 2015-04-07 242 struct bio_vec bvec;
aa4d86163e4e91 Christoph Hellwig 2015-04-07 243 struct req_iterator iter;
e1d39dbf4716cd Ming Lei 2025-03-09 244 struct iov_iter i;
e1d39dbf4716cd Ming Lei 2025-03-09 245 ssize_t len;
e1d39dbf4716cd Ming Lei 2025-03-09 246
e1d39dbf4716cd Ming Lei 2025-03-09 247 if (req_op(rq) == REQ_OP_WRITE) {
e1d39dbf4716cd Ming Lei 2025-03-09 248 int ret;
aa4d86163e4e91 Christoph Hellwig 2015-04-07 249
aa4d86163e4e91 Christoph Hellwig 2015-04-07 @250 rq_for_each_segment(bvec, rq, iter) {
aa4d86163e4e91 Christoph Hellwig 2015-04-07 251 ret = lo_write_bvec(lo->lo_backing_file, &bvec, &pos);
aa4d86163e4e91 Christoph Hellwig 2015-04-07 252 if (ret < 0)
aa4d86163e4e91 Christoph Hellwig 2015-04-07 253 break;
^1da177e4c3f41 Linus Torvalds 2005-04-16 254 cond_resched();
^1da177e4c3f41 Linus Torvalds 2005-04-16 255 }
aa4d86163e4e91 Christoph Hellwig 2015-04-07 256 return ret;
aa4d86163e4e91 Christoph Hellwig 2015-04-07 257 }
aa4d86163e4e91 Christoph Hellwig 2015-04-07 258
aa4d86163e4e91 Christoph Hellwig 2015-04-07 259 rq_for_each_segment(bvec, rq, iter) {
de4eda9de2d957 Al Viro 2022-09-15 260 iov_iter_bvec(&i, ITER_DEST, &bvec, 1, bvec.bv_len);
18e9710ee59ce3 Christoph Hellwig 2017-05-27 261 len = vfs_iter_read(lo->lo_backing_file, &i, &pos, 0);
aa4d86163e4e91 Christoph Hellwig 2015-04-07 262 if (len < 0)
aa4d86163e4e91 Christoph Hellwig 2015-04-07 263 return len;
aa4d86163e4e91 Christoph Hellwig 2015-04-07 264
aa4d86163e4e91 Christoph Hellwig 2015-04-07 265 flush_dcache_page(bvec.bv_page);
^1da177e4c3f41 Linus Torvalds 2005-04-16 266
aa4d86163e4e91 Christoph Hellwig 2015-04-07 267 if (len != bvec.bv_len) {
aa4d86163e4e91 Christoph Hellwig 2015-04-07 268 struct bio *bio;
fd5821404e6823 Jens Axboe 2007-06-12 269
aa4d86163e4e91 Christoph Hellwig 2015-04-07 270 __rq_for_each_bio(bio, rq)
aa4d86163e4e91 Christoph Hellwig 2015-04-07 271 zero_fill_bio(bio);
aa4d86163e4e91 Christoph Hellwig 2015-04-07 272 break;
aa4d86163e4e91 Christoph Hellwig 2015-04-07 273 }
aa4d86163e4e91 Christoph Hellwig 2015-04-07 274 cond_resched();
^1da177e4c3f41 Linus Torvalds 2005-04-16 275 }
^1da177e4c3f41 Linus Torvalds 2005-04-16 276
aa4d86163e4e91 Christoph Hellwig 2015-04-07 277 return 0;
fd5821404e6823 Jens Axboe 2007-06-12 278 }
fd5821404e6823 Jens Axboe 2007-06-12 279
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.
diff --git a/drivers/block/loop.c b/drivers/block/loop.c index 657bf53decf3..6bbbaa4aaf2c 100644 --- a/drivers/block/loop.c +++ b/drivers/block/loop.c @@ -239,31 +239,25 @@ static int lo_write_bvec(struct file *file, struct bio_vec *bvec, loff_t *ppos) return bw; } -static int lo_write_simple(struct loop_device *lo, struct request *rq, - loff_t pos) -{ - struct bio_vec bvec; - struct req_iterator iter; - int ret = 0; - - 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; -} - -static int lo_read_simple(struct loop_device *lo, struct request *rq, - loff_t pos) +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) { iov_iter_bvec(&i, ITER_DEST, &bvec, 1, bvec.bv_len); len = vfs_iter_read(lo->lo_backing_file, &i, &pos, 0); @@ -400,13 +394,13 @@ static void lo_rw_aio_complete(struct kiocb *iocb, long ret) lo_rw_aio_do_completion(cmd); } -static int lo_rw_aio(struct loop_device *lo, struct loop_cmd *cmd, - loff_t pos, int rw) +static int lo_rw_aio(struct loop_device *lo, struct loop_cmd *cmd, loff_t pos) { struct iov_iter iter; struct req_iterator rq_iter; struct bio_vec *bvec; 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) ret = file->f_op->write_iter(&cmd->iocb, &iter); else ret = file->f_op->read_iter(&cmd->iocb, &iter); @@ -498,15 +492,11 @@ static int do_req_filebacked(struct loop_device *lo, struct request *rq) case REQ_OP_DISCARD: return lo_fallocate(lo, rq, pos, FALLOC_FL_PUNCH_HOLE); case REQ_OP_WRITE: - if (cmd->use_aio) - return lo_rw_aio(lo, cmd, pos, ITER_SOURCE); - else - return lo_write_simple(lo, rq, pos); 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); default: WARN_ON_ONCE(1); return -EIO;
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(). Signed-off-by: Ming Lei <ming.lei@redhat.com> --- drivers/block/loop.c | 48 ++++++++++++++++++-------------------------- 1 file changed, 19 insertions(+), 29 deletions(-)