Hey Dave, On Wed, Jun 06, 2012 at 02:26:47PM +1000, Dave Chinner wrote: > On Fri, May 25, 2012 at 03:45:36PM -0500, Ben Myers wrote: > > 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@xxxxxxxxxx> Thanks! > 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 that's a good idea. > 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? That sounds pretty good. In particular, I think that making the start and stop of the workqueues correct should be the high priority. I'm not as concerned about the accuracy of the names, or cleaning up xfs_sync.c and xfs_iget.c, but cleanups are worth doing too. I hit a crash related to the xfslogd workqueue awhile back. Mark has taken it up, so there might be a little coordination to do with him. Regards, Ben _______________________________________________ xfs mailing list xfs@xxxxxxxxxxx http://oss.sgi.com/mailman/listinfo/xfs
