Re: [PATCH] xfs: shutdown xfs_sync_worker before the log

[Dave Chinner <david@fromorbit.com>]

On Fri, May 25, 2012 at 03:45:36PM -0500, Ben Myers wrote:
> Hey Dave,
> 
> On Thu, May 24, 2012 at 05:39:52PM -0500, Ben Myers wrote:
> > Anyway, I'll make some time to work on this tomorrow so I can test it over
> > the weekend.
> 
> This is going to spin over the weekend.  See what you think.
> 
> -----------
> 
> xfs:  shutdown xfs_sync_worker before the log
> 
> Revert commit 1307bbd, which uses the s_umount semaphore to provide
> exclusion between xfs_sync_worker and unmount, in favor of shutting down
> the sync worker before freeing the log in xfs_log_unmount.  This is a
> cleaner way of resolving the race between xfs_sync_worker and unmount
> than using s_umount.

Looks fine to me.

Reviewed-by: Dave Chinner <dchinner@redhat.com>

Once you commit this, I think I'll do a followup set of patches that
remove all the problems caused by trying to start and stop unrelated
functionality in the same place.

I think starting by renaming the xfs-syncd workqueue to the
xfs_mount_wq because there's nothing "sync" related about it's
functionality any more.

I'll then kill xfs_syncd_init/stop functions and open code the
intialisation of the work structures and start them in the
appropriate places for their functionality. e.g. reclaim work is
demand started and stops when there's nothing more to do or at
unmount, the flush work is demand started and we need to complete
them all at unmount, and the xfssync work is really now "periodic
log work" so should be started once we complete log recovery
successfullly and stopped before we tear down the log....

Then I can move the xfs_sync_worker to xfs_log.c and rename it.
If I then convert xfs_flush_worker to use the generic writeback code
(writeback_inodes_sb_if_idle) the xfs_sync_data() can go away. That
means the only user of xfs_inode_ag_iterator is the quotaoff code
(xfs_qm_dqrele_all_inodes), so it could be moved elsewhere (like
xfs_inode.c).

Then xfs_quiesce_data() can be moved to xfs-super.c where it can sit
alongside the two functions that call it, and the same can be done
for xfs_quiesce_attr().

That will leave only inode cache reclaim functions in xfs_sync.c.
These are closely aligned to the inode allocation, freeing and cache
lookup functions in xfs_iget.c, so I'm thinking of merging the two
into a single file named xfs_inode_cache.c so both xfs_sync.c and
xfs_iget.c go away.

Thoughts?

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs