On Wednesday, April 8, 2020 5:51 PM Brian Foster wrote:
> The filesystem freeze sequence in XFS waits on any background
> eofblocks or cowblocks scans to complete before the filesystem is
> quiesced. At this point, the freezer has already stopped the
> transaction subsystem, however, which means a truncate or cowblock
> cancellation in progress is likely blocked in transaction
> allocation. This results in a deadlock between freeze and the
> associated scanner.
>
> Fix this problem by holding superblock write protection across calls
> into the block reapers. Since protection for background scans is
> acquired from the workqueue task context, trylock to avoid a similar
> deadlock between freeze and blocking on the write lock.
|-------------------------------------+---------------------------------|
| fsfreeze | eof blocks reaper |
|-------------------------------------+---------------------------------|
| Set sb frozen state to SB_FREEZE_FS | |
| | Start periodic execution |
| | xfs_trans_alloc() |
| | - sb_start_intwrite() |
| | Wait for frozen state to |
| | return to < SB_UNFROZEN state |
| xfs_stop_block_reaping() | |
| - Wait for eof worker to finish | |
|-------------------------------------+---------------------------------|
If we add a blocking lock invocation at the beginning of eof blocks reaper,
then fsfreeze would get blocked at cancel_delayed_work_sync().
However using a trylock, "eof blocks reaper" would return back due to failure
in obtaining the lock and hence it is guaranteed that fsfreeze will make progress.
Hence the changes are logically correct.
Reviewed-by: Chandan Rajendra <chandanrlinux@xxxxxxxxx>
>
> Fixes: d6b636ebb1c9f ("xfs: halt auto-reclamation activities while rebuilding rmap")
> Reported-by: Paul Furtado <paulfurtado91@xxxxxxxxx>
> Signed-off-by: Brian Foster <bfoster@xxxxxxxxxx>
> ---
>
> Note that this has the opposite tradeoff as the approach I originally
> posited [1], specifically that the eofblocks ioctl() now always blocks
> on a frozen fs rather than return -EAGAIN. It's worth pointing out that
> the eofb control structure has a sync flag (that is not used for
> background scans), so yet another approach could be to tie the trylock
> to that.
>
> Brian
>
> [1] https://lore.kernel.org/linux-xfs/20200407163739.GG28936@bfoster/
>
> fs/xfs/xfs_icache.c | 10 ++++++++++
> fs/xfs/xfs_ioctl.c | 5 ++++-
> 2 files changed, 14 insertions(+), 1 deletion(-)
>
> diff --git a/fs/xfs/xfs_icache.c b/fs/xfs/xfs_icache.c
> index a7be7a9e5c1a..8bf1d15be3f6 100644
> --- a/fs/xfs/xfs_icache.c
> +++ b/fs/xfs/xfs_icache.c
> @@ -911,7 +911,12 @@ xfs_eofblocks_worker(
> {
> struct xfs_mount *mp = container_of(to_delayed_work(work),
> struct xfs_mount, m_eofblocks_work);
> +
> + if (!sb_start_write_trylock(mp->m_super))
> + return;
> xfs_icache_free_eofblocks(mp, NULL);
> + sb_end_write(mp->m_super);
> +
> xfs_queue_eofblocks(mp);
> }
>
> @@ -938,7 +943,12 @@ xfs_cowblocks_worker(
> {
> struct xfs_mount *mp = container_of(to_delayed_work(work),
> struct xfs_mount, m_cowblocks_work);
> +
> + if (!sb_start_write_trylock(mp->m_super))
> + return;
> xfs_icache_free_cowblocks(mp, NULL);
> + sb_end_write(mp->m_super);
> +
> xfs_queue_cowblocks(mp);
> }
>
> diff --git a/fs/xfs/xfs_ioctl.c b/fs/xfs/xfs_ioctl.c
> index cdfb3cd9a25b..309958186d33 100644
> --- a/fs/xfs/xfs_ioctl.c
> +++ b/fs/xfs/xfs_ioctl.c
> @@ -2363,7 +2363,10 @@ xfs_file_ioctl(
> if (error)
> return error;
>
> - return xfs_icache_free_eofblocks(mp, &keofb);
> + sb_start_write(mp->m_super);
> + error = xfs_icache_free_eofblocks(mp, &keofb);
> + sb_end_write(mp->m_super);
> + return error;
> }
>
> default:
>
--
chandan
