Testing TRIM with LVM

I recently purchased a Crucial C300 SSD and set it up as my primary drive. I wanted to use LVM, and my reading indicates that LVM added support to pass-through TRIM commands with kernel 2.6.37. I'm running Ubuntu 10.10, but I installed the latest 11.04 kernel, so I'm running kernel 2.6.38-8. I added "discard" to fstab so my mountpoint looks like:

/dev/mapper/vg0-vol0 on / type ext4 (rw,noatime,nodiratime,errors=remount-ro,discard,commit=0)

I found directions on various sites like:
http://duopetalflower.blogspot.com/2010/11/enterprise-kernel-6-has-ssd-trim.html
on how to confirm if TRIM is working. I ran (as root):

dd if=/dev/urandom of=tempfile count=100 bs=512k oflag=direct hdparm --fibmap tempfile
(then took the first begin_LBA)
hdparm --read-sector 191710208 /dev/sda

and it printed:

/dev/sda:
reading sector 191710208: succeeded
3254 3a32 3834 313a 2b36 3030 303a 2030
4544 5542 2047 3728 3a29 5320 7661 6e69
2067 6e69 6564 2078 6f66 2072 7270 646f
............

Then I ran:
rm tempfile
sync
hdparm --read-sector 191710208 /dev/sda

and it still returned:
/dev/sda:
reading sector 191710208: succeeded
3254 3a32 3834 313a 2b36 3030 303a 2030
4544 5542 2047 3728 3a29 5320 7661 6e69
2067 6e69 6564 2078 6f66 2072 7270 646f
.............

If TRIM is working, the sector is supposed to contain all zeros.

Am I doing something wrong or do I need to do something more to get LVM to pass down the TRIM command?

On Tue, Apr 12 2011 at 10:59am -0400,

Quoted 10 lines · [email protected] [email protected]> wrote:

I recently purchased a Crucial C300 SSD and set it up as my primary

drive. I wanted to use LVM, and my reading indicates that LVM added

support to pass-through TRIM commands with kernel 2.6.37. I'm

running Ubuntu 10.10, but I installed the latest 11.04 kernel, so

I'm running kernel 2.6.38-8. I added "discard" to fstab so my

Quoted 7 lines

mountpoint looks like:

/dev/mapper/vg0-vol0 on / type ext4

(rw,noatime,nodiratime,errors=remount-ro,discard,commit=0)

If the discards aren't working then ext4 will print a warning and stop

issuing them, see: http://git.kernel.org/linus/a30eec2a8650a77f7

So if you don't see "discard not supported, disabling" in your dmesg (or

/var/log/messages) then TRIM is likely working as expected.

Quoted 3 lines

I found directions on various sites like:

http://duopetalflower.blogspot.com/2010/11/enterprise-kernel-6-has-ssd-trim.html

Quoted 52 lines

on how to confirm if TRIM is working. I ran (as root):

dd if=/dev/urandom of=tempfile count=100 bs=512k oflag=direct

hdparm --fibmap tempfile

(then took the first begin_LBA)

hdparm --read-sector 191710208 /dev/sda

and it printed:

/dev/sda:

reading sector 191710208: succeeded

3254 3a32 3834 313a 2b36 3030 303a 2030

4544 5542 2047 3728 3a29 5320 7661 6e69

2067 6e69 6564 2078 6f66 2072 7270 646f

............

Then I ran:

rm tempfile

sync

hdparm --read-sector 191710208 /dev/sda

and it still returned:

/dev/sda:

reading sector 191710208: succeeded

3254 3a32 3834 313a 2b36 3030 303a 2030

4544 5542 2047 3728 3a29 5320 7661 6e69

Quoted 7 lines

2067 6e69 6564 2078 6f66 2072 7270 646f

.............

If TRIM is working, the sector is supposed to contain all zeros.

That is only if the SSD implements TRIM so that it zeroes the discarded

blocks. You can check if the device at least knows enough to report

discard_zeroes_data:

cat /sys/block/sda/queue/discard_zeroes_data

Also, even if the SSD does zero, it may take some time -- well after the

discard has completed -- for the SSD to actually zero the blocks (this

is true of some lesser quality SSDs).

Quoted 3 lines

Am I doing something wrong or do I need to do something more to get

LVM to pass down the TRIM command?

LVM (device mapper specifically) passes discards perfectly well. But

the snapshot and dm-crypt targets don't have discard support. All other

targets do have discard support. What is your output for: dmsetup table

Mike

Quoted 134 lines

On Tue, Apr 12 2011 at 10:59am -0400,

Quoted 10 lines · [email protected] [email protected]> wrote:

I recently purchased a Crucial C300 SSD and set it up as my primary

drive. I wanted to use LVM, and my reading indicates that LVM added

support to pass-through TRIM commands with kernel 2.6.37. I'm

running Ubuntu 10.10, but I installed the latest 11.04 kernel, so

I'm running kernel 2.6.38-8. I added "discard" to fstab so my

Quoted 7 lines

mountpoint looks like:

/dev/mapper/vg0-vol0 on / type ext4

(rw,noatime,nodiratime,errors=remount-ro,discard,commit=0)

If the discards aren't working then ext4 will print a warning and stop

issuing them, see: http://git.kernel.org/linus/a30eec2a8650a77f7

So if you don't see "discard not supported, disabling" in your dmesg (or

/var/log/messages) then TRIM is likely working as expected.

Quoted 3 lines

I found directions on various sites like:

http://duopetalflower.blogspot.com/2010/11/enterprise-kernel-6-has-ssd-trim.html

Quoted 52 lines

on how to confirm if TRIM is working. I ran (as root):

dd if=/dev/urandom of=tempfile count=100 bs=512k oflag=direct

hdparm --fibmap tempfile

(then took the first begin_LBA)

hdparm --read-sector 191710208 /dev/sda

and it printed:

/dev/sda:

reading sector 191710208: succeeded

3254 3a32 3834 313a 2b36 3030 303a 2030

4544 5542 2047 3728 3a29 5320 7661 6e69

2067 6e69 6564 2078 6f66 2072 7270 646f

............

Then I ran:

rm tempfile

sync

hdparm --read-sector 191710208 /dev/sda

and it still returned:

/dev/sda:

reading sector 191710208: succeeded

3254 3a32 3834 313a 2b36 3030 303a 2030

4544 5542 2047 3728 3a29 5320 7661 6e69

Quoted 7 lines

2067 6e69 6564 2078 6f66 2072 7270 646f

.............

If TRIM is working, the sector is supposed to contain all zeros.

That is only if the SSD implements TRIM so that it zeroes the discarded

blocks. You can check if the device at least knows enough to report

discard_zeroes_data:

cat /sys/block/sda/queue/discard_zeroes_data

Also, even if the SSD does zero, it may take some time -- well after the

