[PATCH] xfs: defer online discard submission to a workqueue

[PATCH] xfs: defer online discard submission to a workqueue

[Brian Foster]

When online discard is enabled, discards of busy extents are
submitted asynchronously as a bio chain. bio completion and
resulting busy extent cleanup is deferred to a workqueue. Async
discard submission is intended to avoid blocking log forces on a
full discard sequence which can take a noticeable amount of time in
some cases.

We've had reports of this still producing log force stalls with XFS
on VDO, which executes discards synchronously and relies on online
discard in XFS. In particular, there appears to be a limit on how
many discards can execute at one time. When this limit is hit,
discard submission blocks and affects XFS up through log completion.

There is no need for XFS to ever block in this path as busy extents
are cleared on discard completion. Since we already have a workqueue
for discard bio completion, reuse that workqueue for discard
submission and completely separate online discard processing from
iclog completion processing. With this change, the only path that
should ever block on discard completion is the allocation path if it
determines a need to wait on busy extents.

Signed-off-by: Brian Foster <bfoster@redhat.com>
---
 fs/xfs/xfs_log_cil.c  | 13 ++++++++-----
 fs/xfs/xfs_log_priv.h |  1 +
 2 files changed, 9 insertions(+), 5 deletions(-)

diff --git a/fs/xfs/xfs_log_cil.c b/fs/xfs/xfs_log_cil.c
index d3884e08b43c..087e99e80edd 100644
--- a/fs/xfs/xfs_log_cil.c
+++ b/fs/xfs/xfs_log_cil.c
@@ -529,9 +529,11 @@ xlog_discard_endio(
 
 static void
 xlog_discard_busy_extents(
-	struct xfs_mount	*mp,
-	struct xfs_cil_ctx	*ctx)
+	struct work_struct	*work)
 {
+	struct xfs_cil_ctx	*ctx = container_of(work, struct xfs_cil_ctx,
+						    discard_work);
+	struct xfs_mount	*mp = ctx->cil->xc_log->l_mp;
 	struct list_head	*list = &ctx->busy_extents;
 	struct xfs_extent_busy	*busyp;
 	struct bio		*bio = NULL;
@@ -603,9 +605,10 @@ xlog_cil_committed(
 
 	xlog_cil_free_logvec(ctx->lv_chain);
 
-	if (!list_empty(&ctx->busy_extents))
-		xlog_discard_busy_extents(mp, ctx);
-	else
+	if (!list_empty(&ctx->busy_extents)) {
+		INIT_WORK(&ctx->discard_work, xlog_discard_busy_extents);
+		queue_work(xfs_discard_wq, &ctx->discard_work);
+	} else
 		kmem_free(ctx);
 }
 
diff --git a/fs/xfs/xfs_log_priv.h b/fs/xfs/xfs_log_priv.h
index b5f82cb36202..085bebd51135 100644
--- a/fs/xfs/xfs_log_priv.h
+++ b/fs/xfs/xfs_log_priv.h
@@ -245,6 +245,7 @@ struct xfs_cil_ctx {
 	struct xfs_log_vec	*lv_chain;	/* logvecs being pushed */
 	struct xfs_log_callback	log_cb;		/* completion callback hook. */
 	struct list_head	committing;	/* ctx committing list */
+	struct work_struct	discard_work;
 	struct work_struct	discard_endio_work;
 };
 
-- 
2.17.2

Re: [PATCH] xfs: defer online discard submission to a workqueue

[Christoph Hellwig]

On Mon, Nov 05, 2018 at 01:10:21PM -0500, Brian Foster wrote:
> When online discard is enabled, discards of busy extents are
> submitted asynchronously as a bio chain. bio completion and
> resulting busy extent cleanup is deferred to a workqueue. Async
> discard submission is intended to avoid blocking log forces on a
> full discard sequence which can take a noticeable amount of time in
> some cases.
> 
> We've had reports of this still producing log force stalls with XFS
> on VDO,

Please fix this in VDO instead.  We should not work around out of
tree code making stupid decisions.

Re: [PATCH] xfs: defer online discard submission to a workqueue

[Eric Sandeen]

On 11/5/18 3:51 PM, Christoph Hellwig wrote:
> On Mon, Nov 05, 2018 at 01:10:21PM -0500, Brian Foster wrote:
>> When online discard is enabled, discards of busy extents are
>> submitted asynchronously as a bio chain. bio completion and
>> resulting busy extent cleanup is deferred to a workqueue. Async
>> discard submission is intended to avoid blocking log forces on a
>> full discard sequence which can take a noticeable amount of time in
>> some cases.
>>
>> We've had reports of this still producing log force stalls with XFS
>> on VDO,
> 
> Please fix this in VDO instead.  We should not work around out of
> tree code making stupid decisions.

Is there some downside to Brian's proposal, in principle?
It seems like it would be an improvement for device that might cause
a discard bottleneck like this.

Thanks,
-Eric

Re: [PATCH] xfs: defer online discard submission to a workqueue

[Brian Foster]

On Mon, Nov 05, 2018 at 01:51:39PM -0800, Christoph Hellwig wrote:
> On Mon, Nov 05, 2018 at 01:10:21PM -0500, Brian Foster wrote:
> > When online discard is enabled, discards of busy extents are
> > submitted asynchronously as a bio chain. bio completion and
> > resulting busy extent cleanup is deferred to a workqueue. Async
> > discard submission is intended to avoid blocking log forces on a
> > full discard sequence which can take a noticeable amount of time in
> > some cases.
> > 
> > We've had reports of this still producing log force stalls with XFS
> > on VDO,
> 
> Please fix this in VDO instead.  We should not work around out of
> tree code making stupid decisions.

I assume the "stupid decision" refers to sync discard execution. I'm not
familiar with the internals of VDO, this is just what I was told. My
understanding is that these discards can stack up and take enough time
that a limit on outstanding discards is required, which now that I think
of it makes me somewhat skeptical of the whole serial execution thing.
Hitting that outstanding discard request limit is what bubbles up the
stack and affects XFS by holding up log forces, since new discard
submissions are presumably blocked on completion of the oldest
outstanding request.

I'm not quite sure what happens in the block layer if that limit were
lifted. Perhaps it assumes throttling responsibility directly via
queues/plugs? I'd guess that at minimum we'd end up blocking indirectly
somewhere (via memory allocation pressure?) anyways, so ISTM that some
kind of throttling is inevitable in this situation. What am I missing?

Brian

Re: [PATCH] xfs: defer online discard submission to a workqueue

[Dave Chinner]

On Tue, Nov 06, 2018 at 09:23:11AM -0500, Brian Foster wrote:
> On Mon, Nov 05, 2018 at 01:51:39PM -0800, Christoph Hellwig wrote:
> > On Mon, Nov 05, 2018 at 01:10:21PM -0500, Brian Foster wrote:
> > > When online discard is enabled, discards of busy extents are
> > > submitted asynchronously as a bio chain. bio completion and
> > > resulting busy extent cleanup is deferred to a workqueue. Async
> > > discard submission is intended to avoid blocking log forces on a
> > > full discard sequence which can take a noticeable amount of time in
> > > some cases.
> > > 
> > > We've had reports of this still producing log force stalls with XFS
> > > on VDO,
> > 
> > Please fix this in VDO instead.  We should not work around out of
> > tree code making stupid decisions.
> 
> I assume the "stupid decision" refers to sync discard execution. I'm not
> familiar with the internals of VDO, this is just what I was told.

IMO, what VDO does is irrelevant - any call to submit_bio() can
block if the request queue is full. Hence if we've drowned the queue
in discards and the device is slow at discards, then we are going to
block submitting discards.

> My
> understanding is that these discards can stack up and take enough time
> that a limit on outstanding discards is required, which now that I think
> of it makes me somewhat skeptical of the whole serial execution thing.
> Hitting that outstanding discard request limit is what bubbles up the
> stack and affects XFS by holding up log forces, since new discard
> submissions are presumably blocked on completion of the oldest
> outstanding request.

Exactly.

> I'm not quite sure what happens in the block layer if that limit were
> lifted. Perhaps it assumes throttling responsibility directly via
> queues/plugs? I'd guess that at minimum we'd end up blocking indirectly
> somewhere (via memory allocation pressure?) anyways, so ISTM that some
> kind of throttling is inevitable in this situation. What am I missing?

We still need to throttle discards - they have to play nice with all
the other IO we need to dispatch concurrently.

I have two issues with the proposed patch:

1. it puts both discard dispatch and completion processing on the
one work qeueue, so if the queue is filled with dispatch requests,
IO completion queuing gets blocked. That's not the best thing to be
doing.

2. log forces no longer wait for discards to be dispatched - they
just queue them. This means the mechanism xfs_extent_busy_flush()
uses to dispatch pending discards (synchrnous log force) can return
before discards have even been dispatched to disk. Hence we can
expect to see longer wait and tail latencies when busy extents are
encountered by the allocator. Whether this is a problem or not needs
further investigation.

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

Re: [PATCH] xfs: defer online discard submission to a workqueue

[Brian Foster]

On Wed, Nov 07, 2018 at 08:18:02AM +1100, Dave Chinner wrote:
> On Tue, Nov 06, 2018 at 09:23:11AM -0500, Brian Foster wrote:
> > On Mon, Nov 05, 2018 at 01:51:39PM -0800, Christoph Hellwig wrote:
> > > On Mon, Nov 05, 2018 at 01:10:21PM -0500, Brian Foster wrote:
> > > > When online discard is enabled, discards of busy extents are
> > > > submitted asynchronously as a bio chain. bio completion and
> > > > resulting busy extent cleanup is deferred to a workqueue. Async
> > > > discard submission is intended to avoid blocking log forces on a
> > > > full discard sequence which can take a noticeable amount of time in
> > > > some cases.
> > > > 
> > > > We've had reports of this still producing log force stalls with XFS
> > > > on VDO,
> > > 
> > > Please fix this in VDO instead.  We should not work around out of
> > > tree code making stupid decisions.
> > 
> > I assume the "stupid decision" refers to sync discard execution. I'm not
> > familiar with the internals of VDO, this is just what I was told.
> 
> IMO, what VDO does is irrelevant - any call to submit_bio() can
> block if the request queue is full. Hence if we've drowned the queue
> in discards and the device is slow at discards, then we are going to
> block submitting discards.
> 
> > My
> > understanding is that these discards can stack up and take enough time
> > that a limit on outstanding discards is required, which now that I think
> > of it makes me somewhat skeptical of the whole serial execution thing.
> > Hitting that outstanding discard request limit is what bubbles up the
> > stack and affects XFS by holding up log forces, since new discard
> > submissions are presumably blocked on completion of the oldest
> > outstanding request.
> 
> Exactly.
> 
> > I'm not quite sure what happens in the block layer if that limit were
> > lifted. Perhaps it assumes throttling responsibility directly via
> > queues/plugs? I'd guess that at minimum we'd end up blocking indirectly
> > somewhere (via memory allocation pressure?) anyways, so ISTM that some
> > kind of throttling is inevitable in this situation. What am I missing?
> 
> We still need to throttle discards - they have to play nice with all
> the other IO we need to dispatch concurrently.
> 
> I have two issues with the proposed patch:
> 
> 1. it puts both discard dispatch and completion processing on the
> one work qeueue, so if the queue is filled with dispatch requests,
> IO completion queuing gets blocked. That's not the best thing to be
> doing.
> 

This is an unbound workqueue with max_active == 0. AIUI, that means we
can have something like 256 execution contexts (worker threads?) per
cpu. Given that, plus the batching that occurs in XFS due to delayed
logging and discard bio chaining, that seems rather unlikely. Unless I'm
misunderstanding the mechanism, I think that means filling the queue as
such and blocking discard submission basically consumes one of those
contexts.

Of course, the CIL context structure appears to be technically unbound
as well and it's trivial to just add a separate submission workqueue,
but I'd like to at least make sure we're on the same page as to the need
(so it can be documented clearly as well).

> 2. log forces no longer wait for discards to be dispatched - they
> just queue them. This means the mechanism xfs_extent_busy_flush()
> uses to dispatch pending discards (synchrnous log force) can return
> before discards have even been dispatched to disk. Hence we can
> expect to see longer wait and tail latencies when busy extents are
> encountered by the allocator. Whether this is a problem or not needs
> further investigation.
> 

Firstly, I think latency is kind of moot in the problematic case. The
queue is already drowning in requests that presumably are going to take
minutes to complete. In that case, the overhead of kicking a workqueue
to do the submission is probably negligible.

That leaves the normal/active case. I suspect that the overhead here may
still be hard to notice given the broader overhead of online discard in
the first place. The aforementioned batching means we could be sending a
good number of discards all at once that ultimately need to be processed
(with no priority to the particular one an allocator could be waiting
on). The larger the chain, the longer the whole sequence is going to
take to clear a particular busy extent relative to the additional
latency of a workqueue. We could also just be sending one discard,
though waiting on that one particular extent could mean that we're
either not being as smart as we could be in extent selection or that
we're simply running low on free space.

I suppose if this approach is ultimately problemic for whatever reason,
we could look into some kind of heuristic to track outstanding
discards/chains and continue to do direct submission until we cross some
magic threshold. I'm not sure it's worth that complexity given the
limited use cases for online discard unless there's some serious
unforeseen problem, though. IIRC, this was all doing synchronous,
serialized submission not too long ago after all.

Anyways, I'll run some file creation/deletion tests against an SSD
and/or a thin vol and see what falls out..

Brian

> Cheers,
> 
> Dave.
> -- 
> Dave Chinner
> david@fromorbit.com

Re: [PATCH] xfs: defer online discard submission to a workqueue

[Dave Chinner]

On Wed, Nov 07, 2018 at 08:42:24AM -0500, Brian Foster wrote:
> On Wed, Nov 07, 2018 at 08:18:02AM +1100, Dave Chinner wrote:
> > On Tue, Nov 06, 2018 at 09:23:11AM -0500, Brian Foster wrote:
> > > On Mon, Nov 05, 2018 at 01:51:39PM -0800, Christoph Hellwig wrote:
> > > > On Mon, Nov 05, 2018 at 01:10:21PM -0500, Brian Foster wrote:
> > > > > When online discard is enabled, discards of busy extents are
> > > > > submitted asynchronously as a bio chain. bio completion and
> > > > > resulting busy extent cleanup is deferred to a workqueue. Async
> > > > > discard submission is intended to avoid blocking log forces on a
> > > > > full discard sequence which can take a noticeable amount of time in
> > > > > some cases.
> > > > > 
> > > > > We've had reports of this still producing log force stalls with XFS
> > > > > on VDO,
> > > > 
> > > > Please fix this in VDO instead.  We should not work around out of
> > > > tree code making stupid decisions.
> > > 
> > > I assume the "stupid decision" refers to sync discard execution. I'm not
> > > familiar with the internals of VDO, this is just what I was told.
> > 
> > IMO, what VDO does is irrelevant - any call to submit_bio() can
> > block if the request queue is full. Hence if we've drowned the queue
> > in discards and the device is slow at discards, then we are going to
> > block submitting discards.
> > 
> > > My
> > > understanding is that these discards can stack up and take enough time
> > > that a limit on outstanding discards is required, which now that I think
> > > of it makes me somewhat skeptical of the whole serial execution thing.
> > > Hitting that outstanding discard request limit is what bubbles up the
> > > stack and affects XFS by holding up log forces, since new discard
> > > submissions are presumably blocked on completion of the oldest
> > > outstanding request.
> > 
> > Exactly.
> > 
> > > I'm not quite sure what happens in the block layer if that limit were
> > > lifted. Perhaps it assumes throttling responsibility directly via
> > > queues/plugs? I'd guess that at minimum we'd end up blocking indirectly
> > > somewhere (via memory allocation pressure?) anyways, so ISTM that some
> > > kind of throttling is inevitable in this situation. What am I missing?
> > 
> > We still need to throttle discards - they have to play nice with all
> > the other IO we need to dispatch concurrently.
> > 
> > I have two issues with the proposed patch:
> > 
> > 1. it puts both discard dispatch and completion processing on the
> > one work qeueue, so if the queue is filled with dispatch requests,
> > IO completion queuing gets blocked. That's not the best thing to be
> > doing.
> > 
> 
> This is an unbound workqueue with max_active == 0. AIUI, that means we
> can have something like 256 execution contexts (worker threads?) per
> cpu.

.....
        WQ_MAX_ACTIVE           = 512,    /* I like 512, better ideas? */
        WQ_MAX_UNBOUND_PER_CPU  = 4,      /* 4 * #cpus for unbound wq */
        WQ_DFL_ACTIVE           = WQ_MAX_ACTIVE / 2,
};

