Re: review: use correct buffer flags when reading superblock

Lachlan McIlroy
2007-10-11T07:19:20Z

[-- Attachment #1: Type: text/plain, Size: 1191 bytes --]

David Chinner wrote:
> On Thu, Oct 11, 2007 at 12:42:25PM +1000, Lachlan McIlroy wrote:
>> David Chinner wrote:
>>> On Wed, Oct 10, 2007 at 06:37:47PM +1000, Lachlan McIlroy wrote:
>>>> When reading the superblock during log recovery we are not setting
>>>> the correct buffer flags. Specifically we are not turning off flags
>>>> we do not need such as XBF_ASYNC that is causing the synchronous
>>>> xfs_iowait() to hang. We should also turn off XBF_WRITE and remove
>>>> the buffer from the delay write queue just to be safe.
>>> We really don't need the removal of the write flags - the XFS_bflush()
>>> call above the xfs_getsb() call guarantees that they won't be set....
>> It's not obvious though. It wasn't obvious that ASYNC was still set
>> and look where that got us.
>
> Sure, but it is incorrect to have them set at that point - incorrect
> enough that it might cause corruption. That is, if they are set
> then the changes made during log replay have not been written to
> disk and we've got bigger problems than a hanging I/O to worry
> about....
>
> So rather than clearing them, we should be asserting that the
> write flags are not set at all.
>

New patch attached.

[-- Attachment #2: xfs_getsb.diff --]
[-- Type: text/x-patch, Size: 455 bytes --]

--- fs/xfs/xfs_log_recover.c_1.329 2007-10-10 15:59:18.000000000 +1000
+++ fs/xfs/xfs_log_recover.c 2007-10-11 15:03:00.000000000 +1000
@@ -3824,7 +3824,10 @@ xlog_do_recover(
*/
bp = xfs_getsb(log->l_mp, 0);
XFS_BUF_UNDONE(bp);
+ ASSERT(!(XFS_BUF_ISWRITE(bp)));
+ ASSERT(!(XFS_BUF_ISDELAYWRITE(bp)));
XFS_BUF_READ(bp);
+ XFS_BUF_UNASYNC(bp);
xfsbdstrat(log->l_mp, bp);
if ((error = xfs_iowait(bp))) {
xfs_ioerror_alert("xlog_do_recover",