All of lore.kernel.org
 help / color / mirror / Atom feed
From: Eric Sandeen <sandeen@redhat.com>
To: Mike Snitzer <snitzer@redhat.com>
Cc: Christoph Hellwig <hch@infradead.org>,
	dm-devel@redhat.com, DarkNovaNick@gmail.com,
	linux-lvm@redhat.com, linux-ext4@vger.kernel.org
Subject: Re: do not disable ext4 discards on first discard failure? [was: Re: dm snapshot: ignore discards issued to the snapshot-origin target]
Date: Thu, 28 Apr 2011 16:28:17 -0500	[thread overview]
Message-ID: <4DB9DBF1.9060901@redhat.com> (raw)
In-Reply-To: <20110428205935.GA24979@redhat.com>

On 4/28/11 3:59 PM, Mike Snitzer wrote:
> [cc'ing linux-ext4]
> 
> On Thu, Apr 28 2011 at  3:53am -0400,
> Christoph Hellwig <hch@infradead.org> wrote:
> 
>> On Wed, Apr 27, 2011 at 08:19:13PM -0400, Mike Snitzer wrote:
>>> Discards pose a problem for the snapshot-origin target because they are
>>> treated as writes.  Treating a discard as a write would trigger a
>>> copyout to the snapshot.  Such copyout can prove too costly in the face
>>> of otherwise benign scenarios (e.g. create a snapshot and then mkfs.ext4
>>> the origin -- mkfs.ext4 discards the entire volume by default, which
>>> would copyout the entire origin volume to the snapshot).
>>
>> You also need to make sure that we don't claim discard_zeroes_data for
>> the origin volume in this case.  Especially as ext4 started to rely
>> on this actually working (very bad idea IMHO, but that's another story)
> 
> Eric Sandeen helped me see that having the DM snapshot-origin target
> return success but actually ignore discards is just bad form.
> 
> Especially when you consider that this exercise was motivated by the
> fact that ext4 will disable discards on the first discard failure, see:
> http://www.redhat.com/archives/dm-devel/2011-April/msg00070.html
> 
> Eric and I think it is best to revert this commit:
> a30eec2 ext4: stop issuing discards if not supported by device
> 
> (though ideally ext4 would still WARN_ONCE per superblock with something
>  like: "discard failed, please consider disabling discard support")
> 
> 1) The user asked for discards (with '-o discard' mount option)
>    - what is the real harm in coninuing to issue them even if it _seems_
>      they aren't supported?

TBH I sent a30eec2 on a whim.  Seemed reasonable at the time, but if
discard-ability changes over time, it may not be the best plan.

> 2) assuming the entire block device uniformly supports discards can
>    be flawed (a DM device's discard support can vary based on logical
>    offset).

I still think that concats of floppies, usb disks, and ssds should be rare, so I'm less concerned about that ;)

I think Mike is right though, that if you do not do anything with a discard, you should return -EOPNOTSUPP, and not pretend that you honored it.
 
We should, IMHO, deal with the truth of the matter at the filesystem caller.

-Eric

> Thoughts?


WARNING: multiple messages have this Message-ID (diff)
From: Eric Sandeen <sandeen@redhat.com>
To: Mike Snitzer <snitzer@redhat.com>
Cc: Christoph Hellwig <hch@infradead.org>,
	dm-devel@redhat.com, linux-ext4@vger.kernel.org,
	DarkNovaNick@gmail.com, linux-lvm@redhat.com
Subject: Re: [linux-lvm] do not disable ext4 discards on first discard failure? [was: Re: dm snapshot: ignore discards issued to the snapshot-origin target]
Date: Thu, 28 Apr 2011 16:28:17 -0500	[thread overview]
Message-ID: <4DB9DBF1.9060901@redhat.com> (raw)
In-Reply-To: <20110428205935.GA24979@redhat.com>