/* unbound wq's aren't per-cpu, scale max_active according to #cpus */
#define WQ_UNBOUND_MAX_ACTIVE   \
        max_t(int, WQ_MAX_ACTIVE, num_possible_cpus() * WQ_MAX_UNBOUND_PER_CPU)


IOWs, unbound queues are not per-cpu and they are execution limited
to max(512, NR_CPUS * 4) kworker threads.

The default (for max_active = 0), however, is still WQ_DFL_ACTIVE so
the total number of active workers for the unbound xfs_discard_wq
is 256.


> Given that, plus the batching that occurs in XFS due to delayed
> logging and discard bio chaining, that seems rather unlikely. Unless I'm
> misunderstanding the mechanism, I think that means filling the queue as
> such and blocking discard submission basically consumes one of those
> contexts.

Yes, but consider the situation where we've got a slow discard
device and we're removing a file with millions of extents. We've got
to issue millions of discard ops in this case. because dispatch
queueing is not bound, we're easily going to overflow the discard
workqueue because the freeing transactions will run far ahead of the
discard operations. Sooner or later we consume all 256 discard_wq
worker threads with blocked discard submission.  Then both log IO
completion and discard I ocompletion will block on workqueue
submission and we deadlock because discard completion can't
run....

> 
> Of course, the CIL context structure appears to be technically unbound

