| Message ID | 1462322283-72777-1-git-send-email-snitzer@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Tue, May 03, 2016 at 08:38:03PM -0400, Mike Snitzer wrote: > Commit 38f25255330 ("block: add __blkdev_issue_discard") incorrectly > disallowed the return of -EOPNOTSUPP if the device didn't support > discard (or secure discard). I assume dm relied in this? The different handling of early vs late -EOPNOTSUPP is the old code is rather odd, so when restoring it I'd rather have a comment explaining why the different handling is needed. -- To unsubscribe from this list: send the line "unsubscribe linux-block" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, May 04 2016 at 11:11am -0400, Christoph Hellwig <hch@lst.de> wrote: > On Tue, May 03, 2016 at 08:38:03PM -0400, Mike Snitzer wrote: > > Commit 38f25255330 ("block: add __blkdev_issue_discard") incorrectly > > disallowed the return of -EOPNOTSUPP if the device didn't support > > discard (or secure discard). > > I assume dm relied in this? The different handling of early vs > late -EOPNOTSUPP is the old code is rather odd, so when restoring > it I'd rather have a comment explaining why the different handling > is needed. It isn't needed by DM. The device-mapper-test-suite's thinp discard tests did catch the change in behaviour but that is besides the point. Why do you feel it makes sense to mask the fact that discards issued to a device that doesn't support them is unsupported? Especially when it is established behavior of the blkdev_issue_discard() interface to do so. I see no point in breaking established behaviour. If you do then please explain. -- To unsubscribe from this list: send the line "unsubscribe linux-block" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, May 04, 2016 at 11:20:38AM -0400, Mike Snitzer wrote: > > > Commit 38f25255330 ("block: add __blkdev_issue_discard") incorrectly > > > disallowed the return of -EOPNOTSUPP if the device didn't support > > > discard (or secure discard). > > > > I assume dm relied in this? The different handling of early vs > > late -EOPNOTSUPP is the old code is rather odd, so when restoring > > it I'd rather have a comment explaining why the different handling > > is needed. > > It isn't needed by DM. The device-mapper-test-suite's thinp discard > tests did catch the change in behaviour but that is besides the point. > > Why do you feel it makes sense to mask the fact that discards issued to > a device that doesn't support them is unsupported? Especially when it > is established behavior of the blkdev_issue_discard() interface to do > so. > > I see no point in breaking established behaviour. If you do then please > explain. Let's take a step back and define at what point ENOTSUPP should be ignored, and until when it should not. Once we have a good answer to that question I'd be happy with updating the kernel. Are you / dm fine with always returning ENOTSUPP? -- To unsubscribe from this list: send the line "unsubscribe linux-block" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, May 05 2016 at 10:08am -0400, Christoph Hellwig <hch@lst.de> wrote: > On Wed, May 04, 2016 at 11:20:38AM -0400, Mike Snitzer wrote: > > > > Commit 38f25255330 ("block: add __blkdev_issue_discard") incorrectly > > > > disallowed the return of -EOPNOTSUPP if the device didn't support > > > > discard (or secure discard). > > > > > > I assume dm relied in this? The different handling of early vs > > > late -EOPNOTSUPP is the old code is rather odd, so when restoring > > > it I'd rather have a comment explaining why the different handling > > > is needed. > > > > It isn't needed by DM. The device-mapper-test-suite's thinp discard > > tests did catch the change in behaviour but that is besides the point. > > > > Why do you feel it makes sense to mask the fact that discards issued to > > a device that doesn't support them is unsupported? Especially when it > > is established behavior of the blkdev_issue_discard() interface to do > > so. > > > > I see no point in breaking established behaviour. If you do then please > > explain. > > Let's take a step back and define at what point ENOTSUPP should be > ignored, and until when it should not. Once we have a good answer > to that question I'd be happy with updating the kernel. > > Are you / dm fine with always returning ENOTSUPP? I have a new header that gets into the details. I'll soon post it as part of the series I have to fixup DM thinp's async discard support to use your new __blkdev_issue_discard(). (Not that restoring the early -EOPNOTSUPP return is related to DM thinp in any way but it is just easier to blast out what I have). DM doesn't rely on EOPNOTSUPP return; but it is useful information for the caller. DM (and other stacked devices) do introduce the need for this nuance. Anyway, the blkdev_issue_discard() interface is well established so I'm missing why we'd break it just because. So: no I'm not fine with always returning ENOTSUPP because it could break some userspace code. That said, I don't know of userspace code that cares.. but there easily could be some that does. -- To unsubscribe from this list: send the line "unsubscribe linux-block" 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/block/blk-lib.c b/block/blk-lib.c index ccbce2b..23d7f30 100644 --- a/block/blk-lib.c +++ b/block/blk-lib.c @@ -109,11 +109,14 @@ int blkdev_issue_discard(struct block_device *bdev, sector_t sector, blk_start_plug(&plug); ret = __blkdev_issue_discard(bdev, sector, nr_sects, gfp_mask, type, &bio); - if (!ret && bio) + if (!ret && bio) { ret = submit_bio_wait(type, bio); + if (ret == -EOPNOTSUPP) + ret = 0; + } blk_finish_plug(&plug); - return ret != -EOPNOTSUPP ? ret : 0; + return ret; } EXPORT_SYMBOL(blkdev_issue_discard);
Commit 38f25255330 ("block: add __blkdev_issue_discard") incorrectly disallowed the return of -EOPNOTSUPP if the device didn't support discard (or secure discard). Fixes: 38f25255330 ("block: add __blkdev_issue_discard") Signed-off-by: Mike Snitzer <snitzer@redhat.com> --- block/blk-lib.c | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-)