On 4/28/11 3:59 PM, Mike Snitzer wrote:
> [cc'ing linux-ext4]
> 
> On Thu, Apr 28 2011 at  3:53am -0400,
> Christoph Hellwig <hch@infradead.org> wrote:
> 
>> On Wed, Apr 27, 2011 at 08:19:13PM -0400, Mike Snitzer wrote:
>>> Discards pose a problem for the snapshot-origin target because they are
>>> treated as writes.  Treating a discard as a write would trigger a
>>> copyout to the snapshot.  Such copyout can prove too costly in the face
>>> of otherwise benign scenarios (e.g. create a snapshot and then mkfs.ext4
>>> the origin -- mkfs.ext4 discards the entire volume by default, which
>>> would copyout the entire origin volume to the snapshot).
>>
>> You also need to make sure that we don't claim discard_zeroes_data for
>> the origin volume in this case.  Especially as ext4 started to rely
>> on this actually working (very bad idea IMHO, but that's another story)
> 
> Eric Sandeen helped me see that having the DM snapshot-origin target
> return success but actually ignore discards is just bad form.
> 
> Especially when you consider that this exercise was motivated by the
> fact that ext4 will disable discards on the first discard failure, see:
> http://www.redhat.com/archives/dm-devel/2011-April/msg00070.html
> 
> Eric and I think it is best to revert this commit:
> a30eec2 ext4: stop issuing discards if not supported by device
> 
> (though ideally ext4 would still WARN_ONCE per superblock with something
>  like: "discard failed, please consider disabling discard support")
> 
> 1) The user asked for discards (with '-o discard' mount option)
>    - what is the real harm in coninuing to issue them even if it _seems_
>      they aren't supported?

TBH I sent a30eec2 on a whim.  Seemed reasonable at the time, but if
discard-ability changes over time, it may not be the best plan.

> 2) assuming the entire block device uniformly supports discards can
>    be flawed (a DM device's discard support can vary based on logical
>    offset).

I still think that concats of floppies, usb disks, and ssds should be rare, so I'm less concerned about that ;)

I think Mike is right though, that if you do not do anything with a discard, you should return -EOPNOTSUPP, and not pretend that you honored it.
 
We should, IMHO, deal with the truth of the matter at the filesystem caller.

-Eric

> Thoughts?

  reply	other threads:[~2011-04-28 21:28 UTC|newest]

