* Keith Busch ([email protected]) wrote:
> From: Keith Busch <[email protected]>
>
> BLK_STS_INVAL indicates the I/O request itself was invalid (for example a
> misaligned direct I/O), not that the device has failed. dm-raid1 treated
> any read or write completion error as a device failure: it failed the
> mirror leg, retried on the alternatives - which fail identically - and
> eventually returned EIO while spuriously degrading the array.
>
> Since commit 5ff3f74e145a ("block: simplify direct io validity check") the
> direct I/O path no longer rejects misaligned buffers up front, so an
> invalid bio now reaches the lower block layers, which fail it with
> BLK_STS_INVAL. dm-io collapses the block status into a per-region error
> bit before invoking the completion callback, so record BLK_STS_INVAL on
> the originating bio and have the dm-raid1 read, write and end_io paths
> propagate it instead of failing the device.
>
> This mirrors the raid1/raid10 fix in commit f7b24c7b41f23
> ("md/raid1,raid10: don't fail devices for invalid IO errors") for the
> device-mapper mirror target.
>
> Fixes: 7eac33186957 ("iomap: simplify direct io validity check")
> Fixes: 5ff3f74e145a ("block: simplify direct io validity check")
> Reported-by: Dr. David Alan Gilbert <[email protected]>
> Reported-by: Vjaceslavs Klimovs <[email protected]>
> Signed-off-by: Keith Busch <[email protected]>
OK, for this pair I think would be fair for a Tested-by me as well;
they certainly resolve the hang and the WARN/BUGs.
I still see the errors as EIO on my tests, and on the older mirror type
get the stuck resync on write, and on the newer mirror I see the write
apparently succeed (did it really?)
I suggest given the BUG/WARN and the number of people who've tripped over
this, and it's triggerable as a normal user, that it's a candidate for stable.
Thanks again,
Dave
> ---
> drivers/md/dm-io.c | 14 +++++++++++++-
> drivers/md/dm-raid1.c | 28 +++++++++++++++++++++++++++-
> 2 files changed, 40 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/md/dm-io.c b/drivers/md/dm-io.c
> index 28adfeb58f240..f382e9f9be059 100644
> --- a/drivers/md/dm-io.c
> +++ b/drivers/md/dm-io.c
> @@ -37,6 +37,7 @@ struct io {
> struct dm_io_client *client;
> io_notify_fn callback;
> void *context;
> + struct bio *orig_bio;
> void *vma_invalidate_address;
> unsigned long vma_invalidate_size;
> } __aligned(DM_IO_MAX_REGIONS);
> @@ -132,8 +133,18 @@ static void complete_io(struct io *io)
>
> static void dec_count(struct io *io, unsigned int region, blk_status_t error)
> {
> - if (error)
> + if (error) {
> set_bit(region, &io->error_bits);
> + /*
> + * BLK_STS_INVAL means the bio was not valid for the underlying
> + * device (e.g. a misaligned direct I/O), which is a caller
> error
> + * rather than a device failure. Record it on the original bio
> so
> + * bio-based targets can propagate it instead of treating it as
> a
> + * media error and failing the device.
> + */
> + if (error == BLK_STS_INVAL && io->orig_bio)
> + io->orig_bio->bi_status = error;
> + }
>
> if (atomic_dec_and_test(&io->count))
> complete_io(io);
> @@ -398,6 +409,7 @@ static void async_io(struct dm_io_client *client,
> unsigned int num_regions,
> io->client = client;
> io->callback = fn;
> io->context = context;
> + io->orig_bio = dp->orig_bio;
>
> io->vma_invalidate_address = dp->vma_invalidate_address;
> io->vma_invalidate_size = dp->vma_invalidate_size;
> diff --git a/drivers/md/dm-raid1.c b/drivers/md/dm-raid1.c
> index de5c00704e69c..022ad791c2957 100644
> --- a/drivers/md/dm-raid1.c
> +++ b/drivers/md/dm-raid1.c
> @@ -524,6 +524,17 @@ static void read_callback(unsigned long error, void
> *context)
> return;
> }
>
> + /*
> + * BLK_STS_INVAL means the bio was not valid for the underlying device,
> + * e.g. a misaligned direct I/O. That is a caller error, not a device
> + * failure, so propagate it rather than failing the mirror and retrying
> + * on the other legs, which would fail the same way.
> + */
> + if (bio->bi_status == BLK_STS_INVAL) {
> + bio_endio(bio);
> + return;
> + }
> +
> fail_mirror(m, DM_RAID1_READ_ERROR);
>
> if (likely(default_ok(m)) || mirror_available(m->ms, bio)) {
> @@ -622,6 +633,16 @@ static void write_callback(unsigned long error, void
> *context)
> return;
> }
>
> + /*
> + * BLK_STS_INVAL means the bio was not valid for the underlying device,
> + * e.g. a misaligned direct I/O. Propagate the error without degrading
> + * the array.
> + */
> + if (bio->bi_status == BLK_STS_INVAL) {
> + bio_endio(bio);
> + return;
> + }
> +
> /*
> * If the bio is discard, return an error, but do not
> * degrade the array.
> @@ -1262,7 +1283,12 @@ static int mirror_end_io(struct dm_target *ti, struct
> bio *bio,
> return DM_ENDIO_DONE;
> }
>
> - if (*error == BLK_STS_NOTSUPP)
> + /*
> + * BLK_STS_INVAL means the bio was not valid for the underlying device,
> + * e.g. a misaligned direct I/O. Propagate it rather than failing the
> + * mirror and retrying, which would fail the same way on every leg.
> + */
> + if (*error == BLK_STS_NOTSUPP || *error == BLK_STS_INVAL)
> goto out;
>
> if (bio->bi_opf & REQ_RAHEAD)
> --
> 2.52.0
>
--
-----Open up your eyes, open up your mind, open up your code -------
/ Dr. David Alan Gilbert | Running GNU/Linux | Happy \
\ dave @ treblig.org | | In Hex /
\ _________________________|_____ http://www.treblig.org |_______/