discard has completed -- for the SSD to actually zero the blocks (this

is true of some lesser quality SSDs).

Quoted 3 lines

Am I doing something wrong or do I need to do something more to get

LVM to pass down the TRIM command?

LVM (device mapper specifically) passes discards perfectly well. But

Quoted 7 lines

the snapshot and dm-crypt targets don't have discard support. All other

targets do have discard support. What is your output for: dmsetup table

Mike

I get 0 back when I do "cat /sys/block/sda/queue/discard_zeroes_data" so does this mean that my drive doesn't zero out the TRIMed blocks? I don't really care if it does or does not, I just want to make sure the drive is getting the TRIM commands as the benchmarks I've seen show performance degradation if they aren't used.

Dne 13.4.2011 03:47, [email protected] napsal(a):

Quoted 66 lines · On Apr 12, 2011 6:47pm, Mike Snitzer [email protected]> wrote:

Quoted 58 lines

On Tue, Apr 12 2011 at 10:59am -0400,

Quoted 28 lines · [email protected] [email protected]> wrote:

I recently purchased a Crucial C300 SSD and set it up as my primary

drive. I wanted to use LVM, and my reading indicates that LVM added

support to pass-through TRIM commands with kernel 2.6.37. I'm

running Ubuntu 10.10, but I installed the latest 11.04 kernel, so

I'm running kernel 2.6.38-8. I added "discard" to fstab so my

mountpoint looks like:

Quoted 5 lines

/dev/mapper/vg0-vol0 on / type ext4

(rw,noatime,nodiratime,errors=remount-ro,discard,commit=0)

If the discards aren't working then ext4 will print a warning and stop

issuing them, see: http://git.kernel.org/linus/a30eec2a8650a77f7

Quoted 1 line

Quoted 1 line

So if you don't see "discard not supported, disabling" in your dmesg

(or

Quoted 10 lines

Quoted 3 lines

/var/log/messages) then TRIM is likely working as expected.

Quoted 7 lines

Quoted 7 lines

Quoted 5 lines

I found directions on various sites like:

http://duopetalflower.blogspot.com/2010/11/enterprise-kernel-6-has-ssd-trim.html

Quoted 152 lines

Quoted 145 lines

Quoted 120 lines

on how to confirm if TRIM is working. I ran (as root):

dd if=/dev/urandom of=tempfile count=100 bs=512k oflag=direct

hdparm --fibmap tempfile

(then took the first begin_LBA)

hdparm --read-sector 191710208 /dev/sda

and it printed:

/dev/sda:

reading sector 191710208: succeeded

3254 3a32 3834 313a 2b36 3030 303a 2030

4544 5542 2047 3728 3a29 5320 7661 6e69

2067 6e69 6564 2078 6f66 2072 7270 646f

............

Then I ran:

rm tempfile

sync

hdparm --read-sector 191710208 /dev/sda

and it still returned:

/dev/sda:

reading sector 191710208: succeeded

3254 3a32 3834 313a 2b36 3030 303a 2030

4544 5542 2047 3728 3a29 5320 7661 6e69

2067 6e69 6564 2078 6f66 2072 7270 646f

.............

Quoted 1 line

If TRIM is working, the sector is supposed to contain all zeros.

That is only if the SSD implements TRIM so that it zeroes the discarded

blocks. You can check if the device at least knows enough to report

discard_zeroes_data:

cat /sys/block/sda/queue/discard_zeroes_data

Quoted 1 line

Quoted 1 line

Also, even if the SSD does zero, it may take some time -- well after

the

Quoted 52 lines

Quoted 42 lines

discard has completed -- for the SSD to actually zero the blocks (this

is true of some lesser quality SSDs).

Quoted 5 lines

Am I doing something wrong or do I need to do something more to get

LVM to pass down the TRIM command?

LVM (device mapper specifically) passes discards perfectly well. But

the snapshot and dm-crypt targets don't have discard support. All other

targets do have discard support. What is your output for: dmsetup table

Quoted 1 line

Mike

Quoted 1 line

I get 0 back when I do "cat /sys/block/sda/queue/discard_zeroes_data"

so does

Quoted 1 line

this mean that my drive doesn't zero out the TRIMed blocks? I don't

really

Quoted 1 line

care if it does or does not, I just want to make sure the drive is

getting the

Quoted 1 line

TRIM commands as the benchmarks I've seen show performance degradation

if they

Quoted 1 line

aren't used.

As I've the same drive - what kind of performance degradation do you see?

Note: this drive (unlike many others) is processing TRIM command on

background. So 'traditional' benchmark do not provide valuable results.

(As we've been evaluating this drive somewhat to compare with Intel).

Outcome is - when you issue TRIM command - it's immediately finished - but the

performance of drive is temporarily decreased while the drive is performing

block trimming for some time. Thus you might get 'impressive' results when

some benchmarks are evaluating TRIM performance ;)

Zdenek

I get 0 back when I do "cat /sys/block/sda/queue/discard_zeroes_data" so does this mean that my drive doesn't zero out the TRIMed blocks? I don't really care if it does or does not, I just want to make sure the drive is getting the TRIM commands as the benchmarks I've seen show performance degradation if they aren't used.

Regarding snapshot targets, the only instance of "discard not supported" came as I did a snapshot today:

Apr 12 15:27:58 darknovanick kernel: [65147.211211] EXT4-fs warning
(device dm-0): release_blocks_on_commit:2672: discard not supported,
disabling
Apr 12 15:28:16 darknovanick kernel: [65166.103061] EXT4-fs (dm-1):
72 orphan inodes deleted
Apr 12 15:28:16 darknovanick kernel: [65166.103062] EXT4-fs (dm-1):
recovery complete
Apr 12 15:28:17 darknovanick kernel: [65166.232257] EXT4-fs (dm-1):

mounted filesystem with ordered data mode. Opts: (null)

This snapshot was of a LV on the SSD, but the snapshot itself was being stored on a mechanical hard drive. Does this message mean that discard was just disabled for the snapshot itself (which was deleted a half hour later), or is it now disabled for the entire volume now that I've done a snapshot, even though its been removed?

Quoted 6 lines · On Tue, Apr 12 2011 at 9:47pm -0400, [email protected]…

I get 0 back when I do "cat /sys/block/sda/queue/discard_zeroes_data" so does this mean that my drive doesn't zero out the TRIMed blocks? I don't really care if it does or does not, I just want to make sure the drive is getting the TRIM commands as the benchmarks I've seen show performance degradation if they aren't used.

Right, means TRIM doesn't zero the TRIM'd blocks.

But otherwise it looks like you're in good shape relative to TRIM.

Quoted 11 lines

Regarding snapshot targets, the only instance of "discard not supported" came as I did a snapshot today:

