xfs: move global xfslogd workqueue to per-mount
PATCH 1/1 xfs: move global xfslogd workqueue to per-mount
The xfslogd workqueue is a global, single-job workqueue for buffer ioend processing. This means we allow for a single work item at a time for all possible XFS mounts on a system. fsstress testing in loopback XFS over XFS configurations has reproduced xfslogd deadlocks due to the single threaded nature of the queue and dependencies introduced between the separate XFS instances by online discard (-o discard).
Discard over a loopback device converts the discard request to a hole punch (fallocate) on the underlying file. Online discard requests are issued synchronously and from xfslogd context in XFS, hence the xfslogd workqueue is blocked in the upper fs waiting on a hole punch request to be servied in the lower fs. If the lower fs issues I/O that depends on xfslogd to complete, both filesystems end up hung indefinitely. This is reproduced reliabily by generic/013 on XFS->loop->XFS test devices with the '-o discard' mount option.
Further, docker implementations appear to use this kind of configuration for container instance filesystems by default (container fs->dm-> loop->base fs) and therefore are subject to this deadlock when running on XFS.
Replace the global xfslogd workqueue with a per-mount variant. This guarantees each mount access to a single worker and prevents deadlocks due to inter-fs dependencies introduced by discard.
---
Hi all,
Thoughts? An alternative was to increase max jobs on the existing workqueue, but this seems more in line with how we manage workqueues these days.
Brian
fs/xfs/xfs_buf.c | 13 ++----------- fs/xfs/xfs_mount.h | 1 + fs/xfs/xfs_super.c | 11 ++++++++++- 3 files changed, 13 insertions(+), 12 deletions(-)
fs/xfs/xfs_buf.c +2 -11
@@ -44,8 +44,6 @@static kmem_zone_t *xfs_buf_zone; -static struct workqueue_struct *xfslogd_workqueue; -#ifdef XFS_BUF_LOCK_TRACKING # define XB_SET_OWNER (bp) ((bp)->b_last_holder = current->pid)# define XB_CLEAR_OWNER (bp) ((bp)->b_last_holder = -1)
@@ -1053,7 +1051,8 @@ xfs_buf_ioend_async( struct xfs_buf *bp) {INIT_WORK (&bp->b_iodone_work, xfs_buf_ioend_work); -queue_work (xfslogd_workqueue, &bp->b_iodone_work); +queue_work (bp->b_target->bt_mount->m_xfslogd_workqueue, + &bp->b_iodone_work); }void
@@ -1882,15 +1881,8 @@ xfs_buf_init(void)if (!xfs_buf_zone) goto out; - xfslogd_workqueue =alloc_workqueue ("xfslogd" , -WQ_MEM_RECLAIM |WQ_HIGHPRI |WQ_FREEZABLE ,1 ); -if (!xfslogd_workqueue) - goto out_free_buf_zone; -return 0 ; - out_free_buf_zone: -kmem_zone_destroy (xfs_buf_zone); out:return -ENOMEM ; }
@@ -1898,6 +1890,5 @@ xfs_buf_init(void)void xfs_buf_terminate (void ) { -destroy_workqueue (xfslogd_workqueue);kmem_zone_destroy (xfs_buf_zone); }
fs/xfs/xfs_mount.h +1 -0
@@ -168,6 +168,7 @@ typedef struct xfs_mount {/* low free space thresholds */ struct xfs_kobj m_kobj; +struct workqueue_struct *m_xfslogd_workqueue;struct workqueue_struct *m_data_workqueue;struct workqueue_struct *m_unwritten_workqueue;struct workqueue_struct *m_cil_workqueue;
fs/xfs/xfs_super.c +10 -1
@@ -842,10 +842,16 @@ STATIC intxfs_init_mount_workqueues ( struct xfs_mount *mp) { + mp->m_xfslogd_workqueue =alloc_workqueue ("xfslogd/%s" , +WQ_MEM_RECLAIM |WQ_HIGHPRI |WQ_FREEZABLE ,1 , + mp->m_fsname); +if (!mp->m_xfslogd_workqueue) + goto out; + mp->m_data_workqueue =alloc_workqueue ("xfs-data/%s" ,WQ_MEM_RECLAIM |WQ_FREEZABLE ,0 , mp->m_fsname);if (!mp->m_data_workqueue) - goto out; + goto out_destroy_xfslogd; mp->m_unwritten_workqueue =alloc_workqueue ("xfs-conv/%s" ,WQ_MEM_RECLAIM |WQ_FREEZABLE ,0 , mp->m_fsname);
@@ -884,6 +890,8 @@ out_destroy_unwritten:destroy_workqueue (mp->m_unwritten_workqueue); out_destroy_data_iodone_queue:destroy_workqueue (mp->m_data_workqueue); +out_destroy_xfslogd: +destroy_workqueue (mp->m_xfslogd_workqueue); out:return -ENOMEM ; }
@@ -898,6 +906,7 @@ xfs_destroy_mount_workqueues(destroy_workqueue (mp->m_cil_workqueue);destroy_workqueue (mp->m_data_workqueue);destroy_workqueue (mp->m_unwritten_workqueue); +destroy_workqueue (mp->m_xfslogd_workqueue); }/*
Signature · 6 lines
1.8.3.1 _______________________________________________ xfs mailing list [email protected] http://oss.sgi.com/mailman/listinfo/xfs
Quoted 25 lines · Brian Foster
The xfslogd workqueue is a global, single-job workqueue for buffer ioend processing. This means we allow for a single work item at a time for all possible XFS mounts on a system. fsstress testing in loopback XFS over XFS configurations has reproduced xfslogd deadlocks due to the single threaded nature of the queue and dependencies introduced between the separate XFS instances by online discard (-o discard).
Discard over a loopback device converts the discard request to a hole punch (fallocate) on the underlying file. Online discard requests are issued synchronously and from xfslogd context in XFS, hence the xfslogd workqueue is blocked in the upper fs waiting on a hole punch request to be servied in the lower fs. If the lower fs issues I/O that depends on xfslogd to complete, both filesystems end up hung indefinitely. This is reproduced reliabily by generic/013 on
XFS->loop->XFStest devices with the '-o discard' mount option.Further, docker implementations appear to use this kind of configuration for container instance filesystems by default (container fs->dm->
loop->basefs) and therefore are subject to this deadlock when running on XFS.Replace the global xfslogd workqueue with a per-mount variant. This guarantees each mount access to a single worker and prevents deadlocks due to inter-fs dependencies introduced by discard.
Quoted 8 lines
Signed-off-by Brian Foster <[email protected]>---
Hi all,
Thoughts? An alternative was to increase max jobs on the existing workqueue, but this seems more in line with how we manage workqueues these days.
First thing is that it's no longer a "log" workqueue. It's an async buffer completion workqueue, so we really should rename it. Especially as this change would mean we now have m_log_workqueue for the log and m_xfslogd_workqueue for buffer completion...
Indeed, is the struct xfs_mount the right place for this? Shouldn't it be on the relevant buftarg that the buffer is associated with?
Quoted 21 lines
Brian
fs/xfs/xfs_buf.c | 13 ++----------- fs/xfs/xfs_mount.h | 1 + fs/xfs/xfs_super.c | 11 ++++++++++- 3 files changed, 13 insertions(+), 12 deletions(-)fs/xfs/xfs_buf.c +0
-2@@ -44,8 +44,6 @@static kmem_zone_t *xfs_buf_zone; -static struct workqueue_struct *xfslogd_workqueue; -#ifdef XFS_BUF_LOCK_TRACKING # define XB_SET_OWNER (bp) ((bp)->b_last_holder = current->pid)# define XB_CLEAR_OWNER (bp) ((bp)->b_last_holder = -1)
fs/xfs/xfs_buf.c xfs_buf_ioend_async( · line 1056
Quoted 7 lines
fs/xfs/xfs_buf.c +2
-1@@ -1053,7 +1051,8 @@ xfs_buf_ioend_async( struct xfs_buf *bp) {INIT_WORK (&bp->b_iodone_work, xfs_buf_ioend_work); -queue_work (xfslogd_workqueue, &bp->b_iodone_work); +queue_work (bp->b_target->bt_mount->m_xfslogd_workqueue, + &bp->b_iodone_work); }
ie. queue_work(bp->b_target->bt_iodone_wq, &bp->b_iodone_work);
Cheers,
Dave.
Signature · 7 lines
Dave Chinner [email protected] _______________________________________________ xfs mailing list [email protected] http://oss.sgi.com/mailman/listinfo/xfs
Quoted 31 lines · Dave Chinner
Quoted 30 lines · Brian Foster
The xfslogd workqueue is a global, single-job workqueue for buffer ioend processing. This means we allow for a single work item at a time for all possible XFS mounts on a system. fsstress testing in loopback XFS over XFS configurations has reproduced xfslogd deadlocks due to the single threaded nature of the queue and dependencies introduced between the separate XFS instances by online discard (-o discard).
Discard over a loopback device converts the discard request to a hole punch (fallocate) on the underlying file. Online discard requests are issued synchronously and from xfslogd context in XFS, hence the xfslogd workqueue is blocked in the upper fs waiting on a hole punch request to be servied in the lower fs. If the lower fs issues I/O that depends on xfslogd to complete, both filesystems end up hung indefinitely. This is reproduced reliabily by generic/013 on
XFS->loop->XFStest devices with the '-o discard' mount option.Further, docker implementations appear to use this kind of configuration for container instance filesystems by default (container fs->dm->
loop->basefs) and therefore are subject to this deadlock when running on XFS.Replace the global xfslogd workqueue with a per-mount variant. This guarantees each mount access to a single worker and prevents deadlocks due to inter-fs dependencies introduced by discard.
Signed-off-by Brian Foster <[email protected]>---
Hi all,
Quoted 9 lines
Quoted 3 lines
Thoughts? An alternative was to increase max jobs on the existing workqueue, but this seems more in line with how we manage workqueues these days.
First thing is that it's no longer a "log" workqueue. It's an async buffer completion workqueue, so we really should rename it. Especially as this change would mean we now have
m_log_workqueuefor the log andm_xfslogd_workqueuefor buffer completion...
Ok, sounds good. The name didn't make much sense to me given what it's doing. ;) I guess it's historical.
Quoted 3 lines
Indeed, is the struct
xfs_mountthe right place for this? Shouldn't it be on the relevant buftarg that the buffer is associated with?
That makes sense from a generic design perspective: an iodone queue per buffer target. That does introduce a behavior change that we need to consider the side effects of. This queue currently is one request at a time and retaining that configuration for per-buftarg queues still allows for concurrency between log buf iodone processing and metadata buf iodone processing when the log is a separate device.
It's not clear to me why this is a max_active=1 queue, so for that reason I'm more hesitant to change behavior beyond what is a clear separation between mounts. Do we have any serialization/locking hacks around that depend on this condition? Also I suspect this means we increase the possibility of things like adding items to the AIL (xlog_iodone()) and pulling them off (e.g., xfs_iflush_done()) on separate cpus, which makes me wonder if there are hidden performance ramifications to such a change.
Maybe none of this matters and the queue config is also a historical relic..?
Brian
Quoted 31 lines
Quoted 21 lines
Brian
fs/xfs/xfs_buf.c | 13 ++----------- fs/xfs/xfs_mount.h | 1 + fs/xfs/xfs_super.c | 11 ++++++++++- 3 files changed, 13 insertions(+), 12 deletions(-)fs/xfs/xfs_buf.c +0
-2@@ -44,8 +44,6 @@static kmem_zone_t *xfs_buf_zone; -static struct workqueue_struct *xfslogd_workqueue; -#ifdef XFS_BUF_LOCK_TRACKING # define XB_SET_OWNER (bp) ((bp)->b_last_holder = current->pid)# define XB_CLEAR_OWNER (bp) ((bp)->b_last_holder = -1)fs/xfs/xfs_buf.c xfs_buf_ioend_async( · line 1056
Quoted 7 lines
fs/xfs/xfs_buf.c +2
-1@@ -1053,7 +1051,8 @@ xfs_buf_ioend_async( struct xfs_buf *bp) {INIT_WORK (&bp->b_iodone_work, xfs_buf_ioend_work); -queue_work (xfslogd_workqueue, &bp->b_iodone_work); +queue_work (bp->b_target->bt_mount->m_xfslogd_workqueue, + &bp->b_iodone_work); }ie.
queue_work(bp->b_target->bt_iodone_wq, &bp->b_iodone_work);
Quoted 6 lines
Cheers,
Dave.
Signature · 2 lines
Dave Chinner [email protected]
_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs