Re: xfs: stop the sync worker before xfs_unmountfs

Ben Myers
2012-09-13T16:42:41Z

Hi Dave,

On Thu, Sep 13, 2012 at 09:14:06AM +1000, Dave Chinner wrote:
> On Wed, Sep 12, 2012 at 01:33:47PM -0500, Ben Myers wrote:
> > See what you think of this. Not heavily tested yet, and not pretty... but it
> > is fairly minimal.
> ....
> > Signed-off-by: Ben Myers <bpm@sgi.com>
> >
> > Index: xfs/fs/xfs/xfs_super.c
> > ===================================================================
> > --- xfs.orig/fs/xfs/xfs_super.c
> > +++ xfs/fs/xfs/xfs_super.c
> > @@ -919,6 +919,7 @@ xfs_fs_put_super(
> > struct xfs_mount *mp = XFS_M(sb);
> >
> > xfs_filestream_unmount(mp);
> > + cancel_delayed_work_sync(&mp->m_sync_work);
> > xfs_unmountfs(mp);
> > xfs_syncd_stop(mp);
> > xfs_freesb(mp);
>
> This is the only hunk in the patch needed to fix the problem.
>
> The rest of the patch does not change the order in which the sync
> worker is started and stopped - it just open codes it next to the
> xfs_syncd_start/stop calls. Essentially, all it does is obfuscate
> the real fix that is being made here and makes it harder to
> understand what the relationship between xfs_sync_worker() and
> xfs_syncd_start/stop is supposed to be.
>
> So a minimal patch, IMO, is just the above hunk....

Ah, you're right. I was working on the assumption that it is best not to
cancel the work twice. There really is no harm in cancel_delayed_work_sync
both in xfs_fs_put_super and in xfs_syncd_stop. However, we don't want them
spread around willy nilly. That can become obfuscatory too. I suggest we
should remove the cancel_delayed_work_sync(&mp->m_sync_work) in
xfs_log_unmount, leftover from commit 11159a05. It seems like that one hasn't
been effective.

Maybe we don't want to do that in this patch... I'll just add a comment and
repost.

Thanks,
Ben