Thread overview: 104+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2011-04-12 14:59 [linux-lvm] Testing TRIM with LVM DarkNovaNick
2011-04-12 23:47 ` Mike Snitzer
2011-04-13  1:47   ` DarkNovaNick
2011-04-13  8:41     ` Zdenek Kabelac
2011-04-13 15:38       ` DarkNovaNick
2011-04-13 22:40     ` [PATCH] dm snapshot: add discard support to the snapshot-origin target [was: Re: Testing TRIM with LVM] Mike Snitzer
2011-04-13 22:40       ` [linux-lvm] " Mike Snitzer
2011-04-13 23:48       ` Mike Snitzer
2011-04-13 23:48         ` [linux-lvm] " Mike Snitzer
2011-04-26 17:32         ` Mike Snitzer
2011-04-26 17:32           ` [linux-lvm] " Mike Snitzer
2011-04-28  0:19           ` [PATCH] dm snapshot: ignore discards issued to the snapshot-origin target Mike Snitzer
2011-04-28  0:19             ` [linux-lvm] " Mike Snitzer
2011-04-28  7:53             ` Christoph Hellwig
2011-04-28  7:53               ` [linux-lvm] [dm-devel] " Christoph Hellwig
2011-04-28 20:59               ` do not disable ext4 discards on first discard failure? [was: Re: dm snapshot: ignore discards issued to the snapshot-origin target] Mike Snitzer
2011-04-28 20:59                 ` [linux-lvm] " Mike Snitzer
2011-04-28 21:28                 ` Eric Sandeen [this message]
2011-04-28 21:28                   ` Eric Sandeen
2011-04-28 22:59                   ` Alasdair G Kergon
2011-04-28 22:59                     ` Alasdair G Kergon
2011-04-28 23:01                     ` Eric Sandeen
2011-04-28 23:01                       ` Eric Sandeen
2011-04-28 23:11                       ` Alasdair G Kergon
2011-04-28 23:11                         ` Alasdair G Kergon
2011-04-29  1:12                 ` Andreas Dilger
2011-04-29  1:12                   ` [linux-lvm] " Andreas Dilger
2011-04-29 13:55                   ` Mike Snitzer
2011-04-29 13:55                     ` [linux-lvm] " Mike Snitzer
2011-04-29  9:30                 ` Lukas Czerner
2011-04-29  9:30                   ` [linux-lvm] " Lukas Czerner
2011-04-29 12:24                   ` [dm-devel] " Alasdair G Kergon
2011-04-29 12:24                     ` [linux-lvm] " Alasdair G Kergon
2011-04-29 12:29                     ` Christoph Hellwig
2011-04-29 12:29                       ` [linux-lvm] " Christoph Hellwig
2011-04-29 14:28                       ` Eric Sandeen
2011-04-29 14:28                         ` [linux-lvm] " Eric Sandeen
2011-04-29 15:13                         ` Ray Morris
2011-04-29 15:13                           ` Ray Morris
2011-05-04 16:33                         ` Ted Ts'o
2011-05-04 16:33                           ` [linux-lvm] " Ted Ts'o
2011-05-04 16:51                           ` Eric Sandeen
2011-05-04 16:57                             ` Lukas Czerner
2011-05-04 17:02                           ` Lukas Czerner
2011-05-04 17:02                             ` [linux-lvm] " Lukas Czerner
2011-05-02  7:16                     ` Lukas Czerner
2011-05-02  7:16                       ` [linux-lvm] " Lukas Czerner
2011-05-02  8:13                       ` Alasdair G Kergon
2011-05-02  8:13                         ` [linux-lvm] " Alasdair G Kergon
2011-05-02  8:19                         ` Christoph Hellwig
2011-05-02  8:19                           ` [linux-lvm] " Christoph Hellwig
2011-05-02 10:24                           ` Lukas Czerner
2011-05-02 10:24                             ` [linux-lvm] " Lukas Czerner
2011-05-02 12:48                             ` Mike Snitzer
2011-05-02 12:48                               ` [linux-lvm] " Mike Snitzer
2011-05-02 13:05                               ` Lukas Czerner
2011-05-02 13:05                                 ` [linux-lvm] " Lukas Czerner
2011-05-02 14:47                                 ` Eric Sandeen
2011-05-02 14:47                                   ` [linux-lvm] " Eric Sandeen
2011-05-02 14:48                                   ` Christoph Hellwig
2011-05-02 14:48                                     ` [linux-lvm] " Christoph Hellwig
2011-05-02 14:58                                   ` Lukas Czerner
2011-05-02 14:58                                     ` [linux-lvm] " Lukas Czerner
2011-05-02 13:48                             ` [dm-devel] " Martin K. Petersen
2011-05-02 13:48                               ` [linux-lvm] " Martin K. Petersen
2011-05-02 14:20                             ` Martin K. Petersen
2011-05-02 14:20                               ` [linux-lvm] [dm-devel] " Martin K. Petersen
2011-05-02 14:39                               ` Lukas Czerner
2011-05-02 14:39                                 ` [linux-lvm] " Lukas Czerner
2011-05-02 14:50                                 ` Martin K. Petersen
2011-05-02 14:50                                   ` [linux-lvm] " Martin K. Petersen
2011-05-02 14:58                                 ` Mike Snitzer
2011-05-02 14:58                                   ` [linux-lvm] " Mike Snitzer
2011-05-02 16:58                                 ` [dm-devel] " Martin K. Petersen
2011-05-02 16:58                                   ` [linux-lvm] " Martin K. Petersen
2011-05-03  8:57                                   ` Lukas Czerner
2011-05-03  8:57                                     ` [linux-lvm] " Lukas Czerner
2011-05-04 15:10                                     ` Martin K. Petersen
2011-05-04 15:10                                       ` [linux-lvm] " Martin K. Petersen
2011-05-04 16:02                                       ` Mike Snitzer
2011-05-04 16:02                                         ` [linux-lvm] " Mike Snitzer
2011-05-04 16:50                                         ` Martin K. Petersen
2011-05-04 16:50                                           ` [linux-lvm] " Martin K. Petersen
2011-05-04 18:03                                           ` Mike Snitzer
2011-05-04 18:03                                             ` [linux-lvm] " Mike Snitzer
2011-05-04 17:10                                       ` [dm-devel] " Lukas Czerner
2011-05-04 17:10                                         ` [linux-lvm] " Lukas Czerner
2011-05-04 17:32                                         ` Martin K. Petersen
2011-05-04 17:32                                           ` [linux-lvm] " Martin K. Petersen
2011-05-04 17:35                                           ` Lukas Czerner
2011-05-04 17:35                                             ` [linux-lvm] " Lukas Czerner
2011-05-18 12:16                                       ` Mike Snitzer
2011-05-18 12:16                                         ` [linux-lvm] " Mike Snitzer
2011-05-18 12:52                                         ` Mike Snitzer
2011-05-18 12:52                                           ` [linux-lvm] " Mike Snitzer
2011-05-04 15:16                                     ` [dm-devel] " Martin K. Petersen
2011-05-04 15:16                                       ` [linux-lvm] " Martin K. Petersen
2011-05-04 16:12                                       ` Lukas Czerner
2011-05-04 16:12                                         ` [linux-lvm] " Lukas Czerner
2011-05-05  8:33                                       ` Karel Zak
2011-05-05  8:33                                         ` [linux-lvm] " Karel Zak
2011-05-05 10:48                                         ` Lukas Czerner
2011-05-05 10:48                                           ` [linux-lvm] " Lukas Czerner
2011-04-14 15:31       ` [linux-lvm] [PATCH] dm snapshot: add discard support to the snapshot-origin target [was: Re: Testing TRIM wi DarkNovaNick

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=4DB9DBF1.9060901@redhat.com \
    --to=sandeen@redhat.com \
    --cc=DarkNovaNick@gmail.com \
    --cc=dm-devel@redhat.com \
    --cc=hch@infradead.org \
    --cc=linux-ext4@vger.kernel.org \
    --cc=linux-lvm@redhat.com \
    --cc=snitzer@redhat.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.