This makes dm raid and dm raid1 (mirroring) consistent with md raid,
which supports discard when only some of the devices support discard.
Another patch will be needed to fix the queue discard limits sysfs files,
fixing `fstrim --fstab`, but these patches suffice to fix `fstrim /` and
I haven't finished figuring out how the queue discard limits are set yet.
Paul Wise (3):
dm: add support for targets that allow discard when one device does
dm raid: only check for RAID 4/5/6 once during discard support setup
dm raid/raid1: enable discard support when any devices support discard
drivers/md/dm-cache-target.c | 2 +-
drivers/md/dm-clone-target.c | 2 +-
drivers/md/dm-log-writes.c | 2 +-
drivers/md/dm-raid.c | 21 ++++++++++-----------
drivers/md/dm-raid1.c | 1 +
drivers/md/dm-table.c | 32 +++++++++++++++++++++-----------
drivers/md/dm-thin.c | 8 ++++----
drivers/md/dm-zoned-target.c | 2 +-
include/linux/device-mapper.h | 13 ++++++++-----
include/uapi/linux/dm-ioctl.h | 4 ++--
10 files changed, 50 insertions(+), 37 deletions(-)
--
2.26.1
The RAID level 4/5/6 check no longer looks at the devices in the array,
so it isn't necessary for it to be checked once for each device,
so check it before the loop over the devices.
This makes the code cleaner and easier to understand since it
disentangles whole-array checks from per-device checks.
Commit 48920ff2a5a9 ("block: remove the discard_zeroes_data flag") removed
the per-device discard_zeroes_data check since REQ_OP_WRITE_ZEROES
operation was used everywhere and commit 48cf06bc5f50 ("dm raid: add
discard support for RAID levels 4, 5 and 6") introduced the RAID 4/5/6
check.
Signed-off-by: Paul Wise <[email protected]>
---
drivers/md/dm-raid.c | 15 +++++----------
1 file changed, 5 insertions(+), 10 deletions(-)
diff --git a/drivers/md/dm-raid.c b/drivers/md/dm-raid.c
index 9a18bef0a5ff..0f95e50e62a8 100644
--- a/drivers/md/dm-raid.c
+++ b/drivers/md/dm-raid.c
@@ -2944,13 +2944,16 @@ static int rs_setup_reshape(struct raid_set *rs)
static void configure_discard_support(struct raid_set *rs)
{
int i;
- bool raid456;
struct dm_target *ti = rs->ti;
/*
* XXX: RAID level 4,5,6 require zeroing for safety.
*/
- raid456 = rs_is_raid456(rs);
+ if (rs_is_raid456(rs) && !devices_handle_discard_safely) {
+ DMERR("raid456 discard support disabled due to discard_zeroes_data uncertainty.");
+ DMERR("Set dm-raid.devices_handle_discard_safely=Y to override.");
+ return;
+ }
for (i = 0; i < rs->raid_disks; i++) {
struct request_queue *q;
@@ -2961,14 +2964,6 @@ static void configure_discard_support(struct raid_set *rs)
q = bdev_get_queue(rs->dev[i].rdev.bdev);
if (!q || !blk_queue_discard(q))
return;
-
- if (raid456) {
- if (!devices_handle_discard_safely) {
- DMERR("raid456 discard support disabled due to discard_zeroes_data uncertainty.");
- DMERR("Set dm-raid.devices_handle_discard_safely=Y to override.");
- return;
- }
- }
}
ti->num_discard_bios = 1;
--
2.26.1
This will allow fstrim to work on filesystems on dm RAID arrays with
both HDDs and SSDs or dm raid SSD arrays with varying discard support,
which should increase the lifetime of the SSDs that support discard.
This makes dm raid and dm raid1 (mirroring) consistent with md raid,
which supports discard when only some of the devices support discard.
The existing code prevents this from being enabled with RAID 4/5/6,
which require more certainty about the behaviour of underlying devices
after a discard has been issued and processed.
Simply enable discard and return from the configure_discard_support
function when any of the underlying devices has support for discards,
since there are now no other checks in the device check loop.
Mixed discard support for md RAID types was added in these commits:
commit c83057a1f4f9 ("md: raid 0 supports TRIM")
commit 2ff8cc2c6d4e ("md: raid 1 supports TRIM")
commit 532a2a3fba8d ("md: raid 10 supports TRIM")
commit f1cad2b68ed1 ("md: linear supports TRIM")
Signed-off-by: Paul Wise <[email protected]>
---
drivers/md/dm-raid.c | 8 ++++++--
drivers/md/dm-raid1.c | 1 +
2 files changed, 7 insertions(+), 2 deletions(-)
diff --git a/drivers/md/dm-raid.c b/drivers/md/dm-raid.c
index 0f95e50e62a8..63f5d05021a9 100644
--- a/drivers/md/dm-raid.c
+++ b/drivers/md/dm-raid.c
@@ -2962,11 +2962,15 @@ static void configure_discard_support(struct raid_set *rs)
continue;
q = bdev_get_queue(rs->dev[i].rdev.bdev);
- if (!q || !blk_queue_discard(q))
+ if (q && blk_queue_discard(q)) {
+ ti->discard_support = DM_DISCARD_ANY_DEVS;
+ ti->num_discard_bios = 1;
return;
+ }
}
- ti->num_discard_bios = 1;
+ ti->discard_support = DM_DISCARD_ALL_DEVS;
+ ti->num_discard_bios = 0;
}
/*
diff --git a/drivers/md/dm-raid1.c b/drivers/md/dm-raid1.c
index 089aed57e083..2bfed681dd3f 100644
--- a/drivers/md/dm-raid1.c
+++ b/drivers/md/dm-raid1.c
@@ -1114,6 +1114,7 @@ static int mirror_ctr(struct dm_target *ti, unsigned int argc, char **argv)
goto err_free_context;
ti->num_flush_bios = 1;
+ ti->discard_support = DM_DISCARD_ANY_DEVS;
ti->num_discard_bios = 1;
ti->per_io_data_size = sizeof(struct dm_raid1_bio_record);
--
2.26.1
Rename the discards_supported variable to discard_support since
the discard support isn't just turned on or off and there
are now three types of discard support for dm targets.
Switch discard_support to using an enum so that the other types of
discard support (all devices or ignore devices) can be accommodated.
This is needed so that dm raid and dm raid1 can support discard for
arrays of devices of varying discard support where this is safe.
Signed-off-by: Paul Wise <[email protected]>
---
drivers/md/dm-cache-target.c | 2 +-
drivers/md/dm-clone-target.c | 2 +-
drivers/md/dm-log-writes.c | 2 +-
drivers/md/dm-table.c | 32 +++++++++++++++++++++-----------
drivers/md/dm-thin.c | 8 ++++----
drivers/md/dm-zoned-target.c | 2 +-
include/linux/device-mapper.h | 13 ++++++++-----
include/uapi/linux/dm-ioctl.h | 4 ++--
8 files changed, 39 insertions(+), 26 deletions(-)
diff --git a/drivers/md/dm-cache-target.c b/drivers/md/dm-cache-target.c
index d3bb355819a4..9aba81324579 100644
--- a/drivers/md/dm-cache-target.c
+++ b/drivers/md/dm-cache-target.c
@@ -2459,7 +2459,7 @@ static int cache_create(struct cache_args *ca, struct cache **result)
ti->flush_supported = true;
ti->num_discard_bios = 1;
- ti->discards_supported = true;
+ ti->discard_support = DM_DISCARD_IGN_DEVS;
ti->per_io_data_size = sizeof(struct per_bio_data);
diff --git a/drivers/md/dm-clone-target.c b/drivers/md/dm-clone-target.c
index 5ce96ddf1ce1..f5b2c1998628 100644
--- a/drivers/md/dm-clone-target.c
+++ b/drivers/md/dm-clone-target.c
@@ -1938,7 +1938,7 @@ static int clone_ctr(struct dm_target *ti, unsigned int argc, char **argv)
ti->flush_supported = true;
/* Enable discards */
- ti->discards_supported = true;
+ ti->discard_support = DM_DISCARD_IGN_DEVS;
ti->num_discard_bios = 1;
ti->private = clone;
diff --git a/drivers/md/dm-log-writes.c b/drivers/md/dm-log-writes.c
index 8ea20b56b4d6..9414214c250d 100644
--- a/drivers/md/dm-log-writes.c
+++ b/drivers/md/dm-log-writes.c
@@ -593,7 +593,7 @@ static int log_writes_ctr(struct dm_target *ti, unsigned int argc, char **argv)
ti->num_flush_bios = 1;
ti->flush_supported = true;
ti->num_discard_bios = 1;
- ti->discards_supported = true;
+ ti->discard_support = DM_DISCARD_IGN_DEVS;
ti->per_io_data_size = sizeof(struct per_bio_data);
ti->private = lc;
return 0;
diff --git a/drivers/md/dm-table.c b/drivers/md/dm-table.c
index 0a2cc197f62b..0e5a38da0ca8 100644
--- a/drivers/md/dm-table.c
+++ b/drivers/md/dm-table.c
@@ -789,8 +789,8 @@ int dm_table_add_target(struct dm_table *t, const char *type,
t->highs[t->num_targets++] = tgt->begin + tgt->len - 1;
- if (!tgt->num_discard_bios && tgt->discards_supported)
- DMWARN("%s: %s: ignoring discards_supported because num_discard_bios is zero.",
+ if (!tgt->num_discard_bios && (tgt->discard_support != DM_DISCARD_ALL_DEVS))
+ DMWARN("%s: %s: ignoring discard support because num_discard_bios is zero.",
dm_device_name(t->md), type);
return 0;
@@ -1786,6 +1786,14 @@ static bool dm_table_supports_write_zeroes(struct dm_table *t)
return true;
}
+static int device_discard_capable(struct dm_target *ti, struct dm_dev *dev,
+ sector_t start, sector_t len, void *data)
+{
+ struct request_queue *q = bdev_get_queue(dev->bdev);
+
+ return q && blk_queue_discard(q);
+}
+
static int device_not_discard_capable(struct dm_target *ti, struct dm_dev *dev,
sector_t start, sector_t len, void *data)
{
@@ -1805,15 +1813,17 @@ static bool dm_table_supports_discards(struct dm_table *t)
if (!ti->num_discard_bios)
return false;
- /*
- * Either the target provides discard support (as implied by setting
- * 'discards_supported') or it relies on _all_ data devices having
- * discard support.
- */
- if (!ti->discards_supported &&
- (!ti->type->iterate_devices ||
- ti->type->iterate_devices(ti, device_not_discard_capable, NULL)))
- return false;
+ if (ti->discard_support == DM_DISCARD_IGN_DEVS) {
+ continue;
+ } else if (ti->discard_support == DM_DISCARD_ANY_DEVS) {
+ if (!ti->type->iterate_devices ||
+ !ti->type->iterate_devices(ti, device_discard_capable, NULL))
+ return false;
+ } else {
+ if (!ti->type->iterate_devices ||
+ ti->type->iterate_devices(ti, device_not_discard_capable, NULL))
+ return false;
+ }
}
return true;
diff --git a/drivers/md/dm-thin.c b/drivers/md/dm-thin.c
index fa8d5464c1fb..f9384e1a44ce 100644
--- a/drivers/md/dm-thin.c
+++ b/drivers/md/dm-thin.c
@@ -3402,11 +3402,11 @@ static int pool_ctr(struct dm_target *ti, unsigned argc, char **argv)
ti->num_discard_bios = 1;
/*
- * Setting 'discards_supported' circumvents the normal
- * stacking of discard limits (this keeps the pool and
+ * Setting this circumvents the normal stacking
+ * of discard limits (this keeps the pool and
* thin devices' discard limits consistent).
*/
- ti->discards_supported = true;
+ ti->discard_support = DM_DISCARD_IGN_DEVS;
}
ti->private = pt;
@@ -4266,7 +4266,7 @@ static int thin_ctr(struct dm_target *ti, unsigned argc, char **argv)
/* In case the pool supports discards, pass them on. */
if (tc->pool->pf.discard_enabled) {
- ti->discards_supported = true;
+ ti->discard_support = DM_DISCARD_IGN_DEVS;
ti->num_discard_bios = 1;
}
diff --git a/drivers/md/dm-zoned-target.c b/drivers/md/dm-zoned-target.c
index f4f83d39b3dc..76e7886cba9e 100644
--- a/drivers/md/dm-zoned-target.c
+++ b/drivers/md/dm-zoned-target.c
@@ -796,7 +796,7 @@ static int dmz_ctr(struct dm_target *ti, unsigned int argc, char **argv)
ti->num_write_zeroes_bios = 1;
ti->per_io_data_size = sizeof(struct dmz_bioctx);
ti->flush_supported = true;
- ti->discards_supported = true;
+ ti->discard_support = DM_DISCARD_IGN_DEVS;
/* The exposed capacity is the number of chunks that can be mapped */
ti->len = (sector_t)dmz_nr_chunks(dmz->metadata) << dev->zone_nr_sectors_shift;
diff --git a/include/linux/device-mapper.h b/include/linux/device-mapper.h
index af48d9da3916..67cb0f7427e4 100644
--- a/include/linux/device-mapper.h
+++ b/include/linux/device-mapper.h
@@ -252,6 +252,12 @@ struct target_type {
#define DM_TARGET_ZONED_HM 0x00000040
#define dm_target_supports_zoned_hm(type) ((type)->features & DM_TARGET_ZONED_HM)
+enum dm_discard_mode {
+ DM_DISCARD_ALL_DEVS = 0,
+ DM_DISCARD_ANY_DEVS = 1,
+ DM_DISCARD_IGN_DEVS = 2,
+};
+
struct dm_target {
struct dm_table *table;
struct target_type *type;
@@ -315,11 +321,8 @@ struct dm_target {
*/
bool flush_supported:1;
- /*
- * Set if this target needs to receive discards regardless of
- * whether or not its underlying devices have support.
- */
- bool discards_supported:1;
+ /* Use to specify the discard support of this target */
+ enum dm_discard_mode discard_support;
};
/* Each target can link one of these into the table */
diff --git a/include/uapi/linux/dm-ioctl.h b/include/uapi/linux/dm-ioctl.h
index 6622912c2342..360bd55c1681 100644
--- a/include/uapi/linux/dm-ioctl.h
+++ b/include/uapi/linux/dm-ioctl.h
@@ -272,9 +272,9 @@ enum {
#define DM_DEV_SET_GEOMETRY _IOWR(DM_IOCTL, DM_DEV_SET_GEOMETRY_CMD, struct dm_ioctl)
#define DM_VERSION_MAJOR 4
-#define DM_VERSION_MINOR 42
+#define DM_VERSION_MINOR 43
#define DM_VERSION_PATCHLEVEL 0
-#define DM_VERSION_EXTRA "-ioctl (2020-02-27)"
+#define DM_VERSION_EXTRA "-ioctl (2020-04-14)"
/* Status bits */
#define DM_READONLY_FLAG (1 << 0) /* In/Out */
--
2.26.1
On Sun, Apr 19 2020 at 3:30am -0400,
Paul Wise <[email protected]> wrote:
> This makes dm raid and dm raid1 (mirroring) consistent with md raid,
> which supports discard when only some of the devices support discard.
>
> Another patch will be needed to fix the queue discard limits sysfs files,
> fixing `fstrim --fstab`, but these patches suffice to fix `fstrim /` and
> I haven't finished figuring out how the queue discard limits are set yet.
>
> Paul Wise (3):
> dm: add support for targets that allow discard when one device does
> dm raid: only check for RAID 4/5/6 once during discard support setup
> dm raid/raid1: enable discard support when any devices support discard
>
> drivers/md/dm-cache-target.c | 2 +-
> drivers/md/dm-clone-target.c | 2 +-
> drivers/md/dm-log-writes.c | 2 +-
> drivers/md/dm-raid.c | 21 ++++++++++-----------
> drivers/md/dm-raid1.c | 1 +
> drivers/md/dm-table.c | 32 +++++++++++++++++++++-----------
> drivers/md/dm-thin.c | 8 ++++----
> drivers/md/dm-zoned-target.c | 2 +-
> include/linux/device-mapper.h | 13 ++++++++-----
> include/uapi/linux/dm-ioctl.h | 4 ++--
> 10 files changed, 50 insertions(+), 37 deletions(-)
You went overboard with implementation before checking to see if your
work would be well received. Your 2/3 patch header shows you're
capable of analyzing past commits to explain the evolution of code,
etc. But yet you make no mention of this commit header which explicitly
speaks to why what you're proposing is _not_ acceptable:
commit 8a74d29d541cd86569139c6f3f44b2d210458071
Author: Mike Snitzer <[email protected]>
Date: Tue Nov 14 15:40:52 2017 -0500
dm: discard support requires all targets in a table support discards
A DM device with a mix of discard capabilities (due to some underlying
devices not having discard support) _should_ just return -EOPNOTSUPP for
the region of the device that doesn't support discards (even if only by
way of the underlying driver formally not supporting discards). BUT,
that does ask the underlying driver to handle something that it never
advertised support for. In doing so we're exposing users to the
potential for a underlying disk driver hanging if/when a discard is
issued a the device that is incapable and never claimed to support
discards.
Fix this by requiring that each DM target in a DM table provide discard
support as a prereq for a DM device to advertise support for discards.
This may cause some configurations that were happily supporting discards
(even in the face of a mix of discard support) to stop supporting
discards -- but the risk of users hitting driver hangs, and forced
reboots, outweighs supporting those fringe mixed discard
configurations.
Cc: [email protected]
Signed-off-by: Mike Snitzer <[email protected]>
I haven't looked closely at MD raid in this area but if you trully think
underlying MD raid can cope with issuing discards to devices that don't
support them (or that it avoids issuing them?) then please update
dm-raid.c to conditionally set ti->discard_supported (if not all devices
support discard). That is how to inform DM core that the target knows
better and it will manage discards issued to it. It keeps the change
local to dm-raid.c without the flag-day you're proposing.
Mike
On Sun, 2020-04-19 at 09:19 -0400, Mike Snitzer wrote:
> You went overboard with implementation before checking to see if your
> work would be well received. Your 2/3 patch header shows you're
> capable of analyzing past commits to explain the evolution of code,
> etc. But yet you make no mention of this commit header which explicitly
> speaks to why what you're proposing is _not_ acceptable:
>
> commit 8a74d29d541cd86569139c6f3f44b2d210458071
> Author: Mike Snitzer <[email protected]>
> Date: Tue Nov 14 15:40:52 2017 -0500
>
> dm: discard support requires all targets in a table support discards
I do remember seeing this commit while working on this, I guess I
ignored it in my attempts to get fstrim working on my rootfs, woops.
> I haven't looked closely at MD raid in this area but if you trully think
> underlying MD raid can cope with issuing discards to devices that don't
> support them (or that it avoids issuing them?) then please update
> dm-raid.c to conditionally set ti->discard_supported (if not all devices
> support discard). That is how to inform DM core that the target knows
> better and it will manage discards issued to it. It keeps the change
> local to dm-raid.c without the flag-day you're proposing.
On my system I have a HDD and an SSD, with /boot on MD RAID and / on
ext4 on DM RAID on 2 DM crypt volumes. In this setup fstrim works on
/boot but does not work on / and with my patches it works on / again.
In addition I don't see any messages in dmesg or other issues when
doing fstrim / with my patches.
I think I might have been worried that discards_supported has other
side effects but grepping the code now I see that was unfounded.
I'll switch the next version to just setting discards_supported.
I still think that my proposed overboard design is clearer though :)
You'll see from the following command that MD raid 0/1/10 arrays enable
discards when any device supports discards:
git grep -wW discard_supported
It appears that the block layer ignores discard requests when the queue
for the block device indicates that discard is not supported on it:
git grep -wW __blkdev_issue_discard
It seems to me that where possible DM/MD letting the block layer decide
to pass on or ignore discard requests is the right design. I'm possibly
incorrectly assuming that all block device drivers will correctly
advertise support for discard without false positive/negatives.
BTW, any idea where I should fix the `fstrim --fstab` issue? It is
expecting the queue/discard_granularity sysfs entry to be non-zero.
From my initial debugging attempts it seems raid_io_hints is at fault.
Thanks for your initial response and any further insight you can give.
--
bye,
pabs
https://bonedaddy.net/pabs3/
On 4/19/20 4:48 PM, Paul Wise wrote:
> On Sun, 2020-04-19 at 09:19 -0400, Mike Snitzer wrote:
>
>> You went overboard with implementation before checking to see if your
>> work would be well received. Your 2/3 patch header shows you're
>> capable of analyzing past commits to explain the evolution of code,
>> etc. But yet you make no mention of this commit header which explicitly
>> speaks to why what you're proposing is _not_ acceptable:
>>
>> commit 8a74d29d541cd86569139c6f3f44b2d210458071
>> Author: Mike Snitzer <[email protected]>
>> Date: Tue Nov 14 15:40:52 2017 -0500
>>
>> dm: discard support requires all targets in a table support discards
>
> I do remember seeing this commit while working on this, I guess I
> ignored it in my attempts to get fstrim working on my rootfs, woops.
>
>> I haven't looked closely at MD raid in this area but if you trully think
>> underlying MD raid can cope with issuing discards to devices that don't
>> support them (or that it avoids issuing them?) then please update
>> dm-raid.c to conditionally set ti->discard_supported (if not all devices
>> support discard). That is how to inform DM core that the target knows
>> better and it will manage discards issued to it. It keeps the change
>> local to dm-raid.c without the flag-day you're proposing.
>
> On my system I have a HDD and an SSD, with /boot on MD RAID and / on
> ext4 on DM RAID on 2 DM crypt volumes. In this setup fstrim works on
> /boot but does not work on / and with my patches it works on / again.
> In addition I don't see any messages in dmesg or other issues when
> doing fstrim / with my patches.
Did you have discard allowed on both dm-crypt devices? dm-crypt (kernel)
does not allow discards by default.
Regards O.
On Mon, 2020-04-20 at 09:35 +0200, Ondrej Kozina wrote:
> Did you have discard allowed on both dm-crypt devices? dm-crypt
> (kernel) does not allow discards by default.
I did not, I guess that explains why I got no errors.
--
bye,
pabs
https://bonedaddy.net/pabs3/
On 4/20/20 12:13 PM, Paul Wise wrote:
> On Mon, 2020-04-20 at 09:35 +0200, Ondrej Kozina wrote:
>
>> Did you have discard allowed on both dm-crypt devices? dm-crypt
>> (kernel) does not allow discards by default.
>
> I did not, I guess that explains why I got no errors.
>
FYI if you use LUKS2 metadata format for encrypted drives, you can
enable discards by default once and for all by following command:
cryptsetup open /dev/sdx sdx_unlocked --allow-discards --persistent
Any following unlock will enable discards automatically.
Regards O.
On Sun, 2020-04-19 at 09:19 -0400, Mike Snitzer wrote:
> You went overboard with implementation before checking to see if your
> work would be well received.
...
> I haven't looked closely at MD raid in this area but if you trully think
> underlying MD raid can cope with issuing discards to devices that don't
> support them (or that it avoids issuing them?) then please update
> dm-raid.c to conditionally set ti->discard_supported (if not all devices
> support discard). That is how to inform DM core that the target knows
> better and it will manage discards issued to it. It keeps the change
> local to dm-raid.c without the flag-day you're proposing.
So, now that I know that my approach to this was completely bogus,
what *is* the correct way to safely enable mixed-discard support?
It seems to me that the right way would be one of these options:
* a sysfs toggle for the block layer
* an lvchange based option for the dm layer
I'm leaning towards the latter for my personal use-case but the former
would make Linux much more flexible but would touch more code and have
the potential for more damage.
--
bye,
pabs
https://bonedaddy.net/pabs3/