[RFC v2 00/17] bio cleanups

Andreas Gruenbacher <[email protected]>
Newsgroups gmane.linux.kernel.device-mapper.devel,gmane.linux.block,gmane.comp.file-systems.btrfs,gmane.linux.raid,gmane.linux.kernel
Message-ID <[email protected]>
Hello,

we are not quite careful enough about setting bio->bi_status in all
places (see BACKGROUND below).  This patch queue tries to fix this by
systematically eliminating the direct assignments to bi_status sprinkled
all throughout the code.  Please comment.

The patches are currently still based on v6.18.

GIT tree:
https://git.kernel.org/pub/scm/linux/kernel/git/gfs2/linux-gfs2.git/log/?h=bio-cleanups

The first version of this patch queue is avaliable here:
https://lore.kernel.org/linux-block/[email protected]/

With these changes, only a few direct assignments to bio->bi_status
remain, in BTRFS and in MD, and SOME OF THOSE MAY BE UNSAFE.  Could the
maintainers of those subsystems please have a look?

Once the remaining direct assignments to bi_status are gone, we may want
to think about "write protecting" bi_status to prevent unintended new
direct assignments from creeping back in.


BACKGROUND

'struct bio' objects start out with their bi_status field initialized to
BLK_STS_OK (0).  When the bio completes, that field needs to be left
unchanged in case of success, and set to a BLK_STS_* error code
otherwise.

This is important when bios are chained (bio_chain()) because then,
multiple execution contexts will race updating the same bi_status field.
When an execution context resets bi_status to BLK_STS_OK (0) during bio
completion, this could hide the error code of the adjacent bio in the
chain.

When more than a single bio fails in a chain, we know that the resulting
bi_status will not be BLK_STS_OK, but we don't know which of the status
error codes will survive.


CRYPTO FALLBACK (SATYA TANGIRALA?)

Related to chained bios but unrelated to setting bio->bi_status,
blk_crypto_fallback_encrypt_bio() in block/blk-crypto-fallback.c swaps
out bi_private and bi_end_io and reuses the same bio for downcalls, then
restores those fields in blk_crypto_fallback_decrypt_endio() before
calling bio_endio() again on the same bio.  This will at the very least
break with chained bios because it will mess up __bi_remaining.


Thanks,
Andreas

Andreas Gruenbacher (17):
  xfs: don't clobber bi_status in xfs_zone_alloc_and_submit
  bio: rename bio_chain arguments
  bio: use bio_io_error more often
  bio: add bio_set_status
  bio: use bio_set_status for BLK_STS_* status codes
  bio: do not check bio->bi_status before assigning to it
  block: consecutive blk_status_t error codes
  block: fix blk_status_to_{errno,str} inconsistency
  block: turn blk_errors array into a macro
  block: optimize blk_status <=> errno conversion
  bio: bio_set_status from non-zero errno
  bio: do not check bio->bi_status before assigning to it (part 2)
  xfs: use bio_set_status in xfs_zone_alloc_and_submit
  bio: switch to bio_set_status in submit_bio_noacct
  bio: never set bi_status to BLK_STS_OK during completion
  bio: never set bi_status to BLK_STS_OK during completion (part 2)
  bio: add bio_endio_status

 block/bio-integrity-auto.c       |   3 +-
 block/bio.c                      |  25 +++----
 block/blk-core.c                 | 123 ++++++++++++++++---------------
 block/blk-crypto-fallback.c      |  22 +++---
 block/blk-crypto-internal.h      |   2 +-
 block/blk-crypto.c               |   4 +-
 block/blk-merge.c                |   6 +-
 block/blk-mq.c                   |  10 +--
 block/fops.c                     |   6 +-
 block/t10-pi.c                   |   2 +-
 drivers/block/aoe/aoecmd.c       |   8 +-
 drivers/block/aoe/aoedev.c       |   2 +-
 drivers/block/drbd/drbd_int.h    |   3 +-
 drivers/block/drbd/drbd_req.c    |   7 +-
 drivers/block/ps3vram.c          |   3 +-
 drivers/block/zram/zram_drv.c    |   4 +-
 drivers/md/bcache/bcache.h       |   3 +-
 drivers/md/bcache/request.c      |   8 +-
 drivers/md/dm-cache-target.c     |   9 ++-
 drivers/md/dm-ebs-target.c       |   2 +-
 drivers/md/dm-flakey.c           |   2 +-
 drivers/md/dm-integrity.c        |  34 ++++-----
 drivers/md/dm-mpath.c            |   6 +-
 drivers/md/dm-pcache/dm_pcache.c |   3 +-
 drivers/md/dm-raid1.c            |   7 +-
 drivers/md/dm-thin.c             |   7 +-
 drivers/md/dm-vdo/data-vio.c     |   3 +-
 drivers/md/dm-verity-target.c    |   2 +-
 drivers/md/dm-writecache.c       |   7 +-
 drivers/md/dm-zoned-target.c     |   3 +-
 drivers/md/dm.c                  |   4 +-
 drivers/md/md.c                  |   8 +-
 drivers/md/raid1-10.c            |   3 +-
 drivers/md/raid1.c               |   2 +-
 drivers/md/raid10.c              |  20 +++--
 drivers/md/raid5.c               |   4 +-
 drivers/nvdimm/btt.c             |   4 +-
 drivers/nvdimm/pmem.c            |   4 +-
 fs/btrfs/bio.c                   |   8 +-
 fs/btrfs/direct-io.c             |   2 +-
 fs/btrfs/raid56.c                |   6 +-
 fs/crypto/bio.c                  |   2 +-
 fs/erofs/fileio.c                |   4 +-
 fs/erofs/fscache.c               |   8 +-
 fs/f2fs/data.c                   |   6 +-
 fs/f2fs/segment.c                |   3 +-
 fs/iomap/ioend.c                 |   3 +-
 fs/verity/verify.c               |   2 +-
 fs/xfs/xfs_aops.c                |   3 +-
 fs/xfs/xfs_zone_alloc.c          |   4 +-
 include/linux/bio.h              |  28 ++++++-
 include/linux/blk_types.h        |   2 +-
 include/linux/blkdev.h           |  18 ++++-
 53 files changed, 236 insertions(+), 238 deletions(-)

-- 
2.52.0
lmpx.com only provides a reader for public news (NNTP) servers. It is not affiliated with the servers or forums shown here and is not responsible for the content of articles, which is written by their respective authors.