I'm missing something here - What bit of that structure is unbound?

> as well and it's trivial to just add a separate submission workqueue,
> but I'd like to at least make sure we're on the same page as to the need
> (so it can be documented clearly as well).

A separate submission queue doesn't really solve log Io completion
blocking problem. Yes, it solves the discard completion deadlock,
but we eventually end up in the same place on sustained discard
workloads with submission queuing blocking on a full work queue.

Workqueues are no the way to solve unbound queue depth problems.
That's what Kernel threads are for. e.g. this is the reason the
xfsaild is a kernel thread, not a work queue. The amount of
writeback work queued on the AIL can be hundreds of thousands of
objects, way more than a workqueue can handle. This discard problem
is no different - concurrent dispatch through kworker threads buys
us nothing - we just fill the request queue from hundreds of threads
instead of filling it from just one.

The workqueue approach has other problems, too, like dispatch across
worker threads means discard is not FIFO scheduled - it's completely
random as to the order in which discards get fed to the device
request queue. Hence discards can be starved because whenever the
worker thread runs to process it's queue it finds the device request
queue already full and blocks again.

Having a single kernel thread that walks the discard queue on each
context and then each context in sequence order gives us FIFO
dispatch of discard requests. It would block only on full request
queues giving us a much more predictable log-force-to-completion
latency. It allows for the possiblity of merging discards across
multiple CIL contexts, to directly control the rate of discard, and
to skip small discards or even -turn off discard- when the backlog
gets too great.

