| Message ID | 20150610081138.GA3841@lst.de (mailing list archive) |
|---|---|
| State | Not Applicable |
| Headers | show |
On Wed, Jun 10 2015 at 4:11am -0400, Christoph Hellwig <hch@lst.de> 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@redhat.com> 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@lst.de> > 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@lst.de> > --- > 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. -- To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, Jun 10 2015 at 11:26am -0400, Mike Snitzer <snitzer@redhat.com> wrote: > On Wed, Jun 10 2015 at 4:11am -0400, > Christoph Hellwig <hch@lst.de> 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. See: https://git.kernel.org/cgit/linux/kernel/git/device-mapper/linux-dm.git/commit/?h=for-next&id=f368f463e4cef696ad6b102dbaf5c10dfca7cc63 -- To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, Jun 10, 2015 at 12:01:12PM -0400, Mike Snitzer wrote: > > I'll queue a patch to rename 'error' to 'error_bits' where appropriate. > > See: https://git.kernel.org/cgit/linux/kernel/git/device-mapper/linux-dm.git/commit/?h=for-next&id=f368f463e4cef696ad6b102dbaf5c10dfca7cc63 Can we wait with this until we're done with bi_error? -- To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, Jun 10 2015 at 12:04pm -0400, Christoph Hellwig <hch@lst.de> wrote: > On Wed, Jun 10, 2015 at 12:01:12PM -0400, Mike Snitzer wrote: > > > I'll queue a patch to rename 'error' to 'error_bits' where appropriate. > > > > See: https://git.kernel.org/cgit/linux/kernel/git/device-mapper/linux-dm.git/commit/?h=for-next&id=f368f463e4cef696ad6b102dbaf5c10dfca7cc63 > > Can we wait with this until we're done with bi_error? Sure, I've moved it out to dm-4.3 -- To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
So Is it safe to use raid 6 with btrfs for home storage now? I will have backup on ZFS. On Wed, Jun 10, 2015 at 11:50 AM, Mike Snitzer <snitzer@redhat.com> wrote: > On Wed, Jun 10 2015 at 12:04pm -0400, > Christoph Hellwig <hch@lst.de> wrote: > >> On Wed, Jun 10, 2015 at 12:01:12PM -0400, Mike Snitzer wrote: >> > > I'll queue a patch to rename 'error' to 'error_bits' where appropriate. >> > >> > See: https://git.kernel.org/cgit/linux/kernel/git/device-mapper/linux-dm.git/commit/?h=for-next&id=f368f463e4cef696ad6b102dbaf5c10dfca7cc63 >> >> Can we wait with this until we're done with bi_error? > > Sure, I've moved it out to dm-4.3 > -- > To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html -- To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, Jun 10, 2015 at 11:26:49AM -0400, Mike Snitzer wrote: > 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. I think this also happens in the old code before my patch, e.g.: static void clone_endio(struct bio *bio, int error) { int r = error; ... if (endio) { r = endio(tio->ti, bio, error); ... } if (unlikely(r == -EREMOTEIO && (bio->bi_rw & REQ_WRITE_SAME) && so we already check the return value that comes from the endio handler, not any different from my patch. In the original code r and error are basically always the same, execept after this: if (!bio_flagged(bio, BIO_UPTODATE) && !error) error = -EIO; error might be -EIO and r might be 0. Now if we take the endio branch we replace both error and r with the return value of endio for all branches that don't immediately return or BUG(). For the non endio branch we might check in REQ_WRITE_SAME branch, but r can only be -EREMOTEIO if it got that value from error, so it doesn't matter which one we test there. Based on that I'm pretty sure my prep patch is transformation that does not change behavior. -- To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, Jun 11, 2015 at 09:53:27AM +0200, Christoph Hellwig wrote: > On Wed, Jun 10, 2015 at 11:26:49AM -0400, Mike Snitzer wrote: > > 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. > > I think this also happens in the old code before my patch, e.g.: Ok, after and audit of the instance I noticed that returning 0 from ->end_io even if an error is passed is a valid calling convention, and with that the below analysis is invalid. I'll take it back and will do the simplest possible conversion for now and return my attention to sorting out this mess later. -- To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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);