xfs
[Top] [All Lists]

Re: 2.6.39-rc3, 2.6.39-rc4: XFS lockup - regression since 2.6.38

To: Dave Chinner <david@xxxxxxxxxxxxx>
Subject: Re: 2.6.39-rc3, 2.6.39-rc4: XFS lockup - regression since 2.6.38
From: Christoph Hellwig <hch@xxxxxxxxxxxxx>
Date: Thu, 5 May 2011 08:39:59 -0400
Cc: linux-kernel@xxxxxxxxxxxxxxx, Markus Trippelsdorf <markus@xxxxxxxxxxxxxxx>, Bruno Pr?mont <bonbons@xxxxxxxxxxxxxxxxx>, xfs-masters@xxxxxxxxxxx, xfs@xxxxxxxxxxx, Christoph Hellwig <hch@xxxxxxxxxxxxx>, Alex Elder <aelder@xxxxxxx>, Dave Chinner <dchinner@xxxxxxxxxx>
In-reply-to: <20110505122117.GB26837@dastard>
References: <20110423224403.5fd1136a@xxxxxxxxxxxx> <20110427050850.GG12436@dastard> <20110427182622.05a068a2@xxxxxxxxxxxx> <20110428194528.GA1627@xxxxxxxxxxxxxx> <20110429011929.GA13542@dastard> <20110504005736.GA2958@xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx> <20110505002126.GA26797@dastard> <20110505022613.GA26837@dastard> <20110505122117.GB26837@dastard>
User-agent: Mutt/1.5.21 (2010-09-15)
> The third problem is that updating the push target is not safe on 32
> bit machines. We cannot copy a 64 bit LSN without the possibility of
> corrupting the result when racing with another updating thread. We
> have function to do this update safely without needing to care about
> 32/64 bit issues - xfs_trans_ail_copy_lsn() - so use that when
> updating the AIL push target.

But reading xa_target without xa_lock isn't safe on 32-bit either, is it?

For the first read it can trivially be moved into the critical
section a few lines below, and the second one should probably use
XFS_LSN_CMP.

> @@ -482,19 +481,24 @@ xfs_ail_worker(
>       /* assume we have more work to do in a short while */
>       tout = 10;
>       if (!count) {
> +out_done:

Jumping into conditionals is really ugly.  By initializing count a bit
earlier you can just jump in front of the if/else clauses.  And while
you're there maybe moving the tout = 10; into an else clause would
also make the code more readable.
an uninitialied used of tout.

> +             if (ailp->xa_target == target ||
> +                 (test_and_set_bit(XFS_AIL_PUSHING_BIT, &ailp->xa_flags)))

no need for braces around the test_and_set_bit call.

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