Apr 12 15:27:58 darknovanick kernel: [65147.211211] EXT4-fs warning
(device dm-0): release_blocks_on_commit:2672: discard not supported,
disabling
Apr 12 15:28:16 darknovanick kernel: [65166.103061] EXT4-fs (dm-1):
72 orphan inodes deleted
Apr 12 15:28:16 darknovanick kernel: [65166.103062] EXT4-fs (dm-1):
recovery complete
Apr 12 15:28:17 darknovanick kernel: [65166.232257] EXT4-fs (dm-1):

Quoted 7 lines

mounted filesystem with ordered data mode. Opts: (null)

This snapshot was of a LV on the SSD, but the snapshot itself was being stored on a mechanical hard drive. Does this message mean that discard was just disabled for the snapshot itself (which was deleted a half hour later), or is it now disabled for the entire volume now that I've done a snapshot, even though its been removed?

Unfortunately, the failed discard to the snapshot-origin device disables ext4's discards for the entire volume (even after you remove the snapshot).

But the following patch fixes that:

Subject: dm snapshot: add discard support to the snapshot-origin target

Allow the snapshot-origin target to pass discards to the origin device (after any copyout of the region being discarded; but generally speaking that copyout won't be needed as the region will have already been copied to the cow).

So for example, when you remove a file from an ext4 mounted origin volume it triggers copyout to the snapshot. When the DISCARD is then issued during the subsequent journal commit no copyout is needed.

Quoted 8 lines · Mike Snitzer

Unfortunately, the failed discard to the snapshot-origin device disables ext4's discards for the entire volume (even after you remove the snapshot).

But the following patch fixes that:

Subject: dm snapshot: add discard support to the snapshot-origin target

Quoted 8 lines

Allow the snapshot-origin target to pass discards to the origin device (after any copyout of the region being discarded; but generally speaking that copyout won't be needed as the region will have already been copied to the cow).

So for example, when you remove a file from an ext4 mounted origin volume it triggers copyout to the snapshot. When the DISCARD is then issued during the subsequent journal commit no copyout is needed.

It should be noted that discards are treated as writes. This means that a snapshot should not be taken of an origin volume before a filesystem is placed on that origin. Doing so may result in the entire origin volume being discarded (mkfs.ext4 does this) which would cause copyout of the entire origin volume to the snapshot.

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).

Quoted 6 lines · Mike Snitzer

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)

[cc'ing linux-ext4]

Quoted 11 lines · Christoph Hellwig

Quoted 6 lines · Mike Snitzer

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?

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

Thoughts?

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

Quoted 1 line · Eric Sandeen

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

It's the 'undefined' cases that cause us the trouble though: we do have to return something and I prefer to work with defined and documented behaviour, rather than pretending something is 'undefined' or 'unimportant' and later finding people relied on its actual behaviour.

Alasdair

If you are saying that a target should return EOPNOTSUPP when it does not support the operation, then we are in violent agreement. :)

Quoted 11 lines · Christoph Hellwig

Quoted 6 lines · Mike Snitzer

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?

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

Quoted 6 lines · Mike Snitzer

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")

Quoted 3 lines

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?

Aren't discard operations considered barriers? If there are needless discard operations being sent to the disk then it seems like this would have a non-zero performance penalty, just by virtue of blocking IO submissions at the block device layer.

Quoted 3 lines

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

Hybrid storage is potentially an increasingly popular configuration. We've been looking at DM-mapped SSD RAID-1 + HDD RAID-6 arrays for ext4 to allow storing only the metadata on SSD.

That said, it may
make sense to have the DM translation layer check the component devices of an LV to see if discard is supported on any of them, and only return -EOPNOTSUPP if none of them do.

[cc'ing linux-ext4]

Quoted 11 lines · Christoph Hellwig

Quoted 6 lines · Mike Snitzer

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?
2) assuming the entire block device uniformly supports discards can be flawed (a DM device's discard support can vary based on logical offset).

Thoughts?
--
To unsubscribe from this list: send the line "unsubscribe linux-ext4" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

1. We need to honor all the "discard limits" so the discard bios does not actually fail.
2. If the device is composed of various other devices, we should return -EOPNOTSUPP is none of the devices support discard.
3. We should succeed, if at least one of the devices support discard and it does not fail for any reason.
4. We should not advertise discard_zeroes_data if any of the devices does not zero data or does not support discard.
I am not sure how "hard" is to assure those conditions in DM. If those conditions are met, we can rely on consistent information in the layers above.

FYI, that disable discard on failure seems to be an ext4 special and no one else picked up that stupid idea. If ext4 wants to misbehave for that just let them..

Quoted 2 lines

no one else picked up that stupid idea. If ext4 wants to misbehave for that just let them..

It was my "stupid idea," and I'm ok with reverting it ;)

-Eric

Quoted 3 lines · Christoph Hellwig

FYI, that disable discard on failure seems to be an ext4 special and no one else picked up that stupid idea. If ext4 wants to misbehave for that just let them..

It was my "stupid idea," and I'm ok with reverting it ;)

-Eric
_______________________________________________
linux-lvm mailing list
[email protected]
https://www.redhat.com/mailman/listinfo/linux-lvm
read the LVM HOW-TO at http://tldp.org/HOWTO/LVM-HOWTO/

Quoted 3 lines · Christoph Hellwig

FYI, that disable discard on failure seems to be an ext4 special and no one else picked up that stupid idea. If ext4 wants to misbehave for that just let them..

It was my "stupid idea," and I'm ok with reverting it ;)

Quoted 6 lines · Eric Sandeen

Quoted 3 lines · Christoph Hellwig

FYI, that disable discard on failure seems to be an ext4 special and no one else picked up that stupid idea. If ext4 wants to misbehave for that just let them..

It was my "stupid idea," and I'm ok with reverting it ;)

I think I forgot to send the patch out, but it's been reverted in the ext4 master branch, commit id: d9f34504e695. It's more than a revert, actually, since I also dropped error checking for FITRIM. Otherwise an attempt to use FITRIM would stop after hitting the first dm region that didn't support discards.

- Ted

Quoted 14 lines · Ted Ts'o

Quoted 6 lines · Eric Sandeen

Quoted 3 lines · Christoph Hellwig

FYI, that disable discard on failure seems to be an ext4 special and no one else picked up that stupid idea. If ext4 wants to misbehave for that just let them..

It was my "stupid idea," and I'm ok with reverting it ;)

I think I forgot to send the patch out, but it's been reverted in the ext4 master branch, commit id: d9f34504e695. It's more than a revert, actually, since I also dropped error checking for FITRIM. Otherwise an attempt to use FITRIM would stop after hitting the first dm region that didn't support discards.

Quoted 1 line

- Ted

The xfs must-be-posted-and-reviewed-before-commit policy would be helpful here, I think.

If it's understood and agreed upon, people will get more diligent about review, and it'll be easier to remember to send the patch out - because it's policy.

Thanks,
-Eric

Quoted 6 lines · Eric Sandeen

Quoted 3 lines · Christoph Hellwig

FYI, that disable discard on failure seems to be an ext4 special and no one else picked up that stupid idea. If ext4 wants to misbehave for that just let them..

It was my "stupid idea," and I'm ok with reverting it ;)

I think I forgot to send the patch out, but it's been reverted in the ext4 master branch, commit id: d9f34504e695. It's more than a revert, actually, since I also dropped error checking for FITRIM. Otherwise an attempt to use FITRIM would stop after hitting the first dm region that didn't support discards.

- Ted

Quoted 11 lines · Lukas Czerner

1. We need to honor all the "discard limits" so the discard bios does not actually fail.
2. If the device is composed of various other devices, we should return -EOPNOTSUPP is none of the devices support discard.
3. We should succeed, if at least one of the devices support discard and it does not fail for any reason.
4. We should not advertise discard_zeroes_data if any of the devices does not zero data or does not support discard.
I am not sure how "hard" is to assure those conditions in DM. If those conditions are met, we can rely on consistent information in the layers above.

Remember that -EOPNOTSUPP return applies only to that one *specific* discard. It does not tell you for sure whether or not another future discard to the device will succeed. (It's a property of offset - if there are several devices underneath - and of time - if the device or one below it gets reconfigured.)

The core issue here is whether a filesytem should decide that the receipt of a single -EOPNOTSUPP is a reason never to send any more, whether a more sophisticated algorithm should be used (considering the proportion/offsets of them over given periods of time and retrying later), or whether more comprehensive information about the discard capabilities of the device should be presented - and whether this should be handled automatically or whether it should be under userspace control (i.e. the sysadmin can instruct the filesystem what to do).

dm's role is simply to handle a discard if it can, and report back if it couldn't. Additionally it could provide more comprehensive information about the discard capabilities of the device, but my sense is that most consider this unnecessary as normally dm devices will have a coherent behaviour throughout.

Alasdair

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!

Quoted 3 lines · Lukas Czerner

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.

Quoted 5 lines · Alasdair G Kergon

Quoted 3 lines · Lukas Czerner

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!

Quoted 3 lines

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.

Quoted 10 lines · Christoph Hellwig

Quoted 8 lines · Alasdair G Kergon

Quoted 3 lines · Lukas Czerner

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.

Quoted 3 lines

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!

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).

Quoted 16 lines · Lukas Czerner

Quoted 13 lines · Christoph Hellwig

Quoted 8 lines · Alasdair G Kergon

Quoted 3 lines · Lukas Czerner

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.

Quoted 4 lines

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.

Quoted 10 lines

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.

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.

Mike

On Mon, 2 May 2011, Mike Snitzer wrote:

Quoted 3 lines

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:))

