Re: [PATCH V2] block: correctly fallback for zeroout

[Mike Snitzer]

On Mon, Jun 13 2016 at  4:20am -0400,
Christoph Hellwig <hch@xxxxxxxxxxxxx> wrote:

> On Fri, Jun 10, 2016 at 09:49:44PM -0400, Martin K. Petersen wrote:
> > >> What does the extra io_err buy us? Just have this function return an
> > >> error. And then in blkdev_issue_discard if you get -EOPNOTSUPP you
> > >> special case it there.
> > 
> > Shaohua> The __blkdev_issue_discard returns -EOPNOTSUPP if disk doesn't
> > Shaohua> support discard.  in that case, blkdev_issue_discard doesn't
> > Shaohua> return 0. blkdev_issue_discard only returns 0 if IO error is
> > Shaohua> -EOPNOTSUPP.
> > 
> > Oh, I see. The sanity checks are now in __blkdev_issue_discard() so
> > there is no way to distinguish between -EOPNOTSUPP and the other
> > -EOPNOTSUPP. *sigh*
> 
> We can move the sanity checks out.  Or even better get rid of the
> stupid behavior of ignoring the late -EOPNOTSUPP in this low level
> helper and instead leaving it to the caller(s) that care.

I'm not onboard with blkdev_issue_discard() no longer masking the late
return of -EOPNOTSUPP.

I'd be fine with moving the early -EOPNOTSUPP checks and the masking of
late -EOPNOTSUPP out to blkdev_issue_discard().  But to be clear,
the masking of late -EOPNOTSUPP return is there for stacking drivers
like MD and DM.  So long as the upper level ioctl code, filesystems, etc
makes use of blkdev_issue_discard() then they'll still get the benefit
of that masking.

drivers/md/dm-thin.c is now using the new async __blkdev_issue_discard()
and it'll only ever do so to a device it knows supports discards -- BUT
it could be that the DM thin-pool's data device is itself a stacked
device that doesn't uniformly support discards throughout its entire
logical address space.  So it could issue a discard to a portion of the
stacked data device that will return -EOPNOTSUPP.. so long story short:
making this change to remove this so-called "stupid behaviour" will
require code like drivers/md/dm-thin.c:issue_discard(() to check the
return from __blkdev_issue_discard() and if it is -EOPNOTSUPP then it
should return 0.

> So far the DM test suite seems to be the only one that does.

The device-mapper-test-suite was only ever relying on
blkdev_issue_discard()'s early return of -EOPNOTSUPP.

> > I am OK with your patch as a stable fix but this really needs to be
> > fixed up properly.
> 
> And I'd much prefer to get this right now.  It's not like this is
> recently introduced behavior.

We need to sequence the fixes such that stable kernels get the zeroout
fallback fixed.  Right?  Not sure if that is a goal of shli's though..

In 4.7-rc, where you introduced __blkdev_issue_discard and I made
dm-thin.c consume it, I'm fine with seeing __blkdev_issue_discard stop
masking -EOPNOTSUPP... but at the same time that change is made
dm-thin.c would need to be fixed (in the same commit as the interface
change).  Though I'm now missing what lifting the -EOPNOTSUPP behavior
into blkdev_issue_discard() buys us... maybe purity of the new async
__blkdev_issue_discard()?

Mike