Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1412723 > unrolled thread

Re: [PATCH] block: correctly fallback for zeroout

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH] block: correctly fallback for zeroout "Martin K. Petersen" <martin.petersen@oracle.com> - 2016-06-03 05:10 +0200
    Re: block: correctly fallback for zeroout Mike Snitzer <snitzer@redhat.com> - 2016-06-03 06:00 +0200
      Re: block: correctly fallback for zeroout "Martin K. Petersen" <martin.petersen@oracle.com> - 2016-06-07 04:40 +0200
        Re: block: correctly fallback for zeroout Christoph Hellwig <hch@infradead.org> - 2016-06-07 08:40 +0200
          Re: block: correctly fallback for zeroout "Martin K. Petersen" <martin.petersen@oracle.com> - 2016-06-10 04:10 +0200

#1412723 — Re: [PATCH] block: correctly fallback for zeroout

>>>>> "Christoph" == Christoph Hellwig <hch@infradead.org> writes:

Christoph> As part of that I also removed the strange EOPNOTSUPP ignore,
Christoph> but Mike reverted it just because it changed something in the
Christoph> dm testsuite.

Mike?

Christoph> I still believe we should never ignore it in this helper, and
Christoph> only do so in callers that believe it's the right thing.

Yeah.

I really wish EOPNOTSUPP would just go away except for ioctl callers.
Now that we have real bi_error I don't understand why we need it.

-- 
Martin K. Petersen	Oracle Linux Engineering

[toc] | [next] | [standalone]

#1412767 — Re: block: correctly fallback for zeroout

On Thu, Jun 02 2016 at 11:06pm -0400,
Martin K. Petersen <martin.petersen@oracle.com> wrote:

> >>>>> "Christoph" == Christoph Hellwig <hch@infradead.org> writes:
> 
> Christoph> As part of that I also removed the strange EOPNOTSUPP ignore,
> Christoph> but Mike reverted it just because it changed something in the
> Christoph> dm testsuite.
> 
> Mike?

Yes? ;)

Seems there is some serious confusion going on here.  The entirety of
hch's post (to which you quoted a subset) makes little sense to me.

shli's patch builds ontop of latest blk-lib.c code yet hch said this::
"We've split blkdev_issue_discard into __blkdev_issue_discard and a
small wrapper around in for 4.7, so this will need a bit of an update."

And hch never "removed the strange EOPNOTSUPP ignore".  He preserved it
(see his commit 38f25255330's "return ret != -EOPNOTSUPP ? ret : 0;"
that I adjusted in commit bbd848e0f -- _and_ he expanded it to eat the
early return that I restored).

So I can only infer that hch is still missing why my revert fixes
historic blkdev_issue_discard() behavior that his commit regressed.
Please read commit bbd848e0f's header.  That at least details the early
vs late -EOPNOTSUPP blkdev_issue_discard() return.

But all that nuance aside, AFAICT my commit bbd848e0f ("block: reinstate
early return of -EOPNOTSUPP from blkdev_issue_discard") really has
_nothing_ to do with the issue shli is addressing with his fix.

> Christoph> I still believe we should never ignore it in this helper, and
> Christoph> only do so in callers that believe it's the right thing.
> 
> Yeah.

Hmm...

You agreed to what hch said there about how we should probably always
return EOPNOTSUPP but then you immediately elaborated with details that
mean you don't agree:
 
> I really wish EOPNOTSUPP would just go away except for ioctl callers.
> Now that we have real bi_error I don't understand why we need it.

But hch was originally in favor of _always_ dropping EOPNOTSUPP on the
floor (that is what his commit 38f25255330 did).  Then he said he
disagrees with these interfaces playing games with masking EOPNOTSUPP --
to which you seemingly really don't agree.  Unless I'm completely
misreading you.

Anyway, shli is at least making it so that blkdev_issue_zerout() can
fallback to other mechanisms as needed.

[toc] | [prev] | [next] | [standalone]

#1415631 — Re: block: correctly fallback for zeroout

>>>>> "Mike" == Mike Snitzer <snitzer@redhat.com> writes:

Mike> But hch was originally in favor of _always_ dropping EOPNOTSUPP on
Mike> the floor (that is what his commit 38f25255330 did).  Then he said
Mike> he disagrees with these interfaces playing games with masking
Mike> EOPNOTSUPP -- to which you seemingly really don't agree.  Unless
Mike> I'm completely misreading you.

Userland apps rely on EOPNOTSUPP, we can't break that.

What I don't like this is "soft" error special casing of EOPNOTSUPP in
the actual implementation of discard, write same, etc. These functions
should return either success or failure. And the ioctl wrapper should
then decide whether to return EOPNOTSUPP, EIO or EPONIES.

I.e. separate the policy from the implementation. This would also solve
some of the grievances for the target folks.

-- 
Martin K. Petersen	Oracle Linux Engineering

[toc] | [prev] | [next] | [standalone]

#1415745 — Re: block: correctly fallback for zeroout

On Mon, Jun 06, 2016 at 10:32:38PM -0400, Martin K. Petersen wrote:
> >>>>> "Mike" == Mike Snitzer <snitzer@redhat.com> writes:
> 
> Mike> But hch was originally in favor of _always_ dropping EOPNOTSUPP on
> Mike> the floor (that is what his commit 38f25255330 did).  Then he said
> Mike> he disagrees with these interfaces playing games with masking
> Mike> EOPNOTSUPP -- to which you seemingly really don't agree.  Unless
> Mike> I'm completely misreading you.
> 
> Userland apps rely on EOPNOTSUPP, we can't break that.

Rely on what exactly?  Current we return EOPNOTSUPP if the device
doesn't claim to support discards, but it returns 0 if the device first
claims to support it but then fails the I/O.

[toc] | [prev] | [next] | [standalone]

#1418989 — Re: block: correctly fallback for zeroout

>>>>> "Christoph" == Christoph Hellwig <hch@infradead.org> writes:

>> Userland apps rely on EOPNOTSUPP, we can't break that.

Christoph> Rely on what exactly?  Current we return EOPNOTSUPP if the
Christoph> device doesn't claim to support discards, but it returns 0 if
Christoph> the device first claims to support it but then fails the I/O.

Hopefully we can clean up this when/if we go the fallocate() route.

-- 
Martin K. Petersen	Oracle Linux Engineering

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web