[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Date: Thu, 5 May 2011 08:39:59 -0400
From: Christoph Hellwig <hch@...radead.org>
To: Dave Chinner <david@...morbit.com>
Cc: linux-kernel@...r.kernel.org,
Markus Trippelsdorf <markus@...ppelsdorf.de>,
Bruno Pr?mont <bonbons@...ux-vserver.org>,
xfs-masters@....sgi.com, xfs@....sgi.com,
Christoph Hellwig <hch@...radead.org>,
Alex Elder <aelder@....com>, Dave Chinner <dchinner@...hat.com>
Subject: Re: 2.6.39-rc3, 2.6.39-rc4: XFS lockup - regression since 2.6.38
> 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.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Powered by blists - more mailing lists