review: use correct buffer flags when reading superblock

PATCH Lachlan McIlroy 3 participants 10 messages Oct 10, 2007 – Oct 19, 2007

Share & tools

Lachlan McIlroy

author

2007-10-10 08:33 UTC #0 lore raw

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.

Lachlan

xfs/xfs_log_recover.c renamed from xfs/xfs_log_recover.c_1.329 +3 -0

@@ -3824,7 +3824,10 @@ xlog_do_recover(
 	 */
 	bp = xfs_getsb(log->l_mp, 0);
 	XFS_BUF_UNDONE(bp);
+	XFS_BUF_UNWRITE(bp);
+	XFS_BUF_UNDELAYWRITE(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",

Christoph Hellwig

2007-10-10 10:04 UTC #1 lore raw

Quoted 5 lines · Lachlan McIlroy

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.

Where are these set up in the first time? AFAICS the buffer only written out by xfs_unmountfs_writesb, xfs_syncsub and xfs_trans_log_buf, and all these should only ever happen after log recovery has finished.

David Chinner

2007-10-10 11:25 UTC #2 lore raw

Quoted 10 lines · Christoph Hellwig

Quoted 5 lines · Lachlan McIlroy
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.
Where are these set up in the first time? AFAICS the buffer only written out by xfs_unmountfs_writesb, xfs_syncsub and xfs_trans_log_buf, and all these should only ever happen after log recovery has finished.

It can also be written by xfsbufd when it has been bdwrite() or as a result of log tail pushing.

And in the case of log recovery, if the superblock buffer is in a transaction that is recovered, xlog_recover_do_buffer_trans() will bdwrite() it after it has been recovered. Hence it will be on the delwri list.

Once recovery is complete, xlog_do_recover() then calls XFS_bflush() to write them all out and wait for them. This is down by xfs_flush_buftarg() with the wait flag set, so the async flag on the buffer should be cleared. This is normally the case.

The issue here is that when we openteh buftarg, we start up the xfsbufd which will periodically flush the delwri list asynchronously. If recovery takes long enough, it may flush the delwri list before recovery completes.In this case, instead of doing a sync write we'll get an async write being done and that leaves the XBF_ASYNC flag hanging around on the buffer at I/O completion.

Because the superblock buffer is XBF_FS_MANAGED, it does not get torn down when it is clean and has no references, so the XBF_ASYNC flag never gets cleared unless the fs specifically clears it. If the superblock is then not recovered out of any further transactions during recovery after xfsbufd flushed it, the XBF_ASYNC flag remains set for the re-read that is issued in xlog_do_recover() and we hang.....

Cheers,

Dave.

Signature · 3 lines

Dave Chinner
Principal Engineer
SGI Australian Software Group

Christoph Hellwig

2007-10-18 16:07 UTC #3 lore raw

Quoted 6 lines · David Chinner

Quoted 3 lines
Where are these set up in the first time? AFAICS the buffer only written out by xfs_unmountfs_writesb, xfs_syncsub and xfs_trans_log_buf, and all these should only ever happen after log recovery has finished.
It can also be written by xfsbufd when it has been bdwrite() or as a result of log tail pushing.

Hmm, true.

Quoted 7 lines

Because the superblock buffer is XBF_FS_MANAGED, it does not get torn down when it is clean and has no references, so the XBF_ASYNC flag never gets cleared unless the fs specifically clears it. If the superblock is then not recovered out of any further transactions during recovery after xfsbufd flushed it, the XBF_ASYNC flag remains set for the re-read that is issued in xlog_do_recover() and we hang.....

Makes sense as an explanation. I still don't really like patch, maybe we should always clear the ASYNC flag in the b_iodone callback?

Lachlan McIlroy

author

2007-10-19 01:27 UTC #4 lore raw

Quoted 17 lines · Christoph Hellwig

Quoted 5 lines · David Chinner
Quoted 3 lines
Where are these set up in the first time? AFAICS the buffer only written out by xfs_unmountfs_writesb, xfs_syncsub and xfs_trans_log_buf, and all these should only ever happen after log recovery has finished.
It can also be written by xfsbufd when it has been bdwrite() or as a result of log tail pushing.
Hmm, true.
Quoted 7 lines
Because the superblock buffer is XBF_FS_MANAGED, it does not get torn down when it is clean and has no references, so the XBF_ASYNC flag never gets cleared unless the fs specifically clears it. If the superblock is then not recovered out of any further transactions during recovery after xfsbufd flushed it, the XBF_ASYNC flag remains set for the re-read that is issued in xlog_do_recover() and we hang.....

Quoted 3 lines

Makes sense as an explanation. I still don't really like patch, maybe we should always clear the ASYNC flag in the b_iodone callback?

That sounds like a good idea. Or get rid of the XBF_FS_MANAGED special case and get a new fresh buffer each time.

David Chinner

2007-10-19 02:14 UTC #5 lore raw

Quoted 10 lines · Lachlan McIlroy

Quoted 9 lines · Christoph Hellwig
Quoted 7 lines · David Chinner
Because the superblock buffer is XBF_FS_MANAGED, it does not get torn down when it is clean and has no references, so the XBF_ASYNC flag never gets cleared unless the fs specifically clears it. If the superblock is then not recovered out of any further transactions during recovery after xfsbufd flushed it, the XBF_ASYNC flag remains set for the re-read that is issued in xlog_do_recover() and we hang.....

Quoted 4 lines

Quoted 2 lines
Makes sense as an explanation. I still don't really like patch, maybe we should always clear the ASYNC flag in the b_iodone callback?
That sounds like a good idea.

<shrug>

Makes no real difference - you just have to be careful where the ASYNC flag is cleared because it is used throughout the io completion code....

Quoted 2 lines

Or get rid of the XBF_FS_MANAGED special
case and get a new fresh buffer each time.

I don't think we want to do that. It will add substantial overhead because the superblock is the single most used buffer in the filesysem. It's typically gained during transaction commit, at which time we really, really want to get it quickly and this is known to be a performance limiting bottleneck.....

Cheers,

Dave.

Signature · 3 lines

Dave Chinner
Principal Engineer
SGI Australian Software Group

David Chinner

2007-10-10 11:28 UTC #6 lore raw

In reply to Lachlan McIlroy

Quoted 5 lines · Lachlan McIlroy

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....

Cheers,

Dave.

Signature · 3 lines

Dave Chinner
Principal Engineer
SGI Australian Software Group

Lachlan McIlroy

author

2007-10-11 02:37 UTC #7 lore raw

Quoted 10 lines · David Chinner

Quoted 5 lines · Lachlan McIlroy
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.

David Chinner

2007-10-11 03:30 UTC #8 lore raw

Quoted 8 lines · Lachlan McIlroy

Quoted 7 lines · David Chinner
Quoted 5 lines · Lachlan McIlroy
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.

Quoted 5 lines

Quoted 2 lines
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.

Cheers,

Dave.

Signature · 3 lines

Dave Chinner
Principal Engineer
SGI Australian Software Group

Lachlan McIlroy

author

2007-10-11 07:19 UTC #9 lore raw

Quoted 13 lines · David Chinner

Quoted 11 lines · Lachlan McIlroy
Quoted 8 lines · David Chinner
Quoted 5 lines · Lachlan McIlroy
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.

Quoted 9 lines

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.

xfs/xfs_log_recover.c renamed from xfs/xfs_log_recover.c_1.329 +3 -0

@@ -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",