sync discard

XFS recently added support for async discards. While this can be a win for some workloads and devices, there are also cases where async bursty discard will severly harm the latencies of reads and writes.

Add a 'discard_sync' mount flag to revert to using sync discard, issuing them one at the time and waiting for each one. This fixes a big performance regression we had moving to kernels that include the XFS async discard support.

Signed-off-by Jens Axboe <[email protected]>

---

 fs/xfs/xfs_log_cil.c | 18 ++++++++++++++----
 fs/xfs/xfs_mount.h   |  1 +
 fs/xfs/xfs_super.c   |  7 ++++++-
 3 files changed, 21 insertions(+), 5 deletions(-)

fs/xfs/xfs_log_cil.c +14 -4

index 4668403b1741..4eced3eceea5 100644
@@ -552,10 +552,16 @@ xlog_discard_busy_extents(
 	struct bio		*bio = NULL;
 	struct blk_plug		plug;
 	int			error = 0;
+	int			discard_flags = 0;
+	bool			sync_discard = mp->m_flags & XFS_MOUNT_DISCARD_SYNC;
 
 	ASSERT(mp->m_flags & XFS_MOUNT_DISCARD);
 
-	blk_start_plug(&plug);
+	if (!sync_discard)
+		blk_start_plug(&plug);
+	else
+		discard_flags = BLKDEV_DISCARD_SYNC;
+
 	list_for_each_entry(busyp, list, list) {
 		trace_xfs_discard_extent(mp, busyp->agno, busyp->bno,
 					 busyp->length);

@@ -563,7 +569,7 @@ xlog_discard_busy_extents(
 		error = __blkdev_issue_discard(mp->m_ddev_targp->bt_bdev,
 				XFS_AGB_TO_DADDR(mp, busyp->agno, busyp->bno),
 				XFS_FSB_TO_BB(mp, busyp->length),
-				GFP_NOFS, 0, &bio);
+				GFP_NOFS, discard_flags, &bio);
 		if (error && error != -EOPNOTSUPP) {
 			xfs_info(mp,
 	 "discard failed for extent [0x%llx,%u], error %d",

@@ -574,14 +580,18 @@ xlog_discard_busy_extents(
 		}
 	}
 
-	if (bio) {
+	if (sync_discard) {
+		xfs_extent_busy_clear(mp, &ctx->busy_extents, false);
+		kmem_free(ctx);
+	} else if (bio) {
 		bio->bi_private = ctx;
 		bio->bi_end_io = xlog_discard_endio;
 		submit_bio(bio);
+		blk_finish_plug(&plug);
 	} else {
 		xlog_discard_endio_work(&ctx->discard_endio_work);
+		blk_finish_plug(&plug);
 	}
-	blk_finish_plug(&plug);
 }
 
 /*

fs/xfs/xfs_mount.h +1 -0

index 10b90bbc5162..3f0b7b9106c7 100644
@@ -219,6 +219,7 @@ typedef struct xfs_mount {
 						   operations, typically for
 						   disk errors in metadata */
 #define XFS_MOUNT_DISCARD	(1ULL << 5)	/* discard unused blocks */
+#define XFS_MOUNT_DISCARD_SYNC	(1ULL << 6)	/* discard unused blocks sync */
 #define XFS_MOUNT_NOALIGN	(1ULL << 7)	/* turn off stripe alignment
 						   allocations */
 #define XFS_MOUNT_ATTR2		(1ULL << 8)	/* allow use of attr2 format */

fs/xfs/xfs_super.c +6 -1

index d71424052917..6d960bb4725f 100644
@@ -83,7 +83,7 @@ enum {
 	Opt_quota, Opt_noquota, Opt_usrquota, Opt_grpquota, Opt_prjquota,
 	Opt_uquota, Opt_gquota, Opt_pquota,
 	Opt_uqnoenforce, Opt_gqnoenforce, Opt_pqnoenforce, Opt_qnoenforce,
-	Opt_discard, Opt_nodiscard, Opt_dax, Opt_err,
+	Opt_discard, Opt_nodiscard, Opt_dax, Opt_err, Opt_discard_sync,
 };
 
 static const match_table_t tokens = {

@@ -129,6 +129,7 @@ static const match_table_t tokens = {
 	{Opt_pqnoenforce,"pqnoenforce"},/* project quota limit enforcement */
 	{Opt_qnoenforce, "qnoenforce"},	/* same as uqnoenforce */
 	{Opt_discard,	"discard"},	/* Discard unused blocks */
+	{Opt_discard_sync,"discard_sync"},/* Discard unused blocks sync */
 	{Opt_nodiscard,	"nodiscard"},	/* Do not discard unused blocks */
 
 	{Opt_dax,	"dax"},		/* Enable direct access to bdev pages */

@@ -363,11 +364,15 @@ xfs_parseargs(
 			mp->m_qflags |= (XFS_GQUOTA_ACCT | XFS_GQUOTA_ACTIVE);
 			mp->m_qflags &= ~XFS_GQUOTA_ENFD;
 			break;
+		case Opt_discard_sync:
+			mp->m_flags |= XFS_MOUNT_DISCARD_SYNC;
+			/* fall through and set XFS_MOUNT_DISCARD too */
 		case Opt_discard:
 			mp->m_flags |= XFS_MOUNT_DISCARD;
 			break;
 		case Opt_nodiscard:
 			mp->m_flags &= ~XFS_MOUNT_DISCARD;
+			mp->m_flags &= ~XFS_MOUNT_DISCARD_SYNC;
 			break;
 #ifdef CONFIG_FS_DAX
 		case Opt_dax:

Signature · 6 lines

2.7.4

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

Quoted 5 lines · Jens Axboe

XFS recently added support for async discards. While this can be a win for some workloads and devices, there are also cases where async bursty discard will severly harm the latencies of reads and writes.

Quoted 7 lines

Add a 'discard_sync' mount flag to revert to using sync discard, issuing them one at the time and waiting for each one. This fixes a big performance regression we had moving to kernels that include the XFS async discard support.

Signed-off-by Jens Axboe <[email protected]>

---

Hm, I figured the async discard stuff would have been a pretty clear win all around, but then again I'm not terribly familiar with what happens with discards beneath the fs. I do know that the previous behavior would cause fs level latencies due to holding up log I/O completion while discards completed one at a time. My understanding is that this lead to online discard being pretty much universally "not recommended" in favor of fstrim.

Do you have any more data around the workload where the old sync discard behavior actually provides an overall win over the newer behavior? Is it purely a matter of workload, or is it a workload+device thing with how discards may be handled by certain devices?

I'm ultimately not against doing this if it's useful for somebody and is otherwise buried under a mount option, but it would be nice to see if there's opportunity to improve the async mechanism before resorting to that. Is the issue here too large bio chains, too many chains at once, or just simply too many discards (regardless of chaining) at the same time?

I'm wondering if xfs could separate discard submission from log I/O completion entirely and then perhaps limit/throttle submission somehow or another (Christoph, thoughts?) via a background task. Perhaps doing something like that may even eliminate the need for some discards on busy filesystems with a lot of block free -> reallocation activity, but I'm just handwaving atm.

Quoted 14 lines · Brian Foster

Quoted 5 lines · Jens Axboe

XFS recently added support for async discards. While this can be a win for some workloads and devices, there are also cases where async bursty discard will severly harm the latencies of reads and writes.

Quoted 7 lines

Add a 'discard_sync' mount flag to revert to using sync discard, issuing them one at the time and waiting for each one. This fixes a big performance regression we had moving to kernels that include the XFS async discard support.

Signed-off-by Jens Axboe <[email protected]>

---

Quoted 7 lines

Hm, I figured the async discard stuff would have been a pretty clear win all around, but then again I'm not terribly familiar with what happens with discards beneath the fs. I do know that the previous behavior would cause fs level latencies due to holding up log I/O completion while discards completed one at a time. My understanding is that this lead to online discard being pretty much universally "not recommended" in favor of fstrim.

It's not a secret that most devices suck at discard.

While the async
discard is nifty and I bet works well for some cases, it can also cause a flood of discards on the device side which does not work well for other cases.

Quoted 23 lines · Jens Axboe

Quoted 21 lines · Brian Foster

Quoted 5 lines · Jens Axboe

XFS recently added support for async discards. While this can be a win for some workloads and devices, there are also cases where async bursty discard will severly harm the latencies of reads and writes.

Quoted 7 lines

Add a 'discard_sync' mount flag to revert to using sync discard, issuing them one at the time and waiting for each one. This fixes a big performance regression we had moving to kernels that include the XFS async discard support.

Signed-off-by Jens Axboe <[email protected]>

---

Hm, I figured the async discard stuff would have been a pretty clear win all around, but then again I'm not terribly familiar with what happens with discards beneath the fs. I do know that the previous behavior would cause fs level latencies due to holding up log I/O completion while discards completed one at a time. My understanding is that this lead to online discard being pretty much universally "not recommended" in favor of fstrim.

Quoted 1 line

It's not a secret that most devices suck at discard.

How can we know if a device sucks at discard?

Quoted 4 lines

While the async
discard is nifty and I bet works well for some cases, it can also cause a flood of discards on the device side which does not work well for other cases.

Shouldn't async then be only enabled if the device used supports it well? Or should a blacklist per device be more suitable? Which is more popular?

Quoted 23 lines · Jens Axboe

Quoted 21 lines · Brian Foster

Quoted 5 lines · Jens Axboe

XFS recently added support for async discards. While this can be a win for some workloads and devices, there are also cases where async bursty discard will severly harm the latencies of reads and writes.

Quoted 7 lines

Add a 'discard_sync' mount flag to revert to using sync discard, issuing them one at the time and waiting for each one. This fixes a big performance regression we had moving to kernels that include the XFS async discard support.

Signed-off-by Jens Axboe <[email protected]>

---

Hm, I figured the async discard stuff would have been a pretty clear win all around, but then again I'm not terribly familiar with what happens with discards beneath the fs. I do know that the previous behavior would cause fs level latencies due to holding up log I/O completion while discards completed one at a time. My understanding is that this lead to online discard being pretty much universally "not recommended" in favor of fstrim.

Quoted 1 line

It's not a secret that most devices suck at discard.

How can we know if a device sucks at discard?

Quoted 26 lines · Luis R. Rodriguez

Quoted 24 lines · Jens Axboe

Quoted 14 lines · Brian Foster

Quoted 5 lines · Jens Axboe

XFS recently added support for async discards. While this can be a win for some workloads and devices, there are also cases where async bursty discard will severly harm the latencies of reads and writes.

Quoted 7 lines

Add a 'discard_sync' mount flag to revert to using sync discard, issuing them one at the time and waiting for each one. This fixes a big performance regression we had moving to kernels that include the XFS async discard support.

Signed-off-by Jens Axboe <[email protected]>

---

Quoted 7 lines

Hm, I figured the async discard stuff would have been a pretty clear win all around, but then again I'm not terribly familiar with what happens with discards beneath the fs. I do know that the previous behavior would cause fs level latencies due to holding up log I/O completion while discards completed one at a time. My understanding is that this lead to online discard being pretty much universally "not recommended" in favor of fstrim.

It's not a secret that most devices suck at discard.

Quoted 1 line

How can we know if a device sucks at discard?

I was going to ask the same thing. ;) "Meh, punt to the admin!"

I'm having deja vu but can't remember why. Seems like this has come up before and we thought it should be a block device tunable, not pushed down from the filesystem. Is that possible?

Quoted 33 lines · Eric Sandeen

Quoted 24 lines · Luis R. Rodriguez

Quoted 23 lines · Jens Axboe

Quoted 21 lines · Brian Foster

Quoted 5 lines · Jens Axboe

XFS recently added support for async discards. While this can be a win for some workloads and devices, there are also cases where async bursty discard will severly harm the latencies of reads and writes.

Quoted 7 lines

Add a 'discard_sync' mount flag to revert to using sync discard, issuing them one at the time and waiting for each one. This fixes a big performance regression we had moving to kernels that include the XFS async discard support.

Signed-off-by Jens Axboe <[email protected]>

---

Hm, I figured the async discard stuff would have been a pretty clear win all around, but then again I'm not terribly familiar with what happens with discards beneath the fs. I do know that the previous behavior would cause fs level latencies due to holding up log I/O completion while discards completed one at a time. My understanding is that this lead to online discard being pretty much universally "not recommended" in favor of fstrim.

Quoted 3 lines

Quoted 1 line

It's not a secret that most devices suck at discard.

How can we know if a device sucks at discard?

I was going to ask the same thing. ;) "Meh, punt to the admin!"

Quoted 3 lines

I'm having deja vu but can't remember why. Seems like this has come up before and we thought it should be a block device tunable, not pushed down from the filesystem. Is that possible?

The problem is that it'll depend on the workload as well. The device in may laptop is fine with discard for my workload, which is very light. But if you are running RocksDB on it, and doing heavy compactions and deletes, it probably would not be.

Quoted 38 lines · Jens Axboe

Quoted 36 lines · Eric Sandeen

Quoted 24 lines · Luis R. Rodriguez

Quoted 23 lines · Jens Axboe

Quoted 21 lines · Brian Foster

Quoted 5 lines · Jens Axboe

XFS recently added support for async discards. While this can be a win for some workloads and devices, there are also cases where async bursty discard will severly harm the latencies of reads and writes.

Quoted 7 lines

Add a 'discard_sync' mount flag to revert to using sync discard, issuing them one at the time and waiting for each one. This fixes a big performance regression we had moving to kernels that include the XFS async discard support.

Signed-off-by Jens Axboe <[email protected]>

---

Hm, I figured the async discard stuff would have been a pretty clear win all around, but then again I'm not terribly familiar with what happens with discards beneath the fs. I do know that the previous behavior would cause fs level latencies due to holding up log I/O completion while discards completed one at a time. My understanding is that this lead to online discard being pretty much universally "not recommended" in favor of fstrim.

Quoted 3 lines

Quoted 1 line

It's not a secret that most devices suck at discard.

How can we know if a device sucks at discard?

I was going to ask the same thing. ;) "Meh, punt to the admin!"

I'm having deja vu but can't remember why. Seems like this has come up before and we thought it should be a block device tunable, not pushed down from the filesystem. Is that possible?

Quoted 4 lines

The problem is that it'll depend on the workload as well. The device in may laptop is fine with discard for my workload, which is very light. But if you are running RocksDB on it, and doing heavy compactions and deletes, it probably would not be.

Ok, but I'm not sure how that precludes a block device tunable? You'd tune it for your workload, right?

Or is the concern that it could only be for the entire block device, and perhaps different partitions have different workloads?

Sorry, caveman filesystem guy doesn't completely understand block devices.

Quoted 1 line · Eric Sandeen

On 4/30/18 2:21 PM, Jens Axboe wrote:

Quoted 2 lines

Ok, but I'm not sure how that precludes a block device tunable? You'd tune it for your workload, right?

What kind of tunable are you thinking of? Right now we have one tunable, which is the max discard size. Patch #1 actually helps make this do what it should for sync discards, instead of just building a bio chain and submitting that all at once.

Quoted 4 lines

Or is the concern that it could only be for the entire block device, and perhaps different partitions have different workloads?

Sorry, caveman filesystem guy doesn't completely understand block devices.

I just don't know what you are trying to tune :-)

Quoted 2 lines · Jens Axboe

Quoted 1 line · Eric Sandeen

On 4/30/18 2:21 PM, Jens Axboe wrote:

...

Quoted 11 lines

Quoted 2 lines

Ok, but I'm not sure how that precludes a block device tunable? You'd tune it for your workload, right?

What kind of tunable are you thinking of? Right now we have one tunable, which is the max discard size. Patch #1 actually helps make this do what it should for sync discards, instead of just building a bio chain and submitting that all at once.

Quoted 3 lines

Or is the concern that it could only be for the entire block device, and perhaps different partitions have different workloads?

Quoted 3 lines

Quoted 1 line

Sorry, caveman filesystem guy doesn't completely understand block devices.

I just don't know what you are trying to tune :-)

I'm wondering if "make discards synchronous" couldn't just be a block device tunable, rather than a filesystem mount option tunable.

Quoted 14 lines · Brian Foster

Quoted 5 lines · Jens Axboe

XFS recently added support for async discards. While this can be a win for some workloads and devices, there are also cases where async bursty discard will severly harm the latencies of reads and writes.

Quoted 7 lines

Add a 'discard_sync' mount flag to revert to using sync discard, issuing them one at the time and waiting for each one. This fixes a big performance regression we had moving to kernels that include the XFS async discard support.

Signed-off-by Jens Axboe <[email protected]>

---

Quoted 7 lines

Hm, I figured the async discard stuff would have been a pretty clear win all around, but then again I'm not terribly familiar with what happens with discards beneath the fs. I do know that the previous behavior would cause fs level latencies due to holding up log I/O completion while discards completed one at a time. My understanding is that this lead to online discard being pretty much universally "not recommended" in favor of fstrim.

It's not a secret that most devices suck at discard. While the async discard is nifty and I bet works well for some cases, it can also cause a flood of discards on the device side which does not work well for other cases.

Quoted 4 lines

Do you have any more data around the workload where the old sync discard behavior actually provides an overall win over the newer behavior? Is it purely a matter of workload, or is it a workload+device thing with how discards may be handled by certain devices?

The worse read latencies were observed on more than one device type, making it sync again was universally a win. We've had many issues with discard, one trick that is often used is to chop up file deletion into smaller chunks. Say you want to kill 10GB of data, you do it incrementally, since 10G of discard usually doesn't behave very nicely. If you make that async, then you're back to square one.

Quoted 6 lines

I'm ultimately not against doing this if it's useful for somebody and is otherwise buried under a mount option, but it would be nice to see if there's opportunity to improve the async mechanism before resorting to that. Is the issue here too large bio chains, too many chains at once, or just simply too many discards (regardless of chaining) at the same time?

Well, ultimately you'd need better scheduling of the discards, but for most devices what really works the best is a simple "just do one at the time". The size constraint was added to further limit the impact.

Honestly, I think the only real improvement would be on the device side. Most folks want discard as an advisory hint, and it should not impact current workloads at all. In reality, many implementations are much more strict and even include substantial flash writes. For the cases where we can't just turn it off (I'd love to), we at least need to make it less intrusive.

Quoted 6 lines

I'm wondering if xfs could separate discard submission from log I/O completion entirely and then perhaps limit/throttle submission somehow or another (Christoph, thoughts?) via a background task. Perhaps doing something like that may even eliminate the need for some discards on busy filesystems with a lot of block free -> reallocation activity, but I'm just handwaving atm.

Just having the sync vs async option is the best split imho. The async could potentially be scheduled. I don't think more involved logic belongs in the fs.

Signature · 6 lines

Jens Axboe

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

XFS recently added support for async discards. While this can be a win for some workloads and devices, there are also cases where async bursty discard will severly harm the latencies of reads and writes.

Add a 'discard_sync' mount flag to revert to using sync discard, issuing them one at the time and waiting for each one. This fixes a big performance regression we had moving to kernels that include the XFS async discard support.

Quoted 4 lines · Jens Axboe

XFS recently added support for async discards. While this can be a win for some workloads and devices, there are also cases where async bursty discard will severly harm the latencies of reads and writes.

FWIW, convention is it document the performance regression in the commit message, not leave the reader to guess at what it was....

Did anyone analyse the pattern of discards being issued to work out what pattern was worse for async vs sync discard? is it lots of little discards, large extents being discarded, perhaps a problem with the request request queue starving other IOs because we queue so many async discards in such a short time (which is the difference in behaviour vs the old code), or something else?

Quoted 4 lines

Add a 'discard_sync' mount flag to revert to using sync discard, issuing them one at the time and waiting for each one. This fixes a big performance regression we had moving to kernels that include the XFS async discard support.

I'm not a fan of adding a mount option to work around bad, unpredictable performance due to a mount option we recommend you don't use because it results in bad, unpredictable performance.

Without any details of the discard pattern that results in problems I don't think we should be changing anything - adding an opaque, user-unfriendly mount option does nothing to address the underlying problem - it's just a hack to work around the symptoms being seen...

More details of the regression and the root cause analysis is needed, please.

Quoted 8 lines · Dave Chinner

Quoted 4 lines · Jens Axboe

XFS recently added support for async discards. While this can be a win for some workloads and devices, there are also cases where async bursty discard will severly harm the latencies of reads and writes.

FWIW, convention is it document the performance regression in the commit message, not leave the reader to guess at what it was....

Yeah I'll give you that, I can improve the commit message for sure.

Quoted 6 lines

Did anyone analyse the pattern of discards being issued to work out what pattern was worse for async vs sync discard? is it lots of little discards, large extents being discarded, perhaps a problem with the request request queue starving other IOs because we queue so many async discards in such a short time (which is the difference in behaviour vs the old code), or something else?

What was observed was a big discard which would previously have gone down as smaller discards now going down as either one or many discards. Looking at the blktrace data, it's the difference between

discard 1 queue
discard 1 complete
discatd 2 queue
discard 2 complete
[...]
discard n queue
discard n complete

which is now

discard 1 queue
discard 2 queue
[...]
discard n queue
[...]
discard 1 complete
discard 2 complete
[...]
discard n complete

Note that we set a max discard size of 64MB for most devices, since it's been shown to have less impact on latencies for the IO that jobs actually care about.

Quoted 6 lines

I don't think we should be changing anything - adding an opaque, user-unfriendly mount option does nothing to address the underlying problem - it's just a hack to work around the symptoms being seen...

More details of the regression and the root cause analysis is needed, please.

It brings back the same behavior as we had before, which performs better for us. It's preventing users of XFS+discard from upgrading, which is sad.

Quoted 43 lines · Jens Axboe

Quoted 8 lines · Dave Chinner

Quoted 4 lines · Jens Axboe

XFS recently added support for async discards. While this can be a win for some workloads and devices, there are also cases where async bursty discard will severly harm the latencies of reads and writes.

FWIW, convention is it document the performance regression in the commit message, not leave the reader to guess at what it was....

Yeah I'll give you that, I can improve the commit message for sure.

Quoted 6 lines

Did anyone analyse the pattern of discards being issued to work out what pattern was worse for async vs sync discard? is it lots of little discards, large extents being discarded, perhaps a problem with the request request queue starving other IOs because we queue so many async discards in such a short time (which is the difference in behaviour vs the old code), or something else?

What was observed was a big discard which would previously have gone down as smaller discards now going down as either one or many discards. Looking at the blktrace data, it's the difference between

discard 1 queue
discard 1 complete
discatd 2 queue
discard 2 complete
[...]
discard n queue
discard n complete

which is now

discard 1 queue
discard 2 queue
[...]
discard n queue
[...]
discard 1 complete
discard 2 complete
[...]
discard n complete

Quoted 3 lines

Note that we set a max discard size of 64MB for most devices, since it's been shown to have less impact on latencies for the IO that jobs actually care about.

IOWs, this has nothing to do with XFS behaviour, and everything to do with suboptimal scheduling control for concurrent queued discards in the block layer....

XFS can issue discard requests of up to 8GB (on 4k block size filesystems), and how they are optimised is completely up to the block layer. blkdev_issue_discard() happens to be synchronous (for historical reasons) and that does not match our asynchronous log IO model. it's always been a cause of total filesystem stall problems for XFS because we can't free log space until all the pending discards have been issued. Hence we can see global filesystems stalls of *minutes* with synchronous discards on bad devices and can cause OOM and all sorts of other nasty cascading failures.

Async discard dispatch solves this problem for XFS - discards no longer block forward journal progress, and we really don't want to go back to having that ticking timebomb in XFS. Handling concurrent discard requests in an optimal manner is not a filesystem problem - it's an IO scheduling problem.

Essentially, we've exposed yet another limitation of the block/IO layer handling of discard requests in the linux storage stack - it does not have a configurable discard queue depth.

I'd much prefer these problems get handled at the IO scheduling layer where there is visibulity of device capabilities and request queue state. i.e. we should be throttling async discards just like we can throttle read or write IO, especially given there are many devices that serialise discards at the device level (i.e. not queued device commands). This solves the problem for everyone and makes things much better for the future where hardware implementations are likely to get better and support more and more concurrency in discard operations.

IMO, the way the high level code dispatches discard requests is irrelevant here - this is a problem to do with queue depth and IO scheduling/throttling. That's why I don't think adding permanent ABI changes to filesystems to work around this problem is the right solution....

Quoted 7 lines

Quoted 6 lines

I don't think we should be changing anything - adding an opaque, user-unfriendly mount option does nothing to address the underlying problem - it's just a hack to work around the symptoms being seen...

More details of the regression and the root cause analysis is needed, please.

Quoted 3 lines

It brings back the same behavior as we had before, which performs better for us. It's preventing users of XFS+discard from upgrading, which is sad.

Yes, it does, but so would having the block layer to throttle device discard requests in flight to a queue depth of 1. And then we don't have to change XFS at all.

Quoted 3 lines · Dave Chinner

Yes, it does, but so would having the block layer to throttle device discard requests in flight to a queue depth of 1. And then we don't have to change XFS at all.

I'm perfectly fine with making that change by default, and much easier for me since I don't have to patch file systems.

Quoted 7 lines · Jens Axboe

Quoted 3 lines · Dave Chinner

Yes, it does, but so would having the block layer to throttle device discard requests in flight to a queue depth of 1. And then we don't have to change XFS at all.

I'm perfectly fine with making that change by default, and much easier for me since I don't have to patch file systems.

Totally untested, but this should do the trick. It ensures we have a QD of 1 (per caller), which should be sufficient.

If people tune down the discard size, then you'll be blocking waiting for discards on issue.

block/blk-lib.c +2 -3

index a676084d4740..0bf9befcc863 100644
@@ -11,16 +11,19 @@
 #include "blk.h"
 
 static struct bio *next_bio(struct bio *bio, unsigned int nr_pages,
-		gfp_t gfp)
+			    gfp_t gfp)
 {
-	struct bio *new = bio_alloc(gfp, nr_pages);
-
+	/*

Quoted 36 lines · Jens Axboe

Quoted 7 lines · Jens Axboe

Quoted 3 lines · Dave Chinner

Yes, it does, but so would having the block layer to throttle device discard requests in flight to a queue depth of 1. And then we don't have to change XFS at all.

I'm perfectly fine with making that change by default, and much easier for me since I don't have to patch file systems.

Totally untested, but this should do the trick. It ensures we have a QD of 1 (per caller), which should be sufficient.

If people tune down the discard size, then you'll be blocking waiting for discards on issue.

block/blk-lib.c +6 -5

index a676084d4740..0bf9befcc863 100644
@@ -11,16 +11,19 @@
 #include "blk.h"
 
 static struct bio *next_bio(struct bio *bio, unsigned int nr_pages,
-		gfp_t gfp)
+			    gfp_t gfp)
 {
-	struct bio *new = bio_alloc(gfp, nr_pages);
-
+	/*
+	 * Devices suck at discard, so if we have to break up the bio
+	 * size due to the max discard size setting, wait for the
+	 * previous one to finish first.
+	 */
 	if (bio) {
-		bio_chain(bio, new);
-		submit_bio(bio);

block/blk-lib.c line 24

Quoted 3 lines

block/blk-lib.c +2 -0

+		submit_bio_wait(bio);
+		bio_put(bio);
 	}

This only addresses the case where __blkdev_issue_discard() breaks up a single large discard, right? It seems like a brute force solution, too, because it will do so even when the underlying device is idle and there's no need to throttle.

Shouldn't the throttling logic at least look at device congestion? i.e. if the device is not backlogged, then we should be able to issue the discard without problems.

I ask this because this only addresses throttling the "discard large extent" case when the discard limit is set low. i.e. your exact problem case. We know that XFS can issue large numbers of discontiguous async discards in a single batch - this patch does not address that case and so it will still cause starvation problems.

If we look at device congestion in determining how to throttle/back off during discard issuing, then it doesn't matter what max_discard_sectors is set to - it will throttle in all situations that cause device overloads and starvations....

Cheers,

Dave.

Signature · 6 lines

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

Quoted 36 lines · Jens Axboe

Quoted 7 lines · Jens Axboe

Quoted 3 lines · Dave Chinner

Yes, it does, but so would having the block layer to throttle device discard requests in flight to a queue depth of 1. And then we don't have to change XFS at all.

I'm perfectly fine with making that change by default, and much easier for me since I don't have to patch file systems.

Totally untested, but this should do the trick. It ensures we have a QD of 1 (per caller), which should be sufficient.

If people tune down the discard size, then you'll be blocking waiting for discards on issue.

block/blk-lib.c +6 -5

index a676084d4740..0bf9befcc863 100644
@@ -11,16 +11,19 @@
 #include "blk.h"
 
 static struct bio *next_bio(struct bio *bio, unsigned int nr_pages,
-		gfp_t gfp)
+			    gfp_t gfp)
 {
-	struct bio *new = bio_alloc(gfp, nr_pages);
-
+	/*
+	 * Devices suck at discard, so if we have to break up the bio
+	 * size due to the max discard size setting, wait for the
+	 * previous one to finish first.
+	 */
 	if (bio) {
-		bio_chain(bio, new);
-		submit_bio(bio);

block/blk-lib.c line 24

Quoted 3 lines

block/blk-lib.c +2 -0

+		submit_bio_wait(bio);
+		bio_put(bio);
 	}

This only addresses the case where __blkdev_issue_discard() breaks up a single large discard, right? It seems like a brute force solution, too, because it will do so even when the underlying device is idle and there's no need to throttle.

Shouldn't the throttling logic at least look at device congestion? i.e. if the device is not backlogged, then we should be able to issue the discard without problems.

I ask this because this only addresses throttling the "discard large extent" case when the discard limit is set low. i.e. your exact problem case. We know that XFS can issue large numbers of discontiguous async discards in a single batch - this patch does not address that case and so it will still cause starvation problems.

If we look at device congestion in determining how to throttle/back off during discard issuing, then it doesn't matter what max_discard_sectors is set to - it will throttle in all situations that cause device overloads and starvations....

Jens,

block/blk-wbt.h enum wbt_flags { · line 17

Quoted 9 lines

block/blk-wbt.h +1 -0

index a232c98fbf4d..aec5bc82d580 100644
@@ -14,12 +14,17 @@ enum wbt_flags {
 	WBT_TRACKED		= 1,	/* write, tracked for throttling */
 	WBT_READ		= 2,	/* read */
 	WBT_KSWAPD		= 4,	/* write, from kswapd */
+	WBT_TRIM		= 8,

The term TRIM does not apply to NVMe, nor SCSI. Please call it WBT_DISCARD.

Quoted 44 lines · Dave Chinner

Quoted 43 lines · Jens Axboe

Quoted 8 lines · Dave Chinner

Quoted 4 lines · Jens Axboe

XFS recently added support for async discards. While this can be a win for some workloads and devices, there are also cases where async bursty discard will severly harm the latencies of reads and writes.

FWIW, convention is it document the performance regression in the commit message, not leave the reader to guess at what it was....

Yeah I'll give you that, I can improve the commit message for sure.

Quoted 6 lines

Did anyone analyse the pattern of discards being issued to work out what pattern was worse for async vs sync discard? is it lots of little discards, large extents being discarded, perhaps a problem with the request request queue starving other IOs because we queue so many async discards in such a short time (which is the difference in behaviour vs the old code), or something else?

What was observed was a big discard which would previously have gone down as smaller discards now going down as either one or many discards. Looking at the blktrace data, it's the difference between

discard 1 queue
discard 1 complete
discatd 2 queue
discard 2 complete
[...]
discard n queue
discard n complete

which is now

discard 1 queue
discard 2 queue
[...]
discard n queue
[...]
discard 1 complete
discard 2 complete
[...]
discard n complete

Quoted 3 lines

Quoted 3 lines

Note that we set a max discard size of 64MB for most devices, since it's been shown to have less impact on latencies for the IO that jobs actually care about.

Quoted 4 lines

IOWs, this has nothing to do with XFS behaviour, and everything to do with suboptimal scheduling control for concurrent queued discards in the block layer....

You could argue that, and it's fallout from XFS being the first user of async discard. Prior to that, we've never had that use case. I'm quite fine making all discards throttle to depth 1.

Quoted 31 lines

XFS can issue discard requests of up to 8GB (on 4k block size filesystems), and how they are optimised is completely up to the block layer. blkdev_issue_discard() happens to be synchronous (for historical reasons) and that does not match our asynchronous log IO model. it's always been a cause of total filesystem stall problems for XFS because we can't free log space until all the pending discards have been issued. Hence we can see global filesystems stalls of *minutes* with synchronous discards on bad devices and can cause OOM and all sorts of other nasty cascading failures.

Async discard dispatch solves this problem for XFS - discards no longer block forward journal progress, and we really don't want to go back to having that ticking timebomb in XFS. Handling concurrent discard requests in an optimal manner is not a filesystem problem - it's an IO scheduling problem.

Essentially, we've exposed yet another limitation of the block/IO layer handling of discard requests in the linux storage stack - it does not have a configurable discard queue depth.

I'd much prefer these problems get handled at the IO scheduling layer where there is visibulity of device capabilities and request queue state. i.e. we should be throttling async discards just like we can throttle read or write IO, especially given there are many devices that serialise discards at the device level (i.e. not queued device commands). This solves the problem for everyone and makes things much better for the future where hardware implementations are likely to get better and support more and more concurrency in discard operations.

Quoted 5 lines

IMO, the way the high level code dispatches discard requests is irrelevant here - this is a problem to do with queue depth and IO scheduling/throttling. That's why I don't think adding permanent ABI changes to filesystems to work around this problem is the right solution....

If you get off your high horse for a bit, this is essentially a performance regression. I can either migrate folks off of XFS, or I can come up with something that works for them. It's pretty easy to claim this is "yet another limitation of the IO stack", but it's really not fair to make crazy claims like that when it's an entirely new use case. Let's try to state things objectively and fairly.

This work should probably have been done before making XFS discard async. This isn't the first fallout we've had from that code.

Quoted 11 lines

Quoted 10 lines

Quoted 6 lines

I don't think we should be changing anything - adding an opaque, user-unfriendly mount option does nothing to address the underlying problem - it's just a hack to work around the symptoms being seen...

More details of the regression and the root cause analysis is needed, please.

It brings back the same behavior as we had before, which performs better for us. It's preventing users of XFS+discard from upgrading, which is sad.

Quoted 3 lines

Yes, it does, but so would having the block layer to throttle device discard requests in flight to a queue depth of 1. And then we don't have to change XFS at all.

I'm perfectly fine with making that change by default, and much easier for me since I don't have to patch file systems.

Signature · 6 lines

Jens Axboe

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

We recently had a pretty severe perf regression with the XFS async discard. This small series add a SYNC issue discard flag, and also limits the chain size for sync discards. Patch 2 adds support for reverting XFS back to doign sync discards.

Quoted 4 lines · Jens Axboe

We recently had a pretty severe perf regression with the XFS async discard. This small series add a SYNC issue discard flag, and also limits the chain size for sync discards. Patch 2 adds support for reverting XFS back to doign sync discards.

Please explain the hardware and workload that you see this with. Also given that it is a hardware limitation a quirk limiting the size of discard requests and/or number of outstanding discards would be more useful than falling back to a broken mode of issueing block commands.