* [PATCH 1/2] xfs: introduce XFS_BMAPI_STACK_SWITCH
2012-10-05 1:06 [PATCH 0/2] xfs: fix AGF/alloc workqueue deadlock Dave Chinner
@ 2012-10-05 1:06 ` Dave Chinner
2012-10-05 13:27 ` Mark Tinguely
2012-10-05 1:06 ` [PATCH 2/2] xfs: move allocation stack switch up to xfs_bmapi_allocate Dave Chinner
2012-10-18 22:51 ` [PATCH 0/2] xfs: fix AGF/alloc workqueue deadlock Ben Myers
2 siblings, 1 reply; 6+ messages in thread
From: Dave Chinner @ 2012-10-05 1:06 UTC (permalink / raw)
To: xfs
From: Dave Chinner <dchinner@redhat.com>
Certain allocation paths through xfs_bmapi_write() are in situations
where we have limited stack available. These are almost always in
the buffered IO writeback path when convertion delayed allocation
extents to real extents.
The current stack switch occurs for userdata allocations, which
means we also do stack switches for preallocation, direct IO and
unwritten extent conversion, even those these call chains have never
been implicated in a stack overrun.
Hence, let's target just the single stack overun offended for stack
switches. To do that, introduce a XFS_BMAPI_STACK_SWITCH flag that
the caller can pass xfs_bmapi_write() to indicate it should switch
stacks if it needs to do allocation.
Signed-off-by: Dave Chinner <dchinner@redhat.com>
---
fs/xfs/xfs_alloc.c | 2 +-
fs/xfs/xfs_alloc.h | 1 +
fs/xfs/xfs_bmap.c | 4 ++++
fs/xfs/xfs_bmap.h | 5 ++++-
fs/xfs/xfs_iomap.c | 4 +++-
5 files changed, 13 insertions(+), 3 deletions(-)
diff --git a/fs/xfs/xfs_alloc.c b/fs/xfs/xfs_alloc.c
index 0287f3b..43f791b 100644
--- a/fs/xfs/xfs_alloc.c
+++ b/fs/xfs/xfs_alloc.c
@@ -2447,7 +2447,7 @@ xfs_alloc_vextent(
{
DECLARE_COMPLETION_ONSTACK(done);
- if (!args->userdata)
+ if (!args->stack_switch)
return __xfs_alloc_vextent(args);
diff --git a/fs/xfs/xfs_alloc.h b/fs/xfs/xfs_alloc.h
index 93be4a6..ef7d488 100644
--- a/fs/xfs/xfs_alloc.h
+++ b/fs/xfs/xfs_alloc.h
@@ -123,6 +123,7 @@ typedef struct xfs_alloc_arg {
struct completion *done;
struct work_struct work;
int result;
+ char stack_switch;
} xfs_alloc_arg_t;
/*
diff --git a/fs/xfs/xfs_bmap.c b/fs/xfs/xfs_bmap.c
index e1545ec..9125955 100644
--- a/fs/xfs/xfs_bmap.c
+++ b/fs/xfs/xfs_bmap.c
@@ -2441,6 +2441,7 @@ xfs_bmap_btalloc(
args.tp = ap->tp;
args.mp = mp;
args.fsbno = ap->blkno;
+ args.stack_switch = ap->stack_switch;
/* Trim the allocation back to the maximum an AG can fit. */
args.maxlen = MIN(ap->length, XFS_ALLOC_AG_MAX_USABLE(mp));
@@ -4675,6 +4676,9 @@ xfs_bmapi_allocate(
return error;
}
+ if (flags & XFS_BMAPI_STACK_SWITCH)
+ bma->stack_switch = 1;
+
error = xfs_bmap_alloc(bma);
if (error)
return error;
diff --git a/fs/xfs/xfs_bmap.h b/fs/xfs/xfs_bmap.h
index 803b56d..b68c598 100644
--- a/fs/xfs/xfs_bmap.h
+++ b/fs/xfs/xfs_bmap.h
@@ -77,6 +77,7 @@ typedef struct xfs_bmap_free
* from written to unwritten, otherwise convert from unwritten to written.
*/
#define XFS_BMAPI_CONVERT 0x040
+#define XFS_BMAPI_STACK_SWITCH 0x080
#define XFS_BMAPI_FLAGS \
{ XFS_BMAPI_ENTIRE, "ENTIRE" }, \
@@ -85,7 +86,8 @@ typedef struct xfs_bmap_free
{ XFS_BMAPI_PREALLOC, "PREALLOC" }, \
{ XFS_BMAPI_IGSTATE, "IGSTATE" }, \
{ XFS_BMAPI_CONTIG, "CONTIG" }, \
- { XFS_BMAPI_CONVERT, "CONVERT" }
+ { XFS_BMAPI_CONVERT, "CONVERT" }, \
+ { XFS_BMAPI_STACK_SWITCH, "STACK_SWITCH" }
static inline int xfs_bmapi_aflag(int w)
@@ -133,6 +135,7 @@ typedef struct xfs_bmalloca {
char userdata;/* set if is user data */
char aeof; /* allocated space at eof */
char conv; /* overwriting unwritten extents */
+ char stack_switch;
} xfs_bmalloca_t;
/*
diff --git a/fs/xfs/xfs_iomap.c b/fs/xfs/xfs_iomap.c
index f858b90..a066cf1 100644
--- a/fs/xfs/xfs_iomap.c
+++ b/fs/xfs/xfs_iomap.c
@@ -575,7 +575,9 @@ xfs_iomap_write_allocate(
* pointer that the caller gave to us.
*/
error = xfs_bmapi_write(tp, ip, map_start_fsb,
- count_fsb, 0, &first_block, 1,
+ count_fsb,
+ XFS_BMAPI_STACK_SWITCH,
+ &first_block, 1,
imap, &nimaps, &free_list);
if (error)
goto trans_cancel;
--
1.7.10
_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs
^ permalink raw reply related [flat|nested] 6+ messages in thread* Re: [PATCH 1/2] xfs: introduce XFS_BMAPI_STACK_SWITCH
2012-10-05 1:06 ` [PATCH 1/2] xfs: introduce XFS_BMAPI_STACK_SWITCH Dave Chinner
@ 2012-10-05 13:27 ` Mark Tinguely
0 siblings, 0 replies; 6+ messages in thread
From: Mark Tinguely @ 2012-10-05 13:27 UTC (permalink / raw)
To: Dave Chinner; +Cc: xfs
On 10/04/12 20:06, Dave Chinner wrote:
> From: Dave Chinner<dchinner@redhat.com>
>
> Certain allocation paths through xfs_bmapi_write() are in situations
> where we have limited stack available. These are almost always in
> the buffered IO writeback path when convertion delayed allocation
> extents to real extents.
>
> The current stack switch occurs for userdata allocations, which
> means we also do stack switches for preallocation, direct IO and
> unwritten extent conversion, even those these call chains have never
> been implicated in a stack overrun.
>
> Hence, let's target just the single stack overun offended for stack
> switches. To do that, introduce a XFS_BMAPI_STACK_SWITCH flag that
> the caller can pass xfs_bmapi_write() to indicate it should switch
> stacks if it needs to do allocation.
>
> Signed-off-by: Dave Chinner<dchinner@redhat.com>
> ---
Looks good.
Reviewed-by: Mark Tinguely <tinguely@sgi.com>
_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs
^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH 2/2] xfs: move allocation stack switch up to xfs_bmapi_allocate
2012-10-05 1:06 [PATCH 0/2] xfs: fix AGF/alloc workqueue deadlock Dave Chinner
2012-10-05 1:06 ` [PATCH 1/2] xfs: introduce XFS_BMAPI_STACK_SWITCH Dave Chinner
@ 2012-10-05 1:06 ` Dave Chinner
2012-10-05 13:28 ` Mark Tinguely
2012-10-18 22:51 ` [PATCH 0/2] xfs: fix AGF/alloc workqueue deadlock Ben Myers
2 siblings, 1 reply; 6+ messages in thread
From: Dave Chinner @ 2012-10-05 1:06 UTC (permalink / raw)
To: xfs
From: Dave Chinner <dchinner@redhat.com>
Switching stacks are xfs_alloc_vextent can cause deadlocks when we
run out of worker threads on the allocation workqueue. This can
occur because xfs_bmap_btalloc can make multiple calls to
xfs_alloc_vextent() and even if xfs_alloc_vextent() fails it can
return with the AGF locked in the current allocation transaction.
If we then need to make another allocation, and all the allocation
worker contexts are exhausted because the are blocked waiting for
the AGF lock, holder of the AGF cannot get it's xfs-alloc_vextent
work completed to release the AGF. Hence allocation effectively
deadlocks.
To avoid this, move the stack switch one layer up to
xfs_bmapi_allocate() so that all of the allocation attempts in a
single switched stack transaction occur in a single worker context.
This avoids the problem of an allocation being blocked waiting for
a worker thread whilst holding the AGF.
Signed-off-by: Dave Chinner <dchinner@redhat.com>
---
fs/xfs/xfs_alloc.c | 42 +-----------------------------------
fs/xfs/xfs_alloc.h | 4 ----
fs/xfs/xfs_bmap.c | 60 ++++++++++++++++++++++++++++++++++++++++++----------
fs/xfs/xfs_bmap.h | 4 ++++
4 files changed, 54 insertions(+), 56 deletions(-)
diff --git a/fs/xfs/xfs_alloc.c b/fs/xfs/xfs_alloc.c
index 43f791b..335206a 100644
--- a/fs/xfs/xfs_alloc.c
+++ b/fs/xfs/xfs_alloc.c
@@ -2208,7 +2208,7 @@ xfs_alloc_read_agf(
* group or loop over the allocation groups to find the result.
*/
int /* error */
-__xfs_alloc_vextent(
+xfs_alloc_vextent(
xfs_alloc_arg_t *args) /* allocation argument structure */
{
xfs_agblock_t agsize; /* allocation group size */
@@ -2418,46 +2418,6 @@ error0:
return error;
}
-static void
-xfs_alloc_vextent_worker(
- struct work_struct *work)
-{
- struct xfs_alloc_arg *args = container_of(work,
- struct xfs_alloc_arg, work);
- unsigned long pflags;
-
- /* we are in a transaction context here */
- current_set_flags_nested(&pflags, PF_FSTRANS);
-
- args->result = __xfs_alloc_vextent(args);
- complete(args->done);
-
- current_restore_flags_nested(&pflags, PF_FSTRANS);
-}
-
-/*
- * Data allocation requests often come in with little stack to work on. Push
- * them off to a worker thread so there is lots of stack to use. Metadata
- * requests, OTOH, are generally from low stack usage paths, so avoid the
- * context switch overhead here.
- */
-int
-xfs_alloc_vextent(
- struct xfs_alloc_arg *args)
-{
- DECLARE_COMPLETION_ONSTACK(done);
-
- if (!args->stack_switch)
- return __xfs_alloc_vextent(args);
-
-
- args->done = &done;
- INIT_WORK_ONSTACK(&args->work, xfs_alloc_vextent_worker);
- queue_work(xfs_alloc_wq, &args->work);
- wait_for_completion(&done);
- return args->result;
-}
-
/*
* Free an extent.
* Just break up the extent address and hand off to xfs_free_ag_extent
diff --git a/fs/xfs/xfs_alloc.h b/fs/xfs/xfs_alloc.h
index ef7d488..feacb06 100644
--- a/fs/xfs/xfs_alloc.h
+++ b/fs/xfs/xfs_alloc.h
@@ -120,10 +120,6 @@ typedef struct xfs_alloc_arg {
char isfl; /* set if is freelist blocks - !acctg */
char userdata; /* set if this is user data */
xfs_fsblock_t firstblock; /* io first block allocated */
- struct completion *done;
- struct work_struct work;
- int result;
- char stack_switch;
} xfs_alloc_arg_t;
/*
diff --git a/fs/xfs/xfs_bmap.c b/fs/xfs/xfs_bmap.c
index 9125955..83d0cf3 100644
--- a/fs/xfs/xfs_bmap.c
+++ b/fs/xfs/xfs_bmap.c
@@ -2441,7 +2441,6 @@ xfs_bmap_btalloc(
args.tp = ap->tp;
args.mp = mp;
args.fsbno = ap->blkno;
- args.stack_switch = ap->stack_switch;
/* Trim the allocation back to the maximum an AG can fit. */
args.maxlen = MIN(ap->length, XFS_ALLOC_AG_MAX_USABLE(mp));
@@ -4620,12 +4619,11 @@ xfs_bmapi_delay(
STATIC int
-xfs_bmapi_allocate(
- struct xfs_bmalloca *bma,
- int flags)
+__xfs_bmapi_allocate(
+ struct xfs_bmalloca *bma)
{
struct xfs_mount *mp = bma->ip->i_mount;
- int whichfork = (flags & XFS_BMAPI_ATTRFORK) ?
+ int whichfork = (bma->flags & XFS_BMAPI_ATTRFORK) ?
XFS_ATTR_FORK : XFS_DATA_FORK;
struct xfs_ifork *ifp = XFS_IFORK_PTR(bma->ip, whichfork);
int tmp_logflags = 0;
@@ -4658,25 +4656,25 @@ xfs_bmapi_allocate(
* Indicate if this is the first user data in the file, or just any
* user data.
*/
- if (!(flags & XFS_BMAPI_METADATA)) {
+ if (!(bma->flags & XFS_BMAPI_METADATA)) {
bma->userdata = (bma->offset == 0) ?
XFS_ALLOC_INITIAL_USER_DATA : XFS_ALLOC_USERDATA;
}
- bma->minlen = (flags & XFS_BMAPI_CONTIG) ? bma->length : 1;
+ bma->minlen = (bma->flags & XFS_BMAPI_CONTIG) ? bma->length : 1;
/*
* Only want to do the alignment at the eof if it is userdata and
* allocation length is larger than a stripe unit.
*/
if (mp->m_dalign && bma->length >= mp->m_dalign &&
- !(flags & XFS_BMAPI_METADATA) && whichfork == XFS_DATA_FORK) {
+ !(bma->flags & XFS_BMAPI_METADATA) && whichfork == XFS_DATA_FORK) {
error = xfs_bmap_isaeof(bma, whichfork);
if (error)
return error;
}
- if (flags & XFS_BMAPI_STACK_SWITCH)
+ if (bma->flags & XFS_BMAPI_STACK_SWITCH)
bma->stack_switch = 1;
error = xfs_bmap_alloc(bma);
@@ -4713,7 +4711,7 @@ xfs_bmapi_allocate(
* A wasdelay extent has been initialized, so shouldn't be flagged
* as unwritten.
*/
- if (!bma->wasdel && (flags & XFS_BMAPI_PREALLOC) &&
+ if (!bma->wasdel && (bma->flags & XFS_BMAPI_PREALLOC) &&
xfs_sb_version_hasextflgbit(&mp->m_sb))
bma->got.br_state = XFS_EXT_UNWRITTEN;
@@ -4741,6 +4739,45 @@ xfs_bmapi_allocate(
return 0;
}
+static void
+xfs_bmapi_allocate_worker(
+ struct work_struct *work)
+{
+ struct xfs_bmalloca *args = container_of(work,
+ struct xfs_bmalloca, work);
+ unsigned long pflags;
+
+ /* we are in a transaction context here */
+ current_set_flags_nested(&pflags, PF_FSTRANS);
+
+ args->result = __xfs_bmapi_allocate(args);
+ complete(args->done);
+
+ current_restore_flags_nested(&pflags, PF_FSTRANS);
+}
+
+/*
+ * Some allocation requests often come in with little stack to work on. Push
+ * them off to a worker thread so there is lots of stack to use. Otherwise just
+ * call directly to avoid the context switch overhead here.
+ */
+int
+xfs_bmapi_allocate(
+ struct xfs_bmalloca *args)
+{
+ DECLARE_COMPLETION_ONSTACK(done);
+
+ if (!args->stack_switch)
+ return __xfs_bmapi_allocate(args);
+
+
+ args->done = &done;
+ INIT_WORK_ONSTACK(&args->work, xfs_bmapi_allocate_worker);
+ queue_work(xfs_alloc_wq, &args->work);
+ wait_for_completion(&done);
+ return args->result;
+}
+
STATIC int
xfs_bmapi_convert_unwritten(
struct xfs_bmalloca *bma,
@@ -4926,6 +4963,7 @@ xfs_bmapi_write(
bma.conv = !!(flags & XFS_BMAPI_CONVERT);
bma.wasdel = wasdelay;
bma.offset = bno;
+ bma.flags = flags;
/*
* There's a 32/64 bit type mismatch between the
@@ -4941,7 +4979,7 @@ xfs_bmapi_write(
ASSERT(len > 0);
ASSERT(bma.length > 0);
- error = xfs_bmapi_allocate(&bma, flags);
+ error = xfs_bmapi_allocate(&bma);
if (error)
goto error0;
if (bma.blkno == NULLFSBLOCK)
diff --git a/fs/xfs/xfs_bmap.h b/fs/xfs/xfs_bmap.h
index b68c598..5f469c3 100644
--- a/fs/xfs/xfs_bmap.h
+++ b/fs/xfs/xfs_bmap.h
@@ -136,6 +136,10 @@ typedef struct xfs_bmalloca {
char aeof; /* allocated space at eof */
char conv; /* overwriting unwritten extents */
char stack_switch;
+ int flags;
+ struct completion *done;
+ struct work_struct work;
+ int result;
} xfs_bmalloca_t;
/*
--
1.7.10
_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs
^ permalink raw reply related [flat|nested] 6+ messages in thread* Re: [PATCH 2/2] xfs: move allocation stack switch up to xfs_bmapi_allocate
2012-10-05 1:06 ` [PATCH 2/2] xfs: move allocation stack switch up to xfs_bmapi_allocate Dave Chinner
@ 2012-10-05 13:28 ` Mark Tinguely
0 siblings, 0 replies; 6+ messages in thread
From: Mark Tinguely @ 2012-10-05 13:28 UTC (permalink / raw)
To: Dave Chinner; +Cc: xfs
On 10/04/12 20:06, Dave Chinner wrote:
> From: Dave Chinner<dchinner@redhat.com>
>
> Switching stacks are xfs_alloc_vextent can cause deadlocks when we
> run out of worker threads on the allocation workqueue. This can
> occur because xfs_bmap_btalloc can make multiple calls to
> xfs_alloc_vextent() and even if xfs_alloc_vextent() fails it can
> return with the AGF locked in the current allocation transaction.
>
> If we then need to make another allocation, and all the allocation
> worker contexts are exhausted because the are blocked waiting for
> the AGF lock, holder of the AGF cannot get it's xfs-alloc_vextent
> work completed to release the AGF. Hence allocation effectively
> deadlocks.
>
> To avoid this, move the stack switch one layer up to
> xfs_bmapi_allocate() so that all of the allocation attempts in a
> single switched stack transaction occur in a single worker context.
> This avoids the problem of an allocation being blocked waiting for
> a worker thread whilst holding the AGF.
>
> Signed-off-by: Dave Chinner<dchinner@redhat.com>
Looks good.
Reviewed-by: Mark Tinguely <tinguely@sgi.com>
_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 0/2] xfs: fix AGF/alloc workqueue deadlock.
2012-10-05 1:06 [PATCH 0/2] xfs: fix AGF/alloc workqueue deadlock Dave Chinner
2012-10-05 1:06 ` [PATCH 1/2] xfs: introduce XFS_BMAPI_STACK_SWITCH Dave Chinner
2012-10-05 1:06 ` [PATCH 2/2] xfs: move allocation stack switch up to xfs_bmapi_allocate Dave Chinner
@ 2012-10-18 22:51 ` Ben Myers
2 siblings, 0 replies; 6+ messages in thread
From: Ben Myers @ 2012-10-18 22:51 UTC (permalink / raw)
To: Dave Chinner; +Cc: xfs
Hi Dave,
On Fri, Oct 05, 2012 at 11:06:57AM +1000, Dave Chinner wrote:
> This is a followup from the last conversation with Mark about this
> deadlock. I haven't heard anything in the last coupl eof days, so I
> figured I'd just write the patches that did what I thought is
> needed.
>
> Basically, the stack switch is only needed for userdata in the
> delayed allocation path, and nowhere else at this point in time. To
> make it so we switch stacks only at this point in time, I introduced
> a flag to pass to xfs_bmapi_write(). This means we don't switch
> stacks during direct IO, unwritten extent conversion or
> preallocation via fallocate/XFS_IOC_RESVSP.
>
> I then moved the stack switch to xfs_bmapi_allocate(), which is
> where allocation happens. All the allocation calls within a single
> userdata extent allocation occur in this function (both data extent
> and bmap btree blocks), so switching stacks here will ensure that
> only mapping calls that require allocation will have the stack
> switched.
>
> Comments welcome...
These two patches have been committed to git://oss.sgi.com/xfs/xfs.git, master
and for-next branches.
Regards,
Ben
_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs
^ permalink raw reply [flat|nested] 6+ messages in thread