The workqueue approach just doesn't allow anything like this to be
done because every discard context is kept separate from every other
context and there is no coordination at all between them.

> > 2. log forces no longer wait for discards to be dispatched - they
> > just queue them. This means the mechanism xfs_extent_busy_flush()
> > uses to dispatch pending discards (synchrnous log force) can return
> > before discards have even been dispatched to disk. Hence we can
> > expect to see longer wait and tail latencies when busy extents are
> > encountered by the allocator. Whether this is a problem or not needs
> > further investigation.
> > 
> 
> Firstly, I think latency is kind of moot in the problematic case. The
> queue is already drowning in requests that presumably are going to take
> minutes to complete. In that case, the overhead of kicking a workqueue
> to do the submission is probably negligible.

Yes, the overhead of kicking the queue is negliable. That's not the
problem though. By queuing discards rather than submitting them we
go from a single FIFO dispatch model (by in-order iclog IO
completion) to a concurrent, uncoordinated dispatch model.  It's the
loss of FIFO behaviour because the synchrnous log force no longer
controls dispatch order that leads to unpredictable and long tail
latencies in dispatch completion, hence causing the problems for the
proceeses now waiting on specific extent discard completion rather
than just the log force.

In some cases they'll get woken faster (don't ahve to wait for
discards to be dispatched), but it is equally likely they'll have to
wait for much, much longer. In essence, the async dispatch by
workqueue model removes all assumptions we've made about the
predictablility of discard completion latency. FIFO is predictable,
concurrent async dispatch by workqueue is completely unpredictable.

If we really want to do fully async dispatch of discards, I think we
need to use a controllable "single dispatch by kernel thread" model
like the AIL, not use workqueues and spray the dispatch in an
uncoordinated, uncontrollable manner across hundreds of kernel
threads....

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

Re: [PATCH] xfs: defer online discard submission to a workqueue

[Brian Foster]

On Thu, Nov 08, 2018 at 11:38:06AM +1100, Dave Chinner wrote:
> On Wed, Nov 07, 2018 at 08:42:24AM -0500, Brian Foster wrote:
> > On Wed, Nov 07, 2018 at 08:18:02AM +1100, Dave Chinner wrote:
> > > On Tue, Nov 06, 2018 at 09:23:11AM -0500, Brian Foster wrote:
> > > > On Mon, Nov 05, 2018 at 01:51:39PM -0800, Christoph Hellwig wrote:
> > > > > On Mon, Nov 05, 2018 at 01:10:21PM -0500, Brian Foster wrote:
> > > > > > When online discard is enabled, discards of busy extents are
> > > > > > submitted asynchronously as a bio chain. bio completion and
> > > > > > resulting busy extent cleanup is deferred to a workqueue. Async
> > > > > > discard submission is intended to avoid blocking log forces on a
> > > > > > full discard sequence which can take a noticeable amount of time in
> > > > > > some cases.
> > > > > > 
> > > > > > We've had reports of this still producing log force stalls with XFS
> > > > > > on VDO,
> > > > > 
> > > > > Please fix this in VDO instead.  We should not work around out of
> > > > > tree code making stupid decisions.
> > > > 
> > > > I assume the "stupid decision" refers to sync discard execution. I'm not
> > > > familiar with the internals of VDO, this is just what I was told.
> > > 
> > > IMO, what VDO does is irrelevant - any call to submit_bio() can
> > > block if the request queue is full. Hence if we've drowned the queue
> > > in discards and the device is slow at discards, then we are going to
> > > block submitting discards.
> > > 
> > > > My
> > > > understanding is that these discards can stack up and take enough time
> > > > that a limit on outstanding discards is required, which now that I think
> > > > of it makes me somewhat skeptical of the whole serial execution thing.
> > > > Hitting that outstanding discard request limit is what bubbles up the
> > > > stack and affects XFS by holding up log forces, since new discard
> > > > submissions are presumably blocked on completion of the oldest
> > > > outstanding request.
> > > 
> > > Exactly.
> > > 
> > > > I'm not quite sure what happens in the block layer if that limit were
> > > > lifted. Perhaps it assumes throttling responsibility directly via
> > > > queues/plugs? I'd guess that at minimum we'd end up blocking indirectly
> > > > somewhere (via memory allocation pressure?) anyways, so ISTM that some
> > > > kind of throttling is inevitable in this situation. What am I missing?
> > > 
> > > We still need to throttle discards - they have to play nice with all
> > > the other IO we need to dispatch concurrently.
> > > 
> > > I have two issues with the proposed patch:
> > > 
> > > 1. it puts both discard dispatch and completion processing on the
> > > one work qeueue, so if the queue is filled with dispatch requests,
> > > IO completion queuing gets blocked. That's not the best thing to be
> > > doing.
> > > 
> > 
> > This is an unbound workqueue with max_active == 0. AIUI, that means we
> > can have something like 256 execution contexts (worker threads?) per
> > cpu.
> 
> .....
>         WQ_MAX_ACTIVE           = 512,    /* I like 512, better ideas? */
>         WQ_MAX_UNBOUND_PER_CPU  = 4,      /* 4 * #cpus for unbound wq */
>         WQ_DFL_ACTIVE           = WQ_MAX_ACTIVE / 2,
> };
> 
> /* unbound wq's aren't per-cpu, scale max_active according to #cpus */
> #define WQ_UNBOUND_MAX_ACTIVE   \
>         max_t(int, WQ_MAX_ACTIVE, num_possible_cpus() * WQ_MAX_UNBOUND_PER_CPU)
> 
> 
> IOWs, unbound queues are not per-cpu and they are execution limited
> to max(512, NR_CPUS * 4) kworker threads.
> 
> The default (for max_active = 0), however, is still WQ_DFL_ACTIVE so
> the total number of active workers for the unbound xfs_discard_wq
> is 256.
> 

Sounds about right..

> 
> > Given that, plus the batching that occurs in XFS due to delayed
> > logging and discard bio chaining, that seems rather unlikely. Unless I'm
> > misunderstanding the mechanism, I think that means filling the queue as
> > such and blocking discard submission basically consumes one of those
> > contexts.
> 
> Yes, but consider the situation where we've got a slow discard
> device and we're removing a file with millions of extents. We've got
> to issue millions of discard ops in this case. because dispatch
> queueing is not bound, we're easily going to overflow the discard
> workqueue because the freeing transactions will run far ahead of the
> discard operations. Sooner or later we consume all 256 discard_wq
> worker threads with blocked discard submission.  Then both log IO
> completion and discard I ocompletion will block on workqueue
> submission and we deadlock because discard completion can't
> run....
> 
> > 
> > Of course, the CIL context structure appears to be technically unbound
> 
> I'm missing something here - What bit of that structure is unbound?
> 

I mean to say that the ctx is not a fixed/limited resource in the
context of a discard submission workqueue. There's no throttling, so we
can essentially discard as many busy extent lists as the user can
generate (this is essentially the same point as your million extent file
example above).

> > as well and it's trivial to just add a separate submission workqueue,
> > but I'd like to at least make sure we're on the same page as to the need
> > (so it can be documented clearly as well).
> 
> A separate submission queue doesn't really solve log Io completion
> blocking problem. Yes, it solves the discard completion deadlock,
> but we eventually end up in the same place on sustained discard
> workloads with submission queuing blocking on a full work queue.
> 

Well, for the purposes of this patch it's fine that we still block on
discard submission. The point is just that we do so in a separate
context from log I/O completion and thus avoid associated log force
stalls.

> Workqueues are no the way to solve unbound queue depth problems.
> That's what Kernel threads are for. e.g. this is the reason the
> xfsaild is a kernel thread, not a work queue. The amount of
> writeback work queued on the AIL can be hundreds of thousands of
> objects, way more than a workqueue can handle. This discard problem
> is no different - concurrent dispatch through kworker threads buys
> us nothing - we just fill the request queue from hundreds of threads
> instead of filling it from just one.
> 

Fair, but the point wasn't really to buy any kind of high level discard
submission improvement over the current scheme. The goal was to avoid
log force stalls on a time consuming operation that has no explicit
serialization requirement with respect to log force completion...

> The workqueue approach has other problems, too, like dispatch across
> worker threads means discard is not FIFO scheduled - it's completely
> random as to the order in which discards get fed to the device
> request queue. Hence discards can be starved because whenever the
> worker thread runs to process it's queue it finds the device request
> queue already full and blocks again.
> 

... but Ok. I can see that the worst case of creating and blocking
hundreds of worker threads on discard submission kind of creates a mess
even without considering the resulting effect on the discard operations
themselves. There's no real point or need for all of those threads.

> Having a single kernel thread that walks the discard queue on each
> context and then each context in sequence order gives us FIFO
> dispatch of discard requests. It would block only on full request
> queues giving us a much more predictable log-force-to-completion
> latency. It allows for the possiblity of merging discards across
> multiple CIL contexts, to directly control the rate of discard, and
> to skip small discards or even -turn off discard- when the backlog
> gets too great.
> 
> The workqueue approach just doesn't allow anything like this to be
> done because every discard context is kept separate from every other
> context and there is no coordination at all between them.
> 

The high level kthread approach sounds like a nice idea to me. I'm not
sure that it's technically necessary to address this specific problem
though. I.e., it would be nice if we could still separate enhancement
from bug fix.

> > > 2. log forces no longer wait for discards to be dispatched - they
> > > just queue them. This means the mechanism xfs_extent_busy_flush()
> > > uses to dispatch pending discards (synchrnous log force) can return
> > > before discards have even been dispatched to disk. Hence we can
> > > expect to see longer wait and tail latencies when busy extents are
> > > encountered by the allocator. Whether this is a problem or not needs
> > > further investigation.
> > > 
> > 
> > Firstly, I think latency is kind of moot in the problematic case. The
> > queue is already drowning in requests that presumably are going to take
> > minutes to complete. In that case, the overhead of kicking a workqueue
> > to do the submission is probably negligible.
> 
> Yes, the overhead of kicking the queue is negliable. That's not the
> problem though. By queuing discards rather than submitting them we
> go from a single FIFO dispatch model (by in-order iclog IO
> completion) to a concurrent, uncoordinated dispatch model.  It's the
> loss of FIFO behaviour because the synchrnous log force no longer
> controls dispatch order that leads to unpredictable and long tail
> latencies in dispatch completion, hence causing the problems for the
> proceeses now waiting on specific extent discard completion rather
> than just the log force.
> 

I think I see what you're getting at here: discard submission via log
completion means that submission is serialized (with respect to other
iclogs) and in log buf order, since that is enforced by the log callback
invoking code. Since the callbacks execute one at a time, blocking in
submission likely means the same, single log I/O completion execution
context ends up iterating over and processing the other iclogs with
pending callbacks once the current submit blockage clears. Yes?

If I'm following that correctly, couldn't we provide very similar
behavior by using a separate "ordered" workqueue for submission? The
ordered wq sets max_active to 1, which means we still have a single
submitter, yet we still create a queue that disconnects submit from log
force completion. I suppose we could also allocate a separate structure
from xfs_cil_ctx to track pending discard jobs and slim down memory
usage closer to something that might be required for a thread-based
implementation.

>From there, we can still explore additional enhancements incrementally,
such as pushing some of the busy extent processing/sorting into the
workqueue, eventually converting the wq-based mechanism into a
thread-based one, etc. Thoughts?

Brian

> In some cases they'll get woken faster (don't ahve to wait for
> discards to be dispatched), but it is equally likely they'll have to
> wait for much, much longer. In essence, the async dispatch by
> workqueue model removes all assumptions we've made about the
> predictablility of discard completion latency. FIFO is predictable,
> concurrent async dispatch by workqueue is completely unpredictable.
> 
> If we really want to do fully async dispatch of discards, I think we
> need to use a controllable "single dispatch by kernel thread" model
> like the AIL, not use workqueues and spray the dispatch in an
> uncoordinated, uncontrollable manner across hundreds of kernel
> threads....
> 
> Cheers,
> 
> Dave.
> -- 
> Dave Chinner
> david@fromorbit.com

Re: [PATCH] xfs: defer online discard submission to a workqueue

[Dave Chinner]

On Thu, Nov 08, 2018 at 08:50:02AM -0500, Brian Foster wrote:
> On Thu, Nov 08, 2018 at 11:38:06AM +1100, Dave Chinner wrote:
> > On Wed, Nov 07, 2018 at 08:42:24AM -0500, Brian Foster wrote:
> > > On Wed, Nov 07, 2018 at 08:18:02AM +1100, Dave Chinner wrote:
> > Yes, but consider the situation where we've got a slow discard
> > device and we're removing a file with millions of extents. We've got
> > to issue millions of discard ops in this case. because dispatch
> > queueing is not bound, we're easily going to overflow the discard
> > workqueue because the freeing transactions will run far ahead of the
> > discard operations. Sooner or later we consume all 256 discard_wq
> > worker threads with blocked discard submission.  Then both log IO
> > completion and discard I ocompletion will block on workqueue
> > submission and we deadlock because discard completion can't
> > run....
> > 
> > > 
> > > Of course, the CIL context structure appears to be technically unbound
> > 
> > I'm missing something here - What bit of that structure is unbound?
> > 
> 
> I mean to say that the ctx is not a fixed/limited resource in the
> context of a discard submission workqueue. There's no throttling, so we
> can essentially discard as many busy extent lists as the user can
> generate (this is essentially the same point as your million extent file
> example above).

It is bound. It's bound by log size and the number of checkpoints we
can have uncompleted at any point in time. That can be a high bound
on large logs, but it is limited and it is throttled - you can't
create new contexts if there is no log space available, and hence
new contexts are throttled on tail pushing.

> > > as well and it's trivial to just add a separate submission workqueue,
> > > but I'd like to at least make sure we're on the same page as to the need
> > > (so it can be documented clearly as well).
> > 
> > A separate submission queue doesn't really solve log Io completion
> > blocking problem. Yes, it solves the discard completion deadlock,
> > but we eventually end up in the same place on sustained discard
> > workloads with submission queuing blocking on a full work queue.
> > 
> 
> Well, for the purposes of this patch it's fine that we still block on
> discard submission. The point is just that we do so in a separate
> context from log I/O completion and thus avoid associated log force
> stalls.

And, in doing so, remove any bound we have on the number of
outstanding uncompleted discards. You're essentially trading
"slow things progressively as discard load goes up" with "nothing
blocks until we hit a breakdown point and the system stops until the
discard queue drains".

We have a limited queue depth of discard operations right now, and
even that is deep enough to cause problems.  Removing that queue
depth bound won't improve things - it will just allow the pending
discard queue to run deeper and get further and further behind the
extent free operations that are being committed. This will only make
the stalls and problems completely unpredictable and uncontrollable,
and poses a true breakdown possibility where allocation in every AG
is stalled holding AGF locks waiting for a huge queue of discards to
be worked through.

We have to throttle at some point to prevent us from getting to
these severe breakdown conditions. Moving discards out from under
the log force removes the only feedback loop we have to throttle
discards back to a manageable level. What we have is not perfect,
but we can't just remove that feedback loop without providing
something else to replace it.

> > The workqueue approach just doesn't allow anything like this to be
> > done because every discard context is kept separate from every other
> > context and there is no coordination at all between them.
> > 
> 
> The high level kthread approach sounds like a nice idea to me. I'm not
> sure that it's technically necessary to address this specific problem
> though. I.e., it would be nice if we could still separate enhancement
> from bug fix.

Just removing the discards from the log force context is just hiding
a source of latency without addressing the cause of the latency.
i.e. there's a huge amount of discard to be managed, and the only
management we have to constrain them to the rate at which we retire
the extent free transactions.

> > > > 2. log forces no longer wait for discards to be dispatched - they
> > > > just queue them. This means the mechanism xfs_extent_busy_flush()
> > > > uses to dispatch pending discards (synchrnous log force) can return
> > > > before discards have even been dispatched to disk. Hence we can
> > > > expect to see longer wait and tail latencies when busy extents are
> > > > encountered by the allocator. Whether this is a problem or not needs
> > > > further investigation.
> > > > 
> > > 
> > > Firstly, I think latency is kind of moot in the problematic case. The
> > > queue is already drowning in requests that presumably are going to take
> > > minutes to complete. In that case, the overhead of kicking a workqueue
> > > to do the submission is probably negligible.
> > 
> > Yes, the overhead of kicking the queue is negliable. That's not the
> > problem though. By queuing discards rather than submitting them we
> > go from a single FIFO dispatch model (by in-order iclog IO
> > completion) to a concurrent, uncoordinated dispatch model.  It's the
> > loss of FIFO behaviour because the synchrnous log force no longer
> > controls dispatch order that leads to unpredictable and long tail
> > latencies in dispatch completion, hence causing the problems for the
> > proceeses now waiting on specific extent discard completion rather
> > than just the log force.
> > 
> 
> I think I see what you're getting at here: discard submission via log
> completion means that submission is serialized (with respect to other
> iclogs) and in log buf order, since that is enforced by the log callback
> invoking code. Since the callbacks execute one at a time, blocking in
> submission likely means the same, single log I/O completion execution
> context ends up iterating over and processing the other iclogs with
> pending callbacks once the current submit blockage clears. Yes?

Essentially.

> If I'm following that correctly, couldn't we provide very similar
> behavior by using a separate "ordered" workqueue for submission?

Workqueues are not bound depth queues - they are "concurrency
managed" queues. And ordered workqueue is simply a FIFO with a
single processing context. You can queue as much as you like to it,
but it will only process it with a single thread.

> The
> ordered wq sets max_active to 1, which means we still have a single
> submitter,

We have a single /worker/ - we can submit as many independent works
as we want to it and they just stack up on the queue. There is no
throttling or feedback here, it's just a black hole.

> From there, we can still explore additional enhancements incrementally,
> such as pushing some of the busy extent processing/sorting into the
> workqueue, eventually converting the wq-based mechanism into a
> thread-based one, etc. Thoughts?

As I always say: do it once, do it properly, or don't touch it at
all.

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

Re: [PATCH] xfs: defer online discard submission to a workqueue

[Christoph Hellwig]

On Mon, Nov 05, 2018 at 04:20:24PM -0600, Eric Sandeen wrote:
> Is there some downside to Brian's proposal, in principle?
> It seems like it would be an improvement for device that might cause
> a discard bottleneck like this.

We create another bottleneck, we lost thottling, we don't flush
the workqueue when needed and probably a few more.

Nevermind that I don't see any point in optimizing for out of tree
code that no one even bothers to upstream.  All this VDO here and there
crap from Red Hat in the last year is really more than enough.

Re: [PATCH] xfs: defer online discard submission to a workqueue

[Christoph Hellwig]

On Tue, Nov 06, 2018 at 09:23:11AM -0500, Brian Foster wrote:
> My
> understanding is that these discards can stack up and take enough time
> that a limit on outstanding discards is required, which now that I think
> of it makes me somewhat skeptical of the whole serial execution thing.
> Hitting that outstanding discard request limit is what bubbles up the
> stack and affects XFS by holding up log forces, since new discard
> submissions are presumably blocked on completion of the oldest
> outstanding request.

We don't do strict ordering or request, but eventually requests
waiting for completion will block others from being submitted.

> I'm not quite sure what happens in the block layer if that limit were
> lifted. Perhaps it assumes throttling responsibility directly via
> queues/plugs? I'd guess that at minimum we'd end up blocking indirectly
> somewhere (via memory allocation pressure?) anyways, so ISTM that some
> kind of throttling is inevitable in this situation. What am I missing?