I still think we need to stick with the simple rule: "EOPNOTSUPP returned for a particular bio means that it is not supported for that particular bio" - I don't know what else we can do, without creating an ambiguity...

This does, however, suck for the layer calling in to a complex device.

What is the overhead for sending discard bios down to a device that does not support it?

Quoted 1 line · Lukas Czerner

On Mon, 2 May 2011, Mike Snitzer wrote:

...

Quoted 4 lines

Quoted 3 lines

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.

Quoted 6 lines

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:))

I still think we need to stick with the simple rule: "EOPNOTSUPP returned for a particular bio means that it is not supported for that particular bio" - I don't know what else we can do, without creating an ambiguity...

This does, however, suck for the layer calling in to a complex device.

What is the overhead for sending discard bios down to a device that does not support it?

-Eric

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

"Lukas" == Lukas Czerner <[email protected]> writes:

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

"Lukas" == Lukas Czerner <[email protected]> writes:

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

"Lukas" == Lukas Czerner <[email protected]> writes:

Lukas> So I gave it a try. First of all the device composed of SSD and Lukas> spinning disk does export discard_support information properly, Lukas> however it also advertise discard_zeroes_data which is wrong and Lukas> possibly dangerous and should be fixed!

I can't reproduce this here. If I mix discard and non-discard devices things work correctly. discard_zeroes_data also gets cleared if I mix discard-capable drives where one zeroes and one doesn't.

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

"Lukas" == Lukas Czerner <[email protected]> writes:

Quoted 12 lines · Martin K. Petersen

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

"Lukas" == Lukas Czerner <[email protected]> writes:

Lukas> So I gave it a try. First of all the device composed of SSD and Lukas> spinning disk does export discard_support information properly, Lukas> however it also advertise discard_zeroes_data which is wrong and Lukas> possibly dangerous and should be fixed!

I can't reproduce this here. If I mix discard and non-discard devices things work correctly. discard_zeroes_data also gets cleared if I mix discard-capable drives where one zeroes and one doesn't.

[root@trim ~]# hdparm -I /dev/sdb | grep -i trim [root@trim ~]# cat /sys/block/sdb/queue/discard_zeroes_data 0
[root@trim ~]# hdparm -I /dev/sdd | grep -i trim
* Data Set Management TRIM supported
* Deterministic read after TRIM
[root@trim ~]# cat /sys/block/sdd/queue/discard_zeroes_data 1
[root@trim ~]# pvcreate /dev/sdd1
Physical volume "/dev/sdd1" successfully created
[root@trim ~]# pvcreate /dev/sdb3
Physical volume "/dev/sdb3" successfully created

[root@trim ~]# vgcreate vg_test /dev/sdd1 /dev/sdb3
Volume group "vg_test" successfully created
[root@trim ~]# lvcreate -L 3500M vg_test
Logical volume "lvol0" created
[root@trim ~]# ls -lah /dev/mapper/vg_test-lvol0
lrwxrwxrwx. 1 root root 7 2. kvě 10.30 /dev/mapper/vg_test-lvol0 -> ../dm-0 [root@trim ~]# cat /sys/block/dm-0/queue/discard_zeroes_data
1

So I assume it got fixes just recently ?. I'll give it a try with the up-to-date kernel.

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

"Lukas" == Lukas Czerner <[email protected]> writes:

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

"Lukas" == Lukas Czerner <[email protected]> writes:

Lukas,

Lukas> [root@trim ~]# lvcreate -L 3500M vg_test
Lukas> Logical volume "lvol0" created

Ok, so here's what I think is going on. You're creating a linear target which happens to fit inside the first PV.

Here's two devices. One that supports discard_zeroes_data=1 (8:17) and one that doesn't (8:49).

# dmsetup table 
foo-bar: 0 1032192 striped 2 32 8:17 384 8:49 384
foo-baz: 0 106496 linear 8:17 516480

# grep . /sys/block/d*/queue/discard_z*
/sys/block/dm-0/queue/discard_zeroes_data:0
/sys/block/dm-1/queue/discard_zeroes_data:1

LV foo/bar (dm-0) is striped, straddling two devices with incompatible values. Hence 0.

