[RFC] xfs: wait for the write of the superblock on unmount

[RFC] xfs: wait for the write of the superblock on unmount

[tinguely]

[-- Attachment #1: xfs-lock_sbcount_wait.patch --]
[-- Type: text/plain, Size: 1466 bytes --]

Sorry, I have been distracted away from this regression. This was previously
titled "xfs: synchronously write the superblock on unmount".

xfs_wait_buftarg() does not wait for the completion of the write of the
uncached superblock. This write can race with the shutdown of the log and
causes a panic if the write does not win the race.

The log write of the superblock is important for possible recovery, but a
second syncronous write of the same superblock seems redundant. Would just
waiting for the iodone() of the log write before tearing down the log be
enough?

Signed-off-by: Mark Tinguely <tinguely@sgi.com>

---
 fs/xfs/xfs_mount.c |    8 ++++++++
 1 file changed, 8 insertions(+)

Index: b/fs/xfs/xfs_mount.c
===================================================================
--- a/fs/xfs/xfs_mount.c
+++ b/fs/xfs/xfs_mount.c
@@ -1457,6 +1457,7 @@ void
 xfs_unmountfs(
 	struct xfs_mount	*mp)
 {
+	struct xfs_buf		*bp = mp->m_sb_bp;
 	__uint64_t		resblks;
 	int			error;
 
@@ -1529,6 +1530,13 @@ xfs_unmountfs(
 	xfs_ail_push_all_sync(mp->m_ail);
 	xfs_wait_buftarg(mp->m_ddev_targp);
 
+	/*
+	 * Wait for the superblock to be written out. The superblock buffer
+	 * will remain locked until the iodone().
+	 */
+	xfs_buf_lock(bp);
+	xfs_buf_unlock(bp);
+
 	xfs_log_unmount_write(mp);
 	xfs_log_unmount(mp);
 	xfs_uuid_unmount(mp);


_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [RFC] xfs: wait for the write of the superblock on unmount

[Dave Chinner]

On Wed, Jul 18, 2012 at 12:33:58PM -0500, tinguely@sgi.com wrote:
> Sorry, I have been distracted away from this regression. This was previously
> titled "xfs: synchronously write the superblock on unmount".
> 
> xfs_wait_buftarg() does not wait for the completion of the write of the
> uncached superblock. This write can race with the shutdown of the log and
> causes a panic if the write does not win the race.
> 
> The log write of the superblock is important for possible recovery, but a
> second syncronous write of the same superblock seems redundant. Would just
> waiting for the iodone() of the log write before tearing down the log be
> enough?

Yes. i.e. something like:

	/*
	 * The superblock buffer is uncached, so xfs_wait_buftarg()
	 * will not wait for it. Hence we need to explicitly wait
	 * for IO completion on the superblock to occur here.
	 */
	error = xfs_buf_iowait(mp->m_sb_bp);
	if (error)
		AAAAIEEEE!

This fix is also needed in xfs_quiesce_attr() for the freeze and
ro,remount cases as well.

Cheers,

Dave.

-- 
Dave Chinner
david@fromorbit.com

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [RFC] xfs: wait for the write of the superblock on unmount

[Mark Tinguely]

On 07/18/12 20:58, Dave Chinner wrote:
> On Wed, Jul 18, 2012 at 12:33:58PM -0500, tinguely@sgi.com wrote:
>> Sorry, I have been distracted away from this regression. This was previously
>> titled "xfs: synchronously write the superblock on unmount".
>>
>> xfs_wait_buftarg() does not wait for the completion of the write of the
>> uncached superblock. This write can race with the shutdown of the log and
>> causes a panic if the write does not win the race.
>>
>> The log write of the superblock is important for possible recovery, but a
>> second syncronous write of the same superblock seems redundant. Would just
>> waiting for the iodone() of the log write before tearing down the log be
>> enough?
>
> Yes. i.e. something like:
>
> 	/*
> 	 * The superblock buffer is uncached, so xfs_wait_buftarg()
> 	 * will not wait for it. Hence we need to explicitly wait
> 	 * for IO completion on the superblock to occur here.
> 	 */
> 	error = xfs_buf_iowait(mp->m_sb_bp);
> 	if (error)
> 		AAAAIEEEE!
>
> This fix is also needed in xfs_quiesce_attr() for the freeze and
> ro,remount cases as well.
>
> Cheers,
>
> Dave.
>

Thanks for the feedback.

I chose the ugly lock to block on the superblock buffer because it has 
XBF_ASYNC set at that point and won't go through the complete() patch. I 
will look at turning the XBF_ASYNC flag off and wait on a xfs_buf_iowait().

--Mark.

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [RFC] xfs: wait for the write of the superblock on unmount

[Mark Tinguely]

On 07/20/12 08:21, Mark Tinguely wrote:
> On 07/18/12 20:58, Dave Chinner wrote:
>> On Wed, Jul 18, 2012 at 12:33:58PM -0500, tinguely@sgi.com wrote:
>>> Sorry, I have been distracted away from this regression. This was
>>> previously
>>> titled "xfs: synchronously write the superblock on unmount".
>>>
>>> xfs_wait_buftarg() does not wait for the completion of the write of the
>>> uncached superblock. This write can race with the shutdown of the log
>>> and
>>> causes a panic if the write does not win the race.
>>>
>>> The log write of the superblock is important for possible recovery,
>>> but a
>>> second syncronous write of the same superblock seems redundant. Would
>>> just
>>> waiting for the iodone() of the log write before tearing down the log be
>>> enough?
>>
>> Yes. i.e. something like:
>>
>> /*
>> * The superblock buffer is uncached, so xfs_wait_buftarg()
>> * will not wait for it. Hence we need to explicitly wait
>> * for IO completion on the superblock to occur here.
>> */
>> error = xfs_buf_iowait(mp->m_sb_bp);
>> if (error)
>> AAAAIEEEE!
>>
>> This fix is also needed in xfs_quiesce_attr() for the freeze and
>> ro,remount cases as well.
>>
>> Cheers,
>>
>> Dave.
>>
>
> Thanks for the feedback.
>
> I chose the ugly lock to block on the superblock buffer because it has
> XBF_ASYNC set at that point and won't go through the complete() patch. I
> will look at turning the XBF_ASYNC flag off and wait on a xfs_buf_iowait().
>
> --Mark.

Looks like the xfsaild_push() sets the XBF_ASYNC flag on the superblock 
buffer. Waiting on the xfs_buf_iowait() won't work but have to use the 
less elegant lock.

--Mark.

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [RFC] xfs: wait for the write of the superblock on unmount

[Christoph Hellwig]

On Wed, Jul 18, 2012 at 12:33:58PM -0500, tinguely@sgi.com wrote:
> Sorry, I have been distracted away from this regression. This was previously
> titled "xfs: synchronously write the superblock on unmount".
> 
> xfs_wait_buftarg() does not wait for the completion of the write of the
> uncached superblock. This write can race with the shutdown of the log and
> causes a panic if the write does not win the race.
> 
> The log write of the superblock is important for possible recovery, but a
> second syncronous write of the same superblock seems redundant. Would just
> waiting for the iodone() of the log write before tearing down the log be
> enough?

This doesn't look beautiful, but I suspect there's no good way around
it.  Can you add your explanation from the reply on why xfs_buf_iowait
does not work here to the comment above the lock/unlock pair?

With that:

Reviewed-by: Christoph Hellwig <hch@lst.de>

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [RFC] xfs: wait for the write of the superblock on unmount

[Mark Tinguely]

On 07/24/12 08:57, Christoph Hellwig wrote:
> On Wed, Jul 18, 2012 at 12:33:58PM -0500, tinguely@sgi.com wrote:
>> Sorry, I have been distracted away from this regression. This was previously
>> titled "xfs: synchronously write the superblock on unmount".
>>
>> xfs_wait_buftarg() does not wait for the completion of the write of the
>> uncached superblock. This write can race with the shutdown of the log and
>> causes a panic if the write does not win the race.
>>
>> The log write of the superblock is important for possible recovery, but a
>> second syncronous write of the same superblock seems redundant. Would just
>> waiting for the iodone() of the log write before tearing down the log be
>> enough?
>
> This doesn't look beautiful, but I suspect there's no good way around
> it.  Can you add your explanation from the reply on why xfs_buf_iowait
> does not work here to the comment above the lock/unlock pair?
>
> With that:
>
> Reviewed-by: Christoph Hellwig<hch@lst.de>

Thank-you. I will repost with the same xfs_buf_lock() entry to 
xfs_quiesce_attr().

--Mark.

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs