xfs: fix transaction allocation deadlock in IO path
PATCH 1/1 xfs: fix transaction allocation deadlock in IO path
From: Dave Chinner <[email protected]>
xfs_trans_alloc() does GFP_KERNEL allocation, and we can call it while holding pages locked for writeback in the ->writepages path. The memory allocation is allowed to wait on pages under writeback, and so can wait on pages that are held locked in writeback by the caller.
This affects both pre-IO submission and post-IO submission paths. Hence xfs_setsize_trans_alloc(), xfs_reflink_end_cow(), xfs_iomap_write_unwritten() and xfs_reflink_cancel_cow_range(). xfs_iomap_write_unwritten() already does the right thing, but the others don't. Fix them.
--- fs/xfs/xfs_aops.c | 3 ++- fs/xfs/xfs_reflink.c | 4 ++-- 2 files changed, 4 insertions(+), 3 deletions(-)
fs/xfs/xfs_aops.c +2 -1
@@ -209,7 +209,8 @@ xfs_setfilesize_trans_alloc(struct xfs_trans *tp;int error; - error =xfs_trans_alloc (mp, &M_RES (mp)->tr_fsyncts,0 ,0 ,0 , &tp); + error =xfs_trans_alloc (mp, &M_RES (mp)->tr_fsyncts,0 ,0 , +XFS_TRANS_NOFS , &tp);if (error)return error;
fs/xfs/xfs_reflink.c +2 -2
@@ -668,7 +668,7 @@ xfs_reflink_cancel_cow_range(/* Start a rolling transaction to remove the mappings */ error =xfs_trans_alloc (ip->i_mount, &M_RES (ip->i_mount)->tr_write, -0 ,0 ,0 , &tp); +0 ,0 ,XFS_TRANS_NOFS , &tp);if (error) goto out;
@@ -741,7 +741,7 @@ xfs_reflink_end_cow( (unsigned int )(end_fsb - offset_fsb), XFS_DATA_FORK); error =xfs_trans_alloc (ip->i_mount, &M_RES (ip->i_mount)->tr_write, - resblks,0 ,XFS_TRANS_RESERVE , &tp); + resblks,0 ,XFS_TRANS_RESERVE |XFS_TRANS_NOFS , &tp);if (error) goto out;
Signature · 1 line
2.16.1
Quoted 8 lines · Dave Chinner
From: Dave Chinner <[email protected]>
xfs_trans_alloc()doesGFP_KERNELallocation, and we can call it while holding pages locked for writeback in the ->writepages path. The memory allocation is allowed to wait on pages under writeback, and so can wait on pages that are held locked in writeback by the caller.
It looks like xfs_start_page_writeback() sets the page as writeback and unlocks it. I suspect this doesn't affect the actual problem if allocation waits on writeback, but rather something like "tagged as writeback" might be more clear than "held locked in writeback." Otherwise this seems fine to me:
Quoted 17 lines
This affects both pre-IO submission and post-IO submission paths. Hence
xfs_setsize_trans_alloc(),xfs_reflink_end_cow(),xfs_iomap_write_unwritten()andxfs_reflink_cancel_cow_range().xfs_iomap_write_unwritten()already does the right thing, but the others don't. Fix them.Signed-off-by Dave Chinner <[email protected]>--- fs/xfs/xfs_aops.c | 3 ++- fs/xfs/xfs_reflink.c | 4 ++-- 2 files changed, 4 insertions(+), 3 deletions(-)fs/xfs/xfs_aops.c +0
-0
fs/xfs/xfs_aops.c xfs_setfilesize_trans_alloc( · line 213
Quoted 6 lines
fs/xfs/xfs_aops.c +2
-1@@ -209,7 +209,8 @@ xfs_setfilesize_trans_alloc(struct xfs_trans *tp;int error; - error =xfs_trans_alloc (mp, &M_RES (mp)->tr_fsyncts,0 ,0 ,0 , &tp); + error =xfs_trans_alloc (mp, &M_RES (mp)->tr_fsyncts,0 ,0 , +XFS_TRANS_NOFS , &tp);
Was there another reason we preallocate this transaction where we don't seem to for others?
Brian
Quoted 32 lines
fs/xfs/xfs_aops.c +0
-0if (error)return error;fs/xfs/xfs_reflink.c +2
-2@@ -668,7 +668,7 @@ xfs_reflink_cancel_cow_range(/* Start a rolling transaction to remove the mappings */ error =xfs_trans_alloc (ip->i_mount, &M_RES (ip->i_mount)->tr_write, -0 ,0 ,0 , &tp); +0 ,0 ,XFS_TRANS_NOFS , &tp);if (error) goto out;@@ -741,7 +741,7 @@ xfs_reflink_end_cow( (unsigned int )(end_fsb - offset_fsb), XFS_DATA_FORK); error =xfs_trans_alloc (ip->i_mount, &M_RES (ip->i_mount)->tr_write, - resblks,0 ,XFS_TRANS_RESERVE , &tp); + resblks,0 ,XFS_TRANS_RESERVE |XFS_TRANS_NOFS , &tp);if (error) goto out;Signature · 6 lines
2.16.1 -- 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 10 lines · Brian Foster
Quoted 8 lines · Dave Chinner
From: Dave Chinner <[email protected]>
xfs_trans_alloc()doesGFP_KERNELallocation, and we can call it while holding pages locked for writeback in the ->writepages path. The memory allocation is allowed to wait on pages under writeback, and so can wait on pages that are held locked in writeback by the caller.
Quoted 4 lines
It looks like
xfs_start_page_writeback()sets the page as writeback and unlocks it. I suspect this doesn't affect the actual problem if allocation waits on writeback, but rather something like "tagged as writeback" might be more clear than "held locked in writeback."
Yeah, taht's what I meant.
Quoted 24 lines
Otherwise this seems fine to me:
Reviewed-by Brian Foster <[email protected]>Quoted 20 lines
This affects both pre-IO submission and post-IO submission paths. Hence
xfs_setsize_trans_alloc(),xfs_reflink_end_cow(),xfs_iomap_write_unwritten()andxfs_reflink_cancel_cow_range().xfs_iomap_write_unwritten()already does the right thing, but the others don't. Fix them.Signed-off-by Dave Chinner <[email protected]>--- fs/xfs/xfs_aops.c | 3 ++- fs/xfs/xfs_reflink.c | 4 ++-- 2 files changed, 4 insertions(+), 3 deletions(-)fs/xfs/xfs_aops.c +0
-0@@ -209,7 +209,8 @@ xfs_setfilesize_trans_alloc(struct xfs_trans *tp;int error;
Quoted 6 lines
fs/xfs/xfs_aops.c xfs_setfilesize_trans_alloc( · line 213
Quoted 3 lines
fs/xfs/xfs_aops.c +2
-1- error =xfs_trans_alloc (mp, &M_RES (mp)->tr_fsyncts,0 ,0 ,0 , &tp); + error =xfs_trans_alloc (mp, &M_RES (mp)->tr_fsyncts,0 ,0 , +XFS_TRANS_NOFS , &tp);Was there another reason we preallocate this transaction where we don't seem to for others?
Because it happens all the time for buffered IO and this prevents stalling the completion workqueue for a simple size update. Unwritten conversion and COW completion are less frequently done, are much larger pieces of work, can require multiple transactions and have many more potential blocking points, and so such an optimisation is much less beneficial in those cases.
Cheers,
Dave.
Signature · 2 lines
Dave Chinner [email protected]
Quoted 8 lines · Dave Chinner
From: Dave Chinner <[email protected]>
xfs_trans_alloc()doesGFP_KERNELallocation, and we can call it while holding pages locked for writeback in the ->writepages path. The memory allocation is allowed to wait on pages under writeback, and so can wait on pages that are held locked in writeback by the caller.
Quoted 7 lines
This affects both pre-IO submission and post-IO submission paths. Hence
xfs_setsize_trans_alloc(),xfs_reflink_end_cow(),xfs_iomap_write_unwritten()andxfs_reflink_cancel_cow_range().xfs_iomap_write_unwritten()already does the right thing, but the others don't. Fix them.Signed-off-by Dave Chinner <[email protected]>
I believe these are two separate regressions though, introduced on separate kernels Can we treat them as such and use respective Fixes tag for them?
Quoted 10 lines
--- fs/xfs/xfs_aops.c | 3 ++- fs/xfs/xfs_reflink.c | 4 ++-- 2 files changed, 4 insertions(+), 3 deletions(-)fs/xfs/xfs_aops.c +0
-0
fs/xfs/xfs_aops.c xfs_setfilesize_trans_alloc( · line 216
Quoted 9 lines
fs/xfs/xfs_aops.c +2
-1@@ -209,7 +209,8 @@ xfs_setfilesize_trans_alloc(struct xfs_trans *tp;int error; - error =xfs_trans_alloc (mp, &M_RES (mp)->tr_fsyncts,0 ,0 ,0 , &tp); + error =xfs_trans_alloc (mp, &M_RES (mp)->tr_fsyncts,0 ,0 , +XFS_TRANS_NOFS , &tp);if (error)return error;
Introduced on v4.7
Quoted 14 lines
fs/xfs/xfs_reflink.c +1
-1@@ -668,7 +668,7 @@ xfs_reflink_cancel_cow_range(/* Start a rolling transaction to remove the mappings */ error =xfs_trans_alloc (ip->i_mount, &M_RES (ip->i_mount)->tr_write, -0 ,0 ,0 , &tp); +0 ,0 ,XFS_TRANS_NOFS , &tp);if (error) goto out;
fs/xfs/xfs_reflink.c xfs_reflink_end_cow( · line 746
Quoted 7 lines
fs/xfs/xfs_reflink.c +1
-1@@ -741,7 +741,7 @@ xfs_reflink_end_cow( (unsigned int )(end_fsb - offset_fsb), XFS_DATA_FORK); error =xfs_trans_alloc (ip->i_mount, &M_RES (ip->i_mount)->tr_write, - resblks,0 ,XFS_TRANS_RESERVE , &tp); + resblks,0 ,XFS_TRANS_RESERVE |XFS_TRANS_NOFS , &tp);if (error) goto out;
For both of the above:
Introduced on v4.9.
Luis
Quoted 15 lines · Luis R. Rodriguez
Quoted 14 lines · Dave Chinner
From: Dave Chinner <[email protected]>
xfs_trans_alloc()doesGFP_KERNELallocation, and we can call it while holding pages locked for writeback in the ->writepages path. The memory allocation is allowed to wait on pages under writeback, and so can wait on pages that are held locked in writeback by the caller.This affects both pre-IO submission and post-IO submission paths. Hence
xfs_setsize_trans_alloc(),xfs_reflink_end_cow(),xfs_iomap_write_unwritten()andxfs_reflink_cancel_cow_range().xfs_iomap_write_unwritten()already does the right thing, but the others don't. Fix them.
Quoted 4 lines
Quoted 1 line
Signed-off-by Dave Chinner <[email protected]>I believe these are two separate regressions though, introduced on separate kernels Can we treat them as such and use respective Fixes tag for them?
Neither are regressions - they are effectively zero-day bugs. In general, I don't use Fixes tags for things that are not regressions and are easily discoverable from the published git history...
Quoted 14 lines
Quoted 14 lines
--- fs/xfs/xfs_aops.c | 3 ++- fs/xfs/xfs_reflink.c | 4 ++-- 2 files changed, 4 insertions(+), 3 deletions(-)fs/xfs/xfs_aops.c +0
-0@@ -209,7 +209,8 @@ xfs_setfilesize_trans_alloc(struct xfs_trans *tp;int error;
Quoted 8 lines
fs/xfs/xfs_aops.c xfs_setfilesize_trans_alloc( · line 216
Quoted 6 lines
fs/xfs/xfs_aops.c +2
-1- error =xfs_trans_alloc (mp, &M_RES (mp)->tr_fsyncts,0 ,0 ,0 , &tp); + error =xfs_trans_alloc (mp, &M_RES (mp)->tr_fsyncts,0 ,0 , +XFS_TRANS_NOFS , &tp);if (error)return error;Fixes 253f4911f297b ("xfs: better xfs_trans_alloc interface")
No, thats wrong - that commit didn't change any behaviour. The original commit:
281627df3eb5 ("xfs: log file size updates at I/O completion time")
called:
tp = xfs_trans_alloc(mp, XFS_TRANS_FSYNC_TS);
which resulted in a GFP_KERNEL allocation via:
tp = _xfs_trans_alloc(mp, type, KM_SLEEP);
So this is a zero-day bug in logging file size updates at IO completion.
Quoted 26 lines
Introduced on v4.7
Quoted 14 lines
fs/xfs/xfs_reflink.c +1
-1@@ -668,7 +668,7 @@ xfs_reflink_cancel_cow_range(/* Start a rolling transaction to remove the mappings */ error =xfs_trans_alloc (ip->i_mount, &M_RES (ip->i_mount)->tr_write, -0 ,0 ,0 , &tp); +0 ,0 ,XFS_TRANS_NOFS , &tp);if (error) goto out;fs/xfs/xfs_reflink.c xfs_reflink_end_cow( · line 746
Quoted 7 lines
fs/xfs/xfs_reflink.c +1
-1@@ -741,7 +741,7 @@ xfs_reflink_end_cow( (unsigned int )(end_fsb - offset_fsb), XFS_DATA_FORK); error =xfs_trans_alloc (ip->i_mount, &M_RES (ip->i_mount)->tr_write, - resblks,0 ,XFS_TRANS_RESERVE , &tp); + resblks,0 ,XFS_TRANS_RESERVE |XFS_TRANS_NOFS , &tp);if (error) goto out;For both of the above:
Quoted 1 line
Fixes 43caeb187deb9 ("xfs: move mappings from cow fork to data fork after copy-write)"
And that's a zero-day, too. So neither are regressions.
-Dave.
Signature · 2 lines
Dave Chinner [email protected]
Quoted 42 lines · Luis R. Rodriguez
Quoted 8 lines · Dave Chinner
From: Dave Chinner <[email protected]>
xfs_trans_alloc()doesGFP_KERNELallocation, and we can call it while holding pages locked for writeback in the ->writepages path. The memory allocation is allowed to wait on pages under writeback, and so can wait on pages that are held locked in writeback by the caller.Quoted 7 lines
This affects both pre-IO submission and post-IO submission paths. Hence
xfs_setsize_trans_alloc(),xfs_reflink_end_cow(),xfs_iomap_write_unwritten()andxfs_reflink_cancel_cow_range().xfs_iomap_write_unwritten()already does the right thing, but the others don't. Fix them.Signed-off-by Dave Chinner <[email protected]>I believe these are two separate regressions though, introduced on separate kernels Can we treat them as such and use respective Fixes tag for them?
Quoted 10 lines
--- fs/xfs/xfs_aops.c | 3 ++- fs/xfs/xfs_reflink.c | 4 ++-- 2 files changed, 4 insertions(+), 3 deletions(-)fs/xfs/xfs_aops.c +0
-0fs/xfs/xfs_aops.c xfs_setfilesize_trans_alloc( · line 216
Quoted 9 lines
fs/xfs/xfs_aops.c +2
-1@@ -209,7 +209,8 @@ xfs_setfilesize_trans_alloc(struct xfs_trans *tp;int error; - error =xfs_trans_alloc (mp, &M_RES (mp)->tr_fsyncts,0 ,0 ,0 , &tp); + error =xfs_trans_alloc (mp, &M_RES (mp)->tr_fsyncts,0 ,0 , +XFS_TRANS_NOFS , &tp);if (error)return error;Fixes 253f4911f297b ("xfs: better xfs_trans_alloc interface")
Quoted 1 line
Introduced on v4.7
I don't think so - prior to that commit, this allocation could still recurse into the filesystem, the only allocation flag used was KM_SLEEP, AFAICT.
Quoted 24 lines
Quoted 14 lines
fs/xfs/xfs_reflink.c +1
-1@@ -668,7 +668,7 @@ xfs_reflink_cancel_cow_range(/* Start a rolling transaction to remove the mappings */ error =xfs_trans_alloc (ip->i_mount, &M_RES (ip->i_mount)->tr_write, -0 ,0 ,0 , &tp); +0 ,0 ,XFS_TRANS_NOFS , &tp);if (error) goto out;fs/xfs/xfs_reflink.c xfs_reflink_end_cow( · line 746
Quoted 7 lines
fs/xfs/xfs_reflink.c +1
-1@@ -741,7 +741,7 @@ xfs_reflink_end_cow( (unsigned int )(end_fsb - offset_fsb), XFS_DATA_FORK); error =xfs_trans_alloc (ip->i_mount, &M_RES (ip->i_mount)->tr_write, - resblks,0 ,XFS_TRANS_RESERVE , &tp); + resblks,0 ,XFS_TRANS_RESERVE |XFS_TRANS_NOFS , &tp);if (error) goto out;For both of the above:
Quoted 3 lines
Fixes 43caeb187deb9 ("xfs: move mappings from cow fork to data fork after copy-write)"Introduced on v4.9.
Perhaps, as that commit did add the new allocations which could recurse.
-Eric
Quoted 8 lines · Dave Chinner
From: Dave Chinner <[email protected]>
xfs_trans_alloc()doesGFP_KERNELallocation, and we can call it while holding pages locked for writeback in the ->writepages path. The memory allocation is allowed to wait on pages under writeback, and so can wait on pages that are held locked in writeback by the caller.
Quoted 7 lines
This affects both pre-IO submission and post-IO submission paths. Hence
xfs_setsize_trans_alloc(),xfs_reflink_end_cow(),xfs_iomap_write_unwritten()andxfs_reflink_cancel_cow_range().xfs_iomap_write_unwritten()already does the right thing, but the others don't. Fix them.Signed-off-by Dave Chinner <[email protected]>
Looks fine,