LV foo/baz (dm-1) is linear and fits inside the first PV. Thus it has discard_zeroes_data=1.

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

"Lukas" == Lukas Czerner <[email protected]> writes:

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

"Lukas" == Lukas Czerner <[email protected]> writes:

Lukas> Nevertheless there is something weird going on, because even when Lukas> I create striped volume I get this:

Could you please try the following patch? It has a bunch of small tweaks to the discard stack in it. I'll split it up before posting for real but I'd like to know if it fixes your issue...

block/libata/scsi: Various logical block provisioning fixes

- Add sysfs documentation for the discard topology parameters

- Fix discard stacking problem

- Switch our libata SAT over to using the WRITE SAME limits

- UNMAP alignment needs to be converted to bytes

- Only report alignment and zeroes_data if the device supports discard

Reported-by Lukas Czerner <[email protected]>

Signed-off-by Martin K. Petersen <[email protected]>

block/blk-settings.c +0 -0

index 1fa7692..42d3bf5 100644

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

"Mike" == Mike Snitzer <[email protected]> writes:

Quoted 9 lines

block/blk-settings.c +1 -0

@@ -166,6 +166,7 @@ void blk_queue_make_request(struct request_queue
 *q, make_request_fn *mfn)
 
 blk_set_default_limits(&q->limits);
 blk_queue_max_hw_sectors(q, BLK_SAFE_MAX_SECTORS);
+ q->limits.discard_zeroes_data = 0;
 
 /*

* by default assume old behaviour and bounce for any highmem page

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

"Mike" == Mike Snitzer <[email protected]> writes:

Mike> lim->discard_zeroes_data = -1; was suspect to me too. Mike> But why default to 1 here?

Because otherwise DM would default to having dzd to "unsupported", meaning the feature would never be turned on regardless of the bottom device capabilities.

The old approach used the -1 value to indicate "has not been set". That was only really intended as a value for the stacking drivers, not for the LLDs. It was a bit of a hack and I'd rather deal with dzd the same way as we do with clustering.

Quoted 9 lines

Quoted 9 lines

block/blk-settings.c +1 -0

@@ -166,6 +166,7 @@ void blk_queue_make_request(struct request_queue
 *q, make_request_fn *mfn)
 
 blk_set_default_limits(&q->limits);
 blk_queue_max_hw_sectors(q, BLK_SAFE_MAX_SECTORS);
+ q->limits.discard_zeroes_data = 0;
 
 /*

* by default assume old behaviour and bounce for any highmem page

Mike> Only to then reset to 0 here? Shouldn't we default to 0 and only Mike> set to 1 where applicable (e.g. sd_config_discard)?

My first approach was to set it in dm-table.c before stacking. But I thought it was icky to have the stacking driver ask for defaults and then have to tweak them for things to work correctly.

The other option is to have blk_set_default_stacking_limits(). Or we could add a flag to blk_set_default_limits to indicate whether this is a LLD or a stacking driver.

We already special-case BLK_SAFE_MAX_SECTORS when setting the request function. And that's the only non-stacking user of the default limits call. So that's why I disabled dzd there. Since this is a stable bugfix I also wanted to keep it small and simple. But I'm totally open to suggestions.

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

"Lukas" == Lukas Czerner <[email protected]> writes:

Lukas> Nevertheless there is something weird going on, because even when Lukas> I create striped volume I get this:

Could you please try the following patch? It has a bunch of small tweaks to the discard stack in it. I'll split it up before posting for real but I'd like to know if it fixes your issue...

block/libata/scsi: Various logical block provisioning fixes

- Add sysfs documentation for the discard topology parameters

- Fix discard stacking problem

- Switch our libata SAT over to using the WRITE SAME limits

- UNMAP alignment needs to be converted to bytes

- Only report alignment and zeroes_data if the device supports discard

Reported-by Lukas Czerner <[email protected]>

Signed-off-by Martin K. Petersen <[email protected]>

Documentation/ABI/testing/sysfs-block +64 -0

index 4873c75..c1eb41c 100644
@@ -142,3 +142,67 @@ Description:
 		with the previous I/O request are enabled. When set to 2,
 		all merge tries are disabled. The default value is 0 -
 		which enables all types of merge tries.
+
+What:		/sys/block/<disk>/discard_alignment
+Date:		May 2011
+Contact:	Martin K. Petersen <[email protected]>
+Description:
+		Devices that support discard functionality may
+		internally allocate space in units that are bigger than
+		the exported logical block size. The discard_alignment
+		parameter indicates how many bytes the beginning of the
+		device is offset from the internal allocation unit's
+		natural alignment.
+
+What:		/sys/block/<disk>/<partition>/discard_alignment
+Date:		May 2011
+Contact:	Martin K. Petersen <[email protected]>
+Description:
+		Devices that support discard functionality may
+		internally allocate space in units that are bigger than
+		the exported logical block size. The discard_alignment
+		parameter indicates how many bytes the beginning of the
+		partition is offset from the internal allocation unit's
+		natural alignment.
+
+What:		/sys/block/<disk>/queue/discard_granularity
+Date:		May 2011
+Contact:	Martin K. Petersen <[email protected]>
+Description:
+		Devices that support discard functionality may
+		internally allocate space using units that are bigger
+		than the logical block size. The discard_granularity
+		parameter indicates the size of the internal allocation
+		unit in bytes if reported by the device. Otherwise the
+		discard_granularity will be set to match the device's
+		physical block size. A discard_granularity of 0 means
+		that the device does not support discard functionality.
+
+What:		/sys/block/<disk>/queue/discard_max_bytes
+Date:		May 2011
+Contact:	Martin K. Petersen <[email protected]>
+Description:
+		Devices that support discard functionality may have
+		internal limits on the number of bytes that can be
+		trimmed or unmapped in a single operation. Some storage
+		protocols also have inherent limits on the number of
+		blocks that can be described in a single command. The
+		discard_max_bytes parameter is set by the device driver
+		to the maximum number of bytes that can be discarded in
+		a single operation. Discard requests issued to the
+		device must not exceed this limit. A discard_max_bytes
+		value of 0 means that the device does not support
+		discard functionality.
+
+What:		/sys/block/<disk>/queue/discard_zeroes_data
+Date:		May 2011
+Contact:	Martin K. Petersen <[email protected]>
+Description:
+		Devices that support discard functionality may return
+		stale or random data when a previously discarded block
+		is read back. This can cause problems if the filesystem
+		expects discarded blocks to be explicitly cleared. If a
+		device reports that it deterministically returns zeroes
+		when a discarded area is read the discard_zeroes_data
+		parameter will be set to one. Otherwise it will be 0 and
+		the result of reading a discarded area is undefined.

block/blk-settings.c +2 -1

index 1fa7692..42d3bf5 100644
@@ -120,7 +120,7 @@ void blk_set_default_limits(struct queue_limits *lim)
 	lim->discard_granularity = 0;
 	lim->discard_alignment = 0;
 	lim->discard_misaligned = 0;
-	lim->discard_zeroes_data = -1;
+	lim->discard_zeroes_data = 1;
 	lim->logical_block_size = lim->physical_block_size = lim->io_min = 512;
 	lim->bounce_pfn = (unsigned long)(BLK_BOUNCE_ANY >> PAGE_SHIFT);
 	lim->alignment_offset = 0;

@@ -166,6 +166,7 @@ void blk_queue_make_request(struct request_queue *q, make_request_fn *mfn)
 
 	blk_set_default_limits(&q->limits);
 	blk_queue_max_hw_sectors(q, BLK_SAFE_MAX_SECTORS);
+	q->limits.discard_zeroes_data = 0;
 
 	/*
 	 * by default assume old behaviour and bounce for any highmem page

drivers/ata/libata-scsi.c +1 -1

index e2f57e9e..30ea95f 100644
@@ -2138,7 +2138,7 @@ static unsigned int ata_scsiop_inq_b0(struct ata_scsi_args *args, u8 *rbuf)
 	 * with the unmap bit set.
 	 */
 	if (ata_id_has_trim(args->id)) {
-		put_unaligned_be32(65535 * 512 / 8, &rbuf[20]);
+		put_unaligned_be64(65535 * 512 / 8, &rbuf[36]);
 		put_unaligned_be32(1, &rbuf[28]);
 	}
 

