gconcat appears to correctly forward BIO_DELETE requests to the underlying providers but has no code to handle queries for the candelete attribute. As a result things TRIM/DISCARD will not work properly through gconcat (e.g. ufs won't try to use it and print a warning instead). I think gconcat should advertise delete support whenever any component does. Those that don't can just ignore it - besides, mixed configurations should be rather uncommon. As a sidenote, overriding the candelete property seems like the kind of thing gnop should be able to do.
Created attachment 198680 [details] candidate patch I believe the attached patch will fix the problem. Would you be willing to test it? So far I've only verified that it compiles.
(In reply to Mark Johnston from comment #1) It doesn't crash, but it doesn't appear to fix the issue either. Apparently d_candelete is always false. This seems to be caused by the g_getattr call always returning 1. No idea why this happens though (gmirror on the same partitions works fine).
Created attachment 198757 [details] updated patch Thanks, I think I see the problem. Could you please give this updated patch a try?
Appears to work, thanks!
A commit references this bug: Author: markj Date: Tue Oct 30 00:22:15 UTC 2018 New revision: 339900 URL: https://svnweb.freebsd.org/changeset/base/339900 Log: Have gconcat advertise delete support if one of its disks does. This follows the example set by other multi-disk GEOM classes. PR: 232676 Tested by: noah.bergbauer@tum.de MFC after: 1 month Changes: head/sys/geom/concat/g_concat.c head/sys/geom/concat/g_concat.h
A commit references this bug: Author: markj Date: Thu Nov 29 15:38:28 UTC 2018 New revision: 341237 URL: https://svnweb.freebsd.org/changeset/base/341237 Log: MFC r339900: Have gconcat advertise delete support if one of its disks does. PR: 232676 Changes: _U stable/12/ stable/12/sys/geom/concat/g_concat.c stable/12/sys/geom/concat/g_concat.h
Not sure if the bug is in this patch or in gmirror, but with releng/12 + this patch I run into this problem: # diskinfo -v nvd0s4 | grep TRIM Yes # TRIM/UNMAP support # gconcat label foo nvd0s4 # diskinfo -v concat/foo | grep TRIM Yes # TRIM/UNMAP support # gmirror label foo concat/foo # diskinfo -v mirror/foo | grep TRIM No # TRIM/UNMAP support The gconcat volume clearly advertises delete support but gmirror fails to see this.
Created attachment 200534 [details] fix GEOM::candelete handling (In reply to noah.bergbauer from comment #7) It's an old bug in gmirror/graid that I copied into gconcat. The same problem occurs if you reverse the order, i.e., create the gmirror first and then add the mirror to a concat volume. The attached patch fixes it for me.
Created attachment 200535 [details] fix GEOM::candelete handling Fix a typo in the previous patch.
Works, thank you so much!
A commit references this bug: Author: markj Date: Wed Jan 2 15:52:17 UTC 2019 New revision: 342687 URL: https://svnweb.freebsd.org/changeset/base/342687 Log: Use g_handleattr() to reply to GEOM::candelete queries. g_handleattr() fills out bp->bio_completed; otherwise, g_getattr() returns an error in response to the query. This caused BIO_DELETE support to not be propagated through stacked configurations, e.g., a gconcat of gmirror volumes would not handle BIO_DELETE even when the gmirrors do. g_io_getattr() was not affected by the problem. PR: 232676 Reported and tested by: noah.bergbauer@tum.de MFC after: 1 week Changes: head/sys/geom/concat/g_concat.c head/sys/geom/mirror/g_mirror.c head/sys/geom/raid/g_raid.c