[linux-lvm] do not disable ext4 discards on first discard failure? [was: Re: dm snapshot: ignore discards issued to the snapshot-origin target]

[Lukas Czerner]

On Mon, 2 May 2011, Mike Snitzer wrote:

> On Mon, May 02 2011 at  6:24am -0400,
> Lukas Czerner <lczerner at redhat.com> wrote:
> 
> > On Mon, 2 May 2011, Christoph Hellwig wrote:
> > 
> > > On Mon, May 02, 2011 at 09:13:08AM +0100, Alasdair G Kergon wrote:
> > > > On Mon, May 02, 2011 at 09:16:21AM +0200, Lukas Czerner wrote:
> > > > > However when we have dm device where part of the device supports
> > > > > discard due to underlying hardware capability we just can not return
> > > > > EOPNOTSUPP from blkdev_issue_discard, because it is just not true! 
> > > > 
> > > > EOPNOTSUPP from dm means the operation was not supported on that *one* bio.
> > > > It does *not* tell you anything in general about the device, or whether
> > > > you'd get the same error from different bios in future.
> > > 
> > > Exactly.  We already have the information in the queue limits to tell
> > > the filesystem if discard is supported at all or not.
> > > 
> > 
> > So I gave it a try. First of all the device composed of SSD and spinning
> > disk does export discard_support information properly, however it also
> > advertise discard_zeroes_data which is wrong and possibly dangerous and
> > should be fixed!
> 
> You're effectively advocating that blk_stack_limits() needs to clear the
> topmost device's discard_zeroes_data flag if any one bottom device does
> not have discard_zeroes_data.

Exactly.

> 
> > And second of all, strictly speaking if EOPNOTSUPP from dm means that
> > operation was not supported on that *one* bio, blkdev_issue_discard()
> > should handle that and do not return EOPNOTSUPP further if queue limits
> > tells that device has discard support. Is this acceptable solution for
> > you guys ? I can make that change since I am changing blkdev_issue_discard()
> > anyway. Or, we can make that change in filesystem where we disable
> > discard on mount time, when we notice that discard mount option was
> > specified, but the device does not support it (we should probably do
> > this regardless on blkdev_issue_discard() change).
> 
> The blkdev_issue_discard() change you propose could be fine (mask
> EOPNOTSUPP return if device advertises support for discards) -- though
> Eric said we shouldn't ever say we did something when we didn't.

Exactly, so we should not say that it is not supported when it is, but
we just hit the "wrong" part of the device:) I would just very much like
to keep the abstraction of having one consistent device underneath the
file system and not deal with several devices, or regions with different
behaviour in the file system itself (let the pixies underneath deal with
that, after all not all of us are btrfs:))

> 
> But that blkdev_issue_discard() change is really only safe if the
> discard_zeroes_data flag is cleared by blk_stack_limits() if finds
> inconsistent discard_zeroes_data support.

Agreed

> 
> Mike
> 

Thanks!
-Lukas