We'll still block new allocations waiting for these blocks and
other bits.  Or to put it another way - if your discard implementation
is slow (independent of synchronous or not) your are going to be in
a world of pain with online discard.  That is what it's not default
to start with.

Re: [PATCH] xfs: defer online discard submission to a workqueue

[Brian Foster]

On Fri, Nov 09, 2018 at 11:20:52AM +1100, Dave Chinner wrote:
> On Thu, Nov 08, 2018 at 08:50:02AM -0500, Brian Foster wrote:
> > On Thu, Nov 08, 2018 at 11:38:06AM +1100, Dave Chinner wrote:
> > > On Wed, Nov 07, 2018 at 08:42:24AM -0500, Brian Foster wrote:
> > > > On Wed, Nov 07, 2018 at 08:18:02AM +1100, Dave Chinner wrote:
> > > Yes, but consider the situation where we've got a slow discard
> > > device and we're removing a file with millions of extents. We've got
> > > to issue millions of discard ops in this case. because dispatch
> > > queueing is not bound, we're easily going to overflow the discard
> > > workqueue because the freeing transactions will run far ahead of the
> > > discard operations. Sooner or later we consume all 256 discard_wq
> > > worker threads with blocked discard submission.  Then both log IO
> > > completion and discard I ocompletion will block on workqueue
> > > submission and we deadlock because discard completion can't
> > > run....
> > > 
> > > > 
> > > > Of course, the CIL context structure appears to be technically unbound
> > > 
> > > I'm missing something here - What bit of that structure is unbound?
> > > 
> > 
> > I mean to say that the ctx is not a fixed/limited resource in the
> > context of a discard submission workqueue. There's no throttling, so we
> > can essentially discard as many busy extent lists as the user can
> > generate (this is essentially the same point as your million extent file
> > example above).
> 
> It is bound. It's bound by log size and the number of checkpoints we
> can have uncompleted at any point in time. That can be a high bound
> on large logs, but it is limited and it is throttled - you can't
> create new contexts if there is no log space available, and hence
> new contexts are throttled on tail pushing.
> 

"... in the context of a discard submission workqueue."

We're in violent agreement. You're describing the code as it is today
(where it is bound) and I'm referring to prospective behavior with a
separate submit queue (where it is made unbound by virtue of the wq).
You explain the same problem below that I'm referring to above.

> > > > as well and it's trivial to just add a separate submission workqueue,
> > > > but I'd like to at least make sure we're on the same page as to the need
> > > > (so it can be documented clearly as well).
> > > 
> > > A separate submission queue doesn't really solve log Io completion
> > > blocking problem. Yes, it solves the discard completion deadlock,
> > > but we eventually end up in the same place on sustained discard
> > > workloads with submission queuing blocking on a full work queue.
> > > 
> > 
> > Well, for the purposes of this patch it's fine that we still block on
> > discard submission. The point is just that we do so in a separate
> > context from log I/O completion and thus avoid associated log force
> > stalls.
> 
> And, in doing so, remove any bound we have on the number of
> outstanding uncompleted discards. You're essentially trading
> "slow things progressively as discard load goes up" with "nothing
> blocks until we hit a breakdown point and the system stops until the
> discard queue drains".
> 
> We have a limited queue depth of discard operations right now, and
> even that is deep enough to cause problems.  Removing that queue
> depth bound won't improve things - it will just allow the pending
> discard queue to run deeper and get further and further behind the
> extent free operations that are being committed. This will only make
> the stalls and problems completely unpredictable and uncontrollable,
> and poses a true breakdown possibility where allocation in every AG
> is stalled holding AGF locks waiting for a huge queue of discards to
> be worked through.
> 
> We have to throttle at some point to prevent us from getting to
> these severe breakdown conditions. Moving discards out from under
> the log force removes the only feedback loop we have to throttle
> discards back to a manageable level. What we have is not perfect,
> but we can't just remove that feedback loop without providing
> something else to replace it.
> 

So the fundamental question here is how to manage discard submission
throttling if we disconnect submission from the implicit throttling
provided by ordered log I/O completion. We may be able to loosen this up
significantly from the current approach, but we still need to explore
how much and provide _something_ that addresses severe breakdown
conditions. Makes sense.

> > > The workqueue approach just doesn't allow anything like this to be
> > > done because every discard context is kept separate from every other
> > > context and there is no coordination at all between them.
> > > 
> > 
> > The high level kthread approach sounds like a nice idea to me. I'm not
> > sure that it's technically necessary to address this specific problem
> > though. I.e., it would be nice if we could still separate enhancement
> > from bug fix.
> 
> Just removing the discards from the log force context is just hiding
> a source of latency without addressing the cause of the latency.
> i.e. there's a huge amount of discard to be managed, and the only
> management we have to constrain them to the rate at which we retire
> the extent free transactions.
> 
> > > > > 2. log forces no longer wait for discards to be dispatched - they
> > > > > just queue them. This means the mechanism xfs_extent_busy_flush()
> > > > > uses to dispatch pending discards (synchrnous log force) can return
> > > > > before discards have even been dispatched to disk. Hence we can
> > > > > expect to see longer wait and tail latencies when busy extents are
> > > > > encountered by the allocator. Whether this is a problem or not needs
> > > > > further investigation.
> > > > > 
> > > > 
> > > > Firstly, I think latency is kind of moot in the problematic case. The
> > > > queue is already drowning in requests that presumably are going to take
> > > > minutes to complete. In that case, the overhead of kicking a workqueue
> > > > to do the submission is probably negligible.
> > > 
> > > Yes, the overhead of kicking the queue is negliable. That's not the
> > > problem though. By queuing discards rather than submitting them we
> > > go from a single FIFO dispatch model (by in-order iclog IO
> > > completion) to a concurrent, uncoordinated dispatch model.  It's the
> > > loss of FIFO behaviour because the synchrnous log force no longer
> > > controls dispatch order that leads to unpredictable and long tail
> > > latencies in dispatch completion, hence causing the problems for the
> > > proceeses now waiting on specific extent discard completion rather
> > > than just the log force.
> > > 
> > 
> > I think I see what you're getting at here: discard submission via log
> > completion means that submission is serialized (with respect to other
> > iclogs) and in log buf order, since that is enforced by the log callback
> > invoking code. Since the callbacks execute one at a time, blocking in
> > submission likely means the same, single log I/O completion execution
> > context ends up iterating over and processing the other iclogs with
> > pending callbacks once the current submit blockage clears. Yes?
> 
> Essentially.
> 
> > If I'm following that correctly, couldn't we provide very similar
> > behavior by using a separate "ordered" workqueue for submission?
> 
> Workqueues are not bound depth queues - they are "concurrency
> managed" queues. And ordered workqueue is simply a FIFO with a
> single processing context. You can queue as much as you like to it,
> but it will only process it with a single thread.
> 
> > The
> > ordered wq sets max_active to 1, which means we still have a single
> > submitter,
> 
> We have a single /worker/ - we can submit as many independent works
> as we want to it and they just stack up on the queue. There is no
> throttling or feedback here, it's just a black hole.
> 

Sure, but just using a kthread in its place by itself doesn't indicate
anything to me about throttling. We can just as easily create an
unthrottled thread-based model with an unbound set of busy extent lists,
for example, or in the other extreme, use a workqueue to provide the
exact same throttling model we have today (which wouldn't address the
original problem). Throttling is a distinct element of design from
whether we use a workqueue or kthread or whatever else.

I think it's kind of fruitless to continue on about such implementation
details without working out a higher level means of throttling...

> > From there, we can still explore additional enhancements incrementally,
> > such as pushing some of the busy extent processing/sorting into the
> > workqueue, eventually converting the wq-based mechanism into a
> > thread-based one, etc. Thoughts?
> 
> As I always say: do it once, do it properly, or don't touch it at
> all.
> 

TBH, I'm having a hard time grokking what you consider a proper fix
because we seem to be harping over less consequential implementation
details like what low level mechanisms we can or can't use. I can think
of ways to throttle either with different benefit and limitation
tradeoffs, but I still have no real clarity on what kind of throttling
you envision that demands a particular implementation. If you have a
particular technique or approach of throttling in mind, could you please
just describe it at a high level?

IOW, I don't particularly care what mechanism we ultimately use. I'm
just trying to simplify a prospective implementation if at all possible
based on my limited understanding of the problem space. Exploring the
problem space directly is far more helpful to me than doing so
implicitly via discussions over why a particular mechanism may or may
not be suitable.

So to try to put this on a more productive path... my understanding is
that deferring discard submissions outside of log completion introduces
the following considerations:

- unbound queue depth (i.e., outstanding busy extent tracking and memory
  consumption thereof)
- long tail allocation latency caused by allocation requests for blocks
  that might be deep in the unbound queue (i.e., minutes away from
  availability)

The first strikes me as of more of a memory consumption matter. It's not
immediately clear to me whether this would be handled naturally via the
preexisting busy extent struct allocations or we need to take further
precautions. I think those structures are currently limited indirectly
via being cleared in the context of a limited resource (i.e., the log).
IOW, aggressive busy extent creators are eventually going to be
throttled against log completion because transactions require log space
and log buffers, etc.

The second is the issue you've described where driving a hugely deep
discard submission queue means allocators can get stuck on busy blocks
deep in the queue with unpredictable submission+completion latency.

Given the above, I'm wondering if we could do something like reuse the
existing busy extent tree as a queue. Rather than submit busy extent
lists directly, set a new "discard needed" flag on the busy extents in
the tree on log I/O completion and carry on. Whenever discard needed
extents are present, we'll have some kind of rotorized background
submitter sending sorted discards in our preferred batch size. This
background submitter is still free to block.

In conjunction with that, we already have the ability for allocators to
look up busy extents in the tree and trim, wait or reuse them. So how
about allowing allocators to inject priority into the equation by either
submitting isolated discards themselves or allowing them to manipulate
the queue by reusing extents that are only busy due to pending discard
submissions (i.e., the discard has not yet been submitted)?

I think direct submission provides some level of predictable worst case
latency on the allocation side because if the bg submitter is blocked,
the allocator submission requests should have next priority over the
remaining set of pending discard submissions instead of being blocked on
not only the discard queue, but wherever the XFS submission queue
happens to hold that particular extent. IOW, allocators are bound by the
underlying discard processing engine as they are today, just not
indirectly via the log subsystem.

Beyond that, we may be able to actually provide additional benefits by
allowing allocators to optimize away more discards for extents that are
only still sitting in the busy tree for discard submission (i.e., the
associated extent free transaction has committed to the log). IOW,
there's potential for a larger window for extent reuse optimization, but
I still need to study the busy extent code a bit more to wrap my head
around that.

Finally, I think that still leaves us open to creating a huge set of
outstanding submissions depending on how long discards take to complete,
but as you mentioned earlier, there are probably a variety of ways we
can manage that problem. E.g., start dropping discards of a particular
size when/if we're over a particular threshold, perhaps start throttling
busy extent creators by requiring some form of ticket/reservation into
the busy extent tree on transaction reservation, etc. This may be
secondary problem depending on how effective the latency mitigation
throttling is by itself.

Thoughts on any of that?

Brian

> Cheers,
> 
> Dave.
> -- 
> Dave Chinner
> david@fromorbit.com

Re: [PATCH] xfs: defer online discard submission to a workqueue

[Brian Foster]

On Fri, Nov 09, 2018 at 07:06:10AM -0800, Christoph Hellwig wrote:
> On Tue, Nov 06, 2018 at 09:23:11AM -0500, Brian Foster wrote:
> > My
> > understanding is that these discards can stack up and take enough time
> > that a limit on outstanding discards is required, which now that I think
> > of it makes me somewhat skeptical of the whole serial execution thing.
> > Hitting that outstanding discard request limit is what bubbles up the
> > stack and affects XFS by holding up log forces, since new discard
> > submissions are presumably blocked on completion of the oldest
> > outstanding request.
> 
> We don't do strict ordering or request, but eventually requests
> waiting for completion will block others from being submitted.
> 

Ok, that's kind of what I expected.

> > I'm not quite sure what happens in the block layer if that limit were
> > lifted. Perhaps it assumes throttling responsibility directly via
> > queues/plugs? I'd guess that at minimum we'd end up blocking indirectly
> > somewhere (via memory allocation pressure?) anyways, so ISTM that some
> > kind of throttling is inevitable in this situation. What am I missing?
> 
> We'll still block new allocations waiting for these blocks and
> other bits.  Or to put it another way - if your discard implementation
> is slow (independent of synchronous or not) your are going to be in
> a world of pain with online discard.  That is what it's not default
> to start with.

Sure, it's not really the XFS bits I was asking about here. This is
certainly not a high priority and not a common use case. We're working
through some of the other issues in the other sub-thread. In particular,
I'm wondering if we can provide broader improvements to the overall
mechanism to reduce some of that pain.

Brian