On Wed, Jun 10 2015 at 4:11am -0400,
Christoph Hellwig <hch@xxxxxx> wrote:
> On Thu, Jun 04, 2015 at 11:31:07AM -0400, Mike Snitzer wrote:
> > This patch _really_ concerns me because just in DM alone I found you
> > took liberties that you shouldn't have and created a regression. First
> > issue is a real bug (your proposed dm-io.c:dmio_complete change missed
> > that dm-io uses error_bits and not traditional error code like expected)
>
> Point taken. I already wanted to complain about the mess due to the bio
> error abuse with it's own values in DM in the first posting, guess I
> need to add that to the second one. I don't think overloading common
> interfaces with your private error codes is a good idea, but let's
> leave that for a separate discussion.
I'll queue a patch to rename 'error' to 'error_bits' where appropriate.
> > the other issue being you added extra branching that isn't needed and
> > made review more tedious (dm.c:clone_endio).
>
> I think the code is better than what it was before, but it's still
> a bit of a mess. What do you think of the patch below which I'd
> like to add before the big bi_error patch as a preparatory one?
If you're referring to the mix of error variables I totally agree. Just
don't think we need the extra branching.
> > For DM, please add Signed-off-by: Mike Snitzer <snitzer@xxxxxxxxxx> once
> > you've folded in this patch, thanks!
>
> FYI, that wasn't a foldable patch but updated hunks of the old one. Not
> really a problem, but a little confusing.
Yeap, should have been clearer they were meant to replace your hunks.
> >From f095cbeba5135afa6cf102718319f0d0c1e7b422 Mon Sep 17 00:00:00 2001
> From: Christoph Hellwig <hch@xxxxxx>
> Date: Wed, 10 Jun 2015 10:04:45 +0200
> Subject: dm: use a single error code variable in clone_endio
> MIME-Version: 1.0
> Content-Type: text/plain; charset=UTF-8
> Content-Transfer-Encoding: 8bit
>
> clone_endio currently uses two variables for tracking error state, with
> values getting bounceѕ forth and back between the two, which makes the
> code hard to read.
>
> Signed-off-by: Christoph Hellwig <hch@xxxxxx>
> ---
> drivers/md/dm.c | 22 ++++++++++------------
> 1 file changed, 10 insertions(+), 12 deletions(-)
>
> diff --git a/drivers/md/dm.c b/drivers/md/dm.c
> index 2161ed9..8467976 100644
> --- a/drivers/md/dm.c
> +++ b/drivers/md/dm.c
> @@ -956,7 +956,6 @@ static void disable_write_same(struct mapped_device *md)
>
> static void clone_endio(struct bio *bio, int error)
> {
> - int r = error;
> struct dm_target_io *tio = container_of(bio, struct dm_target_io, clone);
> struct dm_io *io = tio->io;
> struct mapped_device *md = tio->io->md;
> @@ -966,23 +965,22 @@ static void clone_endio(struct bio *bio, int error)
> error = -EIO;
>
> if (endio) {
> - r = endio(tio->ti, bio, error);
> - if (r < 0 || r == DM_ENDIO_REQUEUE)
> - /*
> - * error and requeue request are handled
> - * in dec_pending().
> - */
> - error = r;
> - else if (r == DM_ENDIO_INCOMPLETE)
> + error = endio(tio->ti, bio, error);
> + if (error == DM_ENDIO_INCOMPLETE) {
> /* The target will handle the io */
> return;
> - else if (r) {
> - DMWARN("unimplemented target endio return value: %d", r);
> + }
> +
> + if (error > 0 && error != DM_ENDIO_REQUEUE) {
> + DMWARN("unimplemented target endio return value: %d",
> + error);
> BUG();
> }
> +
> + /* Error and requeue request are handled in dec_pending(). */
> }
>
> - if (unlikely(r == -EREMOTEIO && (bio->bi_rw & REQ_WRITE_SAME) &&
> + if (unlikely(error == -EREMOTEIO && (bio->bi_rw & REQ_WRITE_SAME) &&
> !bdev_get_queue(bio->bi_bdev)->limits.max_write_same_sectors))
> disable_write_same(md);
>
> --
> 1.9.1
Unfortunately by dropping the original error (e.g. -EREMOTEIO) on the
floor (in the 'if (endio) {' branch) you're breaking the REQ_WRITE_SAME
check.
Your new bi_error patch gets away with the redundant error code cleanup
because we can directly check the bio's bi_error for -EREMOTEIO. So
feel free to fold the simplified 'if (error > 0 && error != DM_ENDIO_REQUEUE) {'
in to your new patch -- but not seeing the point of making this prep
patch in advance.
--
dm-devel mailing list
dm-devel@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/dm-devel
