xfs
[Top] [All Lists]

Re: [PATCH 3/8] xfs: initialise xfssync work before running quotachecks

To: Ben Myers <bpm@xxxxxxx>
Subject: Re: [PATCH 3/8] xfs: initialise xfssync work before running quotachecks
From: Dave Chinner <david@xxxxxxxxxxxxx>
Date: Thu, 29 Mar 2012 11:29:49 +1100
Cc: Mark Tinguely <tinguely@xxxxxxx>, xfs@xxxxxxxxxxx
In-reply-to: <20120328194018.GE7762@xxxxxxx>
References: <1332393313-1955-1-git-send-email-david@xxxxxxxxxxxxx> <1332393313-1955-4-git-send-email-david@xxxxxxxxxxxxx> <20120322151548.GS7762@xxxxxxx> <20120322210723.GC5091@dastard> <4F6C7BE7.3060100@xxxxxxx> <20120325232253.GJ5091@dastard> <4F7086FA.9010003@xxxxxxx> <20120326215709.GM5091@dastard> <20120328194018.GE7762@xxxxxxx>
User-agent: Mutt/1.5.21 (2010-09-15)
On Wed, Mar 28, 2012 at 02:40:18PM -0500, Ben Myers wrote:

[ snip repeated arguments about startup races ]

[ insert repeated responses about "fixed in next patch" ]

[ wait for circle to go around again ]

> I suggest 3 and 4 should be combined into one patch.

Why didn't you just say that's what you want? I have no problem
doing that.  I just kept them separate as there are two logical
changes to the fix - startup order changes, and MS_ACTIVE guard
changes.  Do you want me to send a new combined patch?

FWIW, arguing that a patch in a series is unacceptable because
introduces a temporary problem that is fixed by another patch in the
series is not really helpful, as is saying "it has races" and not
explaining them. If you want the temporary problematic state removed
from the series, just *say so* - it is easy to do.

> You've reordered xfs_syncd_stop with respect to xfs_unmount in the
> error path of xfs_fs_fill_super, but not in the regular unmount
> path xfs_fs_put_super.  I think for consistency they should not be
> reordered in the error path of xfs_fs_fill_super.

[ the circle goes around again ]

The second patch makes it consistent in that xfs_syncd_stop() is
always called after xfs_unmountfs().

> As long as workers can run before xfs_mountfs is run, they need to protect
> themselves to ensure that the structures they are using are initialized.
> It looks like xfs_reclaim_worker would do this in the next patch by using
> MS_ACTIVE, but FWICS xfs_sync_worker still does not protect itself.

Go back and read the second patch, and then review what you wrote
here.  Don't waste my time by making me have to explain it again....

Cheers,

Dave.
-- 
Dave Chinner
david@xxxxxxxxxxxxx

<Prev in Thread] Current Thread [Next in Thread>