drivers/scsi/sd.c +2 -1

index bd0806e..cc081dd 100644
@@ -490,7 +490,8 @@ static void sd_config_discard(struct scsi_disk *sdkp, unsigned int mode)
 	unsigned int max_blocks = 0;
 
 	q->limits.discard_zeroes_data = sdkp->lbprz;
-	q->limits.discard_alignment = sdkp->unmap_alignment;
+	q->limits.discard_alignment = sdkp->unmap_alignment *
+		(logical_block_size >> 9);
 	q->limits.discard_granularity =
 		max(sdkp->physical_block_size,
 		    sdkp->unmap_granularity * logical_block_size);

include/linux/blkdev.h +5 -2

index 2ad95fa..1416e3c 100644
@@ -257,7 +257,7 @@ struct queue_limits {
 	unsigned char		misaligned;
 	unsigned char		discard_misaligned;
 	unsigned char		cluster;
-	signed char		discard_zeroes_data;
+	unsigned char		discard_zeroes_data;
 };
 
 struct request_queue

@@ -1066,13 +1066,16 @@ static inline int queue_limit_discard_alignment(struct queue_limits *lim, sector
 {
 	unsigned int alignment = (sector << 9) & (lim->discard_granularity - 1);
 
+	if (!lim->max_discard_sectors)
+		return 0;
+
 	return (lim->discard_granularity + lim->discard_alignment - alignment)
 		& (lim->discard_granularity - 1);
 }
 
 static inline unsigned int queue_discard_zeroes_data(struct request_queue *q)
 {
-	if (q->limits.discard_zeroes_data == 1)
+	if (q->limits.max_discard_sectors && q->limits.discard_zeroes_data == 1)
 		return 1;
 
 	return 0;

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

"Lukas" == Lukas Czerner <[email protected]> writes:

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

"Lukas" == Lukas Czerner <[email protected]> writes:

Lukas> Also I am not sure why this changed discard_max_bytes from Lukas> 4294966784 to 2147450880 in my case, that does not seem Lukas> right...lsblk in the first place apparently overflowed.

2G is correct. The relevant libata patch was missing from the series I submitted during the merge window and I don't see it in the archives. It was at the end of the stack so I guess I had an off-by-one in my commit range...

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

"Lukas" == Lukas Czerner <[email protected]> writes:

Lukas> Nevertheless there is something weird going on, because even when Lukas> I create striped volume I get this:

Could you please try the following patch? It has a bunch of small tweaks to the discard stack in it. I'll split it up before posting for real but I'd like to know if it fixes your issue...

block/libata/scsi: Various logical block provisioning fixes

- Add sysfs documentation for the discard topology parameters

- Fix discard stacking problem

- Switch our libata SAT over to using the WRITE SAME limits

- UNMAP alignment needs to be converted to bytes

- Only report alignment and zeroes_data if the device supports discard

Reported-by Lukas Czerner <[email protected]>

Signed-off-by Martin K. Petersen <[email protected]>

Documentation/ABI/testing/sysfs-block +64 -0

index 4873c75..c1eb41c 100644
@@ -142,3 +142,67 @@ Description:
 		with the previous I/O request are enabled. When set to 2,
 		all merge tries are disabled. The default value is 0 -
 		which enables all types of merge tries.
+
+What:		/sys/block/<disk>/discard_alignment
+Date:		May 2011
+Contact:	Martin K. Petersen <[email protected]>
+Description:
+		Devices that support discard functionality may
+		internally allocate space in units that are bigger than
+		the exported logical block size. The discard_alignment
+		parameter indicates how many bytes the beginning of the
+		device is offset from the internal allocation unit's
+		natural alignment.
+
+What:		/sys/block/<disk>/<partition>/discard_alignment
+Date:		May 2011
+Contact:	Martin K. Petersen <[email protected]>
+Description:
+		Devices that support discard functionality may
+		internally allocate space in units that are bigger than
+		the exported logical block size. The discard_alignment
+		parameter indicates how many bytes the beginning of the
+		partition is offset from the internal allocation unit's
+		natural alignment.
+
+What:		/sys/block/<disk>/queue/discard_granularity
+Date:		May 2011
+Contact:	Martin K. Petersen <[email protected]>
+Description:
+		Devices that support discard functionality may
+		internally allocate space using units that are bigger
+		than the logical block size. The discard_granularity
+		parameter indicates the size of the internal allocation
+		unit in bytes if reported by the device. Otherwise the
+		discard_granularity will be set to match the device's
+		physical block size. A discard_granularity of 0 means
+		that the device does not support discard functionality.
+
+What:		/sys/block/<disk>/queue/discard_max_bytes
+Date:		May 2011
+Contact:	Martin K. Petersen <[email protected]>
+Description:
+		Devices that support discard functionality may have
+		internal limits on the number of bytes that can be
+		trimmed or unmapped in a single operation. Some storage
+		protocols also have inherent limits on the number of
+		blocks that can be described in a single command. The
+		discard_max_bytes parameter is set by the device driver
+		to the maximum number of bytes that can be discarded in
+		a single operation. Discard requests issued to the
+		device must not exceed this limit. A discard_max_bytes
+		value of 0 means that the device does not support
+		discard functionality.
+
+What:		/sys/block/<disk>/queue/discard_zeroes_data
+Date:		May 2011
+Contact:	Martin K. Petersen <[email protected]>
+Description:
+		Devices that support discard functionality may return
+		stale or random data when a previously discarded block
+		is read back. This can cause problems if the filesystem
+		expects discarded blocks to be explicitly cleared. If a
+		device reports that it deterministically returns zeroes
+		when a discarded area is read the discard_zeroes_data
+		parameter will be set to one. Otherwise it will be 0 and
+		the result of reading a discarded area is undefined.

block/blk-settings.c +2 -1

index 1fa7692..42d3bf5 100644
@@ -120,7 +120,7 @@ void blk_set_default_limits(struct queue_limits *lim)
 	lim->discard_granularity = 0;
 	lim->discard_alignment = 0;
 	lim->discard_misaligned = 0;
-	lim->discard_zeroes_data = -1;
+	lim->discard_zeroes_data = 1;
 	lim->logical_block_size = lim->physical_block_size = lim->io_min = 512;
 	lim->bounce_pfn = (unsigned long)(BLK_BOUNCE_ANY >> PAGE_SHIFT);
 	lim->alignment_offset = 0;

@@ -166,6 +166,7 @@ void blk_queue_make_request(struct request_queue *q, make_request_fn *mfn)
 
 	blk_set_default_limits(&q->limits);
 	blk_queue_max_hw_sectors(q, BLK_SAFE_MAX_SECTORS);
+	q->limits.discard_zeroes_data = 0;
 
 	/*
 	 * by default assume old behaviour and bounce for any highmem page

drivers/ata/libata-scsi.c +1 -1

index e2f57e9e..30ea95f 100644
@@ -2138,7 +2138,7 @@ static unsigned int ata_scsiop_inq_b0(struct ata_scsi_args *args, u8 *rbuf)
 	 * with the unmap bit set.
 	 */
 	if (ata_id_has_trim(args->id)) {
-		put_unaligned_be32(65535 * 512 / 8, &rbuf[20]);
+		put_unaligned_be64(65535 * 512 / 8, &rbuf[36]);
 		put_unaligned_be32(1, &rbuf[28]);
 	}
 

drivers/scsi/sd.c +2 -1

index bd0806e..cc081dd 100644
@@ -490,7 +490,8 @@ static void sd_config_discard(struct scsi_disk *sdkp, unsigned int mode)
 	unsigned int max_blocks = 0;
 
 	q->limits.discard_zeroes_data = sdkp->lbprz;
-	q->limits.discard_alignment = sdkp->unmap_alignment;
+	q->limits.discard_alignment = sdkp->unmap_alignment *
+		(logical_block_size >> 9);
 	q->limits.discard_granularity =
 		max(sdkp->physical_block_size,
 		    sdkp->unmap_granularity * logical_block_size);

include/linux/blkdev.h +5 -3

index 2ad95fa..1416e3c 100644
@@ -257,7 +257,7 @@ struct queue_limits {
 	unsigned char		misaligned;
 	unsigned char		discard_misaligned;
 	unsigned char		cluster;
-	signed char		discard_zeroes_data;
+	unsigned char		discard_zeroes_data;
 };
 
 struct request_queue

@@ -1066,13 +1066,16 @@ static inline int queue_limit_discard_alignment(struct queue_limits *lim, sector
 {
 	unsigned int alignment = (sector << 9) & (lim->discard_granularity - 1);
 
+	if (!lim->max_discard_sectors)
+		return 0;
+
 	return (lim->discard_granularity + lim->discard_alignment - alignment)
 		& (lim->discard_granularity - 1);
 }
 
 static inline unsigned int queue_discard_zeroes_data(struct request_queue *q)
 {
-	if (q->limits.discard_zeroes_data == 1)
+	if (q->limits.max_discard_sectors && q->limits.discard_zeroes_data == 1)
 		return 1;
 
 	return 0;
--

dm-devel mailing list
[email protected]
https://www.redhat.com/mailman/listinfo/dm-devel

Hey Martin,

Quoted 5 lines · Martin K. Petersen

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

"Lukas" == Lukas Czerner <[email protected]> writes:

Lukas> Nevertheless there is something weird going on, because even when Lukas> I create striped volume I get this:

Quoted 3 lines

Could you please try the following patch? It has a bunch of small tweaks to the discard stack in it. I'll split it up before posting for real but I'd like to know if it fixes your issue...

Would be ideal to get these fixes staged for 2.6.40.

Do you intend to split these patches up and post for 2.6.40 inclussion?

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

"Lukas" == Lukas Czerner <[email protected]> writes:

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

"Lukas" == Lukas Czerner <[email protected]> writes:

I got tired of poking around in sysfs to find the discard topology. Here's a patch against lsblk that adds a -D option to present this information in a human-readable form:

# lsblk -D
NAME                  DISC-ALN DISC-GRAN DISC-MAX DISC-ZERO
sda                          0        0B       0B         0
└─sda1                       0        0B       0B         0
sdb                          0      512B       2G         1
└─sdb1                       0      512B       2G         1

Signed-off-by Martin K. Petersen <[email protected]>

misc-utils/lsblk.8 +2 -0

index 38ff48f..d7d7aa8 100644
@@ -29,6 +29,8 @@ Print the SIZE column in bytes rather than in human-readable format.
 .IP "\fB\-d, \-\-nodeps\fP"
 Don't print device holders or slaves.  For example "lsblk --nodeps /dev/sda" prints
 information about the sda device only.
+.IP "\fB\-D, \-\-discard\fP"
+Print information about the discard (TRIM, UNMAP) capabilities for each device.
 .IP "\fB\-e, \-\-exclude \fIlist\fP
 Exclude the devices specified by a comma-separated \fIlist\fR of major device numbers.
 Note that RAM disks (major=1) are excluded by default.

misc-utils/lsblk.c +46 -3

index 38326d0..671e690 100644
@@ -77,6 +77,10 @@ enum {
 	COL_ROTA,
 	COL_SCHED,
 	COL_TYPE,
+	COL_DALIGN,
+	COL_DGRAN,
+	COL_DMAX,
+	COL_DZERO,
 
 	__NCOLUMNS
 };

@@ -112,8 +116,11 @@ static struct colinfo infos[__NCOLUMNS] = {
 	[COL_PHYSEC] = { "PHY-SEC", 7, TT_FL_RIGHT, N_("physical sector size") },
 	[COL_LOGSEC] = { "LOG-SEC", 7, TT_FL_RIGHT, N_("logical sector size") },
 	[COL_SCHED]  = { "SCHED",   0.1, 0, N_("I/O scheduler name") },
-	[COL_TYPE]   = { "TYPE",    4, 0, N_("device type") }
-
+	[COL_TYPE]   = { "TYPE",    4, 0, N_("device type") },
+	[COL_DALIGN] = { "DISC-ALN", 6, TT_FL_RIGHT, N_("discard alignment offset") },
+	[COL_DGRAN]  = { "DISC-GRAN", 6, TT_FL_RIGHT, N_("discard granularity") },
+	[COL_DMAX]   = { "DISC-MAX", 6, TT_FL_RIGHT, N_("discard max bytes") },
+	[COL_DZERO]  = { "DISC-ZERO", 1, TT_FL_RIGHT, N_("discard zeroes data") },
 };
 
 struct lsblk {

@@ -702,6 +709,33 @@ static void set_tt_data(struct blkdev_cxt *cxt, int col, int id, struct tt_line
 		if (p)
 			tt_line_set_data(ln, col, p);
 		break;
+	case COL_DALIGN:
+		p = sysfs_strdup(cxt, "discard_alignment");
+		if (p)
+			tt_line_set_data(ln, col, p);
+		break;
+	case COL_DGRAN:
+		p = sysfs_strdup(cxt, "queue/discard_granularity");
+		if (!lsblk->bytes)
+			p = size_to_human_string(atoi(p));
+
+		if (p)
+			tt_line_set_data(ln, col, p);
+		break;
+	case COL_DMAX:
+		p = sysfs_strdup(cxt, "queue/discard_max_bytes");
+
+		if (!lsblk->bytes)
+			p = size_to_human_string(atoi(p));
+
+		if (p)
+			tt_line_set_data(ln, col, p);
+		break;
+	case COL_DZERO:
+		p = sysfs_strdup(cxt, "queue/discard_zeroes_data");
+		if (p)
+			tt_line_set_data(ln, col, p);
+		break;
 	};
 }
 

@@ -930,6 +964,7 @@ static void __attribute__((__noreturn__)) help(FILE *out)
 		" -a, --all            print all devices\n"
 		" -b, --bytes          print SIZE in bytes rather than in human readable format\n"
 		" -d, --nodeps         don't print slaves or holders\n"
+		" -D, --discard        print discard capabilities\n"
 		" -e, --exclude <list> exclude devices by major number (default: RAM disks)\n"
 		" -f, --fs             output info about filesystems\n"
 		" -h, --help           usage information (this)\n"

@@ -967,6 +1002,7 @@ int main(int argc, char *argv[])
 		{ "all",	0, 0, 'a' },
 		{ "bytes",      0, 0, 'b' },
 		{ "nodeps",     0, 0, 'd' },
+		{ "discard",    0, 0, 'D' },
 		{ "help",	0, 0, 'h' },
 		{ "output",     1, 0, 'o' },
 		{ "perms",      0, 0, 'm' },

@@ -987,7 +1023,7 @@ int main(int argc, char *argv[])
 	lsblk = &_ls;
 	memset(lsblk, 0, sizeof(*lsblk));
 
-	while((c = getopt_long(argc, argv, "abde:fhlnmo:irt", longopts, NULL)) != -1) {
+	while((c = getopt_long(argc, argv, "abdDe:fhlnmo:irt", longopts, NULL)) != -1) {
 		switch(c) {
 		case 'a':
 			lsblk->all_devices = 1;

@@ -998,6 +1034,13 @@ int main(int argc, char *argv[])
 		case 'd':
 			lsblk->nodeps = 1;
 			break;
+		case 'D':
+			columns[ncolumns++] = COL_NAME;
+			columns[ncolumns++] = COL_DALIGN;
+			columns[ncolumns++] = COL_DGRAN;
+			columns[ncolumns++] = COL_DMAX;
+			columns[ncolumns++] = COL_DZERO;
+			break;
 		case 'e':
 			parse_excludes(optarg);
 			break;

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

"Lukas" == Lukas Czerner <[email protected]> writes:

I got tired of poking around in sysfs to find the discard topology. Here's a patch against lsblk that adds a -D option to present this information in a human-readable form:

# lsblk -D
NAME                  DISC-ALN DISC-GRAN DISC-MAX DISC-ZERO
sda                          0        0B       0B         0
└─sda1                       0        0B       0B         0
sdb                          0      512B       2G         1
└─sdb1                       0      512B       2G         1

Quoted 5 lines · Martin K. Petersen

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

"Lukas" == Lukas Czerner <[email protected]> writes:

I got tired of poking around in sysfs to find the discard topology. Here's a patch against lsblk that adds a -D option to present this information in a human-readable form:

Applied, thanks.

Quoted 6 lines

# lsblk -D
NAME                  DISC-ALN DISC-GRAN DISC-MAX DISC-ZERO
sda                          0        0B       0B         0
└─sda1                       0        0B       0B         0
sdb                          0      512B       2G         1
└─sdb1                       0      512B       2G         1

I have a question, 2.6.35 on my ThinkPad, non-SSD disk:

NAME DISC-ALN DISC-GRAN DISC-MAX DISC-ZERO
sda 0 0B 0B 0

├─sda1               4294935040        0B       0B         0
├─sda2               4188038656        0B       0B         0
├─sda3               1346205184        0B       0B         0
├─sda4               3231165440        0B       0B         0
├─sda5               4188006400        0B       0B         0
│ └─kzak-home (dm-0)          0        0B       0B         0
└─sda6               2035725312        0B       0B         0

Does is make sense? The DISC-ALN is non-zero but DISC-GRAN is zero.

Note that cat /sys/block/sda/sda*/discard_alignment returns the same numbers.

Karel

Unfortunately, the failed discard to the snapshot-origin device disables

ext4's discards for the entire volume (even after you remove the

snapshot).

But the following patch fixes that:

Subject: dm snapshot: add discard support to the snapshot-origin target

Allow the snapshot-origin target to pass discards to the origin device

(after any copyout of the region being discarded; but generally speaking

that copyout won't be needed as the region will have already been copied

to the cow).

So for example, when you remove a file from an ext4 mounted origin

volume it triggers copyout to the snapshot. When the DISCARD is then

issued during the subsequent journal commit no copyout is needed.

Signed-off-by Mike Snitzer [email protected]>

---

drivers/md/dm-snap.c | 3 ++-

1 files changed, 2 insertions(+), 1 deletions(-)

(quoted diff) +0 -0

index a2d3309..028d216 100644

--- a/drivers/md/dm-snap.c

+++ b/drivers/md/dm-snap.c

drivers/md/dm-snap.c +0 -0

@@ -2076,6 +2076,7 @@ static int origin_ctr(struct dm_target *ti,  
 unsigned int argc, char **argv)
 
 
 
 ti->private = dev;
 

ti->num_flush_requests = 1;

+ ti->num_discard_requests = 1;

return 0;

}

(quoted diff) +1 -1

@@ -2153,7 +2154,7 @@ static int origin_iterate_devices(struct dm_target  
 *ti,
 
 
 
 static struct target_type origin_target = {
 
 .name = "snapshot-origin",
 
- .version = {1, 7, 1},
 
+ .version = {1, 8, 0},

.module = THIS_MODULE,

.ctr = origin_ctr,

.dtr = origin_dtr,