Bug 232676

Summary:

gconcat fails to advertise delete capability

Product:

Base System

Reporter:

noah.bergbauer

Component:

kern

Assignee:

Mark Johnston <markj>

Status:

Closed FIXED

Severity:

Affects Only Me

CC:

markj

Priority:

---

Version:

CURRENT

Hardware:

Any

OS:

Any

Attachments:

Description	Flags
candidate patch	none
updated patch	none
fix GEOM::candelete handling	none
fix GEOM::candelete handling	none

Description noah.bergbauer 2018-10-25 13:37:31 UTC

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.

Comment 1 Mark Johnston freebsd_committer

2018-10-26 21:06:46 UTC

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.

Comment 2 noah.bergbauer 2018-10-29 22:22:47 UTC

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

Comment 3 Mark Johnston freebsd_committer

2018-10-29 23:00:52 UTC

Created attachment 198757 [details]
updated patch

Thanks, I think I see the problem.  Could you please give this updated patch a try?

Comment 4 noah.bergbauer 2018-10-29 23:56:54 UTC

Appears to work, thanks!

Comment 5 commit-hook freebsd_committer

2018-10-30 00:22:53 UTC

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

Comment 6 commit-hook freebsd_committer

2018-11-29 15:39:03 UTC

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

Comment 7 noah.bergbauer 2018-12-25 15:33:38 UTC

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.

Comment 8 Mark Johnston freebsd_committer

2018-12-26 18:37:54 UTC

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.

Comment 9 Mark Johnston freebsd_committer

2018-12-26 18:40:50 UTC

Created attachment 200535 [details]
fix GEOM::candelete handling

Fix a typo in the previous patch.

Comment 10 noah.bergbauer 2018-12-26 19:06:50 UTC

Works, thank you so much!

Comment 11 commit-hook freebsd_committer

2019-01-02 15:53:13 UTC

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